Browse Source

avformat/matroskaenc: Don't ignore tags of chapters written late

The Matroska muxer writes the Chapters early when chapters were already
available when writing the header; in this case any tags pertaining to
these chapters get written, too.

Yet if no chapters had been supplied before writing the header, Chapters
can also be written when writing the trailer if any are supplied. Tags
belonging to these chapters were up until now completely ignored.

This commit changes this: Writing the tags belonging to chapters has
been moved to mkv_write_chapters(). If mkv_write_tags() has not been
called yet (i.e. when chapters are written when writing the header),
the AVIOContext for writing the ordinary Tags element is used, but not
output, as this is left to mkv_write_tags() in order to only write one
Tags element. Yet if mkv_write_tags() has already been called,
mkv_write_chapters() will output a Tags element of its own which only
contains the tags for chapters.

When chapters are available initially, the corresponding tags will now
be the first tags in the Tags element; but the ordering of tags in Tags
is irrelevant anyway.

This commit also makes chapter_id_offset local to mkv_write_chapters()
as it is used only there and not reused at all.

Potentially writing a second Tags element means that the maximum number
of SeekHead entries had to be incremented. All the changes to FATE
result from the ensuing increase in the amount of space reserved for the
SeekHead (21 bytes more).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
Andreas Rheinhardt 4 years ago
parent
commit
4ebfc13c33

+ 43 - 27
libavformat/matroskaenc.c

@@ -55,8 +55,8 @@
 #include "libavcodec/mpeg4audio.h"
 
 /* Level 1 elements we create a SeekHead entry for:
- * Info, Tracks, Chapters, Attachments, Tags and Cues */
-#define MAX_SEEKHEAD_ENTRIES 6
+ * Info, Tracks, Chapters, Attachments, Tags (potentially twice) and Cues */
+#define MAX_SEEKHEAD_ENTRIES 7
 
 #define IS_SEEKABLE(pb, mkv) (((pb)->seekable & AVIO_SEEKABLE_NORMAL) && \
                               !(mkv)->is_live)
@@ -142,8 +142,8 @@ typedef struct MatroskaMuxContext {
     unsigned            nb_attachments;
     int                 have_video;
 
-    uint32_t            chapter_id_offset;
     int                 wrote_chapters;
+    int                 wrote_tags;
 
     int                 reserve_cues_space;
     int                 cluster_size_limit;
@@ -1546,6 +1546,8 @@ static int mkv_write_tags(AVFormatContext *s)
     ebml_master tag, *tagp = IS_SEEKABLE(s->pb, mkv) ? &tag : NULL;
     int i, ret;
 
+    mkv->wrote_tags = 1;
+
     ff_metadata_conv_ctx(s, ff_mkv_metadata_conv, NULL);
 
     if (mkv_check_tag(s->metadata, 0)) {
@@ -1586,21 +1588,6 @@ static int mkv_write_tags(AVFormatContext *s)
         }
     }
 
-    if (mkv->mode != MODE_WEBM) {
-        for (i = 0; i < s->nb_chapters; i++) {
-            AVChapter *ch = s->chapters[i];
-
-            if (!mkv_check_tag(ch->metadata, MATROSKA_ID_TAGTARGETS_CHAPTERUID))
-                continue;
-
-            ret = mkv_write_tag(mkv, ch->metadata, &mkv->tags.bc, NULL,
-                                MATROSKA_ID_TAGTARGETS_CHAPTERUID,
-                                (uint32_t)ch->id + (uint64_t)mkv->chapter_id_offset);
-            if (ret < 0)
-                return ret;
-        }
-    }
-
     if (mkv->nb_attachments && mkv->mode != MODE_WEBM) {
         for (i = 0; i < s->nb_streams; i++) {
             const mkv_track *track = &mkv->tracks[i];
@@ -1629,8 +1616,9 @@ static int mkv_write_tags(AVFormatContext *s)
 static int mkv_write_chapters(AVFormatContext *s)
 {
     MatroskaMuxContext *mkv = s->priv_data;
-    AVIOContext *dyn_cp = NULL, *pb = s->pb;
+    AVIOContext *dyn_cp = NULL, *dyn_tags = NULL, **tags, *pb = s->pb;
     ebml_master editionentry;
+    uint64_t chapter_id_offset = 0;
     AVRational scale = {1, 1E9};
     int i, ret;
 
@@ -1639,7 +1627,7 @@ static int mkv_write_chapters(AVFormatContext *s)
 
     for (i = 0; i < s->nb_chapters; i++)
         if (!s->chapters[i]->id) {
-            mkv->chapter_id_offset = 1;
+            chapter_id_offset = 1;
             break;
         }
 
@@ -1648,8 +1636,13 @@ static int mkv_write_chapters(AVFormatContext *s)
         return ret;
 
     editionentry = start_ebml_master(dyn_cp, MATROSKA_ID_EDITIONENTRY, 0);
-    if (mkv->mode != MODE_WEBM)
+    if (mkv->mode != MODE_WEBM) {
         put_ebml_uint(dyn_cp, MATROSKA_ID_EDITIONFLAGDEFAULT, 1);
+        /* If mkv_write_tags() has already been called, then any tags
+         * corresponding to chapters will be put into a new Tags element. */
+        tags = mkv->wrote_tags ? &dyn_tags : &mkv->tags.bc;
+    } else
+        tags = NULL;
 
     for (i = 0; i < s->nb_chapters; i++) {
         ebml_master chapteratom, chapterdisplay;
@@ -1661,13 +1654,13 @@ static int mkv_write_chapters(AVFormatContext *s)
             av_log(s, AV_LOG_ERROR,
                    "Invalid chapter start (%"PRId64") or end (%"PRId64").\n",
                    chapterstart, chapterend);
-            ffio_free_dyn_buf(&dyn_cp);
-            return AVERROR_INVALIDDATA;
+            ret = AVERROR_INVALIDDATA;
+            goto fail;
         }
 
         chapteratom = start_ebml_master(dyn_cp, MATROSKA_ID_CHAPTERATOM, 0);
         put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERUID,
-                      (uint32_t)c->id + (uint64_t)mkv->chapter_id_offset);
+                      (uint32_t)c->id + chapter_id_offset);
         put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERTIMESTART, chapterstart);
         put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERTIMEEND, chapterend);
         if ((t = av_dict_get(c->metadata, "title", NULL, 0))) {
@@ -1677,12 +1670,34 @@ static int mkv_write_chapters(AVFormatContext *s)
             end_ebml_master(dyn_cp, chapterdisplay);
         }
         end_ebml_master(dyn_cp, chapteratom);
+
+        if (tags && mkv_check_tag(c->metadata, MATROSKA_ID_TAGTARGETS_CHAPTERUID)) {
+            ret = mkv_write_tag(mkv, c->metadata, tags, NULL,
+                                MATROSKA_ID_TAGTARGETS_CHAPTERUID,
+                                (uint32_t)c->id + chapter_id_offset);
+            if (ret < 0)
+                goto fail;
+        }
     }
     end_ebml_master(dyn_cp, editionentry);
     mkv->wrote_chapters = 1;
 
-    return end_ebml_master_crc32(pb, &dyn_cp, mkv,
-                                 MATROSKA_ID_CHAPTERS, 0, 0, 1);
+    ret = end_ebml_master_crc32(pb, &dyn_cp, mkv, MATROSKA_ID_CHAPTERS, 0, 0, 1);
+    if (ret < 0)
+        goto fail;
+    if (dyn_tags)
+        return end_ebml_master_crc32(pb, &dyn_tags, mkv,
+                                     MATROSKA_ID_TAGS, 0, 0, 1);
+    return 0;
+
+fail:
+    if (tags) {
+        /* tags == &mkv->tags.bc can only happen if mkv->tags.bc was
+         * initially NULL, so we never free older tags. */
+        ffio_free_dyn_buf(tags);
+    }
+    ffio_free_dyn_buf(&dyn_cp);
+    return ret;
 }
 
 static const char *get_mimetype(const AVStream *st)
@@ -1878,7 +1893,8 @@ static int mkv_write_header(AVFormatContext *s)
             return ret;
     }
 
-    /* Must come after mkv_write_chapters() because of chapter_id_offset */
+    /* Must come after mkv_write_chapters() to write chapter tags
+     * into the same Tags element as the other tags. */
     ret = mkv_write_tags(s);
     if (ret < 0)
         return ret;

+ 1 - 1
tests/fate/matroska.mak

@@ -12,7 +12,7 @@ fate-matroska-prores-header-insertion-bz2: CMD = framecrc -i $(TARGET_SAMPLES)/m
 FATE_MATROSKA-$(call DEMMUX, MATROSKA, MATROSKA) += fate-matroska-remux
 fate-matroska-remux: CMD = md5pipe -i $(TARGET_SAMPLES)/vp9-test-vectors/vp90-2-2pass-akiyo.webm -color_trc 4 -c:v copy -fflags +bitexact -strict -2 -f matroska
 fate-matroska-remux: CMP = oneline
-fate-matroska-remux: REF = acaf96f1832264d7f2845b16dd4ab082
+fate-matroska-remux: REF = 26fabd90326e3de4bb6afe1b216ce232
 
 FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER VORBIS_PARSER) += fate-matroska-xiph-lacing
 fate-matroska-xiph-lacing: CMD = framecrc -i $(TARGET_SAMPLES)/mkv/xiph_lacing.mka -c:a copy

+ 2 - 2
tests/fate/wavpack.mak

@@ -91,12 +91,12 @@ fate-wavpack-matroskamode: CMD = md5 -i $(TARGET_SAMPLES)/wavpack/special/matros
 FATE_WAVPACK-$(call DEMMUX, WV, MATROSKA) += fate-wavpack-matroska_mux-mono
 fate-wavpack-matroska_mux-mono: CMD = md5pipe -i $(TARGET_SAMPLES)/wavpack/num_channels/mono_16bit_int.wv -c copy -fflags +bitexact -f matroska
 fate-wavpack-matroska_mux-mono: CMP = oneline
-fate-wavpack-matroska_mux-mono: REF = 5388d452e84958d204de9974bb4e3947
+fate-wavpack-matroska_mux-mono: REF = 0a10b6cd4d64c25ced75f52d1c6929a7
 
 FATE_WAVPACK-$(call DEMMUX, WV, MATROSKA) += fate-wavpack-matroska_mux-61
 fate-wavpack-matroska_mux-61: CMD = md5pipe -i $(TARGET_SAMPLES)/wavpack/num_channels/eva_2.22_6.1_16bit-partial.wv -c copy -fflags +bitexact -f matroska
 fate-wavpack-matroska_mux-61: CMP = oneline
-fate-wavpack-matroska_mux-61: REF = c28c51910fc8f3806b1ada1efc82ccae
+fate-wavpack-matroska_mux-61: REF = fee4bb1d6cf2d54f78eddc4483e7c990
 
 FATE_SAMPLES_AVCONV += $(FATE_WAVPACK-yes)
 fate-wavpack: $(FATE_WAVPACK-yes)

+ 2 - 2
tests/ref/fate/aac-autobsf-adtstoasc

@@ -1,5 +1,5 @@
-6ffdfc7f11f06f94c22cda3a29bf576b *tests/data/fate/aac-autobsf-adtstoasc.matroska
-6627 tests/data/fate/aac-autobsf-adtstoasc.matroska
+5088977b7f13181d707a4077771e4552 *tests/data/fate/aac-autobsf-adtstoasc.matroska
+6648 tests/data/fate/aac-autobsf-adtstoasc.matroska
 #extradata 0:        2, 0x0030001c
 #tb 0: 1/1000
 #media_type 0: audio

+ 1 - 1
tests/ref/fate/binsub-mksenc

@@ -1 +1 @@
-8f355f39124753f89ead698e66f98098
+5706cf33f9ded660eb404275ec136127

+ 2 - 2
tests/ref/fate/matroska-flac-extradata-update

@@ -1,5 +1,5 @@
-332bf4d9c92d24478d2a218e81223433 *tests/data/fate/matroska-flac-extradata-update.matroska
-2011 tests/data/fate/matroska-flac-extradata-update.matroska
+3c721898cf2cf3e2e6c43ad58952bd2d *tests/data/fate/matroska-flac-extradata-update.matroska
+2032 tests/data/fate/matroska-flac-extradata-update.matroska
 #extradata 0:       34, 0x7acb09e7
 #extradata 1:       34, 0x7acb09e7
 #extradata 2:       34, 0x443402dd

+ 2 - 2
tests/ref/fate/rgb24-mkv

@@ -1,5 +1,5 @@
-84474651e0744e7049e27dd19b8c18a8 *tests/data/fate/rgb24-mkv.matroska
-58192 tests/data/fate/rgb24-mkv.matroska
+fdc02d700dbe99315a9f0d928a9b935e *tests/data/fate/rgb24-mkv.matroska
+58213 tests/data/fate/rgb24-mkv.matroska
 #tb 0: 1/10
 #media_type 0: video
 #codec_id 0: rawvideo

+ 2 - 2
tests/ref/fate/webm-dash-chapters

@@ -1,5 +1,5 @@
-eada3550583906e53114d30cb39126f5 *tests/data/fate/webm-dash-chapters.webm
-111199 tests/data/fate/webm-dash-chapters.webm
+e7fde2ecc9683a7a5296dab33b028653 *tests/data/fate/webm-dash-chapters.webm
+111220 tests/data/fate/webm-dash-chapters.webm
 #extradata 0:     3469, 0xc6769ddc
 #tb 0: 1/1000
 #media_type 0: audio

+ 2 - 2
tests/ref/lavf-fate/av1.mkv

@@ -1,3 +1,3 @@
-696074a7600fb5a02a434839d09201f0 *tests/data/lavf-fate/lavf.av1.mkv
-55636 tests/data/lavf-fate/lavf.av1.mkv
+bd676bfc89422755920099ec6ebdebe6 *tests/data/lavf-fate/lavf.av1.mkv
+55657 tests/data/lavf-fate/lavf.av1.mkv
 tests/data/lavf-fate/lavf.av1.mkv CRC=0x7c27cc15

+ 2 - 2
tests/ref/lavf/mka

@@ -1,3 +1,3 @@
-a67d0e6113de91ee53ee00b47fa6ff42 *tests/data/lavf/lavf.mka
-43559 tests/data/lavf/lavf.mka
+ea812a6619f4f0d266bec82fcfa54e78 *tests/data/lavf/lavf.mka
+43580 tests/data/lavf/lavf.mka
 tests/data/lavf/lavf.mka CRC=0x3a1da17e

Some files were not shown because too many files changed in this diff