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

[Michael Niedermayer]

On Sat, Feb 21, 2009 at 12:15:11AM +0100, Ivan Schreter wrote:
> Michael Niedermayer wrote:
> > On Fri, Feb 20, 2009 at 09:42:58PM +0100, Ivan Schreter wrote:
> >   
> >> Michael Niedermayer wrote:
> >>     
> >>> On Fri, Feb 20, 2009 at 08:46:29PM +0100, Ivan Schreter wrote:
> >>>   
> >>>       
> >>>> Michael Niedermayer wrote:
> >>>>     
> >>>>         
> >>>>> On Fri, Feb 20, 2009 at 06:52:30PM +0100, Ivan Schreter wrote:
> >>>>>         
> >>>>>           
> >>>>>> Michael Niedermayer wrote:
> >>>>>>             
> >>>>>>             
> >>>>>>> On 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:
> >>>>>>>>>                       
> >>>>>>>>>                   
> >>>>>>>> That was also my question in another post - where do I get the clock 
> >>>>>>>> in lavc?
> >>>>>>>>                         
> >>>>>>>>                 
> >>>>>>> do you really need it?
> >>>>>>>                   
> >>>>>>>               
> >>>>>> Not for convergence, since we can express it via frame count. But for 
> >>>>>> pts/dts computation, definitely. For H.264 case, the stream must have 
> >>>>>> 90kHz clock according to H.264 standard, so it should be OK. But for 
> >>>>>> other codecs, I don't know.
> >>>>>>
> >>>>>> Alternative would be to scale pts/dts to some common unit (e.g., 
> >>>>>> 1/1000000th second) and then rescale back.
> >>>>>>             
> >>>>>>             
> >>>>> the container timebase in which pts/dts are specified is not
> >>>>> guranteed to be 90khz, even if h264 says it is so, it just isnt in 
> >>>>> practice.
> >>>>> things may work out because only mpeg-ps/ts ommit pts/dts "randomly" and
> >>>>> mpeg-ps/ts have 90khz but .mp4/mov/nut/mkv do normally not use 90khz 
> >>>>> timebase
> >>>>>
> >>>>>         
> >>>>>           
> >>>> True, I downloaded some MOV AVCHD trailers and they don't work with 
> >>>> timestamping code correctly. Seem to use 1MHz clock, or so.
> >>>>
> >>>>     
> >>>>         
> >>>>> also rounding is not ok
> >>>>>
> >>>>>         
> >>>>>           
> >>>> We'll need to communicate proper timebase for timestamping from lavf to 
> >>>> lavc in AVCodecParserContext, then, so it can compute timestamps 
> >>>> correctly.
> >>>>     
> >>>>         
> >>> hmm i think the parser should export its dts/pts correction in 
> >>> AVCodecContext.time_base units (i think this is the correct one)
> >>> and the code in lavf should then merge that into its dts/pts
> >>>
> >>>   
> >>>       
> >> The problem is, the timestamps are not integer multiples of time_base. Not 
> >> even the difference between them is an integer multiple - for interlaced 
> >> field picture movies, the timestamps of second field are offset by 1/2 
> >> frame duration. So integer is not sufficient there. Furthermore, current 
> >> context already gets "real" pts/dts from lavf, which only needs to be 
> >> adjusted.
> >>
> >>     
> >
> >   
> >> BTW, AVCodecContext.time_base is something like 1/25 (i.e., frame rate).
> >>     
> >
> > 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


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

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.

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

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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/822ac981/attachment.pgp>