NetEq: Simplify DecoderDatabase::DecoderInfo

webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc

Index: webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc
diff --git a/webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc b/webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc
index b1cb45d2014299ce0a2c5c1fbd10c35c0cd573c5..144a19db5871c22bbbf2beb4b3819fc9b66d9266 100644
--- a/webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc
+++ b/webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc
@@ -23,9 +23,9 @@ namespace webrtc {
 
 TEST(TimestampScaler, TestNoScaling) {
   MockDecoderDatabase db;
-  DecoderDatabase::DecoderInfo info;
-  info.codec_type =
-      NetEqDecoder::kDecoderPCMu;  // Does not use scaled timestamps.
+  // Use PCMu, because it doesn't use scaled timestamps.
+  const DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderPCMu, "", 8000,
+                                          nullptr);
   static const uint8_t kRtpPayloadType = 0;
   EXPECT_CALL(db, GetDecoderInfo(kRtpPayloadType))
       .WillRepeatedly(Return(&info));
@@ -44,9 +44,9 @@ TEST(TimestampScaler, TestNoScaling) {
 
 TEST(TimestampScaler, TestNoScalingLargeStep) {
   MockDecoderDatabase db;
-  DecoderDatabase::DecoderInfo info;
-  info.codec_type =
-      NetEqDecoder::kDecoderPCMu;  // Does not use scaled timestamps.
+  // Use PCMu, because it doesn't use scaled timestamps.
+  const DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderPCMu, "", 8000,
+                                          nullptr);
   static const uint8_t kRtpPayloadType = 0;
   EXPECT_CALL(db, GetDecoderInfo(kRtpPayloadType))
       .WillRepeatedly(Return(&info));
@@ -70,8 +70,9 @@ TEST(TimestampScaler, TestNoScalingLargeStep) {
 
 TEST(TimestampScaler, TestG722) {
   MockDecoderDatabase db;
-  DecoderDatabase::DecoderInfo info;
-  info.codec_type = NetEqDecoder::kDecoderG722;  // Uses a factor 2 scaling.
+  // Use G722, which has a factor 2 scaling.
+  const DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,

[hlundin-webrtc, 2016/04/19 06:47:00]

FYI: This is the typical place where setting an rtp_timestamp_rate other than
the sample rate makes sense, and will be necessary when NetEqDecoder is removed.

[kwiberg-webrtc, 2016/04/19 08:57:30]

Yes. But when we start using a decoder factory that's injected into NetEq, this
information can no longer come from anyplace other than that factory;
specifically, we can't just supply it as a parameter to DecoderInfo. The factory
will have to be able to answer questions about the specifications it uses to
create decoders, I guess.

[hlundin-webrtc, 2016/04/19 09:31:30]

Acknowledged.

+                                          nullptr);
   static const uint8_t kRtpPayloadType = 17;
   EXPECT_CALL(db, GetDecoderInfo(kRtpPayloadType))
       .WillRepeatedly(Return(&info));
@@ -94,8 +95,9 @@ TEST(TimestampScaler, TestG722) {
 
 TEST(TimestampScaler, TestG722LargeStep) {
   MockDecoderDatabase db;
-  DecoderDatabase::DecoderInfo info;
-  info.codec_type = NetEqDecoder::kDecoderG722;  // Uses a factor 2 scaling.
+  // Use G722, which has a factor 2 scaling.
+  const DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,
+                                          nullptr);
   static const uint8_t kRtpPayloadType = 17;
   EXPECT_CALL(db, GetDecoderInfo(kRtpPayloadType))
       .WillRepeatedly(Return(&info));
@@ -122,10 +124,11 @@ TEST(TimestampScaler, TestG722LargeStep) {
 
 TEST(TimestampScaler, TestG722WithCng) {
   MockDecoderDatabase db;
-  DecoderDatabase::DecoderInfo info_g722, info_cng;
-  info_g722.codec_type =
-      NetEqDecoder::kDecoderG722;  // Uses a factor 2 scaling.
-  info_cng.codec_type = NetEqDecoder::kDecoderCNGwb;
+  // Use G722, which has a factor 2 scaling.
+  const DecoderDatabase::DecoderInfo info_g722(NetEqDecoder::kDecoderG722, "",
+                                               8000, nullptr);

[hlundin-webrtc, 2016/04/19 06:47:00]

Hmmm, what does 8000 represent here? Sample rate or RTP timestamp rate? The
sample rate should be 16000 Hz for G.722, but the RTP timestamp rate should be
8000. I think one interpretation (of this inermediate state) is that the number
is the audio sample rate, while the NetEqDecoder type defines the RTP sample
rate. WDYT?

[kwiberg-webrtc, 2016/04/19 08:57:31]

I guess... but I don't know if the current state of the code is coherent enough
to draw such conclusions. For one, this CL demonstrates that rtp_sample_rate_hz
was never set to anything other than fs_hz, and besides, no one was looking at
it, so effectively it didn't exist.

Regarding this particular piece of code: I put 8000 in here because that's what
the default value was before I removed it. I'm not claiming it makes sense, just
that the new code uses the same value as the old code...

[hlundin-webrtc, 2016/04/19 09:31:30]

Acknowledged.

+  const DecoderDatabase::DecoderInfo info_cng(NetEqDecoder::kDecoderCNGwb, "",
+                                              8000, nullptr);

[hlundin-webrtc, 2016/04/19 06:46:59]

This is where things get messy with the current design. CNGwb should (and I
believe will) produce comfort noise at a sample rate of 16000 Hz. But, I believe
that it is correct from a payload registration point of view to register it as
having an RTP sample rate of 8000. This does not make sense with what I wrote
above...

[kwiberg-webrtc, 2016/04/19 08:57:31]

The design for audio decoder (and encoder) factories that ossu@ and I have come
up with calls for the specification to the factory to use basically untranslated
SDP parameters. The downside is that those don't always map straightforwardly to
the parameters the codec will actually use, but at least it'll be consistent and
well-defined, and we can put all the translation logic in the factory, which
actually has some business knowing about those gory details for its codecs.

   static const uint8_t kRtpPayloadTypeG722 = 17;
   static const uint8_t kRtpPayloadTypeCng = 13;
   EXPECT_CALL(db, GetDecoderInfo(kRtpPayloadTypeG722))
@@ -164,9 +167,9 @@ TEST(TimestampScaler, TestG722WithCng) {
 // as many tests here.
 TEST(TimestampScaler, TestG722Packet) {
   MockDecoderDatabase db;
-  DecoderDatabase::DecoderInfo info;
-  info.codec_type =
-      NetEqDecoder::kDecoderG722;  // Does uses a factor 2 scaling.
+  // Use G722, which has a factor 2 scaling.
+  const DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,

[hlundin-webrtc, 2016/04/19 06:46:59]

16000

[kwiberg-webrtc, 2016/04/19 08:57:31]

Done. (I updated all these Herzes to match the actual sample rate of the codec
in question. Since the values were wrong before, I'm pretty sure it doesn't
actually matter, though.)

+                                          nullptr);
   static const uint8_t kRtpPayloadType = 17;
   EXPECT_CALL(db, GetDecoderInfo(kRtpPayloadType))
       .WillRepeatedly(Return(&info));
@@ -193,8 +196,9 @@ TEST(TimestampScaler, TestG722Packet) {
 // we are not doing as many tests here.
 TEST(TimestampScaler, TestG722PacketList) {
   MockDecoderDatabase db;
-  DecoderDatabase::DecoderInfo info;
-  info.codec_type = NetEqDecoder::kDecoderG722;  // Uses a factor 2 scaling.
+  // Use G722, which has a factor 2 scaling.
+  const DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,

[hlundin-webrtc, 2016/04/19 06:47:00]

16000

[kwiberg-webrtc, 2016/04/19 08:57:31]

Done.

+                                          nullptr);
   static const uint8_t kRtpPayloadType = 17;
   EXPECT_CALL(db, GetDecoderInfo(kRtpPayloadType))
       .WillRepeatedly(Return(&info));
@@ -222,8 +226,9 @@ TEST(TimestampScaler, TestG722PacketList) {
 
 TEST(TimestampScaler, TestG722Reset) {
   MockDecoderDatabase db;
-  DecoderDatabase::DecoderInfo info;
-  info.codec_type = NetEqDecoder::kDecoderG722;  // Uses a factor 2 scaling.
+  // Use G722, which has a factor 2 scaling.
+  const DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,

[hlundin-webrtc, 2016/04/19 06:46:59]

16000

[kwiberg-webrtc, 2016/04/19 08:57:31]

Done.

+                                          nullptr);
   static const uint8_t kRtpPayloadType = 17;
   EXPECT_CALL(db, GetDecoderInfo(kRtpPayloadType))
       .WillRepeatedly(Return(&info));
@@ -262,8 +267,8 @@ TEST(TimestampScaler, TestG722Reset) {
 // timestamp scaler.
 TEST(TimestampScaler, TestOpusLargeStep) {
   MockDecoderDatabase db;
-  DecoderDatabase::DecoderInfo info;
-  info.codec_type = NetEqDecoder::kDecoderOpus;
+  const DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderOpus, "", 8000,

[hlundin-webrtc, 2016/04/19 06:47:00]

48000 for Opus.

[kwiberg-webrtc, 2016/04/19 08:57:30]

Done.

+                                          nullptr);
   static const uint8_t kRtpPayloadType = 17;
   EXPECT_CALL(db, GetDecoderInfo(kRtpPayloadType))
       .WillRepeatedly(Return(&info));