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

[Ivan Schreter]

Michael Niedermayer wrote:
> On Fri, Feb 20, 2009 at 03:30:16PM +0100, Ivan Schreter wrote:
>   
>> Michael Niedermayer wrote:
>>     
>>> On Fri, Feb 20, 2009 at 12:00:35PM +0100, Ivan Schreter wrote:
>>>   
>>>       
>>>> Michael Niedermayer wrote:
>>>>     
>>>>         
>>>>> the parser also can set timetamps, they as well are in a container based
>>>>> timebase
>>>>>   
>>>>>       
>>>>>           
>>>> Ok, then here the next patch to handle frame duration properly. Namely, for 
>>>> interlaced videos, it uses frame duration of 3600 (for 50i) instead of 
>>>> 1800, since it computes it from framerate. Since the parser can compute 
>>>> timestamps, it can also compute duration...
>>>>
>>>> Actual duration computation will follow in H.264 parser patch to compute 
>>>> timestamps (it's actually already there in old version of the patch, but 
>>>> not propagated to lavf).
>>>>
>>>> I'll clean up the patches for the parser and post them later today 
>>>> (dependent on this patch, though).
>>>>     
>>>>         
>>> this patch looks wrong, we already have a repeat_pict specifying duration
>>>
>>>   
>>>       
>> Yes, we do. Unfortunately, you can't specify half-frame duration (of 
>> display frame) for field pictures, except you'd set repeat_pict to -1 to 
>> subtract half a frame, which is possibly not what is desired. The 
>>     
>
> i see no problem
> repeat_pict=0 for a field could be duration = 1 field
> repeat_pict=0 for a frame could be duration = 2 fields
> of course repeat_pict=-1 also could be considered
>
>   
I suppose, you are mixing two fields together. We have 
AVCodecParserContext.repeat_pict and Picture.repeat_pict. First used to 
generate frame durations, second communicated with decoded frame to the 
application.

Then I'd go for AVCodecParserContext.repeat_pict == -1, since otherwise 
we'd have to add yet another field and test for it in utils.c code.

>> solution giving parser the control of frame duration seems cleaner to me.
>>     
>
> considering that your code depends on a 90khz clock which is not guranteed
> and that repeat_pict must be fixed and unambigously specified for fields
> anyway i dont see any advantage of ignoring a existing field ans introducing a
> new one
>
>   
That was also my question in another post - where do I get the clock in 
lavc?

> [...]
>   
>> BTW, current solution has anyway a problem: First of all, repeat_pict 
>> during parse time is one (transport stream) frame too late (as it comes 
>> from last decode, not from parse). Second, first parsed frame will have 
>> duration of 0, which is also incorrect (since frame rate and related 
>> stuff are not set yet, I suppose).
>>     
>
> i dont know what you talk about
>   
duration field in AVPacket. First AVPacket has duration == 0, only from 
second packet on, duration is != 0 (after some fields were filled in 
previous frame decoding). If something changes in the stream, duration 
field in AVPacket will react with delay of 1 packet.

So the decoding of AVCodecParserContext.repeat_pict in H.264 must 
actually go to the parser (as done for MPEG video).

BTW, H.264 currently only sets Picture.repeat_pict (and this one 
correctly), but no AVCodecParserContext.repeat_pict.

Regards,

Ivan