(Re-land) AudioEncoderDecoderIsac: Merge the two config structs

This reverts commit 599beb86, which in turn reverted 7c324cac. What
makes it work this time is that we don't remove the option of setting
bit_rate to 0 in order to ask for the default value.

COAUTHOR=henrik.lundin@webrtc.org
BUG=4228, chromium:478161
R=henrik.lundin@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/48199004

Cr-Commit-Position: refs/heads/master@{#9068}

webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h

@@ -25,8 +25,7 @@ class CriticalSectionWrapper;
 template <typename T>
 class AudioEncoderDecoderIsacT : public AudioEncoder, public AudioDecoder {
  public:
-  // For constructing an encoder in instantaneous mode. Allowed combinations
+  // Allowed combinations of sample rate, frame size, and bit rate are
-  // are
   //  - 16000 Hz, 30 ms, 10000-32000 bps
   //  - 16000 Hz, 60 ms, 10000-32000 bps
   //  - 32000 Hz, 30 ms, 10000-56000 bps (if T has super-wideband support)
@@ -34,34 +33,24 @@ class AudioEncoderDecoderIsacT : public AudioEncoder, public AudioDecoder {
   struct Config {
     Config();
     bool IsOk() const;
+
     int payload_type;
     int sample_rate_hz;
     int frame_size_ms;
-    int bit_rate;  // Limit on the short-term average bit rate, in bits/second.
+    int bit_rate;  // Limit on the short-term average bit rate, in bits/s.
-    int max_bit_rate;
     int max_payload_size_bytes;
-  };
+    int max_bit_rate;
 
-  // For constructing an encoder in channel-adaptive mode. Allowed combinations
+    // If true, the encoder will dynamically adjust frame size and bit rate;
-  // are
+    // the configured values are then merely the starting point.
-  //  - 16000 Hz, 30 ms, 10000-32000 bps
+    bool adaptive_mode;
-  //  - 16000 Hz, 60 ms, 10000-32000 bps
+
-  //  - 32000 Hz, 30 ms, 10000-56000 bps (if T has super-wideband support)
+    // In adaptive mode, prevent adaptive changes to the frame size. (Not used
-  //  - 48000 Hz, 30 ms, 10000-56000 bps (if T has super-wideband support)
+    // in nonadaptive mode.)
-  struct ConfigAdaptive {
+    bool enforce_frame_size;
-    ConfigAdaptive();
-    bool IsOk() const;
-    int payload_type;
-    int sample_rate_hz;
-    int initial_frame_size_ms;
-    int initial_bit_rate;
-    int max_bit_rate;
-    bool enforce_frame_size;  // Prevent adaptive changes to the frame size?
-    int max_payload_size_bytes;
   };
 
   explicit AudioEncoderDecoderIsacT(const Config& config);
-  explicit AudioEncoderDecoderIsacT(const ConfigAdaptive& config);
   ~AudioEncoderDecoderIsacT() override;
 
   // AudioEncoder public methods.

webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h

@@ -30,8 +30,10 @@ AudioEncoderDecoderIsacT<T>::Config::Config()
       sample_rate_hz(16000),
       frame_size_ms(30),
       bit_rate(kDefaultBitRate),
+      max_payload_size_bytes(-1),
       max_bit_rate(-1),
-      max_payload_size_bytes(-1) {
+      adaptive_mode(false),
+      enforce_frame_size(false) {
 }
 
 template <typename T>
@@ -47,7 +49,7 @@ bool AudioEncoderDecoderIsacT<T>::Config::IsOk() const {
       if (max_payload_size_bytes > 400)
         return false;
       return (frame_size_ms == 30 || frame_size_ms == 60) &&
-             ((bit_rate >= 10000 && bit_rate <= 32000) || bit_rate == 0);
+             (bit_rate == 0 || (bit_rate >= 10000 && bit_rate <= 32000));
     case 32000:
     case 48000:
       if (max_bit_rate > 160000)
@@ -56,46 +58,7 @@ bool AudioEncoderDecoderIsacT<T>::Config::IsOk() const {
         return false;
       return T::has_swb &&
              (frame_size_ms == 30 &&
-              ((bit_rate >= 10000 && bit_rate <= 56000) || bit_rate == 0));
+              (bit_rate == 0 || (bit_rate >= 10000 && bit_rate <= 56000)));
-    default:
-      return false;
-  }
-}
-
-template <typename T>
-AudioEncoderDecoderIsacT<T>::ConfigAdaptive::ConfigAdaptive()
-    : payload_type(kIsacPayloadType),
-      sample_rate_hz(16000),
-      initial_frame_size_ms(30),
-      initial_bit_rate(kDefaultBitRate),
-      max_bit_rate(-1),
-      enforce_frame_size(false),
-      max_payload_size_bytes(-1) {
-}
-
-template <typename T>
-bool AudioEncoderDecoderIsacT<T>::ConfigAdaptive::IsOk() const {
-  if (max_bit_rate < 32000 && max_bit_rate != -1)
-    return false;
-  if (max_payload_size_bytes < 120 && max_payload_size_bytes != -1)
-    return false;
-  switch (sample_rate_hz) {
-    case 16000:
-      if (max_bit_rate > 53400)
-        return false;
-      if (max_payload_size_bytes > 400)
-        return false;
-      return (initial_frame_size_ms == 30 || initial_frame_size_ms == 60) &&
-             initial_bit_rate >= 10000 && initial_bit_rate <= 32000;
-    case 32000:
-    case 48000:
-      if (max_bit_rate > 160000)
-        return false;
-      if (max_payload_size_bytes > 600)
-        return false;
-      return T::has_swb &&
-             (initial_frame_size_ms == 30 && initial_bit_rate >= 10000 &&
-              initial_bit_rate <= 56000);
     default:
       return false;
   }
@@ -110,11 +73,16 @@ AudioEncoderDecoderIsacT<T>::AudioEncoderDecoderIsacT(const Config& config)
       packet_in_progress_(false) {
   CHECK(config.IsOk());
   CHECK_EQ(0, T::Create(&isac_state_));
-  CHECK_EQ(0, T::EncoderInit(isac_state_, 1));
+  CHECK_EQ(0, T::EncoderInit(isac_state_, config.adaptive_mode ? 0 : 1));
   CHECK_EQ(0, T::SetEncSampRate(isac_state_, config.sample_rate_hz));
-  CHECK_EQ(0, T::Control(isac_state_, config.bit_rate == 0 ? kDefaultBitRate
+  const int bit_rate = config.bit_rate == 0 ? kDefaultBitRate : config.bit_rate;
-                                                           : config.bit_rate,
+  if (config.adaptive_mode) {
-                         config.frame_size_ms));
+    CHECK_EQ(0, T::ControlBwe(isac_state_, bit_rate,
+                              config.frame_size_ms, config.enforce_frame_size));
+
+  } else {
+    CHECK_EQ(0, T::Control(isac_state_, bit_rate, config.frame_size_ms));
+  }
   // When config.sample_rate_hz is set to 48000 Hz (iSAC-fb), the decoder is
   // still set to 32000 Hz, since there is no full-band mode in the decoder.
   CHECK_EQ(0, T::SetDecSampRate(isac_state_,
@@ -124,29 +92,7 @@ AudioEncoderDecoderIsacT<T>::AudioEncoderDecoderIsacT(const Config& config)
              T::SetMaxPayloadSize(isac_state_, config.max_payload_size_bytes));
   if (config.max_bit_rate != -1)
     CHECK_EQ(0, T::SetMaxRate(isac_state_, config.max_bit_rate));
-}
+  CHECK_EQ(0, T::DecoderInit(isac_state_));
-
-template <typename T>
-AudioEncoderDecoderIsacT<T>::AudioEncoderDecoderIsacT(
-    const ConfigAdaptive& config)
-    : payload_type_(config.payload_type),
-      state_lock_(CriticalSectionWrapper::CreateCriticalSection()),
-      decoder_sample_rate_hz_(0),
-      lock_(CriticalSectionWrapper::CreateCriticalSection()),
-      packet_in_progress_(false) {
-  CHECK(config.IsOk());
-  CHECK_EQ(0, T::Create(&isac_state_));
-  CHECK_EQ(0, T::EncoderInit(isac_state_, 0));
-  CHECK_EQ(0, T::SetEncSampRate(isac_state_, config.sample_rate_hz));
-  CHECK_EQ(0, T::ControlBwe(isac_state_, config.initial_bit_rate,
-                            config.initial_frame_size_ms,
-                            config.enforce_frame_size));
-  CHECK_EQ(0, T::SetDecSampRate(isac_state_, config.sample_rate_hz));
-  if (config.max_payload_size_bytes != -1)
-    CHECK_EQ(0,
-             T::SetMaxPayloadSize(isac_state_, config.max_payload_size_bytes));
-  if (config.max_bit_rate != -1)
-    CHECK_EQ(0, T::SetMaxRate(isac_state_, config.max_bit_rate));
 }
 
 template <typename T>

webrtc/modules/audio_coding/codecs/isac/main/source/audio_encoder_isac_unittest.cc

@@ -0,0 +1,56 @@
+/*
+ *  Copyright (c) 2015 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 <limits>
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "webrtc/modules/audio_coding/codecs/isac/main/interface/audio_encoder_isac.h"
+
+namespace webrtc {
+
+namespace {
+
+void TestBadConfig(const AudioEncoderDecoderIsac::Config& config) {
+  EXPECT_FALSE(config.IsOk());
+}
+
+void TestGoodConfig(const AudioEncoderDecoderIsac::Config& config) {
+  EXPECT_TRUE(config.IsOk());
+  AudioEncoderDecoderIsac ed(config);
+}
+
+// Wrap subroutine calls that test things in this, so that the error messages
+// will be accompanied by stack traces that make it possible to tell which
+// subroutine invocation caused the failure.
+#define S(x) do { SCOPED_TRACE(#x); x; } while (0)
+
+}  // namespace
+
+TEST(AudioEncoderIsacTest, TestConfigBitrate) {
+  AudioEncoderDecoderIsac::Config config;
+
+  // The default value is some real, positive value.
+  EXPECT_GT(config.bit_rate, 1);
+  S(TestGoodConfig(config));
+
+  // 0 is another way to ask for the default value.
+  config.bit_rate = 0;
+  S(TestGoodConfig(config));
+
+  // Try some unreasonable values and watch them fail.
+  config.bit_rate = -1;
+  S(TestBadConfig(config));
+  config.bit_rate = 1;
+  S(TestBadConfig(config));
+  config.bit_rate = std::numeric_limits<int>::max();
+  S(TestBadConfig(config));
+}
+
+}  // namespace webrtc

webrtc/modules/audio_coding/main/acm2/acm_generic_codec.cc

@@ -293,51 +293,34 @@ void ACMGenericCodec::ResetAudioEncoder() {
 #endif
 #ifdef WEBRTC_CODEC_ISACFX
   } else if (!STR_CASE_CMP(codec_inst.plname, "ISAC")) {
-    DCHECK_EQ(codec_inst.plfreq, 16000);
     is_isac_ = true;
-    AudioEncoderDecoderIsacFix* enc_dec;
-    if (codec_inst.rate == -1) {
-      // Adaptive mode.
-      AudioEncoderDecoderIsacFix::ConfigAdaptive config;
-      config.payload_type = codec_inst.pltype;
-      enc_dec = new AudioEncoderDecoderIsacFix(config);
-    } else {
-      // Channel independent mode.
     AudioEncoderDecoderIsacFix::Config config;
-      config.bit_rate = codec_inst.rate;
-      config.frame_size_ms = codec_inst.pacsize / 16;
     config.payload_type = codec_inst.pltype;
-      enc_dec = new AudioEncoderDecoderIsacFix(config);
+    config.sample_rate_hz = codec_inst.plfreq;
-    }
+    config.frame_size_ms = rtc::CheckedDivExact(codec_inst.pacsize, 16);
+    if (codec_inst.rate != -1)
+      config.bit_rate = codec_inst.rate;
+    config.max_payload_size_bytes = max_payload_size_bytes_;
+    config.max_bit_rate = max_rate_bps_;
+    config.adaptive_mode = (codec_inst.rate == -1);
+    auto* enc_dec = new AudioEncoderDecoderIsacFix(config);
     decoder_proxy_.SetDecoder(enc_dec);
     audio_encoder_.reset(enc_dec);
 #endif
 #ifdef WEBRTC_CODEC_ISAC
   } else if (!STR_CASE_CMP(codec_inst.plname, "ISAC")) {
     is_isac_ = true;
-    AudioEncoderDecoderIsac* enc_dec;
-    if (codec_inst.rate == -1) {
-      // Adaptive mode.
-      AudioEncoderDecoderIsac::ConfigAdaptive config;
-      config.sample_rate_hz = codec_inst.plfreq;
-      config.initial_frame_size_ms = rtc::CheckedDivExact(
-          1000 * codec_inst.pacsize, config.sample_rate_hz);
-      config.max_payload_size_bytes = max_payload_size_bytes_;
-      config.max_bit_rate = max_rate_bps_;
-      config.payload_type = codec_inst.pltype;
-      enc_dec = new AudioEncoderDecoderIsac(config);
-    } else {
-      // Channel independent mode.
     AudioEncoderDecoderIsac::Config config;
+    config.payload_type = codec_inst.pltype;
     config.sample_rate_hz = codec_inst.plfreq;
+    config.frame_size_ms =
+        rtc::CheckedDivExact(1000 * codec_inst.pacsize, config.sample_rate_hz);
+    if (codec_inst.rate != -1)
       config.bit_rate = codec_inst.rate;
-      config.frame_size_ms = rtc::CheckedDivExact(1000 * codec_inst.pacsize,
-                                                  config.sample_rate_hz);
     config.max_payload_size_bytes = max_payload_size_bytes_;
     config.max_bit_rate = max_rate_bps_;
-      config.payload_type = codec_inst.pltype;
+    config.adaptive_mode = (codec_inst.rate == -1);
-      enc_dec = new AudioEncoderDecoderIsac(config);
+    auto* enc_dec = new AudioEncoderDecoderIsac(config);
-    }
     decoder_proxy_.SetDecoder(enc_dec);
     audio_encoder_.reset(enc_dec);
 #endif

webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc

@@ -361,6 +361,7 @@ class AudioDecoderIsacFloatTest : public AudioDecoderTest {
     AudioEncoderDecoderIsac::Config config;
     config.payload_type = payload_type_;
     config.sample_rate_hz = codec_input_rate_hz_;
+    config.adaptive_mode = false;
     config.frame_size_ms =
         1000 * static_cast<int>(frame_size_) / codec_input_rate_hz_;
 
@@ -380,6 +381,7 @@ class AudioDecoderIsacSwbTest : public AudioDecoderTest {
     AudioEncoderDecoderIsac::Config config;
     config.payload_type = payload_type_;
     config.sample_rate_hz = codec_input_rate_hz_;
+    config.adaptive_mode = false;
     config.frame_size_ms =
         1000 * static_cast<int>(frame_size_) / codec_input_rate_hz_;
 
@@ -399,6 +401,7 @@ class AudioDecoderIsacFixTest : public AudioDecoderTest {
     AudioEncoderDecoderIsacFix::Config config;
     config.payload_type = payload_type_;
     config.sample_rate_hz = codec_input_rate_hz_;
+    config.adaptive_mode = false;
     config.frame_size_ms =
         1000 * static_cast<int>(frame_size_) / codec_input_rate_hz_;
 

webrtc/modules/modules.gyp

@@ -108,6 +108,7 @@
             'audio_coding/codecs/isac/fix/source/filterbanks_unittest.cc',
             'audio_coding/codecs/isac/fix/source/lpc_masking_model_unittest.cc',
             'audio_coding/codecs/isac/fix/source/transform_unittest.cc',
+            'audio_coding/codecs/isac/main/source/audio_encoder_isac_unittest.cc',
             'audio_coding/codecs/isac/main/source/isac_unittest.cc',
             'audio_coding/codecs/opus/audio_encoder_opus_unittest.cc',
             'audio_coding/codecs/opus/opus_unittest.cc',