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

[Reimar Döffinger]

On Sat, Aug 21, 2010 at 12:37:10PM +0200, Pásztor Szilárd wrote:
> On Sat, 21 Aug 2010 08:24:35 +0200, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> >>          if (cur->field_poc[0]==INT_MAX || cur->field_poc[1]==INT_MAX) {
> >>              /* Wait for second field. */
> >> -            *data_size = 0;
> >> +            *data_size = -1;
> > 
> > You can't use it like that, that's a size field and setting it like
> > this would mean *data points to almost 4 GB of data - at least unless
> > you change the API which requires a major verson bump, and that
> > won't happen so soon.
> > That's why I suggested changing the return value.
> 
> Ok, here it is corrected. I introduced -2 as retval.
> Magic numbers are not nice, but that's how it is in
> lavc anyway.

Not, that's not at all how it is, -1 is the only special
value with no meaning, AVERROR(E*) or AVERROR_* is to be
used for anything else.
Some value was suggest on ffmpeg-devel the last time
this was suggested, but I don't remember which,
maybe AVERROR(EAGAIN) or AVERROR(EINVAL) or...

> @@ -796,6 +796,7 @@ static mp_image_t *decode(sh_video_t *sh
>      AVFrame *pic= ctx->pic;
>      AVCodecContext *avctx = ctx->avctx;
>      mp_image_t *mpi=NULL;
> +    static mp_image_t mympi;

At least make it a proper "singleton" e.g. by declaring it
as static const in mp_image.c/h

> @@ -826,7 +827,7 @@ static mp_image_t *decode(sh_video_t *sh
>      ret = avcodec_decode_video2(avctx, pic, &got_picture, &pkt);
>  
>      dr1= ctx->do_dr1;
> -    if(ret<0) mp_msg(MSGT_DECVIDEO, MSGL_WARN, "Error while decoding frame!\n");
> +    if(ret == -1) mp_msg(MSGT_DECVIDEO, MSGL_WARN, "Error while decoding frame!\n");

Not printing anything for e.g. AVERROR(ENOMEM) certainly is not a good idea.

>          if (vfilter) {
> +	    int real;
>              int softskip = (vfilter->control(vfilter, VFCTRL_SKIP_NEXT_FRAME, 0) == CONTROL_TRUE);

Indentation is off.

> @@ -1826,15 +1828,16 @@ static int generate_video_frame(sh_video
>  	if (in_size > max_framesize)
>  	    max_framesize = in_size;
>  	current_module = "decode video";
> -	decoded_frame = decode_video(sh_video, start, in_size, drop_frame, pts);
> +	decoded_frame = decode_video(sh_video, start, in_size, drop_frame, pts, &real);
>  	if (decoded_frame) {
> +	    was_frame = 1;
>  	    update_subtitles(sh_video, sh_video->pts, mpctx->d_sub, 0);
>  	    update_teletext(sh_video, mpctx->demuxer, 0);
>  	    update_osd_msg();
>  	    current_module = "filter video";
>  	    if (filter_video(sh_video, decoded_frame, sh_video->pts))
>  		break;
> -	} else if (drop_frame)
> +	} else if (was_frame && drop_frame) // disable framedropping at the very first frame

This doesn't make any sense at all to me.
You can't just continue processing when decoded_frame is NULL.

> @@ -2381,7 +2384,9 @@ static double update_video(int *blit_fra
>  	void *decoded_frame = NULL;
>  	int drop_frame=0;
>  	int in_size;
> +	int real;
>  
> +    do {
>  	current_module = "video_read_frame";
>  	frame_time = sh_video->next_frame_time;
>  	in_size = video_read_frame(sh_video, &sh_video->next_frame_time,
> @@ -2402,15 +2407,7 @@ static double update_video(int *blit_fra
>  	    return -1;
>  	if (in_size > max_framesize)
>  	    max_framesize = in_size; // stats
> -	sh_video->timer += frame_time;
> -	if (mpctx->sh_audio)
> -	    mpctx->delay -= frame_time;
> -	// video_read_frame can change fps (e.g. for ASF video)
> -	vo_fps = sh_video->fps;
>  	drop_frame = check_framedrop(frame_time);
> -	update_subtitles(sh_video, sh_video->pts, mpctx->d_sub, 0);
> -	update_teletext(sh_video, mpctx->demuxer, 0);
> -	update_osd_msg();
>  	current_module = "decode_video";
>  #ifdef CONFIG_DVDNAV
>  	decoded_frame = mp_dvdnav_restore_smpi(&in_size,&start,decoded_frame);
> @@ -2418,11 +2415,41 @@ static double update_video(int *blit_fra
>  	if (in_size > 0 && !decoded_frame)
>  #endif
>  	decoded_frame = decode_video(sh_video, start, in_size, drop_frame,
> -				     sh_video->pts);
> +				     sh_video->pts, &real);
> +
> +    if (real)

Huh? Wouldn't that possibly use "real" uninitialized in the dvdnav case?