Moved codec-specific audio packet splitting into decoders.

webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc

+/*
+ *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "webrtc/modules/audio_coding/acm2/rent_a_codec.h"
+#include "webrtc/modules/audio_coding/codecs/split_by_samples.h"
+
+namespace webrtc {
+
+using NetEqDecoder = acm2::RentACodec::NetEqDecoder;
+
+class SplitBySamplesTest : public ::testing::TestWithParam<NetEqDecoder> {
+ protected:
+  virtual void SetUp() {
+    decoder_type_ = GetParam();
+    switch (decoder_type_) {
+      case NetEqDecoder::kDecoderPCMu:
+      case NetEqDecoder::kDecoderPCMa:
+        bytes_per_ms_ = 8;
+        samples_per_ms_ = 8;
+        break;
+      case NetEqDecoder::kDecoderPCMu_2ch:
+      case NetEqDecoder::kDecoderPCMa_2ch:
+        bytes_per_ms_ = 2 * 8;
+        samples_per_ms_ = 8;
+        break;
+      case NetEqDecoder::kDecoderG722:
+        bytes_per_ms_ = 8;
+        samples_per_ms_ = 16;
+        break;
+      case NetEqDecoder::kDecoderPCM16B:
+        bytes_per_ms_ = 16;
+        samples_per_ms_ = 8;
+        break;
+      case NetEqDecoder::kDecoderPCM16Bwb:
+        bytes_per_ms_ = 32;
+        samples_per_ms_ = 16;
+        break;
+      case NetEqDecoder::kDecoderPCM16Bswb32kHz:
+        bytes_per_ms_ = 64;
+        samples_per_ms_ = 32;
+        break;
+      case NetEqDecoder::kDecoderPCM16Bswb48kHz:
+        bytes_per_ms_ = 96;
+        samples_per_ms_ = 48;
+        break;
+      case NetEqDecoder::kDecoderPCM16B_2ch:
+        bytes_per_ms_ = 2 * 16;
+        samples_per_ms_ = 8;
+        break;
+      case NetEqDecoder::kDecoderPCM16Bwb_2ch:
+        bytes_per_ms_ = 2 * 32;
+        samples_per_ms_ = 16;
+        break;
+      case NetEqDecoder::kDecoderPCM16Bswb32kHz_2ch:
+        bytes_per_ms_ = 2 * 64;
+        samples_per_ms_ = 32;
+        break;
+      case NetEqDecoder::kDecoderPCM16Bswb48kHz_2ch:
+        bytes_per_ms_ = 2 * 96;
+        samples_per_ms_ = 48;
+        break;
+      case NetEqDecoder::kDecoderPCM16B_5ch:
+        bytes_per_ms_ = 5 * 16;
+        samples_per_ms_ = 8;
+        break;
+      default:
+        assert(false);
+        break;
+    }
+  }
+  size_t bytes_per_ms_;
+  int samples_per_ms_;
+  NetEqDecoder decoder_type_;
+};

[kwiberg-webrtc, 2016/09/12 02:11:02]

Why use the NetEqDecoder enum, instead of feeding the test the numerical values
directly? And why test only the values used by our current codecs?

[ossu, 2016/09/12 11:26:38]

I took the test as much as possible as it was. My goal was to keep the same test
coverage as we had before. I'm not sure that's a good enough reason, though.

+
+// Test splitting sample-based payloads.
+TEST_P(SplitBySamplesTest, PayloadSizes) {
+  struct ExpectedSplit {
+    size_t payload_size_ms;
+    size_t num_splits;
+    // For simplicity. We only expect up to two packets per split.
+    size_t split_sizes[2];
+  };
+  // The payloads are expected to be split as follows:
+  // 10 ms -> 10 ms
+  // 20 ms -> 20 ms
+  // 30 ms -> 30 ms
+  // 40 ms -> 20 + 20 ms
+  // 50 ms -> 25 + 25 ms
+  // 60 ms -> 30 + 30 ms
+  ExpectedSplit expected_splits[] = {

[kwiberg-webrtc, 2016/09/12 02:11:02]

constexpr

[ossu, 2016/09/12 11:26:38]

Sure, but does it really matter? I mean, it's not externally accessible. Do you
expect the compiler might do something better if we tell it this is constexpr?
I'm not saying you're wrong, I'm just interested in what would be gained from it
in cases like these.

[kwiberg-webrtc, 2016/09/13 12:23:37]

Documentation. The important step is making it const; the reader can then tell
at a glance that this variable is read-only. Taking the next step to constexpr
makes it so that the reader can tell at a glance that this really is a
compile-time constant (arguably trivial to do anyway in this particular case,
but still...).

+    {10, 1, {10}},
+    {20, 1, {20}},
+    {30, 1, {30}},
+    {40, 2, {20, 20}},
+    {50, 2, {25, 25}},
+    {60, 2, {30, 30}}
+  };
+
+  // int expected_size_ms[] = {10, 20, 30, 20, 20, 25, 25, 30, 30};
+  // int expected_payload_value[] = {10, 20, 30, 40, 40, 50, 50, 60, 60};
+  // int expected_timestamp_offset_ms[] = {0, 0, 0, 0, 20, 0, 25, 0, 30};
+  // int expected_num_packets[] = {1, 1, 1, 2, 2, 2};

[kwiberg-webrtc, 2016/09/12 02:11:02]

Are these useful or not?

[ossu, 2016/09/12 11:26:38]

Useful during review to make sure the values above match the "old way" of
declaring that information, but shouldn't be kept when landing.

[kwiberg-webrtc, 2016/09/13 12:23:37]

Acknowledged.

+
+  for (const auto& expected_split : expected_splits) {
+    // The payload values are set to be the same as the payload_size, so that
+    // one can distinguish from which packet the split payloads come from.
+    size_t payload_size_bytes = expected_split.payload_size_ms * bytes_per_ms_;

[kwiberg-webrtc, 2016/09/12 02:11:02]

const

[ossu, 2016/09/12 11:26:38]

Acknowledged.

+    rtc::Buffer payload(payload_size_bytes);

[kwiberg-webrtc, 2016/09/12 02:11:02]

const

[ossu, 2016/09/12 11:26:38]

Acknowledged.

+    auto splits =

[kwiberg-webrtc, 2016/09/12 02:11:02]

const

[ossu, 2016/09/12 11:26:38]

Acknowledged.

+        internal::SplitBySamples(payload, bytes_per_ms_, samples_per_ms_);
+    EXPECT_EQ(expected_split.num_splits, splits.size());
+    uint32_t expected_timestamp = 0;
+    uint32_t expected_byte_offset = 0;
+    for (size_t i = 0; i != expected_split.num_splits; ++i) {
+      const auto& split = splits[i];
+      const size_t length_bytes =
+          expected_split.split_sizes[i] * bytes_per_ms_;
+      EXPECT_EQ(length_bytes, split.num_bytes);
+      EXPECT_EQ(expected_timestamp, split.timestamp_offset);
+      EXPECT_EQ(expected_byte_offset, split.byte_offset);
+      expected_timestamp += expected_split.split_sizes[i] * samples_per_ms_;
+      expected_byte_offset += length_bytes;
+    }
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(
+    AudioCodingUtils,
+    SplitBySamplesTest,
+    ::testing::Values(NetEqDecoder::kDecoderPCMu,
+                      NetEqDecoder::kDecoderPCMa,
+                      NetEqDecoder::kDecoderPCMu_2ch,
+                      NetEqDecoder::kDecoderPCMa_2ch,
+                      NetEqDecoder::kDecoderG722,
+                      NetEqDecoder::kDecoderPCM16B,
+                      NetEqDecoder::kDecoderPCM16Bwb,
+                      NetEqDecoder::kDecoderPCM16Bswb32kHz,
+                      NetEqDecoder::kDecoderPCM16Bswb48kHz,
+                      NetEqDecoder::kDecoderPCM16B_2ch,
+                      NetEqDecoder::kDecoderPCM16Bwb_2ch,
+                      NetEqDecoder::kDecoderPCM16Bswb32kHz_2ch,
+                      NetEqDecoder::kDecoderPCM16Bswb48kHz_2ch,
+                      NetEqDecoder::kDecoderPCM16B_5ch));
+
+}  // namespace webrtc