[FFmpeg-cvslog] r17572 - in trunk/libavcodec: mpeg12.c mpegvideo_parser.c
Baptiste Coudurier
baptiste.coudurier
Thu Feb 26 09:19:51 CET 2009
Michael Niedermayer wrote:
> 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
GOP timecode plays no part in the decoding process. Besides timecode
does not reflect a timestamp because it cannot reflect 1001/* timebases.
>> 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
However, we do actually output a whole frame, so 2 fields each time.
2/(2*frame_rate) is 1/frame_rate.
I still don't get why you don't want to use time_base as frame_rate
since it was used this way for a very long time, and treat repeat_pict
after decoding.
I bet nobody ever used AVCodecContext->time_base for anything other than
a way to retrieve frame rate.
Furthermore, you entangle lavc in lavf even more doing this.
>> 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
I'm usually up to 5/6 am on weekends to catch-up patches, and work on
FFmpeg, and I even take some time answering in ffmpeg-user and
libav-user. Do I complain ?
I could spend 2 hours reading fucking bloody H.264 specs, getting even
more mad, this is true.
> 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
>
>>>> 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
These are the only references:
When trick_mode status is false, the number of times N, a picture is
output by the decoding process for progressive
sequences, is specified for each picture by the repeat_first_field and
top_field_first fields in the case of ITU-T
Rec. H.262 | ISO/IEC 13818-2 Video, and is specified through the
sequence header in the case of ISO/IEC 11172-2
Video.
For interlaced sequences, when trick_mode status is false, the number of
times N, a picture is output by the decoding
process for progressive sequences, is specified for each picture by the
repeat_first_field and progressive_frame fields in
the case of ITU-T Rec. H.262 | ISO/IEC 13818-2 Video.
When trick mode status is true, the number of times that a picture shall
be displayed depends on the value of N.
Dealing with trick mode is another story.
>>>>> [...]
>>>>>
>>>>> 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.
I flame when something is broken, and indeed I usually find out that
something broke pretty quickly because I actually _use_ ffmpeg much.
>>>> 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
Here are some problems:
ffmpeg.c:
if (ist->st->codec->time_base.num != 0) {
ist->next_pts += ((int64_t)AV_TIME_BASE *
ist->st->codec->time_base.num) /
ist->st->codec->time_base.den;
}
This is no more correct.
There is an issue in compute_pkt_fields2 when muxing:
if(st->time_base.num*1000LL > st->time_base.den){
*pnum = st->time_base.num;
*pden = st->time_base.den;
}else if(st->codec->time_base.num*1000LL > st->codec->time_base.den){
*pnum = st->codec->time_base.num;
*pden = st->codec->time_base.den;
[...]
compute_frame_duration(&num, &den, st, NULL, pkt);
if (den && num) {
pkt->duration = av_rescale(1, num * (int64_t)st->time_base.den,
den * (int64_t)st->time_base.num);
st->codec->time_base.den is now *2 but pkt->duration will be 1,
this is wrong.
This affects all containers using st->codec->time_base.den as timebase.
And finally there is no way to retrieve mpeg-2 frame rate from the
bitstream.
--
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