[FFmpeg-cvslog] r17572 - in trunk/libavcodec: mpeg12.c mpegvideo_parser.c
Michael Niedermayer
michaelni
Thu Feb 26 00:41:28 CET 2009
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
> >> 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.
i thoght what you meant, is repeat_pict set correctly when progressive
sequence is set.
repeat_pict is set correctly i belive
the time_base though is not set depending on progressive sequence because
that would mean that we could no longer play streams where that bit changes
>
> > [...]
> >
> >> 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.
>
> >> 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)
>
> 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.
>
> 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.
> 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.
you know this does not work, we output frames but their duration is not
in units of the framerate from the mpeg headers
>
> > 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 ...
>
> 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 ...
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.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I wish the Xiph folks would stop pretending they've got something they
do not. Somehow I fear this will remain a wish. -- M?ns Rullg?rd
-------------- 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/8a3f0998/attachment.pgp>
More information about the ffmpeg-cvslog
mailing list