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

[Ivan Schreter]

Michael Niedermayer wrote:
> On Sat, Feb 21, 2009 at 01:15:46PM +0100, Ivan Schreter wrote:
>   
>> Michael Niedermayer wrote:
>>     
>>> [...]  
>>>       
>>> 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.
>>     
>
> the code in av_find_stream_info analyzes frame durations to set r_frame_rate
> of course there may be small adjustments needed but it should not really
> require special handling of h264 ...
>   
Well, no. It computes it from time_base directly, at least from formats 
with reliable TB.

I added tick_per_frame field, which allows the codec to specify how many 
time_base ticks per frame are done, so frame rate can be computed 
correctly. Even ffmpeg.c checks, whether frame rate matches that of the 
codec, so I used the new field there as well.

Patch avcodec_tick_per_frame attached.

However, H.264 MOV doesn't work with it (gets 1/2 frame rate), so for 
now just pre-review. I'm afraid, we'll have to set H.264 time_base 
actually "wrong" and use ticks_per_frame for timestamp computation 
instead as additional divisor. This makes the code also simpler... What 
do you think?

>> So we'd have to special-case H.264 to compute r_frame_rate properly. OK 
>> with you or any better idea?
>>
>>     
With aforementioned patch generalized, so no problem anymore.

> [...]
>>     
>>> 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)
>>     
>
> exporting
> dts_last_dts_delta (cpb_removial_delay)
> pts_dts_delta      (dpb_output_delay)
> from the parser
> seems nicer
>   
I named the fields a bit differently, but otherwise agreed. Also, 
buffering period start needed to be exported.

See patch avcodec_timestamp. I hope I put the timestamping code at 
correct place in utils.c...

>> 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
>>     
>
> i really dont care what happens with old lavf or old apps when tb is fixed
>
>   
Eh, hard words :-)

>> 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
>>
>>     
As mentioned, avcodec_tick_per_frame is inverse - time_base is corrected 
and we export # of time base ticks per frame.

H.264 patch to actually correct it is in h264_tick_per_frame. Note that 
since stream TB is now 1/2 frame duration, repeat_pict has to be 
adjusted appropriately as well. Resulting frame repeat_pict is still 
correct, as it uses frame duration as time base, not stream time base.

Further, h264_timestap patch exports the necessary information for 
timestamp generation in lavf.

> [...]

Regards,

Ivan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: avcodec_tick_per_frame.patch
Type: text/x-patch
Size: 3644 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090222/1204bc93/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: avcodec_timestamp.patch
Type: text/x-patch
Size: 5607 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090222/1204bc93/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: h264_tick_per_frame.patch
Type: text/x-patch
Size: 2725 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090222/1204bc93/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: h264_timestamp.patch
Type: text/x-patch
Size: 759 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090222/1204bc93/attachment-0003.bin>