From 1762d4dc2e075d59a6b0ae0452ea4c3fd0b5a060 Mon Sep 17 00:00:00 2001 From: Moritz Bunkus Date: Wed, 20 Jun 2018 19:27:47 +0200 Subject: [PATCH] Ogg Opus reader: calculate packet timestamps from granulepos The prior algorithm summed up all Opus frame's number of samples and calculated the expected timestamp from there. That expected timestamp was compared to the Ogg page's granule position, and the discard padding was calculated from that. This lead to problems in several cases: 1. When the first packet's granule position was bigger than the number of samples in the Opus frame, the specs say that the first sample's timestamp is positive in that case. mkvmerge was wrongfully using 0 as the timestamp and thought discard padding was needed in such a case. 2. When the first packet's granule position was smaller than the number of samples in the Opus frame (meaning the first sample's timestamp was negative), a technically invalid bitstream, mkvmerge was wrongfully forcing the first packet's timestamp up to 0 and inserting discard padding elements all over the place as its internal calculations got confused. The new algorithm bases the calculation on the last known granule position special-casing for certain Ogg packet numbers: 1. For the first Ogg page the timestamps of all packets is calculated by subtracting each packet's number of samples from the page's granule position in reverse order. This may result in the first packet's timestamp being positive, which mkvmerge now preserves. mkvmerge won't calculate a discard padding for this page. 2. For all other pages each packet's timestamp is calculated by adding each packet's number of samples to the previous Ogg page's end timestamp as given by its granule position. mkvmerge then calculates the discard padding for the last packet of the page by comparing the calculated end timestamp of that packet with the Ogg page's end timestamp as given by the granule position. As a workaround for certain rounding errors encountered in the wild, a discard padding of one sample will be treated as if no discard padding was present for all but the very last packet as indicated by the "end of stream" bit. Fixes #2280. --- NEWS.md | 23 +++++ src/input/r_ogm.cpp | 92 ++++++++++++++++--- tests/results.txt | 5 +- ...st-645ogg_opus_first_timestamp_negative.rb | 6 ++ 4 files changed, 109 insertions(+), 17 deletions(-) create mode 100755 tests/test-645ogg_opus_first_timestamp_negative.rb diff --git a/NEWS.md b/NEWS.md index c6443a03a..9e3dad496 100644 --- a/NEWS.md +++ b/NEWS.md @@ -33,6 +33,29 @@ throwing an exception and mkvmerge aborting on such data. Fixes #2327. * mkvmerge: audio packetizers: mkvmerge will now keep discard padding values if they're present for packets read from Matroska files. Fixes #2296. +* mkvmerge: Ogg Opus reader: packet timestamps aren't calculated by summing up + the duration of all packets starting with timestamp 0 anymore. Instead the + algorithm is based on the Ogg page's granule position and which packet + number is currently timestamped (special handling for the first and last + packets in the stream). + + * This fixes the first timestamp if the first Ogg packet's granule position + is larger than the number of samples in the first packet (= if the first + sample's timestamp is bigger than 0). mkvmerge will keep those offsets now + and inserts "discard padding" only where it's actually needed. + * It also improves handling of invalid files where the first Ogg packet's + granule position is smaller than the number of samples in the first packet + (= the first sample's timestamp is smaller than 0). mkvmerge will now + shift all timestamps up to 0 in such a case instead of inserting "discard + padding" elements all over the place. + * mkvmerge will no longer insert "discard padding" elements if the + difference between a) the calculated number of samples in the packet + according to the granule position and b) the actual number of samples as + calculated from the bitstream is one sample or less and if the packet + isn't the last one in the stream. This circumvents certain rounding + errors. + + Fixes #2280. # Version 24.0.0 "Beyond The Pale" 2018-06-10 diff --git a/src/input/r_ogm.cpp b/src/input/r_ogm.cpp index 28c7abe1d..cd0c83d3d 100644 --- a/src/input/r_ogm.cpp +++ b/src/input/r_ogm.cpp @@ -123,7 +123,7 @@ public: class ogm_a_opus_demuxer_c: public ogm_demuxer_c { protected: - timestamp_c m_calculated_end_timestamp; + timestamp_c m_previous_page_end_timestamp, m_timestamp_shift; static debugging_option_c ms_debug; @@ -1172,7 +1172,7 @@ ogm_a_vorbis_demuxer_c::process_page(int64_t /* granulepos */) { ogm_a_opus_demuxer_c::ogm_a_opus_demuxer_c(ogm_reader_c *p_reader) : ogm_demuxer_c(p_reader) - , m_calculated_end_timestamp{timestamp_c::ns(0)} + , m_timestamp_shift{timestamp_c::ns(0)} { codec = codec_c::look_up(codec_c::type_e::A_OPUS); } @@ -1191,31 +1191,93 @@ void ogm_a_opus_demuxer_c::process_page(int64_t granulepos) { ogg_packet op; - auto ogg_timestamp = timestamp_c::ns(granulepos * 1000000000 / 48000); + auto page_end_timestamp = timestamp_c::ns(granulepos * 1000000000 / 48000); + auto page_duration = timestamp_c::ns(0); + auto packets = std::vector>{}; + auto eos_here = false; while (ogg_stream_packetout(&os, &op) == 1) { - eos |= op.e_o_s; + if (op.e_o_s) { + eos = 1; + eos_here = true; + } if ((4 <= op.bytes) && !memcmp(op.packet, "Opus", 4)) continue; try { - auto packet = std::make_shared(memory_c::clone(op.packet, op.bytes)); - auto toc = mtx::opus::toc_t::decode(packet->data); - m_calculated_end_timestamp += toc.packet_duration; + auto packet = std::make_shared(memory_c::clone(op.packet, op.bytes)); + auto toc = mtx::opus::toc_t::decode(packet->data); + page_duration += toc.packet_duration; - if (m_calculated_end_timestamp > ogg_timestamp) { - packet->discard_padding = m_calculated_end_timestamp - ogg_timestamp; - mxdebug_if(ms_debug, - boost::format("Opus discard padding calculated %1% Ogg timestamp %2% diff %3% samples %4% (Ogg page's granulepos %5%)\n") - % m_calculated_end_timestamp % ogg_timestamp % packet->discard_padding % (packet->discard_padding.to_ns() * 48000 / 1000000000) % granulepos); - } + packets.emplace_back(packet, toc); - reader->m_reader_packetizers[ptzr]->process(packet); } catch (mtx::opus::decode_error &ex) { - mxwarn_fn(m_ti.m_fname, boost::format(Y("Error decoding an Ogg Opus page at source timestamp %1%; page will be skipped: %2%\n")) % format_timestamp(ogg_timestamp) % ex); + mxwarn_fn(m_ti.m_fname, boost::format(Y("Error decoding an Ogg Opus page at Ogg page end timestamp %1%; page will be skipped: %2%\n")) % page_end_timestamp % ex); } } + + if (packets.empty()) + return; + + mxdebug_if(ms_debug, + boost::format("process_page: granulepos %1% = %2% previous page end timestamp %3% page duration %4%\n") + % granulepos % page_end_timestamp % m_previous_page_end_timestamp % page_duration); + + if (!eos_here && !m_previous_page_end_timestamp.valid()) { + // First page in Ogg stream. Streams must not start at 0. If they + // don't, this is signaled by the page containing less samples + // than the difference between the page's granulepos-indicated end + // timestamp and 0. + auto current_timestamp = page_end_timestamp; + auto idx = packets.size(); + + while (idx > 0) { + --idx; + + current_timestamp -= packets[idx].second.packet_duration; + packets[idx].first->timestamp = (current_timestamp + m_timestamp_shift).to_ns(); + } + + if (current_timestamp != timestamp_c::ns(0)) { + if (current_timestamp < timestamp_c::ns(0)) { + m_timestamp_shift = current_timestamp.negate(); + for (auto const &pair : packets) + pair.first->timestamp += m_timestamp_shift.to_ns(); + } + + mxdebug_if(ms_debug, + boost::format("Opus first packet's timestamp is not zero; Ogg page end timestamp %1% page duration %2% first timestamp %3% (Ogg page's granulepos %4%) shift %5%\n") + % page_end_timestamp % page_duration % format_timestamp(packets.front().first->timestamp) % granulepos % m_timestamp_shift); + } + + } else { + // Calculate discard padding based on previous end timestamp and + // number of samples in this page. + + auto current_timestamp = m_previous_page_end_timestamp; + + for (auto const &pair : packets) { + pair.first->timestamp = (current_timestamp + m_timestamp_shift).to_ns(); + current_timestamp += pair.second.packet_duration; + } + + auto &packet = *packets.back().first; + auto samples_over = (current_timestamp - page_end_timestamp).to_ns() * 48'000 / 1'000'000'000; + + if (samples_over > (eos_here ? 0 : 1)) { + packet.discard_padding = current_timestamp - page_end_timestamp; + + mxdebug_if(ms_debug, + boost::format("Opus discard padding calculated: previous Ogg page end timestamp %1% current Ogg page end timestamp %2% discard padding %3% in samples %4% (Ogg page's granulepos %5%)\n") + % m_previous_page_end_timestamp % page_end_timestamp % packet.discard_padding % samples_over % granulepos); + } + } + + m_previous_page_end_timestamp = page_end_timestamp; + + for (auto const &packet : packets) + reader->m_reader_packetizers[ptzr]->process(packet.first); } void diff --git a/tests/results.txt b/tests/results.txt index 2467411a3..21b41489e 100644 --- a/tests/results.txt +++ b/tests/results.txt @@ -460,7 +460,7 @@ T_611info_null_pointer_dereference_for_ebmlbinary:68d2ebe0a701fffbcc5b5f245f1abb T_612dts_provided_timestamp_used_too_early:fbdb6b5df6394eb5c2254bcf956932b2:passed:20170813-175153:0.010423057 T_613vorbis_in_mp4:aa83ac60b8f9d9ad485ac4015bf86626:passed:20170910-175618:0.014743465 T_614white_colour_coordinates_from_mkv:e19564eb32670bd2bcc9926c6bdf31b7-e19564eb32670bd2bcc9926c6bdf31b7:passed:20170916-112249:0.094985684 -T_615opus_discard_padding_in_the_middle:ee3303cbd6fc9f52773515cc8ad98f0d:passed:20170924-230527:0.025602623 +T_615opus_discard_padding_in_the_middle:86650d13cdd4ba9577eb036bf768ee27:passed:20170924-230527:0.025602623 T_616more_than_128_tracks:3c5efa1124d35a15a03fe195f03fa141-3c5efa1124d35a15a03fe195f03fa141:passed:20170925-201313:0.570145315 T_617aac_adts_parse_program_config_element_for_channels:c4278d4a3559723bdb83c825a82dc0ec:passed:20171001-201657:0.037546681 T_618e_ac_3_with_core_and_extensions_in_different_blocks:fb0a0fc618a3cdb7a98e4847c02f488f:passed:20171001-221905:0.044258061 @@ -476,7 +476,7 @@ T_627h264_sps_pps_between_slices_of_same_frame:f81d3b78e48caf83a7a6445424da5581: T_628srt_no_decimal_places:8a50a0fb577069ddcb30fd0d6bf27dc9:passed:20180102-191637:0.01673336 T_629mpeg_ts_broken_pes_packets:497c3a917f833295686bd0c2471b7d47:passed:20180113-212211:0.055502835 T_630propedit_add_statistics_tags_with_compression:fdede2a7722ef0880aebea81977ca1f7-18+70+590:passed:20180118-205706:0.041159571 -T_631ogg_opus_packet_without_data:465fa372a7fc729819e43d5a2ce23716:passed:20180210-164242:0.022683445 +T_631ogg_opus_packet_without_data:e85368f944c0c954f7bbefb352ae60a6:passed:20180210-164242:0.022683445 T_632flv_aac_codec_initialization_more_than_5_bytes:8e7356658d3bac7492a2569ff594db7e:passed:20180222-152609:0.047661003 T_633payload_signalled_but_no_payload_bytes:d4e79b5d295a512b8690cf61cf5737a3:passed:20180304-220823:0.061501973 T_634dialog_normalization_gain_removal:9a438e39ffa72c89a1e9ed940ecf6a0a-5c7588e4be20be2adf8262a5849c5a04-5c7588e4be20be2adf8262a5849c5a04-5c7588e4be20be2adf8262a5849c5a04-2eed32756281880feef7ecaae7d0a9d6-f9bd452a42fc491a728693ca921e4d28-f9bd452a42fc491a728693ca921e4d28-f9bd452a42fc491a728693ca921e4d28-aa91481fb610d3fdc6cce4a5cdc17e46-4996751358eb859de137eb46121de9bc-4996751358eb859de137eb46121de9bc-4996751358eb859de137eb46121de9bc-21b889aab040681e1d6e8a6933a19f17-cad4c07c880efe67d652d1a5409f3bd0-cad4c07c880efe67d652d1a5409f3bd0-cad4c07c880efe67d652d1a5409f3bd0:passed:20180308-204112:1.246752927 @@ -490,3 +490,4 @@ T_641keep_display_unit:f3c2b4b8f52f2d29025052a6be092451-3+16x9-2392d8546623ab626 T_642avc_es_clear_internal_buffers_after_reading_headers:0aaf44ccf543a6a739fd79ab74eed9c3:passed:20180613-174520:0.02386315 T_643mpeg_ts_bad_utf8_in_service_names:f967b2bf3fb4265ec723f14eb667bb9a:passed:20180615-172956:0.019394331 T_644mp3_with_discard_padding:e68074afaedbb820210e31e3cf62febc:passed:20180619-214051:0.038730751 +T_645ogg_opus_first_timestamp_negative:4b513ca85e1cf12b378eedd07c8b4ffa-78778f6067b903cc326bf0f998f9790b:passed:20180620-185732:0.925863056 diff --git a/tests/test-645ogg_opus_first_timestamp_negative.rb b/tests/test-645ogg_opus_first_timestamp_negative.rb new file mode 100755 index 000000000..6565a3444 --- /dev/null +++ b/tests/test-645ogg_opus_first_timestamp_negative.rb @@ -0,0 +1,6 @@ +#!/usr/bin/ruby -w + +# T_645ogg_opus_first_timestamp_negative +describe "mkvmerge / Ogg Opus with first granulepos being smaller than number of samples in packet = negative first timestamp" +test_merge "data/opus/first-timestamp-negative.opus", :args => "--timestamp-scale 1000000" +test_merge "data/opus/first-timestamp-negative.opus", :args => "--timestamp-scale 1000000 --disable-lacing"