[MPlayer-dev-eng] [PATCH] unified timing patch for H264

[Pásztor Szilárd]

It's probably worth settling open points before I make a new patch.

Reimar Döffinger:
> > -                   int drop_frame, double pts)
> > +                   int drop_frame, double pts, int *full_frame)
> 
> This extra argument is no longer used anywhere in this patch,
> so it shouldn't be added here.

Huh? This extra argument is almost the whole point of the patch...
Maybe it could be moved to the other patch, need to check that.

> (And when it's added it might be nicer if it was optional, i.e.
> allowed to be NULL).

This can be done.

> +    *full_frame = 1;
> +    mpi = mpvdec->decode(sh_video, start, in_size, drop_frame);
> 
> I think it would be slightly safer to call
> get_current_video_decoder_lag here, to allow the decode call to
> change it. I admit it's hardly relevant
> though.

This can be done too

> > +    if (mpi && mpi->type == MP_IMGTYPE_NULL) {
> > +   *full_frame = 0;
> > +   mpi = NULL;
> > +    }
> > +    if (correct_pts && pts != MP_NOPTS_VALUE
> > +        && (mpi || sh_video->num_buffered_pts < delay)) {
> 
> Huh? Wasn't it exactly this code that was supposed to behave
> differently for mpi == NULL vs. MP_IMGTYPE_NULL?

No. The point is to propagate the return value ("got_picture") further, and
that's done in full_frame as a return value. Here mpi is set to 0 in such a
case, for the rest of the function to behave the same way as before, and
then checked to buffer PTS only if we have an actual image (except when we
need the buffer to fill up, which will be flooded out in a few frames' time
anyway). So, after this function returns, there's no difference in values at
all, except that we have a full_frame to indicate if we need to wait one
frame duration or not, which is used in mplayer.c.

> > +// Doesn't need any buffer, dummy image
> > +#define MP_IMGTYPE_NULL 6
> 
> Would be good to desribe its purpose.
> As I understand it it does not serve as a dummy
> image but actually as a "no image" indicator.
> Maybe MP_IMGTYPE_INCOMPLETE would be a better name?

OK