[FFmpeg-devel,v2,4/5] avformat/matroskaenc: Remove allocations for attachments

Submitted by Andreas Rheinhardt on Nov. 27, 2019, 5:38 a.m.

Details

Message ID 20191127053842.19481-4-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt Nov. 27, 2019, 5:38 a.m.
If there are attachments to write, the Matroska muxer currently
allocates two objects: An array that contains an entry for each
attachment containing just the stream index of the corresponding
stream and the uid used for this attachment; and a structure with
a pointer to said array and a counter for said array. These uids are
generated via code special to attachments: It uses an AVLFG in the
normal and a sha of the attachment data in the bitexact case. (Said sha
requires an allocation, too.)

But now that an uid is generated for each stream in mkv_init(), there is
no need any more to use special code for generating the uids of
attachments: One can simply use the uid already generated for the
attachment stream. And this makes the whole allocations of the structures
for attachments as well as the structures itself superfluous. They have
been removed.

In case AVFMT_FLAG_BITEXACT is set, the uids will be different from the
old ones which is the reason why the FATE-test lavf-mkv_attachment
needed to be updated. The old method had the drawback that muxing the
same attachment twice in the same file would create a file where two
attachments have the same uid. The new one doesn't.

Also notice that the dynamic buffer used to write the attachments leaks
if an error happens when writing the buffer. By removing the
allocations potential sources of errors have been removed.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/matroskaenc.c     | 63 +++++------------------------------
 tests/ref/lavf/mkv_attachment |  4 +--
 2 files changed, 11 insertions(+), 56 deletions(-)

Patch hide | download patch | download mbox

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index aecd776157..1833d2a122 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -50,7 +50,6 @@ 
 #include "libavutil/random_seed.h"
 #include "libavutil/rational.h"
 #include "libavutil/samplefmt.h"
-#include "libavutil/sha.h"
 #include "libavutil/stereo3d.h"
 
 #include "libavcodec/xiph.h"
@@ -104,16 +103,6 @@  typedef struct mkv_track {
     int64_t         ts_offset;
 } mkv_track;
 
-typedef struct mkv_attachment {
-    int             stream_idx;
-    uint32_t        fileuid;
-} mkv_attachment;
-
-typedef struct mkv_attachments {
-    mkv_attachment  *entries;
-    int             num_entries;
-} mkv_attachments;
-
 #define MODE_MATROSKAv2 0x01
 #define MODE_WEBM       0x02
 
@@ -140,7 +129,6 @@  typedef struct MatroskaMuxContext {
     mkv_seekhead    *seekhead;
     mkv_cues        *cues;
     mkv_track       *tracks;
-    mkv_attachments *attachments;
 
     AVPacket        cur_audio_pkt;
 
@@ -408,10 +396,6 @@  static void mkv_deinit(AVFormatContext *s)
         av_freep(&mkv->cues->entries);
         av_freep(&mkv->cues);
     }
-    if (mkv->attachments) {
-        av_freep(&mkv->attachments->entries);
-        av_freep(&mkv->attachments);
-    }
     av_freep(&mkv->tracks);
 }
 
@@ -1700,14 +1684,17 @@  static int mkv_write_tags(AVFormatContext *s)
     }
 
     if (mkv->have_attachments && mkv->mode != MODE_WEBM) {
-        for (i = 0; i < mkv->attachments->num_entries; i++) {
-            mkv_attachment *attachment = &mkv->attachments->entries[i];
-            AVStream *st = s->streams[attachment->stream_idx];
+        for (i = 0; i < s->nb_streams; i++) {
+            mkv_track *track = &mkv->tracks[i];
+            AVStream *st = s->streams[i];
+
+            if (st->codecpar->codec_type != AVMEDIA_TYPE_ATTACHMENT)
+                continue;
 
             if (!mkv_check_tag(st->metadata, MATROSKA_ID_TAGTARGETS_ATTACHUID))
                 continue;
 
-            ret = mkv_write_tag(s, st->metadata, MATROSKA_ID_TAGTARGETS_ATTACHUID, attachment->fileuid);
+            ret = mkv_write_tag(s, st->metadata, MATROSKA_ID_TAGTARGETS_ATTACHUID, track->uid);
             if (ret < 0)
                 return ret;
         }
@@ -1726,18 +1713,11 @@  static int mkv_write_attachments(AVFormatContext *s)
 {
     MatroskaMuxContext *mkv = s->priv_data;
     AVIOContext *dyn_cp, *pb = s->pb;
-    AVLFG c;
     int i, ret;
 
     if (!mkv->have_attachments)
         return 0;
 
-    mkv->attachments = av_mallocz(sizeof(*mkv->attachments));
-    if (!mkv->attachments)
-        return AVERROR(ENOMEM);
-
-    av_lfg_init(&c, av_get_random_seed());
-
     ret = mkv_add_seekhead_entry(mkv->seekhead, MATROSKA_ID_ATTACHMENTS, avio_tell(pb));
     if (ret < 0) return ret;
 
@@ -1746,20 +1726,14 @@  static int mkv_write_attachments(AVFormatContext *s)
 
     for (i = 0; i < s->nb_streams; i++) {
         AVStream *st = s->streams[i];
+        mkv_track *track = &mkv->tracks[i];
         ebml_master attached_file;
-        mkv_attachment *attachment = mkv->attachments->entries;
         AVDictionaryEntry *t;
         const char *mimetype = NULL;
-        uint32_t fileuid;
 
         if (st->codecpar->codec_type != AVMEDIA_TYPE_ATTACHMENT)
             continue;
 
-        attachment = av_realloc_array(attachment, mkv->attachments->num_entries + 1, sizeof(mkv_attachment));
-        if (!attachment)
-            return AVERROR(ENOMEM);
-        mkv->attachments->entries = attachment;
-
         attached_file = start_ebml_master(dyn_cp, MATROSKA_ID_ATTACHEDFILE, 0);
 
         if (t = av_dict_get(st->metadata, "title", NULL, 0))
@@ -1790,29 +1764,10 @@  static int mkv_write_attachments(AVFormatContext *s)
             return AVERROR(EINVAL);
         }
 
-        if (s->flags & AVFMT_FLAG_BITEXACT) {
-            struct AVSHA *sha = av_sha_alloc();
-            uint8_t digest[20];
-            if (!sha)
-                return AVERROR(ENOMEM);
-            av_sha_init(sha, 160);
-            av_sha_update(sha, st->codecpar->extradata, st->codecpar->extradata_size);
-            av_sha_final(sha, digest);
-            av_free(sha);
-            fileuid = AV_RL32(digest);
-        } else {
-            fileuid = av_lfg_get(&c);
-        }
-        av_log(s, AV_LOG_VERBOSE, "Using %.8"PRIx32" for attachment %d\n",
-               fileuid, mkv->attachments->num_entries);
-
         put_ebml_string(dyn_cp, MATROSKA_ID_FILEMIMETYPE, mimetype);
         put_ebml_binary(dyn_cp, MATROSKA_ID_FILEDATA, st->codecpar->extradata, st->codecpar->extradata_size);
-        put_ebml_uint(dyn_cp, MATROSKA_ID_FILEUID, fileuid);
+        put_ebml_uint(dyn_cp, MATROSKA_ID_FILEUID, track->uid);
         end_ebml_master(dyn_cp, attached_file);
-
-        mkv->attachments->entries[mkv->attachments->num_entries].stream_idx = i;
-        mkv->attachments->entries[mkv->attachments->num_entries++].fileuid  = fileuid;
     }
     end_ebml_master_crc32(pb, &dyn_cp, mkv);
 
diff --git a/tests/ref/lavf/mkv_attachment b/tests/ref/lavf/mkv_attachment
index 230dff369a..790c0f693b 100644
--- a/tests/ref/lavf/mkv_attachment
+++ b/tests/ref/lavf/mkv_attachment
@@ -1,3 +1,3 @@ 
-4a25c53150c09537cd4fcbff8f8f14ff *tests/data/lavf/lavf.mkv_attachment
-472706 tests/data/lavf/lavf.mkv_attachment
+f54cf193e1925eff8a44554116a5e149 *tests/data/lavf/lavf.mkv_attachment
+472703 tests/data/lavf/lavf.mkv_attachment
 tests/data/lavf/lavf.mkv_attachment CRC=0xec6c3c68