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

[Michael Niedermayer]

On Sat, Feb 21, 2009 at 01:15:46PM +0100, Ivan Schreter wrote:
> 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.

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


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

not the lavf pts no.
the AVFrame.pts are independant of the demuxer AVPacket.pts


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

exporting
dts_last_dts_delta (cpb_removial_delay)
pts_dts_delta      (dpb_output_delay)
from the parser
seems nicer


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


> 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

this is not ok
for now just using AV_NOPTS_VALUE until a dts/pts is found should do, 
later, once things are in svn we could try to use
update_initial_timestamps / durations to fill in the timestamps after
seeking

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

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/20090221/d57a621e/attachment.pgp>