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

From: Michael Bradshaw <mjbshaw@google.com>

Signed-off-by: Michael Bradshaw <mjbshaw@google.com>
---
 libavformat/matroskaenc.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

On Tue, Dec 27, 2016 at 09:47:47PM -0800, Michael Bradshaw wrote:
> From: Michael Bradshaw <mjbshaw@google.com>
> 
> Signed-off-by: Michael Bradshaw <mjbshaw@google.com>
> ---
>  libavformat/matroskaenc.c | 38 +++++++++++++++++++++++++++++++-------
>  1 file changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 827d755..2c2c930 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -117,6 +117,7 @@ typedef struct mkv_attachments {
>  typedef struct MatroskaMuxContext {
>      const AVClass  *class;
>      int             mode;
> +    int             timecode_scale;
>      AVIOContext   *dyn_bc;
>      AVIOContext     *tags_bc;
>      ebml_master     tags;
> @@ -1757,7 +1758,7 @@ static int mkv_write_header(AVFormatContext *s)
>          return ret;
>      pb = mkv->info_bc;
>  
> -    put_ebml_uint(pb, MATROSKA_ID_TIMECODESCALE, 1000000);
> +    put_ebml_uint(pb, MATROSKA_ID_TIMECODESCALE, mkv->timecode_scale);
>      if ((tag = av_dict_get(s->metadata, "title", NULL, 0)))
>          put_ebml_string(pb, MATROSKA_ID_TITLE, tag->value);
>      if (!(s->flags & AVFMT_FLAG_BITEXACT)) {
> @@ -1799,11 +1800,11 @@ static int mkv_write_header(AVFormatContext *s)
>          int64_t metadata_duration = get_metadata_duration(s);
>  
>          if (s->duration > 0) {
> -            int64_t scaledDuration = av_rescale(s->duration, 1000, AV_TIME_BASE);
> +            int64_t scaledDuration = av_rescale(s->duration, 1000000000, AV_TIME_BASE * (int64_t)mkv->timecode_scale);
>              put_ebml_float(pb, MATROSKA_ID_DURATION, scaledDuration);
>              av_log(s, AV_LOG_DEBUG, "Write early duration from recording time = %" PRIu64 "\n", scaledDuration);
>          } else if (metadata_duration > 0) {
> -            int64_t scaledDuration = av_rescale(metadata_duration, 1000, AV_TIME_BASE);
> +            int64_t scaledDuration = av_rescale(metadata_duration, 1000000000, AV_TIME_BASE * (int64_t)mkv->timecode_scale);
>              put_ebml_float(pb, MATROSKA_ID_DURATION, scaledDuration);
>              av_log(s, AV_LOG_DEBUG, "Write early duration from metadata = %" PRIu64 "\n", scaledDuration);
>          } else {
> @@ -1864,12 +1865,12 @@ static int mkv_write_header(AVFormatContext *s)
>      // after 4k and on a keyframe
>      if (pb->seekable) {
>          if (mkv->cluster_time_limit < 0)
> -            mkv->cluster_time_limit = 5000;
> +            mkv->cluster_time_limit = av_rescale(5, 1000000000, mkv->timecode_scale);
>          if (mkv->cluster_size_limit < 0)
>              mkv->cluster_size_limit = 5 * 1024 * 1024;
>      } else {
>          if (mkv->cluster_time_limit < 0)
> -            mkv->cluster_time_limit = 1000;
> +            mkv->cluster_time_limit = 1000000000 / mkv->timecode_scale;
>          if (mkv->cluster_size_limit < 0)
>              mkv->cluster_size_limit = 32 * 1024;
>      }
> @@ -2458,6 +2459,7 @@ static int mkv_query_codec(enum AVCodecID codec_id, int std_compliance)
>  
>  static int mkv_init(struct AVFormatContext *s)
>  {
> +    MatroskaMuxContext *mkv = s->priv_data;
>      int i;
>  
>      if (s->avoid_negative_ts < 0) {
> @@ -2465,9 +2467,31 @@ static int mkv_init(struct AVFormatContext *s)
>          s->internal->avoid_negative_ts_use_pts = 1;
>      }
>  
> +    // 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


> +          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.


> +      // 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

[...]

On Sat, Jan 14, 2017 at 3:57 AM, Michael Niedermayer <michaelni@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