[FFmpeg-cvslog] VC1testenc: convert pts values to correct time-base.

[Baptiste Coudurier]

On 1/29/11 4:44 AM, Reimar D?ffinger wrote:
> On Sat, Jan 29, 2011 at 04:13:51AM -0800, Baptiste Coudurier wrote:
>> On 1/29/11 4:08 AM, Reimar D?ffinger wrote:
>>> On Sat, Jan 29, 2011 at 01:05:36PM +0100, Reimar D?ffinger wrote:
>>>> On Sat, Jan 29, 2011 at 03:47:49AM -0800, Baptiste Coudurier wrote:
>>>>> On 1/29/11 3:01 AM, Reimar D?ffinger wrote:
>>>>>> ffmpeg | branch: master | Reimar D?ffinger <Reimar.Doeffinger at gmx.de> | Sat Jan 29 11:56:25 2011 +0100| [76c802e989b61423c1554cf204f96f70b3edb145] | committer: Reimar D?ffinger
>>>>>>
>>>>>> VC1testenc: convert pts values to correct time-base.
>>>>>>
>>>>>> VC1 test container always uses time-base 1 ms, so we must convert
>>>>>> from whatever time-base the application gave us to that, otherwise
>>>>>> the video will play at ridiculous speeds.
>>>>>> It would be possible to signal that a container supports only one
>>>>>> time-base and have code in a layer above do the conversion, but
>>>>>> for a single format this seems over-engineered.
>>>>>>
>>>>>>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=76c802e989b61423c1554cf204f96f70b3edb145
>>>>>> ---
>>>>>>
>>>>>>  libavformat/vc1testenc.c |    5 ++++-
>>>>>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/libavformat/vc1testenc.c b/libavformat/vc1testenc.c
>>>>>> index 507b332..06431da 100644
>>>>>> --- a/libavformat/vc1testenc.c
>>>>>> +++ b/libavformat/vc1testenc.c
>>>>>> @@ -55,11 +55,14 @@ static int vc1test_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>>>>  {
>>>>>>      RCVContext *ctx = s->priv_data;
>>>>>>      ByteIOContext *pb = s->pb;
>>>>>> +    uint32_t pts = av_rescale(pkt->pts,
>>>>>> +                              1000 * (uint64_t)s->streams[0]->time_base.num,
>>>>>> +                              s->streams[0]->time_base.den);
>>>>>
>>>>> This should not be needed if av_set_pts_info is correctly used in
>>>>> write_header.
>>>>
>>>> That means that an application must re-check the time-base after
>>>> av_write_header, that is definitely _not_ documented, or am I
>>>> missing something?
>>>
>>> And I forgot to say that this just pushes the need for conversion
>>> onto the libavformat user, I am not exactly fond of that.
>>
>> This is how it works currently, and since a long time.
>>
>> The user specify the time base using codec->time_base, and the container
>> uses it.
>>
>> Some containers only support one time base (mpeg ps/ts for example
>> 1/90000), so they can't use the one specified.
>>
>> If you have timestamps problems, something is wrong somewhere else, but
>> this hunk should not needed, no muxer do this currently.
> 
> I apologize, I read some other muxers and thought this was supposed
> to be done like this, and nobody spoke up when I claimed that during the first
> review.
> I am not completely wrong actually, gxfenc does do this for the audio stream,
> however all others use rescaling only for otehr things (mostly comparing/converting
> per-stream vs. per-file timestamps).

yes, gxfenc sets the audio timebase to the sample rate to get accurate
timestamps coming in and proceed with needed rounding when rescaling to
the gxf timebase, but gxf requires special audio framing, like mxf, so
it's a bit special IMHO.

> Does the below change look correct?
> 
> diff --git a/libavformat/vc1testenc.c b/libavformat/vc1testenc.c
> index 06431da..3457dbd 100644
> --- a/libavformat/vc1testenc.c
> +++ b/libavformat/vc1testenc.c
> @@ -47,6 +47,7 @@ static int vc1test_write_header(AVFormatContext *s)
>          put_le32(pb, s->streams[0]->r_frame_rate.den);
>      else
>          put_le32(pb, 0xFFFFFFFF); //variable framerate
> +    av_set_pts_info(s->streams[0], 32, 1, 1000);
>  
>      return 0;
>  }
> @@ -55,9 +56,6 @@ static int vc1test_write_packet(AVFormatContext *s, AVPacket *pkt)
>  {
>      RCVContext *ctx = s->priv_data;
>      ByteIOContext *pb = s->pb;
> -    uint32_t pts = av_rescale(pkt->pts,
> -                              1000 * (uint64_t)s->streams[0]->time_base.num,
> -                              s->streams[0]->time_base.den);
>  
>      if (!pkt->size)
>          return 0;

Yes, this looks much better.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org