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

[Michael Niedermayer]

uOn Fri, Feb 20, 2009 at 04:00:34PM +0100, Ivan Schreter wrote:
> 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.

dont forget to document it ...


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

do you really need it?
if its just for convergence_duration, then maybe a convergence_frames
(different name and frame count instead of time) could be used.


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

i still dont understand what you talk about
frame duration for mpeg1/2 should not be delayed and
h264 is just broken ATM


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

yes


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

indeed


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090220/7cb082a0/attachment.pgp>