[FFmpeg-cvslog] r17572 - in trunk/libavcodec: mpeg12.c mpegvideo_parser.c
Michael Niedermayer
michaelni
Wed Feb 25 23:29:35 CET 2009
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
>
> Now every mpeg2 video codec spits:
> Seems stream 0 codec frame rate differs from container frame rate: 50.00
> (50/1) -> 25.00 (25/1)
>
> This is really annoying.
this is easy to fix, its a matter of changing the level at which this is
printed
>
> Also when stream copying muxer gets 1/50 as timebase, this literally
> broke muxers checking codec time base for parameters (MXF).
:(
>
> 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
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)
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.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/20090225/27b8069a/attachment.pgp>
More information about the ffmpeg-cvslog
mailing list