[FFmpeg-cvslog] r17572 - in trunk/libavcodec: mpeg12.c mpegvideo_parser.c

Michael Niedermayer michaelni
Thu Feb 26 05:17:14 CET 2009


On Wed, Feb 25, 2009 at 05:05:59PM -0800, Baptiste Coudurier wrote:
> On 2/25/2009 3:41 PM, Michael Niedermayer wrote:
> > On Wed, Feb 25, 2009 at 03:00:02PM -0800, Baptiste Coudurier wrote:
> >> On 2/25/2009 2:29 PM, Michael Niedermayer wrote:
[...]
> 
> >>>> In the later how the muxer can make the difference between 25 and 50 fps
> >>>> ? Will we get 100 ? If codec is not mpeg-2 then muxers should check for
> >>>> a different value ? This looks odd.
> >>>>
> >>>> How can the application fetch the "real" value of frame rate as stored
> >>>> in mpeg-2 essence ?
> >>>>
> >>>> I'd say add a new field to mention this information if you want, but
> >>>> keep AVCodecContext->time_base as it was before, aka value of frame rate
> >>>> as stored in mpeg-2 essence.
> >>> the API says:
> >>>     /**
> >>>      * This is the fundamental unit of time (in seconds) in terms
> >>>      * of which frame timestamps are represented. For fixed-fps content,
> >>>      * timebase should be 1/framerate and timestamp increments should be
> >>>      * identically 1.
> >>>      * - encoding: MUST be set by user.
> >>>      * - decoding: Set by libavcodec.
> >>>      */
> >>>     AVRational time_base;
> >>>
> >>> and its that way since r4536 2005-08-22, thats almost 4 years
> >>> it was incorrectly set for mpeg2 according to this definition and the
> >>> timebase was corrected to match the API
> >> Humm, how do you interpret "should" ? Because now, this is simple,
> > 
> > i interpret should as, one should do if possible, but one does not have to.
> > 
> > 
> >> time_base is, for mpeg-2, _always_ 1/(2*framerate), and increments are 2
> >> except when repeat_pict.
> >>
> >> So it seems general scenario was changed to accomodate the repeat_pict case.
> >>
> >>> what would be the alternative?
> >>> "This is the fundamental unit of time (in seconds) in terms of which frame
> >>> timestamps are represented. EXCEPT for mpeg2 where it is the value stored
> >>> in the header for historic reasons.
> >>>
> >>> what meaning does this value have then? how can the user pick a time_base
> >>> in which she can represent all timestamps accurately.
> >>> and how should we define the other fields then
> >>> i mean:
> >>>     /**\
> >>>      * presentation timestamp in time_base units (time when frame should be shown to user)\
> >>>      * If AV_NOPTS_VALUE then frame_rate = 1/time_base will be assumed.\
> >>>      * - encoding: MUST be set by user.\
> >>>      * - decoding: Set by libavcodec.\
> >>>      */\
> >>>     int64_t pts;\
> >>>
> >>> or all the new fields we need for h264. Yes h264 also uses a "x2 timebase"
> >>> exactly identical to mpeg2. Should h264 be allowed to use its true timebase
> >>> and set time_base accordingly or should it follow mpeg2s example and
> >>> use 1/25 if later all fields we need for the h264 timestamp generation will
> >>> need something else (that is the true timebase)
> >> I don't know about H.264, but MPEG-2 does not specify any timebase,
> >> strictly speaking, according to specs.
> > 
> > what are you trying to say?
> > mpeg2 might not
> > specify something with the word timebase but it surely specifies the time
> > when to display a frame. And given that time 1/25 is not a valid timebase.
> > (to take the 1/25 vs. 1/50 example here)
> 
> Well, MPEG-2 Video (13818-2) does not specifies the time when to display
> a frame, 

GOP timecodes do specify a time


> it does specify how the decoder outputs fields/frames, and the
> period. 

specifying the durations is equivalent to specifying the times


> Our decoder period is always 1/frame_rate because we do not
> output fields, if we did period would be 1/(frame_rate*2), I agree.

MPEG2 decoder period is specifed in the spec, we follow it and thus
cannot output always at exactly 1/frame_rate


> 
> 13818-1 specifies the time when to decode and display a frame, through
> PTS and DTS.
> 
> MPEG-2 does not have a "pts" information in bitstream, MPEG-4 IIRC, and
> H.264 through SEI, right ?

no

its after 5 in the morning here and instead of sleeping and instead of
fixing this bug, instead of working for the release i explain you what
you could read in the spec
you dont want it to be as it is, but it is that way
h264 stores differences between timestamps in optional SEI, amongth
3 (IIRC) other choices to store one being POC, one being pict_repeat
like mpeg2 and one being fixed fps that is IIRC
which of the 4 choices is used is the encoders decission. I dont think
you will call this mess storing pts
and all this is in a timebase and this timebase is 1/50 not 1/25 like 
mpeg2


> In these case, it may make sense, but I don't
> agree this makes sense in MPEG-2.
> 
> >> It specifies a _frame_rate_ encoded in bitstream, and explicitely detail
> >> the behaviour with repeat_first_field and progressive_sequence.
> > 
> > yes but frame_rate is not the same as time_base
> > if you want that value add a frame_rate field to AVCodecContext and store
> > it there.
> > But time_base is the unit in which timestamps and durations are specified and
> > even if hell melts from your flames you wont be able to store telecine
> > timestamps in frame_rate units.
> 
> AVFrame does not have a duration field, I don't see how this is relevant
> to codecs and AVCodecContext, this is IMHO a problem dealing with
> containers and AVPacket.
> 

> AVFrames has "pts" information for some codecs, and in this case the
> value should be set to what is stored in bitstream.

this requires "1/50" timebase first


> 
> >> AFAIK, I may be wrong, but 13818-1 does not mention anything about
> >> interpolating timestamps in the repeat_field_field case. Furthermore, in
> > 
> > if you think you dont need repeat_field_field for finding the times at which
> > to display frames then please implement that.
> 
> How are pts and dts stored in PS/TS when repeat_pict is used ?

see the spec, its written there


> 
> Does the duration between dts differs from 1/(mpeg2 frame rate) ?

dts are stored once every 0.7 sec or less, their value is in twice the
framerate (+- some allowed clock drift)


> If
> not, then we should not change it and set it to what is in the
> container, if information is not present, you should interpolate using
> repeat_pict, but you will adjust duration in AVStream->time_base, not
> AVCodecContext->time_base.

repeat_pict is in AVCodecContext->time_base units


> 
> > [...]
> > 
> >>> And yes i agree that there is a problem currently, i just dont think that
> >>> keeping time_base at a incorrect value because that was done and happens
> >>> to be convenient is the right solution.
> >>> Its very easy to add a new field to represent if the timebase is likely
> >>> 2x the frame rate or not.
> >>>
> >> In any case IMHO this should have been done before commiting this.
> >>
> >> If you want to change API this much (I consider it a _huge_ change, now
> >> basically all mpeg-2 is 1/50, 1001/60000 and 720p is now 1/100 and
> >> 1001/120000 !!!!!), MAJOR version should be bumped.
> > 
> > bumping the major ver is easy if you want ...
> 
> Well I have no problem with it, users have problems.

Users dont flame me, you do.


> 
> >> This broke all applications using AVCodecContext->time_base as way to
> >> check frame rate. This may be wrong according to API, but there was no
> >> other way to do this.
> > 
> > i know and i am not happy about this, what we need are solutions not
> > flames.
> > 
> > iam not excluding any specific solution that works so your flames are
> > directed the wrong way, iam not rejecting some solution here ...
> 
> I'm not happy because change was made without considering all
> implications and broke work I spent some time on, and which I care about.

Please, let me ask you again, could we concentrate on SOLVING the
problem instead of this flame

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

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- 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-cvslog/attachments/20090226/2b216fad/attachment.pgp>



More information about the ffmpeg-cvslog mailing list