[FFmpeg-cvslog] r17572 - in trunk/libavcodec: mpeg12.c mpegvideo_parser.c
Baptiste Coudurier
baptiste.coudurier
Thu Feb 26 00:00:02 CET 2009
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
>> Does repeat_pict is equal to repeat_first_field ? Because in this case,
>> name is not appropriate and should be changed.
>
> no
>
>
>> Also when progressive sequence is set, whole frame must be duplicated
>> per specs. Is this taken into account ?
>
> yes
I don't think so, progressive sequence is ignored and time base is / 2
in all cases, which is wrong.
> [...]
>
>> 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 ?
>> 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,
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.
It specifies a _frame_rate_ encoded in bitstream, and explicitely detail
the behaviour with repeat_first_field and progressive_sequence.
AFAIK, I may be wrong, but 13818-1 does not mention anything about
interpolating timestamps in the repeat_field_field case. Furthermore, in
trick mode, specs mention that decoders cannot rely on repeat_first_field.
Quote iso 13818-2:
"If progressive_sequence is '1' the period between two successive frames
at the output of the decoding process is the
reciprocal of the frame_rate. See Figure 7-18.
If progressive_sequence is '0' the period between two successive fields
at the output of the decoding process is half of the
reciprocal of the frame_rate. See Figure 7-20."
IMHO Since we are always outputting frames in our decoder, the period
is always 1/frame_rate, whatever value repeat_first_field has.
> 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.
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.
--
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