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

[Reimar Döffinger]

On Sun, Aug 29, 2010 at 12:45:14PM +0200, Pásztor Szilárd wrote:
>  void *decode_video(sh_video_t *sh_video, unsigned char *start, int in_size,
> -                   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.
(And when it's added it might be nicer if it was optional, i.e.
allowed to be NULL).

>      double tt;
> +    int delay = get_current_video_decoder_lag(sh_video);
> +
> +    *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.

> -    if (correct_pts && pts != MP_NOPTS_VALUE) {
> +    //------------------------ frame decoded. --------------------
> +
> +    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?
Also my idea was to keep behaviour unchanged for anything _except_ H.264,
so you'd have to turn around that new condition.
So I guess something like

+    if (correct_pts && pts != MP_NOPTS_VALUE
+        && (*full_frame || sh_video->num_buffered_pts < delay)) {


> +// 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?
I honestly don't know, just something to think about.

> @@ -900,8 +905,12 @@ static mp_image_t *decode(sh_video_t *sh
>          break;
>      }
>  //--
> -
> -    if(!got_picture) return NULL;        // skipped image
> +    if(!got_picture) {
> +	if (avcodec_find_decoder_by_name(sh->codec->dll)->id == CODEC_ID_H264)

avctx->codec->id
Also please avoid the cosmetics (removing the empty line).
With these changes it looks like a patch I am can fully understand
and am quite confident it won't break anything, very good.