From 4757f9fa0e496a17186b07efbe7410d9f2b1668e Mon Sep 17 00:00:00 2001 From: Moritz Bunkus Date: Sat, 2 Jan 2016 17:40:47 +0100 Subject: [PATCH] mkvmerge: don't drop attachments with same name/size from same file --- ChangeLog | 3 +++ src/input/r_avi.cpp | 4 ++-- src/input/r_matroska.cpp | 40 ++++++++++++++++++++---------------- src/input/r_ssa.cpp | 5 ++--- src/input/subtitles.cpp | 6 ++++-- src/merge/mkvmerge.cpp | 40 ++++++++++++++++++------------------ src/merge/output_control.cpp | 34 ++++++++++++++++-------------- src/merge/output_control.h | 27 +++++++----------------- 8 files changed, 79 insertions(+), 80 deletions(-) diff --git a/ChangeLog b/ChangeLog index f96516d0a..30d5165bd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,8 @@ 2016-01-02 Moritz Bunkus + * 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. diff --git a/src/input/r_avi.cpp b/src/input/r_avi.cpp index ceacdf21f..bcdac18d7 100644 --- a/src/input/r_avi.cpp +++ b/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 diff --git a/src/input/r_matroska.cpp b/src/input/r_matroska.cpp index f13a31581..a74ddabe8 100644 --- a/src/input/r_matroska.cpp +++ b/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(att)); - matt.description = to_utf8(FindChildValue(att)); - matt.mime_type = FindChildValue(att); - matt.id = FindChildValue(att); - - auto fdata = FindChild(att); - if (fdata) - matt.data = memory_c::clone(static_cast(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(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(); + matt->name = to_utf8(FindChildValue(att)); + matt->description = to_utf8(FindChildValue(att)); + matt->mime_type = FindChildValue(att); + matt->id = FindChildValue(att); + matt->data = memory_c::clone(static_cast(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)); diff --git a/src/input/r_ssa.cpp b/src/input/r_ssa.cpp index fca0d355f..ca5fdaf40 100644 --- a/src/input/r_ssa.cpp +++ b/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); } diff --git a/src/input/subtitles.cpp b/src/input/subtitles.cpp index dfffb59d4..504ed913b 100644 --- a/src/input/subtitles.cpp +++ b/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(); + 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 = ""; diff --git a/src/merge/mkvmerge.cpp b/src/merge/mkvmerge.cpp index 4ea2fb5a5..0d696b7d5 100644 --- a/src/merge/mkvmerge.cpp +++ b/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 args) { auto ti = std::make_unique(); bool inputs_found = false; bool append_next_file = false; - attachment_t attachment; + auto attachment = std::make_shared(); 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 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 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(); sit++; inputs_found = true; diff --git a/src/merge/output_control.cpp b/src/merge/output_control.cpp index a5d707dab..d00c506e3 100644 --- a/src/merge/output_control.cpp +++ b/src/merge/output_control.cpp @@ -82,7 +82,7 @@ namespace libmatroska { std::vector g_packetizers; std::vector g_files; -std::vector g_attachments; +std::vector g_attachments; std::vector g_track_order; std::vector g_append_mapping; std::unordered_map 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(); auto kax_a = static_cast(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(*s_kax_as) : &GetNextChild(*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(); } } diff --git a/src/merge/output_control.h b/src/merge/output_control.h index 7a545cd48..1fc12992f 100644 --- a/src/merge/output_control.h +++ b/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; struct track_order_t { int64_t file_id; @@ -143,7 +130,7 @@ public: }; extern std::vector g_packetizers; -extern std::vector g_attachments; +extern std::vector g_attachments; extern std::vector g_track_order; extern std::vector g_append_mapping; extern std::unordered_map 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);