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

[Reimar Döffinger]

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).
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;