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

Submitted by Michael Bradshaw on Dec. 28, 2016, 5:47 a.m.

Details

Message ID 20161228054747.40683-2-mjbshaw@gmail.com
State New
Headers show

Commit Message

Michael Bradshaw Dec. 28, 2016, 5:47 a.m.
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(-)

Comments

Michael Niedermayer Jan. 14, 2017, 11:57 a.m.
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

[...]
Michael Bradshaw Jan. 14, 2017, 6:11 p.m.
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

Patch hide | download patch | download mbox

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++) {
+          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;
+          }
+      }
+      // 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);
+      }
+    }
+
     for (i = 0; i < s->nb_streams; i++) {
-        // ms precision is the de-facto standard timescale for mkv files
-        avpriv_set_pts_info(s->streams[i], 64, 1, 1000);
+        avpriv_set_pts_info(s->streams[i], 64, mkv->timecode_scale, 1000000000);
     }
 
     return 0;