Creating controller manager from config string in audio network adaptor.

webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc

Index: webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc
diff --git a/webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc b/webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc
index 9f05b7e7353b6fe16cbfe14592c935fe44c72ca1..4baa98727b35bd21fcf20b2c8687af67d5a3e35d 100644
--- a/webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc
+++ b/webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc
@@ -13,10 +13,115 @@
 #include <cmath>
 #include <utility>
 
+#include "webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h"
+#include "webrtc/modules/audio_coding/audio_network_adaptor/channel_controller.h"
+#include "webrtc/modules/audio_coding/audio_network_adaptor/dtx_controller.h"
+#include "webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h"
+#include "webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h"
 #include "webrtc/system_wrappers/include/clock.h"
 
+#ifdef WEBRTC_AUDIO_NETWORK_ADAPTOR_DEBUG_DUMP
+#ifdef WEBRTC_ANDROID_PLATFORM_BUILD
+#include "external/webrtc/webrtc/modules/audio_coding/audio_network_adaptor/config.pb.h"
+#else
+#include "webrtc/modules/audio_coding/audio_network_adaptor/config.pb.h"
+#endif
+#endif
+
 namespace webrtc {
 
+namespace {
+
+#ifdef WEBRTC_AUDIO_NETWORK_ADAPTOR_DEBUG_DUMP
+
+std::unique_ptr<FecController> CreateFecController(
+    const audio_network_adaptor::config::FecController& config,
+    bool initial_fec_enabled,
+    const Clock* clock) {
+  RTC_DCHECK(config.has_fec_enabling_threshold() &&
+             config.has_fec_disabling_threshold() &&
+             config.has_time_constant_ms());

[kwiberg-webrtc, 2016/09/28 08:59:22]

Perfer to do

  CHECK(a);
  CHECK(b);

instead of

  CHECK(a && b);

You'll get better error messages that way.

[minyue-webrtc, 2016/09/28 12:47:26]

Done.

+
+  auto fec_enabling_threshold = config.fec_enabling_threshold();

[hlundin-webrtc, 2016/09/27 20:21:15]

Why create this copy?

[minyue-webrtc, 2016/09/27 20:50:20]

right. should be auto&

+  RTC_DCHECK(fec_enabling_threshold.has_low_bandwidth_bps() &&
+             fec_enabling_threshold.has_low_bandwidth_packet_loss() &&
+             fec_enabling_threshold.has_high_bandwidth_bps() &&
+             fec_enabling_threshold.has_high_bandwidth_packet_loss());

[kwiberg-webrtc, 2016/09/28 08:59:22]

What will happen if you read a protobuf variable that isn't present? If it isn't
safe, you should probably change all these DCHECKs to CHECKs.

[minyue-webrtc, 2016/09/29 11:56:26]

Done.

+
+  auto fec_disabling_threshold = config.fec_disabling_threshold();
+  RTC_DCHECK(fec_disabling_threshold.has_low_bandwidth_bps() &&
+             fec_disabling_threshold.has_low_bandwidth_packet_loss() &&
+             fec_disabling_threshold.has_high_bandwidth_bps() &&
+             fec_disabling_threshold.has_high_bandwidth_packet_loss());
+
+  return std::unique_ptr<FecController>(new FecController(FecController::Config(
+      initial_fec_enabled,
+      FecController::Config::Threshold(

[hlundin-webrtc, 2016/09/27 20:21:15]

What is the type for fec_enabling_threshold? Isn't it an
FecController::Config::Threshold? If so, why re-packetize it as yet another one?
This pattern repeats several times below.

[minyue-webrtc, 2016/09/27 20:50:20]

because this functions are mainly converting from protobuf to
xxxController::Config

[kwiberg-webrtc, 2016/09/28 08:59:22]

Why do you convert?

[minyue-webrtc, 2016/09/28 12:47:26]

Any better ideas?

[kwiberg-webrtc, 2016/09/28 13:10:24]

Using protobuf directly, instead of converting it to a struct. There may
presumably be good reasons for not doing that (centralized parsing+error
checking?, performance?, ...), which is why I was asking.

[minyue-webrtc, 2016/09/28 16:32:34]

We thought about using protobuf for all Controller configs. The main reason for
ending it here is because we do not want to use protobuf too extensively, since
it may not be supported. When we disable protobuf, we should still be able to
build Controllers.

+          fec_enabling_threshold.low_bandwidth_bps(),
+          fec_enabling_threshold.low_bandwidth_packet_loss(),
+          fec_enabling_threshold.high_bandwidth_bps(),
+          fec_enabling_threshold.high_bandwidth_packet_loss()),
+      FecController::Config::Threshold(
+          fec_disabling_threshold.low_bandwidth_bps(),
+          fec_disabling_threshold.low_bandwidth_packet_loss(),
+          fec_disabling_threshold.high_bandwidth_bps(),
+          fec_disabling_threshold.high_bandwidth_packet_loss()),
+      config.has_time_constant_ms(), clock)));
+}
+
+std::unique_ptr<FrameLengthController> CreateFrameLengthController(
+    const audio_network_adaptor::config::FrameLengthController& config,
+    const std::vector<int>& encoder_frame_lengths_ms,
+    int initial_frame_length_ms) {
+  RTC_DCHECK(config.has_fl_increasing_packet_loss_fraction() &&
+             config.has_fl_decreasing_packet_loss_fraction() &&
+             config.has_fl_20ms_to_60ms_bandwidth_bps() &&
+             config.has_fl_60ms_to_20ms_bandwidth_bps());
+
+  return std::unique_ptr<FrameLengthController>(
+      new FrameLengthController(FrameLengthController::Config(
+          encoder_frame_lengths_ms, initial_frame_length_ms,
+          config.fl_increasing_packet_loss_fraction(),
+          config.fl_decreasing_packet_loss_fraction(),
+          config.fl_20ms_to_60ms_bandwidth_bps(),
+          config.fl_60ms_to_20ms_bandwidth_bps())));
+}
+
+std::unique_ptr<ChannelController> CreateChannelController(
+    const audio_network_adaptor::config::ChannelController& config,
+    size_t num_encoder_channels,
+    size_t intial_channels_to_encode) {
+  RTC_DCHECK(config.has_channel_1_to_2_bandwidth_bps() &&
+             config.has_channel_2_to_1_bandwidth_bps());
+
+  return std::unique_ptr<ChannelController>(new ChannelController(
+      ChannelController::Config(num_encoder_channels, intial_channels_to_encode,
+                                config.channel_1_to_2_bandwidth_bps(),
+                                config.channel_2_to_1_bandwidth_bps())));
+}
+
+std::unique_ptr<DtxController> CreateDtxController(
+    const audio_network_adaptor::config::DtxController& dtx_config,
+    bool initial_dtx_enabled) {
+  RTC_DCHECK(dtx_config.has_dtx_enabling_bandwidth_bps() &&
+             dtx_config.has_dtx_disabling_bandwidth_bps());
+
+  return std::unique_ptr<DtxController>(new DtxController(DtxController::Config(
+      initial_dtx_enabled, dtx_config.dtx_enabling_bandwidth_bps(),
+      dtx_config.dtx_disabling_bandwidth_bps())));
+}
+
+using audio_network_adaptor::BitrateController;
+std::unique_ptr<BitrateController> CreateBitrateController(
+    int initial_bitrate_bps,
+    int initial_frame_length_ms) {
+  return std::unique_ptr<BitrateController>(new BitrateController(
+      BitrateController::Config(initial_bitrate_bps, initial_frame_length_ms)));
+}
+#endif  // WEBRTC_AUDIO_NETWORK_ADAPTOR_DEBUG_DUMP
+
+}  // namespace
+
 ControllerManagerImpl::Config::Config(int min_reordering_time_ms,
                                       float min_reordering_squared_distance,
                                       const Clock* clock)
@@ -26,6 +131,76 @@ ControllerManagerImpl::Config::Config(int min_reordering_time_ms,
 
 ControllerManagerImpl::Config::~Config() = default;
 
+std::unique_ptr<ControllerManager> ControllerManagerImpl::Create(
+    const std::string& config_string,
+    size_t num_encoder_channels,
+    const std::vector<int>& encoder_frame_lengths_ms,
+    size_t intial_channels_to_encode,
+    int initial_frame_length_ms,
+    int initial_bitrate_bps,
+    bool initial_fec_enabled,
+    bool initial_dtx_enabled,
+    const Clock* clock) {
+#ifdef WEBRTC_AUDIO_NETWORK_ADAPTOR_DEBUG_DUMP
+  audio_network_adaptor::config::ControllerManager controller_manager_config;
+  controller_manager_config.ParseFromString(config_string);
+
+  std::vector<std::unique_ptr<Controller>> controllers;
+  std::map<const Controller*, std::pair<int, float>> chracteristic_points;
+
+  for (int i = 0; i < controller_manager_config.controllers_size(); ++i) {
+    auto controller_config = controller_manager_config.controllers(i);
+    std::unique_ptr<Controller> controller;
+    switch (controller_config.controller_case()) {
+      case audio_network_adaptor::config::Controller::kFecController:
+        controller = CreateFecController(controller_config.fec_controller(),
+                                         initial_fec_enabled, clock);
+        break;
+      case audio_network_adaptor::config::Controller::kFrameLengthController:
+        controller = CreateFrameLengthController(
+            controller_config.frame_length_controller(),
+            encoder_frame_lengths_ms, initial_frame_length_ms);
+        break;
+      case audio_network_adaptor::config::Controller::kChannelController:
+        controller = CreateChannelController(
+            controller_config.channel_controller(), num_encoder_channels,
+            intial_channels_to_encode);
+        break;
+      case audio_network_adaptor::config::Controller::kDtxController:
+        controller = CreateDtxController(controller_config.dtx_controller(),
+                                         initial_dtx_enabled);
+        break;
+      case audio_network_adaptor::config::Controller::kBitrateController:
+        controller = CreateBitrateController(initial_bitrate_bps,
+                                             initial_frame_length_ms);
+        break;
+      default:

[hlundin-webrtc, 2016/09/27 20:21:15]

What is controller_config.controller_case()? Is it an enum? If so, you can skip
the default: case. If you skip the default case, the compiler will enforce that
you handle all values in the enum.

Otherwise, change RTC_DCHECK(false) to RTC_NOTREACHED().

[minyue-webrtc, 2016/09/27 20:50:20]

yes, it is enum generated by protobuf. It has an additional ONEOF_NAME_NOT_SET
thing. We can put that explicitly.

[hlundin-webrtc, 2016/09/29 09:31:52]

Then I think RTC_NOTREACHED() is the correct way.

[minyue-webrtc, 2016/09/29 11:56:26]

Done.

+        RTC_DCHECK(false);

[kwiberg-webrtc, 2016/09/28 08:59:22]

RTC_NOTREACHED?

[minyue-webrtc, 2016/09/29 11:56:26]

Done.

+    }
+    if (controller_config.has_scoring_point()) {
+      auto characteristic_point = controller_config.scoring_point();
+      RTC_DCHECK(characteristic_point.has_uplink_bandwidth_bps());
+      RTC_DCHECK(characteristic_point.has_uplink_packet_loss_fraction());
+      chracteristic_points[controller.get()] = std::make_pair<int, float>(
+          characteristic_point.uplink_bandwidth_bps(),
+          characteristic_point.uplink_packet_loss_fraction());
+    }
+    controllers.push_back(std::move(controller));
+  }
+
+  RTC_DCHECK(controller_manager_config.has_min_reordering_time_ms());
+  RTC_DCHECK(controller_manager_config.has_min_reordering_squared_distance());
+  return std::unique_ptr<ControllerManagerImpl>(new ControllerManagerImpl(
+      ControllerManagerImpl::Config(
+          controller_manager_config.min_reordering_time_ms(),
+          controller_manager_config.min_reordering_squared_distance(), clock),
+      std::move(controllers), chracteristic_points));
+#else
+  RTC_DCHECK(false);

[hlundin-webrtc, 2016/09/27 20:21:15]

RTC_NOTREACHED?

[minyue-webrtc, 2016/09/27 20:50:20]

ok, will do.

+  return nullptr;
+#endif  // WEBRTC_AUDIO_NETWORK_ADAPTOR_DEBUG_DUMP
+}
+
 ControllerManagerImpl::ControllerManagerImpl(const Config& config)
     : ControllerManagerImpl(
           config,