[FFmpeg-cvslog] r17572 - in trunk/libavcodec: mpeg12.c mpegvideo_parser.c
Baptiste Coudurier
baptiste.coudurier
Thu Feb 26 02:05:59 CET 2009
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:
>>> On Wed, Feb 25, 2009 at 12:46:14PM -0800, Baptiste Coudurier wrote:
>>>> On 2/25/2009 11:59 AM, Michael Niedermayer wrote:
>>>>> On Wed, Feb 25, 2009 at 11:24:10AM -0800, Baptiste Coudurier wrote:
>>>>>> On 2/25/2009 11:08 AM, Michael Niedermayer wrote:
>>>>>>> On Wed, Feb 25, 2009 at 10:28:11AM -0800, Baptiste Coudurier wrote:
>>>>>>>> On 2/25/2009 2:52 AM, Michael Niedermayer wrote:
>>>>>>>>> On Wed, Feb 25, 2009 at 01:06:02AM -0800, Baptiste Coudurier wrote:
>>>>>>>>>> On 2/24/2009 12:23 PM, cehoyos wrote:
>>>>>>>>>>> Author: cehoyos
>>>>>>>>>>> Date: Tue Feb 24 21:23:19 2009
>>>>>>>>>>> New Revision: 17572
>>>>>>>>>>>
>>>>>>>>>>> Log:
>>>>>>>>>>> Correct time_base and repeat_pict for MPEG2 video.
>>>>>>>>>>>
>>>>>>>>>>> Patch by Ivan Schreter, schreter gmx net
>>>>>>>>>>>
>>>>>>>>>>> Modified:
>>>>>>>>>>> trunk/libavcodec/mpeg12.c
>>>>>>>>>>> trunk/libavcodec/mpegvideo_parser.c
>>>>>>>>>>>
>>>>>>>>>>> Modified: trunk/libavcodec/mpeg12.c
>>>>>>>>>>> ==============================================================================
>>>>>>>>>>> --- trunk/libavcodec/mpeg12.c Tue Feb 24 21:19:59 2009 (r17571)
>>>>>>>>>>> +++ trunk/libavcodec/mpeg12.c Tue Feb 24 21:23:19 2009 (r17572)
>>>>>>>>>>> @@ -1275,7 +1275,7 @@ static int mpeg_decode_postinit(AVCodecC
>>>>>>>>>>> av_reduce(
>>>>>>>>>>> &s->avctx->time_base.den,
>>>>>>>>>>> &s->avctx->time_base.num,
>>>>>>>>>>> - ff_frame_rate_tab[s->frame_rate_index].num * s1->frame_rate_ext.num,
>>>>>>>>>>> + ff_frame_rate_tab[s->frame_rate_index].num * s1->frame_rate_ext.num*2,
>>>>>>>>>>> ff_frame_rate_tab[s->frame_rate_index].den * s1->frame_rate_ext.den,
>>>>>>>>>>> 1<<30);
>>>>>>>>>>> //MPEG-2 aspect
>>>>>>>>>>>
>>>>>>>>>>> Modified: trunk/libavcodec/mpegvideo_parser.c
>>>>>>>>>>> ==============================================================================
>>>>>>>>>>> --- trunk/libavcodec/mpegvideo_parser.c Tue Feb 24 21:19:59 2009 (r17571)
>>>>>>>>>>> +++ trunk/libavcodec/mpegvideo_parser.c Tue Feb 24 21:23:19 2009 (r17572)
>>>>>>>>>>> @@ -81,7 +81,7 @@ static void mpegvideo_extract_headers(AV
>>>>>>>>>>> pc->height |=( vert_size_ext << 12);
>>>>>>>>>>> avctx->bit_rate += (bit_rate_ext << 18) * 400;
>>>>>>>>>>> avcodec_set_dimensions(avctx, pc->width, pc->height);
>>>>>>>>>>> - avctx->time_base.den = pc->frame_rate.den * (frame_rate_ext_n + 1);
>>>>>>>>>>> + avctx->time_base.den = pc->frame_rate.den * (frame_rate_ext_n + 1) * 2;
>>>>>>>>>>> avctx->time_base.num = pc->frame_rate.num * (frame_rate_ext_d + 1);
>>>>>>>>>>> avctx->codec_id = CODEC_ID_MPEG2VIDEO;
>>>>>>>>>>> avctx->sub_id = 2; /* forces MPEG2 */
>>>>>>>>>> I'm pretty sure this commit broke codec frame rate parsing for MPEG-2 in
>>>>>>>>>> MXF.
>>>>>>>>>>
>>>>>>>>>> Seems stream 0 codec frame rate differs from container frame rate: 50.00
>>>>>>>>>> (50/1) -> 25.00 (25/1)
>>>>>>>>>> Input #0, mxf, from 'test.mxf':
>>>>>>>>>> Duration: 00:00:18.20, start: 0.000000, bitrate: 2086 kb/s
>>>>>>>>>> Stream #0.0: Video: mpeg2video, yuv420p, 320x240 [PAR 1:1 DAR 4:3],
>>>>>>>>>> 104857 kb/s, 25 tbr, 25 tbn, 50 tbc
>>>>>>>>>> Stream #0.1: Audio: pcm_s16le, 48000 Hz, stereo, s16, 1536 kb/s
>>>>>>>>>> At least one output file must be specified
>>>>>>>>> elaborate on what is wrong please
>>>>>>>> 50 tbc is wrong, should be 25.
>>>>>>>>
>>>>>>>>>> Worked fine before. You can just create a .mxf with FFmpeg:
>>>>>>>>>> ffmpeg -i <file> -ar 48000 test.mxf
>>>>>>>>>>
>>>>>>>>>> If I remove "* 2" everything is fine again.
>>>>>>>>> cant be, durations (and thus timestamps) in mpeg2 are specified in
>>>>>>>>> half framerate units thus timebase has to be half of 1/framerate.
>>>>>>>> Well, if I remove * 2 it is 25 again.
>>>>>>>>
>>>>>>>> Can you please explain how can durations and timestamps be specified in
>>>>>>>> half framerate ? Do you mean when it is coded as separate fields ?
>>>>>>>> Here one frame at 25 in PS/TS has 3600 duration, this is not half frame
>>>>>>>> rate.
>>>>>>> frames can be output as 3 fields, its done in telecine and its generally
>>>>>>> progressive not interlaced material.
>>>>>>> To specify the stored durations exacly a 1/50 timebase is needed.
>>>>>>> And we cant change the timebase once telecine is encountered and it could
>>>>>>> be later in the movie ...
>>>>>> I think repeat_pict is related to decoding, not demuxing. Besides, a
>>>>>> decoder can choose to ignore repeat_pict in the case of telecine, to
>>>>>> output progressive material. So if decoder decides to honor repeat pict,
>>>>>> it will do after decoding, right ?
>>>>> our decoder doesnt really honor repeat_pict beyond exporting its value.
>>>>>
>>>>>
>>>>>> Duration will depend on what the decoder will do, libavformat cannot
>>>>>> predict anything in this case.
>>>>> in mpeg-ps and mpeg-ts you have a timetamp once every 0.7 second assuming
>>>>> there was no packet loss hitting that timestamp.
>>>>> the only way to know the pts/dts/when to display the frames between is
>>>>> repeat_pict
>>>>> In that sense repeat_pict is relevant for decoding & demuxing.
>>>>>
>>>>> and ignoring repeat_pict entirely does not work because that way neither
>>>>> demuxer nor decoder would have timestamps, if the timestamps where taken
>>>>> tb or 2*tb apart it would fail badly for telecine that has a value between
>>>>> for the average
>>>
>>> [...]
>>>
>>>> Also when stream copying muxer gets 1/50 as timebase, this literally
>>>> broke muxers checking codec time base for parameters (MXF).
>>> :(
>>>
>> What should we do ?
>
> id say, fix code that depends on time_base being equal to some header value
> that is not always a valid time_base.
Ok, I will parse the bitstream to fetch frame rate.
>>>> 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, it does specify how the decoder outputs fields/frames, and the
period. 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.
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 ? 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.
>> 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 ?
Does the duration between dts differs from 1/(mpeg2 frame rate) ? 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.
> [...]
>
>>> 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.
>> 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.
> We also can revert all the changes and forget h264 timestamp support
> for the release or try to hack things up with wrong timebases.
> what we cannot do (and what i fiercly object) is pretend that the
> "mpeg2 header framerate" is a valid timebase.
>
> So if you want we can revert the timebase changes and redefine all variables
> using the timebase to rather use timebase/2 for mpeg2 & h264. Though i
> dont volunteer to implement or maintain that.
Why do feel the need to threaten about maintainership ?
IMHO this is not a constructive approach. This commit broke FFmpeg
behaviour in some cases, I find it legitimate to ensure that everything
is still working fine before applying. If we don't following this rule,
everything should be broken in a near future.
--
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA
Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer http://www.ffmpeg.org
More information about the ffmpeg-cvslog
mailing list