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

[Pásztor Szilárd]

On Sat, 21 Aug 2010 13:44:27 +0200, Reimar Döffinger
<Reimar.Doeffinger at gmx.de> wrote:
> 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...

It must be a dedicated value then and future implementations
must comply to it, otherwise the code will be broken.

>> +    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

Ok.

>> +    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.

This will be changed to checking any negative value except the new one.

>> -	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.

I don't see how this would possibly continue processing if decoded_frame
is null. It is to merely prevent dropping the very first frame, leading to
not initializing proper pts and video never being born (which we certainly
don't want).

>>  #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?

Right. Initialization is needed after the ifdef.
If there are no more things to fix, I'll make a new patch.