[FFmpeg-devel] [PATCH] avformat/matroskaenc: Accept time base hint

[Michael Bradshaw]

On Sat, Jan 14, 2017 at 3:57 AM, Michael Niedermayer <michaelni at gmx.at>
wrote:

> On Tue, Dec 27, 2016 at 09:47:47PM -0800, Michael Bradshaw wrote:
> > +    // ms precision is the de-facto standard timescale for mkv files
> > +    mkv->timecode_scale = 1000000;
> > +
> > +    // If the user has supplied a desired time base, use it if possible
> > +    if (s->nb_streams > 0) {
> > +      AVRational time_base = s->streams[0]->time_base;
> > +      for (i = 1; i < s->nb_streams; i++) {
>
> inconsistent indention
>

Fixed.

> +          if (time_base.num != s->streams[i]->time_base.num ||
> > +              time_base.den != s->streams[i]->time_base.den) {
> > +            time_base = av_make_q(0, 0);
> > +            break;
> > +          }
> > +      }
>
> This looks just at one stream, what if other streams have other
> timebases ?
> you could pick a common tb so that timestamps in any of the requested
> ones can be represented. While this wont always be possible due to
> bits per int constraint i dont think completely ignoring other streams
> is safe.
>

The other streams aren't completely ignored. They're checked to make sure
they all have the same time base (and if they don't, it falls back to the
default 1/1000 time base).

> +      // Make sure the time base is valid, can losslessly be converted to
> > +      // nanoseconds, and isn't longer than 1 second
> > +      if (time_base.num > 0 &&
> > +          time_base.den > 0 &&
> > +          1000000000 % time_base.den == 0 &&
> > +          time_base.num <= time_base.den) {
>
> > +          mkv->timecode_scale = (int)av_rescale(time_base.num,
> 1000000000, time_base.den);
>
> assuming someone asks for 1001/24000 he would get something else
> and something that is lower precission than what the default would
> have been prior to this patch IIUC


Before this patch the time base was always set to 1/1000, which is what
this patch defaults to if the streams don't have a common time base that
can be losslessly converted to nanoseconds, so this patch should never make
things less precise than before.

I have mixed feelings on changing this patch so that it picks a common time
base between multiple streams or truncates a repeating decimal. Matroska
only stores timestamps as multiples of nanoseconds, and I fear
automatically computing a time base might push the time base closer and
closer to 1/1000000000. Using a tiny timebase like that causes extra
chunking in Matroska, as each block in a Cluster only stores its timestamp
as a 16-bit int.

I'd rather my other patch[1] be used to let the user request a specific
time base than trying to auto-compute a time base.

I've updated the patch to fix the indentation as well as ignore the time
base hint for WebM, since WebM muxers are required to use a time base of
1/1000[2]. Apologies for the gmail attachment; I'm still figuring out git
send-email.

[1]: http://ffmpeg.org/pipermail/ffmpeg-devel/2016-December/205017.html
[2]: http://www.webmproject.org/docs/container/#muxer-guidelines
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avformat-matroskaenc-Accept-time-base-hint.patch
Type: application/octet-stream
Size: 5142 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170114/50c6c32f/attachment.obj>