From 34188f232306be190c76f53ef8b4cb68cb531f20 Mon Sep 17 00:00:00 2001
From: Moritz Bunkus <moritz@bunkus.org>
Date: Thu, 21 Jun 2018 15:52:31 +0200
Subject: [PATCH] Ogg Opus: calculate timestamps after gaps from
 page-after-gap's granulepos

Part of #2280.
---
 NEWS.md                                    |  3 ++
 src/input/r_ogm.cpp                        | 39 +++++++++++++++++-----
 tests/results.txt                          |  1 +
 tests/test-646ogg_opus_gap_page_missing.rb |  5 +++
 4 files changed, 40 insertions(+), 8 deletions(-)
 create mode 100755 tests/test-646ogg_opus_gap_page_missing.rb

diff --git a/NEWS.md b/NEWS.md
index 340b72c0d..09132d757 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -60,6 +60,9 @@
     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.
+  * The timestamp of the first packet after a gap in the middle of the stream
+    is now calculated based on the Ogg page the packet belongs to, and not
+    based on the timestamps before the gap.
 
   Fixes #2280.
 
diff --git a/src/input/r_ogm.cpp b/src/input/r_ogm.cpp
index feace7d96..bdc0091b2 100644
--- a/src/input/r_ogm.cpp
+++ b/src/input/r_ogm.cpp
@@ -1198,8 +1198,16 @@ ogm_a_opus_demuxer_c::process_page(int64_t granulepos) {
   auto eos_here           = false;
   auto gap_here           = false;
 
+  while (true) {
+    auto result = ogg_stream_packetout(&os, &op);
+    if (result == 0)
+      break;
+
+    if (result == -1) {
+      gap_here = true;
+      continue;
+    }
 
-  while (ogg_stream_packetout(&os, &op) == 1) {
     if (op.e_o_s) {
       eos      = 1;
       eos_here = true;
@@ -1227,11 +1235,17 @@ ogm_a_opus_demuxer_c::process_page(int64_t granulepos) {
   if (packets.empty())
     return;
 
-  if (!eos_here && is_first_packet) {
-    // 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.
+  if (!eos_here && (is_first_packet || gap_here)) {
+    // Two possible situations that require calculating the start
+    // timestamps backwards from the granule position:
+
+    // 1. 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.
+
+    // 2. If there was a gap, the calculated prior end timestamp is
+    //    the one before the gap and must therefore not be used.
     auto current_timestamp = page_end_timestamp;
     auto idx               = packets.size();
 
@@ -1242,7 +1256,13 @@ ogm_a_opus_demuxer_c::process_page(int64_t granulepos) {
       packets[idx].first->timestamp  = (current_timestamp + m_timestamp_shift).to_ns();
     }
 
-    if (current_timestamp != timestamp_c::ns(0)) {
+
+    if (gap_here) {
+      mxdebug_if(ms_debug,
+                 boost::format("Opus packet after gap; Ogg page end timestamp %1% page duration %2% first timestamp %3%\n")
+                 % page_end_timestamp % page_duration % format_timestamp(packets.front().first->timestamp));
+
+    } else 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)
@@ -1256,7 +1276,10 @@ ogm_a_opus_demuxer_c::process_page(int64_t granulepos) {
 
   } else {
     // Calculate discard padding based on previous end timestamp and
-    // number of samples in this page.
+    // number of samples in this page. This is done so that streams
+    // that aren't strictly spec-compliant (gaps in the middle =
+    // discard padding present not just on the very last packet) can
+    // be handled gracefully, too.
 
     auto current_timestamp = m_previous_page_end_timestamp;
 
diff --git a/tests/results.txt b/tests/results.txt
index 251c93ee0..e310081e7 100644
--- a/tests/results.txt
+++ b/tests/results.txt
@@ -491,3 +491,4 @@ T_642avc_es_clear_internal_buffers_after_reading_headers:0aaf44ccf543a6a739fd79a
 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
+T_646ogg_opus_gap_page_missing:c3c8ff65984de89ce37c2f03520ae83f:passed:20180621-154830:0.009743698
diff --git a/tests/test-646ogg_opus_gap_page_missing.rb b/tests/test-646ogg_opus_gap_page_missing.rb
new file mode 100755
index 000000000..6be48aca9
--- /dev/null
+++ b/tests/test-646ogg_opus_gap_page_missing.rb
@@ -0,0 +1,5 @@
+#!/usr/bin/ruby -w
+
+# T_646ogg_opus_gap_page_missing
+describe "mkvmerge / Ogg Opus gap in stream due to missing page"
+test_merge "data/opus/gap-page-missing.opus"