[MPlayer-dev-eng] [PATCH] A/V sync improvement for TV streams

[Laurent]

On 10/29/08, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Mon, Oct 20, 2008 at 07:23:12PM -0400, Laurent wrote:
> [...]
>
> partial review below, if someone more knowledgeable in the affected code
> wants to review this, then this surely is welcome!
>
> [...]
> > Index: libmpcodecs/ad_pcm.c
> > ===================================================================
> > --- libmpcodecs/ad_pcm.c      (revision 27776)
> > +++ libmpcodecs/ad_pcm.c      (working copy)
> > @@ -7,7 +7,7 @@
> >  #include "libaf/af_format.h"
> >  #include "libaf/reorder_ch.h"
> >
> > -static ad_info_t info =
> > +static const ad_info_t info =
> >  {
> >       "Uncompressed PCM audio decoder",
> >       "pcm",
>
> this hunk is ok and can be commited i think

Removed from the patch, as this is completely unrelated to the current issue.

>
>
> [...]
> > @@ -121,13 +128,39 @@
> >  static int decode_audio(sh_audio_t *sh_audio,unsigned char *buf,int minlen,int maxlen)
> >  {
> >    unsigned len = sh_audio->channels*sh_audio->samplesize;
> > -  len = (minlen + len - 1) / len * len;
> > -  if (len > maxlen)
> > +  minlen = (minlen + len - 1) / len * len;
> > +  if (minlen > maxlen)
> >        // if someone needs hundreds of channels adjust audio_out_minsize
> >        // based on channels in preinit()
> >        return -1;
> > -  len=demux_read_data(sh_audio->ds,buf,len);
> > -  if (len > 0 && sh_audio->channels >= 5) {
> > +
> > +  maxlen = maxlen / len * len;
>
> > +  len = 0;
> > +  struct ad_pcm_context *ctx = sh_audio->context;
>
> this breaks gcc 2.95 support, declaration and statements should not be mixed
> there are a few more such cases that need to be fixed

Fixed.

Also, I reverted to the original Uoti's handling of minlen/maxlen.
What I've done before might have leaded to incomplete decoded samples.

> [...]
> > Index: mplayer.c
> > ===================================================================
> > --- mplayer.c (revision 27776)
> > +++ mplayer.c (working copy)
> > @@ -2085,12 +2085,18 @@
> >  static int sleep_until_update(float *time_frame, float *aq_sleep_time)
> >  {
> >      int frame_time_remaining = 0;
> > +    float delay = 0.0;
> >      current_module="calc_sleep_time";
> >
> >      *time_frame -= GetRelativeTime(); // reset timer
> >
> > -    if (mpctx->sh_audio && !mpctx->d_audio->eof) {
> > -     float delay = mpctx->audio_out->get_delay();
> > +    if (mpctx->sh_audio) {
> > +     // If there still are audio in the buffers,
> > +     // we should continue to sync, especially as
> > +     // the audio demuxer may be EOF temporarily (TV acquisition).
> > +     delay = mpctx->audio_out->get_delay();
> > +    }
> > +    if (delay > 0.0) {
> >       mp_dbg(MSGT_AVSYNC, MSGL_DBG2, "delay=%f\n", delay);
> >
> >       if (autosync) {
>
> I do not know the related code well enough but from my point of view this
> looks odd. EOF supposedly means end of "file" and that really should not be
> true unless it really is the end, i mean there cannot be 2 ends.
> If this variable is really intended to mean some kind of "empty buffer please
> wait" then it should be renamed to a more appropriate name in a seperate
> patch i think.

I modified the comment in the patch. The point is that whenever there
are audio in the buffers, mplayer can continue to synchronize the
video on it.

Thanks,

Laurent
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: libmpcodecs-ad_pcm-syncfix.patch
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20081029/5698391a/attachment.txt>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: mplayer-syncfix.patch
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20081029/5698391a/attachment.asc>