[FFmpeg-devel] [PATCH] H.264 timestamps in h264_parser - complete set

[Ivan Schreter]

Michael Niedermayer wrote:
> On Sat, Feb 21, 2009 at 12:15:11AM +0100, Ivan Schreter wrote:
>   
>> Michael Niedermayer wrote:
>>     
>>> [...]
>>> time_base is set by:
>>>     if(h->sps.timing_info_present_flag){
>>>         s->avctx->time_base= (AVRational){h->sps.num_units_in_tick * 2, h->sps.time_scale};
>>>         if(h->x264_build > 0 && h->x264_build < 44)
>>>             s->avctx->time_base.den *= 2;
>>>         av_reduce(&s->avctx->time_base.num, &s->avctx->time_base.den,
>>>                     s->avctx->time_base.num, s->avctx->time_base.den, 1<<30);
>>>     }
>>>
>>> the parser should set it if no decoder is there doing it and
>>> the *2 in there looks wrong without it you should have a tb useabel for fields
>>>
>>>   
>>>       
>> Yes, it looked wrong to me as well, but it's actually correct. H.264 
>> specifies timebase 1/50, although it's a 25fps video. I.e., there is 
>> always a tick per field and videos with full frames have timestamps 2 
>> ticks apart (field-coded interlaced videos 1 tick). Changing this to 
>> 1/50 would break the rest of FFmpeg, thinking it's a 50fps video, which 
>> it isn't. Alternative would be special handling in lavf, but that's also 
>> no solution.
>>     
>
> fps != timebase and if a video specifes 1/50 thats what should be stored in
> time_base.
> That might break something but its that something that is broken not setting
> tb correctly.
> And i dont think we can fix h264 if we keep a fundamental field like the
> timebase set incorrectly.
> also lavf / ffmpeg has and uses r_frame_rate for the real "base" frame rate
> not AVCodecContext.time_base
>   
... which is computed from AVCodecContext.time_base.

So we'd have to special-case H.264 to compute r_frame_rate properly. OK 
with you or any better idea?

>> Even if we had double framerate to handle integer timestamps, this 
>> somehow doesn't fit with rest of lavf. lavf currently passes full 
>> timestamps to lavc to be corrected by lavc, if it wishes to do so. I 
>> still think telling lavc which time base to use is the right way.
>>     
>
> lavf does not pass timestamps to lavc for correction, but rather for
> reordering. lavc does not know the timebase in which the timestamps
> are specified.
>
>   
OK, noted (reordering). You mean, the resulting decoded picture PTS 
should be set correctly, based on PTS of the packet which results in the 
decoded picture?

H.264 code doesn't access those timestamps at all and PTS of decoded 
picture is always 0. What is this PTS of decoded picture supposed to be 
and in which units?

> Besides i belive it is better to export information to let the outer
> part use it the way it likes instead of sucking things in and changing
> them.
> Also fields like repeat_pict are exported and used outside lavc so this
> is not really different from existing code.
>   
Would something like this be acceptable?

1) add parser_dts and parser_pts in AVCodecParserContext, defaulting to 
AV_NOPTS_VALUE (alternatively, instead of parser_pts we could use 
parser_delay relative to parser_dts)

2a) time_base for H.264 will be fixed, with appropriate special-case in 
lavf (NOTE NOTE NOTE: will break older lavf working with newer lavc)
or
2b) we let time_base as is and add another variable ts_ticks_per_frame, 
which will be set to 2 for H.264, 1 by default

3) if the parser is capable of decoding timestamps, it will fill these 
two with integer timestamps in AVCodec.time_base/ts_ticks_per_frame 
units and instead of current guessing, lavf would use these multiplied 
by (effectively) frame_duration/ts_ticks_per_frame (actually, I'd 
implement it using time bases to prevent rounding errors).

4) if a DTS is available from the container, it would be associated with 
current parser_dts, so latter packets without DTS would simply take the 
delta (e.g., via last_known_stream_dts and last_known_parser_dts in 
AVStream, both initialized to AV_NOPTS_VALUE).

5) in H.264, after a seek the timestamps would begin from 0 again, but 
next container DTS would associate new lavc timestamp, so the timestamps 
would be resumed correctly

Regards,

Ivan