mkvmerge: don't drop attachments with same name/size from same file

ChangeLog

@@ -1,5 +1,8 @@
 2016-01-02  Moritz Bunkus  <moritz@bunkus.org>
 
+        * mkvmerge: bug fix: Matroska attachments with the same name, size
+        and MIME type were not output during file identification.
+
         * MKVToolNix GUI: merge tool enhancement: added a column to the
         "attachments" tab containing the file size.
 

src/input/r_avi.cpp

@@ -931,8 +931,8 @@ avi_reader_c::identify_attachments() {
     }
   }
 
-  for (i = 0; i < g_attachments.size(); i++)
-    id_result_attachment(g_attachments[i].ui_id, g_attachments[i].mime_type, g_attachments[i].data->get_size(), g_attachments[i].name, g_attachments[i].description);
+  for (auto const &attachment : g_attachments)
+    id_result_attachment(attachment->ui_id, attachment->mime_type, attachment->data->get_size(), attachment->name, attachment->description);
 }
 
 void

src/input/r_matroska.cpp

@@ -720,26 +720,30 @@ kax_reader_c::handle_attachments(mm_io_c *io,
     if (!att)
       continue;
 
-    attachment_t matt;
-    matt.name        = to_utf8(FindChildValue<KaxFileName>(att));
-    matt.description = to_utf8(FindChildValue<KaxFileDescription>(att));
-    matt.mime_type   = FindChildValue<KaxMimeType>(att);
-    matt.id          = FindChildValue<KaxFileUID>(att);
-
-    auto fdata       = FindChild<KaxFileData>(att);
-    if (fdata)
-      matt.data = memory_c::clone(static_cast<unsigned char *>(fdata->GetBuffer()), fdata->GetSize());
-
     ++m_attachment_id;
-    attach_mode_e attach_mode;
-    if (   !matt.data->get_size()
-        || matt.mime_type.empty()
-        || matt.name.empty()
-        || ((attach_mode = attachment_requested(m_attachment_id)) == ATTACH_MODE_SKIP))
+
+    auto fdata = FindChild<KaxFileData>(att);
+    if (!fdata)
       continue;
 
-    matt.ui_id          = m_attachment_id;
-    matt.to_all_files   = ATTACH_MODE_TO_ALL_FILES == attach_mode;
+    auto matt         = std::make_shared<attachment_t>();
+    matt->name        = to_utf8(FindChildValue<KaxFileName>(att));
+    matt->description = to_utf8(FindChildValue<KaxFileDescription>(att));
+    matt->mime_type   = FindChildValue<KaxMimeType>(att);
+    matt->id          = FindChildValue<KaxFileUID>(att);
+    matt->data        = memory_c::clone(static_cast<unsigned char *>(fdata->GetBuffer()), fdata->GetSize());
+
+    auto attach_mode  = attachment_requested(m_attachment_id);
+
+    if (   !matt->data->get_size()
+        || matt->mime_type.empty()
+        || matt->name.empty()
+        || (ATTACH_MODE_SKIP == attach_mode))
+      continue;
+
+    matt->ui_id          = m_attachment_id;
+    matt->to_all_files   = ATTACH_MODE_TO_ALL_FILES == attach_mode;
+    matt->source_file    = m_ti.m_fname;
 
     add_attachment(matt);
   }
@@ -2380,7 +2384,7 @@ kax_reader_c::identify() {
   }
 
   for (auto &attachment : g_attachments)
-    id_result_attachment(attachment.ui_id, attachment.mime_type, attachment.data->get_size(), attachment.name, attachment.description, attachment.id);
+    id_result_attachment(attachment->ui_id, attachment->mime_type, attachment->data->get_size(), attachment->name, attachment->description, attachment->id);
 
   if (m_chapters)
     id_result_chapters(count_chapter_atoms(*m_chapters));

src/input/r_ssa.cpp

@@ -99,7 +99,6 @@ ssa_reader_c::identify() {
   id_result_container();
   id_result_track(0, ID_RESULT_TRACK_SUBTITLES, codec_c::get_name(codec_c::type_e::S_SSA_ASS, "SSA/ASS"), info.get());
 
-  size_t i;
-  for (i = 0; i < g_attachments.size(); i++)
-    id_result_attachment(g_attachments[i].ui_id, g_attachments[i].mime_type, g_attachments[i].data->get_size(), g_attachments[i].name, g_attachments[i].description);
+  for (auto const &attachment : g_attachments)
+    id_result_attachment(attachment->ui_id, attachment->mime_type, attachment->data->get_size(), attachment->name, attachment->description);
 }

src/input/subtitles.cpp

@@ -516,7 +516,8 @@ ssa_parser_c::add_attachment_maybe(std::string &name,
     return;
   }
 
-  attachment_t attachment;
+  auto attachment_p = std::make_shared<attachment_t>();
+  auto &attachment  = *attachment_p;
 
   std::string short_name = m_file_name;
   size_t pos             = short_name.rfind('/');
@@ -531,6 +532,7 @@ ssa_parser_c::add_attachment_maybe(std::string &name,
   attachment.name         = m_cc_utf8->utf8(name);
   attachment.description  = (boost::format(SSA_SECTION_FONTS == section ? Y("Imported font from %1%") : Y("Imported picture from %1%")) % short_name).str();
   attachment.to_all_files = true;
+  attachment.source_file  = m_file_name;
 
   size_t data_size        = data_uu.length() % 4;
   data_size               = 3 == data_size ? 2 : 2 == data_size ? 1 : 0;
@@ -549,7 +551,7 @@ ssa_parser_c::add_attachment_maybe(std::string &name,
   if (attachment.mime_type == "")
     attachment.mime_type = "application/octet-stream";
 
-  add_attachment(attachment);
+  add_attachment(attachment_p);
 
   name    = "";
   data_uu = "";

src/merge/mkvmerge.cpp

@@ -1557,7 +1557,7 @@ parse_arg_cluster_length(std::string arg) {
 }
 
 static void
-parse_arg_attach_file(attachment_t &attachment,
+parse_arg_attach_file(attachment_cptr const &attachment,
                       const std::string &arg,
                       bool attach_once) {
   try {
@@ -1566,27 +1566,26 @@ parse_arg_attach_file(attachment_t &attachment,
     mxerror(boost::format(Y("The file '%1%' cannot be attached because it does not exist or cannot be read.\n")) % arg);
   }
 
-  attachment.name         = arg;
-  attachment.to_all_files = !attach_once;
+  attachment->name         = arg;
+  attachment->to_all_files = !attach_once;
 
-  if (attachment.mime_type.empty())
-    attachment.mime_type  = guess_mime_type_and_report(arg);
+  if (attachment->mime_type.empty())
+    attachment->mime_type  = guess_mime_type_and_report(arg);
 
   try {
-    mm_io_cptr io = mm_file_io_c::open(attachment.name);
+    mm_io_cptr io = mm_file_io_c::open(attachment->name);
 
     if (0 == io->get_size())
-      mxerror(boost::format(Y("The size of attachment '%1%' is 0.\n")) % attachment.name);
+      mxerror(boost::format(Y("The size of attachment '%1%' is 0.\n")) % attachment->name);
 
-    attachment.data = memory_c::alloc(io->get_size());
-    io->read(attachment.data->get_buffer(), attachment.data->get_size());
+    attachment->data = memory_c::alloc(io->get_size());
+    io->read(attachment->data->get_buffer(), attachment->data->get_size());
 
   } catch (...) {
-    mxerror(boost::format(Y("The attachment '%1%' could not be read.\n")) % attachment.name);
+    mxerror(boost::format(Y("The attachment '%1%' could not be read.\n")) % attachment->name);
   }
 
   add_attachment(attachment);
-  attachment.clear();
 }
 
 static void
@@ -1962,7 +1961,7 @@ parse_args(std::vector<std::string> args) {
   auto ti               = std::make_unique<track_info_c>();
   bool inputs_found     = false;
   bool append_next_file = false;
-  attachment_t attachment;
+  auto attachment       = std::make_shared<attachment_t>();
 
   for (auto sit = args.cbegin(), sit_end = args.cend(); sit != sit_end; sit++) {
     auto const &this_arg = *sit;
@@ -2071,29 +2070,29 @@ parse_args(std::vector<std::string> args) {
       if (no_next_arg)
         mxerror(Y("'--attachment-description' lacks the description.\n"));
 
-      if (attachment.description != "")
+      if (attachment->description != "")
         mxwarn(Y("More than one description was given for a single attachment.\n"));
-      attachment.description = next_arg;
+      attachment->description = next_arg;
       sit++;
 
     } else if (this_arg == "--attachment-mime-type") {
       if (no_next_arg)
         mxerror(Y("'--attachment-mime-type' lacks the MIME type.\n"));
 
-      if (attachment.mime_type != "")
+      if (attachment->mime_type != "")
         mxwarn(boost::format(Y("More than one MIME type was given for a single attachment. '%1%' will be discarded and '%2%' used instead.\n"))
-               % attachment.mime_type % next_arg);
-      attachment.mime_type = next_arg;
+               % attachment->mime_type % next_arg);
+      attachment->mime_type = next_arg;
       sit++;
 
     } else if (this_arg == "--attachment-name") {
       if (no_next_arg)
         mxerror(Y("'--attachment-name' lacks the name.\n"));
 
-      if (attachment.stored_name != "")
+      if (attachment->stored_name != "")
         mxwarn(boost::format(Y("More than one name was given for a single attachment. '%1%' will be discarded and '%2%' used instead.\n"))
-               % attachment.stored_name % next_arg);
-      attachment.stored_name = next_arg;
+               % attachment->stored_name % next_arg);
+      attachment->stored_name = next_arg;
       sit++;
 
     } else if ((this_arg == "--attach-file") || (this_arg == "--attach-file-once")) {
@@ -2101,6 +2100,7 @@ parse_args(std::vector<std::string> args) {
         mxerror(boost::format(Y("'%1%' lacks the file name.\n")) % this_arg);
 
       parse_arg_attach_file(attachment, next_arg, this_arg == "--attach-file-once");
+      attachment = std::make_shared<attachment_t>();
       sit++;
 
       inputs_found = true;

src/merge/output_control.cpp

@@ -82,7 +82,7 @@ namespace libmatroska {
 
 std::vector<packetizer_t> g_packetizers;
 std::vector<filelist_cptr> g_files;
-std::vector<attachment_t> g_attachments;
+std::vector<attachment_cptr> g_attachments;
 std::vector<track_order_t> g_track_order;
 std::vector<append_spec_t> g_append_mapping;
 std::unordered_map<int64_t, generic_packetizer_c *> g_packetizers_by_track_num;
@@ -350,29 +350,31 @@ add_tags(KaxTag *tags) {
    \return The attachment UID created for this attachment.
 */
 int64_t
-add_attachment(attachment_t attachment) {
+add_attachment(attachment_cptr const &attachment) {
   // If the attachment is coming from an existing file then we should
   // check if we already have another attachment stored. This can happen
   // if we're concatenating files.
-  if (0 != attachment.id) {
+  if (0 != attachment->id) {
     for (auto &ex_attachment : g_attachments)
-      if ((   (ex_attachment.id == attachment.id)
+      if ((   (ex_attachment->id == attachment->id)
            && !hack_engaged(ENGAGE_NO_VARIABLE_DATA))
           ||
-          (   (ex_attachment.name             == attachment.name)
-           && (ex_attachment.description      == attachment.description)
-           && (ex_attachment.data->get_size() == attachment.data->get_size())))
-        return attachment.id;
+          (   (ex_attachment->name             == attachment->name)
+           && (ex_attachment->description      == attachment->description)
+           && (ex_attachment->data->get_size() == attachment->data->get_size())
+           && (ex_attachment->source_file      != attachment->source_file)
+           && !attachment->source_file.empty()))
+        return attachment->id;
 
-    add_unique_number(attachment.id, UNIQUE_ATTACHMENT_IDS);
+    add_unique_number(attachment->id, UNIQUE_ATTACHMENT_IDS);
 
   } else
     // No ID yet. Let's assign one.
-    attachment.id = create_unique_number(UNIQUE_ATTACHMENT_IDS);
+    attachment->id = create_unique_number(UNIQUE_ATTACHMENT_IDS);
 
   g_attachments.push_back(attachment);
 
-  return attachment.id;
+  return attachment->id;
 }
 
 /** \brief Add a packetizer to the list of packetizers
@@ -866,7 +868,9 @@ render_attachments(IOCallback *out) {
   s_kax_as   = std::make_unique<KaxAttachments>();
   auto kax_a = static_cast<KaxAttached *>(nullptr);
 
-  for (auto &attch : g_attachments) {
+  for (auto &attachment_p : g_attachments) {
+    auto attch = *attachment_p;
+
     if ((1 == g_file_num) || attch.to_all_files) {
       kax_a = !kax_a ? &GetChild<KaxAttached>(*s_kax_as) : &GetNextChild<KaxAttached>(*s_kax_as, *kax_a);
 
@@ -1124,9 +1128,9 @@ void
 calc_attachment_sizes() {
   // Calculate the size of all attachments for split control.
   for (auto &att : g_attachments) {
-    g_attachment_sizes_first += att.data->get_size();
-    if (att.to_all_files)
-      g_attachment_sizes_others += att.data->get_size();
+    g_attachment_sizes_first += att->data->get_size();
+    if (att->to_all_files)
+      g_attachment_sizes_others += att->data->get_size();
   }
 }
 

src/merge/output_control.h

@@ -93,26 +93,13 @@ struct packetizer_t {
 };
 
 struct attachment_t {
-  std::string name, stored_name, mime_type, description;
-  uint64_t id;
-  bool to_all_files;
+  std::string name, stored_name, mime_type, description, source_file;
+  uint64_t id{};
+  bool to_all_files{};
   memory_cptr data;
-  int64_t ui_id;
-
-  attachment_t() {
-    clear();
-  }
-  void clear() {
-    name         = "";
-    stored_name  = "";
-    mime_type    = "";
-    description  = "";
-    id           = 0;
-    ui_id        = 0;
-    to_all_files = false;
-    data.reset();
-  }
+  int64_t ui_id{};
 };
+using attachment_cptr = std::shared_ptr<attachment_t>;
 
 struct track_order_t {
   int64_t file_id;
@@ -143,7 +130,7 @@ public:
 };
 
 extern std::vector<packetizer_t> g_packetizers;
-extern std::vector<attachment_t> g_attachments;
+extern std::vector<attachment_cptr> g_attachments;
 extern std::vector<track_order_t> g_track_order;
 extern std::vector<append_spec_t> g_append_mapping;
 extern std::unordered_map<int64_t, generic_packetizer_c *> g_packetizers_by_track_num;
@@ -225,7 +212,7 @@ std::string create_output_name();
 bool set_required_matroska_version(unsigned int required_version);
 bool set_required_matroska_read_version(unsigned int required_version);
 
-int64_t add_attachment(attachment_t attachment);
+int64_t add_attachment(attachment_cptr const &attachment);
 
 #if defined(SYS_UNIX) || defined(SYS_APPLE)
 void sighandler(int signum);