From f2ed4016793ac7f3bfd010ee2be3dc546101826e Mon Sep 17 00:00:00 2001
From: Jared Siskin <jtsiskin@fb.com>
Date: Fri, 18 Jun 2021 10:45:16 -0700
Subject: [PATCH] Fix unscaled timestamps passed to nack_tracker

If timestamp_scaler_ is used, then rtp_header.timestamp, passed to UpdateLastDecodedPacket, will advance at a different rate than the scaled timestamp packet->timestamp, passed to UpdateLastDecodedPacket.

NackTracker::EstimateTimestamp uses timestamp_last_received_rtp_, and NackTracker::TimeToPlay uses timestamp_last_decoded_rtp_.

This difference causes TimeToPlay to continuously increase to huge values, so that every missing packet will be returned from GetNackList, even if RTT > real time to play.

Change-Id: Ie6ca347972edea98a202c9cdd26c6ab3f45a73c4
Bug: None
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/222841
Reviewed-by: Jakob Ivarsson <jakobi@webrtc.org>
Commit-Queue: Jakob Ivarsson <jakobi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34361}
---
 modules/audio_coding/neteq/neteq_impl.cc      |  3 +-
 .../audio_coding/neteq/neteq_impl_unittest.cc | 70 +++++++++++++++++++
 2 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/modules/audio_coding/neteq/neteq_impl.cc b/modules/audio_coding/neteq/neteq_impl.cc
index e9ddbb92a1..fed037bbb1 100644
--- a/modules/audio_coding/neteq/neteq_impl.cc
+++ b/modules/audio_coding/neteq/neteq_impl.cc
@@ -628,8 +628,7 @@ int NetEqImpl::InsertPacketInternal(const RTPHeader& rtp_header,
     if (update_sample_rate_and_channels) {
       nack_->Reset();
     }
-    nack_->UpdateLastReceivedPacket(rtp_header.sequenceNumber,
-                                    rtp_header.timestamp);
+    nack_->UpdateLastReceivedPacket(main_sequence_number, main_timestamp);
   }
 
   // Check for RED payload type, and separate payloads into several packets.
diff --git a/modules/audio_coding/neteq/neteq_impl_unittest.cc b/modules/audio_coding/neteq/neteq_impl_unittest.cc
index b3c25cae2d..53b4dae17d 100644
--- a/modules/audio_coding/neteq/neteq_impl_unittest.cc
+++ b/modules/audio_coding/neteq/neteq_impl_unittest.cc
@@ -736,6 +736,15 @@ class NetEqImplTestSampleRateParameter
   const int initial_sample_rate_hz_;
 };
 
+class NetEqImplTestSdpFormatParameter
+    : public NetEqImplTest,
+      public testing::WithParamInterface<SdpAudioFormat> {
+ protected:
+  NetEqImplTestSdpFormatParameter()
+      : NetEqImplTest(), sdp_format_(GetParam()) {}
+  const SdpAudioFormat sdp_format_;
+};
+
 // This test does the following:
 // 0. Set up NetEq with initial sample rate given by test parameter, and a codec
 //    sample rate of 16000.
@@ -919,6 +928,67 @@ INSTANTIATE_TEST_SUITE_P(SampleRates,
                          NetEqImplTestSampleRateParameter,
                          testing::Values(8000, 16000, 32000, 48000));
 
+TEST_P(NetEqImplTestSdpFormatParameter, GetNackListScaledTimestamp) {
+  UseNoMocks();
+  CreateInstance();
+
+  neteq_->EnableNack(128);
+
+  const uint8_t kPayloadType = 17;  // Just an arbitrary number.
+  const int kPayloadSampleRateHz = sdp_format_.clockrate_hz;
+  const size_t kPayloadLengthSamples =
+      static_cast<size_t>(10 * kPayloadSampleRateHz / 1000);  // 10 ms.
+  const size_t kPayloadLengthBytes = kPayloadLengthSamples * 2;
+  std::vector<uint8_t> payload(kPayloadLengthBytes, 0);
+  RTPHeader rtp_header;
+  rtp_header.payloadType = kPayloadType;
+  rtp_header.sequenceNumber = 0x1234;
+  rtp_header.timestamp = 0x12345678;
+  rtp_header.ssrc = 0x87654321;
+
+  EXPECT_TRUE(neteq_->RegisterPayloadType(kPayloadType, sdp_format_));
+
+  auto insert_packet = [&](bool lost = false) {
+    rtp_header.sequenceNumber++;
+    rtp_header.timestamp += kPayloadLengthSamples;
+    if (!lost)
+      EXPECT_EQ(NetEq::kOK, neteq_->InsertPacket(rtp_header, payload));
+  };
+
+  // Insert and decode 10 packets.
+  for (size_t i = 0; i < 10; ++i) {
+    insert_packet();
+  }
+  AudioFrame output;
+  size_t count_loops = 0;
+  do {
+    bool muted;
+    // Make sure we don't hang the test if we never go to PLC.
+    ASSERT_LT(++count_loops, 100u);
+    EXPECT_EQ(NetEq::kOK, neteq_->GetAudio(&output, &muted));
+  } while (output.speech_type_ == AudioFrame::kNormalSpeech);
+
+  insert_packet();
+
+  insert_packet(/*lost=*/true);
+
+  // Ensure packet gets marked as missing.
+  for (int i = 0; i < 5; ++i) {
+    insert_packet();
+  }
+
+  // Missing packet recoverable with 5ms RTT.
+  EXPECT_THAT(neteq_->GetNackList(5), Not(IsEmpty()));
+
+  // No packets should have TimeToPlay > 500ms.
+  EXPECT_THAT(neteq_->GetNackList(500), IsEmpty());
+}
+
+INSTANTIATE_TEST_SUITE_P(GetNackList,
+                         NetEqImplTestSdpFormatParameter,
+                         testing::Values(SdpAudioFormat("g722", 8000, 1),
+                                         SdpAudioFormat("opus", 48000, 2)));
+
 // This test verifies that NetEq can handle comfort noise and enters/quits codec
 // internal CNG mode properly.
 TEST_F(NetEqImplTest, CodecInternalCng) {