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

@@ -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

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<std::pair<packet_cptr, mtx::opus::toc_t>>{};
+  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<packet_t>(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<packet_t>(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

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

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"