Reland "Reland "Move rtp-specific config out of EncoderSettings.""

This reverts commit 6c2c13a. Reason for revert: Intend to investigate and fix perf problems. Original change's description: > Revert "Reland "Move rtp-specific config out of EncoderSettings."" > > This reverts commit 04dd176. > > Reason for revert: Regression in ramp up perf tests. > > Original change's description: > > Reland "Move rtp-specific config out of EncoderSettings." > > > > This is a reland of bc900cb > > > > Original change's description: > > > Move rtp-specific config out of EncoderSettings. > > > > > > In VideoSendStream::Config, move payload_name and payload_type from > > > EncoderSettings to Rtp. > > > > > > EncoderSettings now contains configuration for VideoStreamEncoder only, > > > and should perhaps be renamed in a follow up cl. It's no longer > > > passed as an argument to VideoCodecInitializer::SetupCodec. > > > > > > The latter then needs a different way to know the codec type, > > > which is provided by a new codec_type member in VideoEncoderConfig. > > > > > > Bug: webrtc:8830 > > > Change-Id: Ifcc691aef1ee6a95e43c0452c5e630d92a511cd6 > > > Reviewed-on: https://webrtc-review.googlesource.com/62062 > > > Commit-Queue: Niels Moller <[email protected]> > > > Reviewed-by: Magnus Jedvert <[email protected]> > > > Reviewed-by: Stefan Holmer <[email protected]> > > > Reviewed-by: Rasmus Brandt <[email protected]> > > > Cr-Commit-Position: refs/heads/master@{#22532} > > > > Bug: webrtc:8830 > > Change-Id: If88ef7d57cdaa4fae3c7b2a97ea5a6e1b833e019 > > Reviewed-on: https://webrtc-review.googlesource.com/63721 > > Reviewed-by: Rasmus Brandt <[email protected]> > > Reviewed-by: Stefan Holmer <[email protected]> > > Commit-Queue: Niels Moller <[email protected]> > > Cr-Commit-Position: refs/heads/master@{#22595} > > [email protected],[email protected],[email protected],[email protected] > > Bug: webrtc:8830,chromium:827080 > Change-Id: Iaaf146de91ec5c0d741b8efdf143f7e173084fef > Reviewed-on: https://webrtc-review.googlesource.com/65520 > Commit-Queue: Niels Moller <[email protected]> > Reviewed-by: Niels Moller <[email protected]> > Cr-Commit-Position: refs/heads/master@{#22677} [email protected],[email protected],[email protected],[email protected] # Not skipping CQ checks because original CL landed > 1 day ago. Bug: webrtc:8830, chromium:827080 Change-Id: I9b62987bf5daced90dfeb3ebb6739c80117c487f Reviewed-on: https://webrtc-review.googlesource.com/66862 Commit-Queue: Niels Moller <[email protected]> Reviewed-by: Niels Moller <[email protected]> Cr-Commit-Position: refs/heads/master@{#22751}
darkGoldC · Apr 5, 2018 · 259a497 · 259a497
1 parent 70ceb08
commit 259a497
Show file tree

Hide file tree

Showing 32 changed files with 161 additions and 164 deletions.
diff --git a/call/bitrate_estimator_tests.cc b/call/bitrate_estimator_tests.cc
@@ -118,10 +118,9 @@ class BitrateEstimatorTest : public test::CallTest {
       video_send_config_.rtp.ssrcs.push_back(kVideoSendSsrcs[0]);
       // Encoders will be set separately per stream.
       video_send_config_.encoder_settings.encoder = nullptr;
-      video_send_config_.encoder_settings.payload_name = "FAKE";
-      video_send_config_.encoder_settings.payload_type =
-          kFakeVideoSendPayloadType;
-      test::FillEncoderConfiguration(1, &video_encoder_config_);
+      video_send_config_.rtp.payload_name = "FAKE";
+      video_send_config_.rtp.payload_type = kFakeVideoSendPayloadType;
+      test::FillEncoderConfiguration(kVideoCodecVP8, 1, &video_encoder_config_);
 
       receive_config_ = VideoReceiveStream::Config(receive_transport_.get());
       // receive_config_.decoders will be set by every stream separately.
@@ -182,10 +181,8 @@ class BitrateEstimatorTest : public test::CallTest {
 
       VideoReceiveStream::Decoder decoder;
       decoder.decoder = &fake_decoder_;
-      decoder.payload_type =
-          test_->video_send_config_.encoder_settings.payload_type;
-      decoder.payload_name =
-          test_->video_send_config_.encoder_settings.payload_name;
+      decoder.payload_type = test_->video_send_config_.rtp.payload_type;
+      decoder.payload_name = test_->video_send_config_.rtp.payload_name;
       test_->receive_config_.decoders.clear();
       test_->receive_config_.decoders.push_back(decoder);
       test_->receive_config_.rtp.remote_ssrc =

diff --git a/call/call.cc b/call/call.cc
@@ -135,8 +135,8 @@ std::unique_ptr<rtclog::StreamConfig> CreateRtcLogStreamConfig(
   rtclog_config->rtcp_mode = config.rtp.rtcp_mode;
   rtclog_config->rtp_extensions = config.rtp.extensions;
 
-  rtclog_config->codecs.emplace_back(config.encoder_settings.payload_name,
-                                     config.encoder_settings.payload_type,
+  rtclog_config->codecs.emplace_back(config.rtp.payload_name,
+                                     config.rtp.payload_type,
                                      config.rtp.rtx.payload_type);
   return rtclog_config;
 }

diff --git a/call/rampup_tests.cc b/call/rampup_tests.cc
@@ -220,7 +220,7 @@ void RampUpTester::ModifyVideoConfigs(
       recv_config.rtp.rtx_ssrc = video_rtx_ssrcs_[i];
       recv_config.rtp
           .rtx_associated_payload_types[send_config->rtp.rtx.payload_type] =
-          send_config->encoder_settings.payload_type;
+          send_config->rtp.payload_type;
     }
     ++i;
   }

diff --git a/call/video_config.h b/call/video_config.h
@@ -126,10 +126,6 @@ class VideoEncoderConfig {
   ~VideoEncoderConfig();
   std::string ToString() const;
 
-  // TODO(nisse): This codec_type member is intended to be the new way
-  // to say which codec to use, when
-  // VideoSendStream::Config::EncoderSettings::payload_name is
-  // deleted. For the transition, both need to coexist.
   VideoCodecType codec_type;
   rtc::scoped_refptr<VideoStreamFactoryInterface> video_stream_factory;
   std::vector<SpatialLayer> spatial_layers;

diff --git a/call/video_send_stream.cc b/call/video_send_stream.cc
@@ -91,9 +91,7 @@ std::string VideoSendStream::Config::ToString() const {
 std::string VideoSendStream::Config::EncoderSettings::ToString() const {
   char buf[1024];
   rtc::SimpleStringBuilder ss(buf);
-  ss << "{payload_name: " << payload_name;
-  ss << ", payload_type: " << payload_type;
-  ss << ", encoder_factory: "
+  ss << "{encoder_factory: "
      << (encoder_factory ? "(VideoEncoderFactory)" : "(nullptr)");
   ss << ", encoder: " << (encoder ? "(VideoEncoder)" : "nullptr");
   ss << '}';
@@ -132,6 +130,8 @@ std::string VideoSendStream::Config::Rtp::ToString() const {
 
   ss << ", nack: {rtp_history_ms: " << nack.rtp_history_ms << '}';
   ss << ", ulpfec: " << ulpfec.ToString();
+  ss << ", payload_name: " << payload_name;
+  ss << ", payload_type: " << payload_type;
 
   ss << ", flexfec: {payload_type: " << flexfec.payload_type;
   ss << ", ssrc: " << flexfec.ssrc;

diff --git a/call/video_send_stream.h b/call/video_send_stream.h
@@ -113,19 +113,9 @@ class VideoSendStream {
 
     struct EncoderSettings {
       EncoderSettings() = default;
-      EncoderSettings(std::string payload_name,
-                      int payload_type,
-                      VideoEncoder* encoder)
-          : payload_name(std::move(payload_name)),
-            payload_type(payload_type),
-            encoder(encoder) {}
+      explicit EncoderSettings(VideoEncoder* encoder) : encoder(encoder) {}
       std::string ToString() const;
 
-      // TODO(nisse): About to be deleted. Unused if the corresponding
-      // fields in the below Rtp struct are set.
-      std::string payload_name;
-      int payload_type = -1;
-
       // TODO(sophiechang): Delete this field when no one is using internal
       // sources anymore.
       bool internal_source = false;

diff --git a/media/engine/fakewebrtccall.cc b/media/engine/fakewebrtccall.cc
@@ -229,19 +229,19 @@ void FakeVideoSendStream::ReconfigureVideoEncoder(
   if (config.encoder_specific_settings != NULL) {
     const unsigned char num_temporal_layers = static_cast<unsigned char>(
         video_streams_.back().num_temporal_layers.value_or(1));
-    if (config_.encoder_settings.payload_name == "VP8") {
+    if (config_.rtp.payload_name == "VP8") {
       config.encoder_specific_settings->FillVideoCodecVp8(&vpx_settings_.vp8);
       if (!video_streams_.empty()) {
         vpx_settings_.vp8.numberOfTemporalLayers = num_temporal_layers;
       }
-    } else if (config_.encoder_settings.payload_name == "VP9") {
+    } else if (config_.rtp.payload_name == "VP9") {
       config.encoder_specific_settings->FillVideoCodecVp9(&vpx_settings_.vp9);
       if (!video_streams_.empty()) {
         vpx_settings_.vp9.numberOfTemporalLayers = num_temporal_layers;
       }
     } else {
       ADD_FAILURE() << "Unsupported encoder payload: "
-                    << config_.encoder_settings.payload_name;
+                    << config_.rtp.payload_name;
     }
   }
   codec_settings_set_ = config.encoder_specific_settings != NULL;

diff --git a/media/engine/webrtcvideoengine.cc b/media/engine/webrtcvideoengine.cc
@@ -1757,8 +1757,8 @@ void WebRtcVideoChannel::WebRtcVideoSendStream::SetCodec(
   parameters_.config.encoder_settings.internal_source =
       info.has_internal_source;
 
-  parameters_.config.encoder_settings.payload_name = codec_settings.codec.name;
-  parameters_.config.encoder_settings.payload_type = codec_settings.codec.id;
+  parameters_.config.rtp.payload_name = codec_settings.codec.name;
+  parameters_.config.rtp.payload_type = codec_settings.codec.id;
   parameters_.config.rtp.ulpfec = codec_settings.ulpfec;
   parameters_.config.rtp.flexfec.payload_type =
       codec_settings.flexfec_payload_type;
@@ -1914,6 +1914,8 @@ WebRtcVideoChannel::WebRtcVideoSendStream::CreateVideoEncoderConfig(
     const VideoCodec& codec) const {
   RTC_DCHECK_RUN_ON(&thread_checker_);
   webrtc::VideoEncoderConfig encoder_config;
+  encoder_config.codec_type = webrtc::PayloadStringToCodecType(codec.name);
+
   bool is_screencast = parameters_.options.is_screencast.value_or(false);
   if (is_screencast) {
     encoder_config.min_transmit_bitrate_bps =

diff --git a/media/engine/webrtcvideoengine_unittest.cc b/media/engine/webrtcvideoengine_unittest.cc
@@ -133,7 +133,7 @@ rtc::scoped_refptr<webrtc::VideoFrameBuffer> CreateBlackFrameBuffer(
 void VerifySendStreamHasRtxTypes(const webrtc::VideoSendStream::Config& config,
                                  const std::map<int, int>& rtx_types) {
   std::map<int, int>::const_iterator it;
-  it = rtx_types.find(config.encoder_settings.payload_type);
+  it = rtx_types.find(config.rtp.payload_type);
   EXPECT_TRUE(it != rtx_types.end() &&
               it->second == config.rtp.rtx.payload_type);
 

diff --git a/modules/video_coding/include/video_codec_initializer.h b/modules/video_coding/include/video_codec_initializer.h
@@ -13,8 +13,10 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
+#include "call/video_config.h"
 #include "call/video_send_stream.h"
 
 namespace webrtc {
@@ -33,12 +35,25 @@ class VideoCodecInitializer {
   // GetBitrateAllocator is called implicitly from here, no need to call again.
   static bool SetupCodec(
       const VideoEncoderConfig& config,
-      const VideoSendStream::Config::EncoderSettings settings,
       const std::vector<VideoStream>& streams,
       bool nack_enabled,
       VideoCodec* codec,
       std::unique_ptr<VideoBitrateAllocator>* bitrate_allocator);
 
+  // TODO(nisse): Deprecated version, with an additional ignored argument.
+  // Delete as soon as downstream users are updated, together with above
+  // includes of "call/video_send_stream.h" and <utility>.
+  static bool SetupCodec(
+      const VideoEncoderConfig& config,
+      const VideoSendStream::Config::EncoderSettings /* settings */,
+      const std::vector<VideoStream>& streams,
+      bool nack_enabled,
+      VideoCodec* codec,
+      std::unique_ptr<VideoBitrateAllocator>* bitrate_allocator) {
+    return SetupCodec(config, streams, nack_enabled, codec,
+                      std::move(bitrate_allocator));
+}
+
   // Create a bitrate allocator for the specified codec. |tl_factory| is
   // optional, if it is populated, ownership of that instance will be
   // transferred to the VideoBitrateAllocator instance.
@@ -49,8 +64,6 @@ class VideoCodecInitializer {
   static VideoCodec VideoEncoderConfigToVideoCodec(
       const VideoEncoderConfig& config,
       const std::vector<VideoStream>& streams,
-      // TODO(nisse): Delete when we can rely on config.codec_type.
-      VideoCodecType codec_type,
       bool nack_enabled);
 };
 

diff --git a/modules/video_coding/video_codec_initializer.cc b/modules/video_coding/video_codec_initializer.cc
@@ -28,23 +28,15 @@ namespace webrtc {
 
 bool VideoCodecInitializer::SetupCodec(
     const VideoEncoderConfig& config,
-    const VideoSendStream::Config::EncoderSettings settings,
     const std::vector<VideoStream>& streams,
     bool nack_enabled,
     VideoCodec* codec,
     std::unique_ptr<VideoBitrateAllocator>* bitrate_allocator) {
-  VideoCodecType codec_type = config.codec_type;
-  // TODO(nisse): Transition hack, the intention is to delete the
-  // |settings| argument and require configuration via
-  // config.codec_type.
-  if (codec_type == kVideoCodecUnknown) {
-    codec_type = PayloadStringToCodecType(settings.payload_name);
-  }
-  if (codec_type == kVideoCodecMultiplex) {
+  if (config.codec_type == kVideoCodecMultiplex) {
     VideoEncoderConfig associated_config = config.Copy();
     associated_config.codec_type = kVideoCodecVP9;
-    if (!SetupCodec(associated_config, settings /* ignored */, streams,
-                    nack_enabled, codec, bitrate_allocator)) {
+    if (!SetupCodec(associated_config, streams, nack_enabled, codec,
+                    bitrate_allocator)) {
       RTC_LOG(LS_ERROR) << "Failed to create stereo encoder configuration.";
       return false;
     }
@@ -53,7 +45,7 @@ bool VideoCodecInitializer::SetupCodec(
   }
 
   *codec =
-      VideoEncoderConfigToVideoCodec(config, streams, codec_type, nack_enabled);
+      VideoEncoderConfigToVideoCodec(config, streams, nack_enabled);
   *bitrate_allocator = CreateBitrateAllocator(*codec);
 
   return true;
@@ -82,15 +74,14 @@ VideoCodecInitializer::CreateBitrateAllocator(const VideoCodec& codec) {
 VideoCodec VideoCodecInitializer::VideoEncoderConfigToVideoCodec(
     const VideoEncoderConfig& config,
     const std::vector<VideoStream>& streams,
-    VideoCodecType codec_type,
     bool nack_enabled) {
   static const int kEncoderMinBitrateKbps = 30;
   RTC_DCHECK(!streams.empty());
   RTC_DCHECK_GE(config.min_transmit_bitrate_bps, 0);
 
   VideoCodec video_codec;
   memset(&video_codec, 0, sizeof(video_codec));
-  video_codec.codecType = codec_type;
+  video_codec.codecType = config.codec_type;
 
   switch (config.content_type) {
     case VideoEncoderConfig::ContentType::kRealtimeVideo:

diff --git a/modules/video_coding/video_codec_initializer_unittest.cc b/modules/video_coding/video_codec_initializer_unittest.cc
@@ -20,10 +20,6 @@
 namespace webrtc {
 
 namespace {
-static const char* kVp8PayloadName = "VP8";
-static const int kVp8PayloadType = 100;
-static const char* kVp9PayloadName = "VP9";
-static const int kVp9PayloadType = 120;
 static const int kDefaultWidth = 1280;
 static const int kDefaultHeight = 720;
 static const int kDefaultFrameRate = 30;
@@ -52,6 +48,8 @@ class VideoCodecInitializerTest : public ::testing::Test {
                 int num_temporal_streams,
                 bool screenshare) {
     config_ = VideoEncoderConfig();
+    config_.codec_type = type;
+
     if (screenshare) {
       config_.min_transmit_bitrate_bps = kDefaultMinTransmitBitrateBps;
       config_.content_type = VideoEncoderConfig::ContentType::kScreen;
@@ -63,16 +61,12 @@ class VideoCodecInitializerTest : public ::testing::Test {
       vp8_settings.numberOfTemporalLayers = num_temporal_streams;
       config_.encoder_specific_settings = new rtc::RefCountedObject<
           webrtc::VideoEncoderConfig::Vp8EncoderSpecificSettings>(vp8_settings);
-      settings_.payload_name = kVp8PayloadName;
-      settings_.payload_type = kVp8PayloadType;
     } else if (type == VideoCodecType::kVideoCodecVP9) {
       VideoCodecVP9 vp9_settings = VideoEncoder::GetDefaultVp9Settings();
       vp9_settings.numberOfSpatialLayers = num_spatial_streams;
       vp9_settings.numberOfTemporalLayers = num_temporal_streams;
       config_.encoder_specific_settings = new rtc::RefCountedObject<
           webrtc::VideoEncoderConfig::Vp9EncoderSpecificSettings>(vp9_settings);
-      settings_.payload_name = kVp9PayloadName;
-      settings_.payload_type = kVp9PayloadType;
     } else if (type != VideoCodecType::kVideoCodecMultiplex) {
       ADD_FAILURE() << "Unexpected codec type: " << type;
     }
@@ -82,8 +76,8 @@ class VideoCodecInitializerTest : public ::testing::Test {
     codec_out_ = VideoCodec();
     bitrate_allocator_out_.reset();
     temporal_layers_.clear();
-    if (!VideoCodecInitializer::SetupCodec(config_, settings_, streams_,
-                                           nack_enabled_, &codec_out_,
+    if (!VideoCodecInitializer::SetupCodec(config_, streams_, nack_enabled_,
+                                           &codec_out_,
                                            &bitrate_allocator_out_)) {
       return false;
     }
@@ -127,7 +121,6 @@ class VideoCodecInitializerTest : public ::testing::Test {
 
   // Input settings.
   VideoEncoderConfig config_;
-  VideoSendStream::Config::EncoderSettings settings_;
   std::vector<VideoStream> streams_;
   bool nack_enabled_;
 

diff --git a/test/call_test.cc b/test/call_test.cc
@@ -194,14 +194,15 @@ void CallTest::CreateVideoSendConfig(VideoSendStream::Config* video_config,
   RTC_DCHECK_LE(num_video_streams + num_used_ssrcs, kNumSsrcs);
   *video_config = VideoSendStream::Config(send_transport);
   video_config->encoder_settings.encoder = &fake_encoder_;
-  video_config->encoder_settings.payload_name = "FAKE";
-  video_config->encoder_settings.payload_type = kFakeVideoSendPayloadType;
+  video_config->rtp.payload_name = "FAKE";
+  video_config->rtp.payload_type = kFakeVideoSendPayloadType;
   video_config->rtp.extensions.push_back(
       RtpExtension(RtpExtension::kTransportSequenceNumberUri,
                    kTransportSequenceNumberExtensionId));
   video_config->rtp.extensions.push_back(RtpExtension(
       RtpExtension::kVideoContentTypeUri, kVideoContentTypeExtensionId));
-  FillEncoderConfiguration(num_video_streams, &video_encoder_config_);
+  FillEncoderConfiguration(kVideoCodecGeneric, num_video_streams,
+                           &video_encoder_config_);
 
   for (size_t i = 0; i < num_video_streams; ++i)
     video_config->rtp.ssrcs.push_back(kVideoSendSsrcs[num_used_ssrcs + i]);
@@ -257,7 +258,7 @@ CallTest::CreateMatchingVideoReceiveConfigs(
   video_config.renderer = &fake_renderer_;
   for (size_t i = 0; i < video_send_config.rtp.ssrcs.size(); ++i) {
     VideoReceiveStream::Decoder decoder =
-        test::CreateMatchingDecoder(video_send_config.encoder_settings);
+        test::CreateMatchingDecoder(video_send_config);
     allocated_decoders_.push_back(
         std::unique_ptr<VideoDecoder>(decoder.decoder));
     video_config.decoders.clear();

diff --git a/test/encoder_settings.cc b/test/encoder_settings.cc
@@ -78,10 +78,12 @@ std::vector<VideoStream> DefaultVideoStreamFactory::CreateEncoderStreams(
   return CreateVideoStreams(width, height, encoder_config);
 }
 
-void FillEncoderConfiguration(size_t num_streams,
+void FillEncoderConfiguration(VideoCodecType codec_type,
+                              size_t num_streams,
                               VideoEncoderConfig* configuration) {
   RTC_DCHECK_LE(num_streams, DefaultVideoStreamFactory::kMaxNumberOfStreams);
 
+  configuration->codec_type = codec_type;
   configuration->number_of_streams = num_streams;
   configuration->video_stream_factory =
       new rtc::RefCountedObject<DefaultVideoStreamFactory>();
@@ -94,23 +96,30 @@ void FillEncoderConfiguration(size_t num_streams,
 }
 
 VideoReceiveStream::Decoder CreateMatchingDecoder(
-    const VideoSendStream::Config::EncoderSettings& encoder_settings) {
+    int payload_type, const std::string& payload_name) {
   VideoReceiveStream::Decoder decoder;
-  decoder.payload_type = encoder_settings.payload_type;
-  decoder.payload_name = encoder_settings.payload_name;
-  if (encoder_settings.payload_name == "H264") {
+  decoder.payload_type = payload_type;
+  decoder.payload_name = payload_name;
+  if (payload_name == "H264") {
     decoder.decoder = H264Decoder::Create().release();
-  } else if (encoder_settings.payload_name == "VP8") {
+  } else if (payload_name == "VP8") {
     decoder.decoder = VP8Decoder::Create().release();
-  } else if (encoder_settings.payload_name == "VP9") {
+  } else if (payload_name == "VP9") {
     decoder.decoder = VP9Decoder::Create().release();
-  } else if (encoder_settings.payload_name == "multiplex") {
+  } else if (payload_name == "multiplex") {
     decoder.decoder = new MultiplexDecoderAdapter(
         new InternalDecoderFactory(), SdpVideoFormat(cricket::kVp9CodecName));
   } else {
     decoder.decoder = new FakeDecoder();
   }
   return decoder;
 }
+
+VideoReceiveStream::Decoder CreateMatchingDecoder(
+    const VideoSendStream::Config& config) {
+  return CreateMatchingDecoder(config.rtp.payload_type,
+                               config.rtp.payload_name);
+}
+
 }  // namespace test
 }  // namespace webrtc