Drop VideoOptions from VideoSendParameters.

BUG=webrtc:5438

Review URL: https://codereview.webrtc.org/1695663003

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

webrtc/media/base/fakemediaengine.h

@@ -441,8 +441,7 @@ class FakeVideoMediaChannel : public RtpHelper<VideoMediaChannel> {
   virtual bool SetSendParameters(const VideoSendParameters& params) {
     return (SetSendCodecs(params.codecs) &&
             SetSendRtpHeaderExtensions(params.extensions) &&
-            SetMaxSendBandwidth(params.max_bandwidth_bps) &&
-            SetOptions(params.options));
+            SetMaxSendBandwidth(params.max_bandwidth_bps));
   }
 
   virtual bool SetRecvParameters(const VideoRecvParameters& params) {
@@ -543,6 +542,10 @@ class FakeVideoMediaChannel : public RtpHelper<VideoMediaChannel> {
   int max_bps_;
 };
 
+// Dummy option class, needed for the DataTraits abstraction in
+// channel_unittest.c.
+class DataOptions {};
+
 class FakeDataMediaChannel : public RtpHelper<DataMediaChannel> {
  public:
   explicit FakeDataMediaChannel(void* unused, const DataOptions& options)

webrtc/media/base/mediachannel.h

@@ -842,8 +842,22 @@ struct RtpParameters {
   RtcpParameters rtcp;
 };
 
-template <class Codec, class Options>
+template <class Codec>
 struct RtpSendParameters : RtpParameters<Codec> {
+  std::string ToString() const override {
+    std::ostringstream ost;
+    ost << "{";
+    ost << "codecs: " << VectorToString(this->codecs) << ", ";
+    ost << "extensions: " << VectorToString(this->extensions) << ", ";
+    ost << "max_bandwidth_bps: " << max_bandwidth_bps << ", ";
+    ost << "}";
+    return ost.str();
+  }
+
+  int max_bandwidth_bps = -1;
+};
+
+struct AudioSendParameters : RtpSendParameters<AudioCodec> {
   std::string ToString() const override {
     std::ostringstream ost;
     ost << "{";
@@ -855,11 +869,7 @@ struct RtpSendParameters : RtpParameters<Codec> {
     return ost.str();
   }
 
-  int max_bandwidth_bps = -1;
-  Options options;
-};
-
-struct AudioSendParameters : RtpSendParameters<AudioCodec, AudioOptions> {
+  AudioOptions options;
 };
 
 struct AudioRecvParameters : RtpParameters<AudioCodec> {
@@ -929,7 +939,7 @@ class VoiceMediaChannel : public MediaChannel {
       std::unique_ptr<webrtc::AudioSinkInterface> sink) = 0;
 };
 
-struct VideoSendParameters : RtpSendParameters<VideoCodec, VideoOptions> {
+struct VideoSendParameters : RtpSendParameters<VideoCodec> {
   // Use conference mode? This flag comes from the remote
   // description's SDP line 'a=x-google-flag:conference', copied over
   // by VideoChannel::SetRemoteContent_w, and ultimately used by
@@ -1050,13 +1060,7 @@ struct SendDataParams {
 
 enum SendDataResult { SDR_SUCCESS, SDR_ERROR, SDR_BLOCK };
 
-struct DataOptions {
-  std::string ToString() const {
-    return "{}";
-  }
-};
-
-struct DataSendParameters : RtpSendParameters<DataCodec, DataOptions> {
+struct DataSendParameters : RtpSendParameters<DataCodec> {
   std::string ToString() const {
     std::ostringstream ost;
     // Options and extensions aren't used.

webrtc/media/engine/webrtcvideoengine2.cc

@@ -613,11 +613,10 @@ WebRtcVideoChannel2::WebRtcVideoChannel2(
       unsignalled_ssrc_handler_(&default_unsignalled_ssrc_handler_),
       video_config_(config.video),
       external_encoder_factory_(external_encoder_factory),
-      external_decoder_factory_(external_decoder_factory) {
+      external_decoder_factory_(external_decoder_factory),
+      default_send_options_(options) {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
 
-  send_params_.options = options;
-
   rtcp_receiver_report_ssrc_ = kDefaultRtcpReceiverReportSsrc;
   sending_ = false;
   default_send_ssrc_ = 0;
@@ -740,15 +739,6 @@ bool WebRtcVideoChannel2::GetChangedSendParameters(
         rtc::Optional<bool>(params.conference_mode);
   }
 
-  // Handle options.
-  // TODO(pbos): Require VideoSendParameters to contain a full set of options
-  // and check if params.options != options_ instead of applying a delta.
-  VideoOptions new_options = send_params_.options;
-  new_options.SetAll(params.options);
-  if (!(new_options == send_params_.options)) {
-    changed_params->options = rtc::Optional<VideoOptions>(new_options);
-  }
-
   // Handle RTCP mode.
   if (params.rtcp.reduced_size != send_params_.rtcp.reduced_size) {
     changed_params->rtcp_mode = rtc::Optional<webrtc::RtcpMode>(
@@ -807,9 +797,6 @@ bool WebRtcVideoChannel2::SetSendParameters(const VideoSendParameters& params) {
     call_->SetBitrateConfig(bitrate_config_);
   }
 
-  if (changed_params.options)
-    send_params_.options.SetAll(*changed_params.options);
-
   {
     rtc::CritScope stream_lock(&stream_crit_);
     for (auto& kv : send_streams_) {
@@ -1000,11 +987,11 @@ bool WebRtcVideoChannel2::AddSendStream(const StreamParams& sp) {
 
   webrtc::VideoSendStream::Config config(this);
   config.suspend_below_min_bitrate = video_config_.suspend_below_min_bitrate;
-  WebRtcVideoSendStream* stream =
-      new WebRtcVideoSendStream(call_, sp, config, external_encoder_factory_,
-                                video_config_.enable_cpu_overuse_detection,
-                                bitrate_config_.max_bitrate_bps, send_codec_,
-                                send_rtp_extensions_, send_params_);
+  WebRtcVideoSendStream* stream = new WebRtcVideoSendStream(
+      call_, sp, config, default_send_options_, external_encoder_factory_,
+      video_config_.enable_cpu_overuse_detection,
+      bitrate_config_.max_bitrate_bps, send_codec_, send_rtp_extensions_,
+      send_params_);
 
   uint32_t ssrc = sp.first_ssrc();
   RTC_DCHECK(ssrc != 0);
@@ -1470,6 +1457,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream(
     webrtc::Call* call,
     const StreamParams& sp,
     const webrtc::VideoSendStream::Config& config,
+    const VideoOptions& options,
     WebRtcVideoEncoderFactory* external_encoder_factory,
     bool enable_cpu_overuse_detection,
     int max_bitrate_bps,
@@ -1487,7 +1475,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream(
       capturer_(nullptr),
       external_encoder_factory_(external_encoder_factory),
       stream_(nullptr),
-      parameters_(config, send_params.options, max_bitrate_bps, codec_settings),
+      parameters_(config, options, max_bitrate_bps, codec_settings),
       pending_encoder_reconfiguration_(false),
       allocated_encoder_(nullptr, webrtc::kVideoCodecUnknown, false),
       sending_(false),
@@ -1783,8 +1771,6 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetSendParameters(
     if (params.conference_mode) {
       parameters_.conference_mode = *params.conference_mode;
     }
-    if (params.options)
-      SetOptions(*params.options);
 
     // Set codecs and options.
     if (params.codec) {

webrtc/media/engine/webrtcvideoengine2.h

@@ -190,7 +190,6 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport {
     rtc::Optional<std::vector<webrtc::RtpExtension>> rtp_header_extensions;
     rtc::Optional<int> max_bandwidth_bps;
     rtc::Optional<bool> conference_mode;
-    rtc::Optional<VideoOptions> options;
     rtc::Optional<webrtc::RtcpMode> rtcp_mode;
   };
 
@@ -234,6 +233,7 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport {
         webrtc::Call* call,
         const StreamParams& sp,
         const webrtc::VideoSendStream::Config& config,
+        const VideoOptions& options,
         WebRtcVideoEncoderFactory* external_encoder_factory,
         bool enable_cpu_overuse_detection,
         int max_bitrate_bps,
@@ -516,6 +516,7 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport {
   // TODO(deadbeef): Don't duplicate information between
   // send_params/recv_params, rtp_extensions, options, etc.
   VideoSendParameters send_params_;
+  VideoOptions default_send_options_;
   VideoRecvParameters recv_params_;
 };
 

webrtc/media/engine/webrtcvideoengine2_unittest.cc

@@ -1048,13 +1048,12 @@ class WebRtcVideoChannel2Test : public WebRtcVideoEngine2Test {
                                         bool expect_created_receive_stream);
 
   FakeVideoSendStream* SetDenoisingOption(
-      const cricket::VideoSendParameters& parameters,
+      uint32_t ssrc,
       cricket::FakeVideoCapturer* capturer,
       bool enabled) {
-    cricket::VideoSendParameters params = parameters;
-    params.options.video_noise_reduction = rtc::Optional<bool>(enabled);
-    // TODO(nisse): Switch to using SetOptions?
-    channel_->SetSendParameters(params);
+    cricket::VideoOptions options;
+    options.video_noise_reduction = rtc::Optional<bool>(enabled);
+    channel_->SetVideoSend(ssrc, true, &options);
     // Options only take effect on the next frame.
     EXPECT_TRUE(capturer->CaptureFrame());
 
@@ -1530,12 +1529,14 @@ TEST_F(WebRtcVideoChannel2Test, UsesCorrectSettingsForScreencast) {
   cricket::VideoCodec codec = kVp8Codec360p;
   cricket::VideoSendParameters parameters;
   parameters.codecs.push_back(codec);
-  parameters.options.screencast_min_bitrate_kbps =
-      rtc::Optional<int>(kScreenshareMinBitrateKbps);
   EXPECT_TRUE(channel_->SetSendParameters(parameters));
-
   AddSendStream();
 
+  VideoOptions min_bitrate_options;
+  min_bitrate_options.screencast_min_bitrate_kbps =
+      rtc::Optional<int>(kScreenshareMinBitrateKbps);
+  channel_->SetVideoSend(last_ssrc_, true, &min_bitrate_options);
+
   cricket::FakeVideoCapturer capturer;
   EXPECT_TRUE(channel_->SetCapturer(last_ssrc_, &capturer));
   cricket::VideoFormat capture_format_hd =
@@ -1565,8 +1566,9 @@ TEST_F(WebRtcVideoChannel2Test, UsesCorrectSettingsForScreencast) {
   // Removing a capturer triggers a black frame to be sent.
   EXPECT_EQ(2, send_stream->GetNumberOfSwappedFrames());
   EXPECT_TRUE(channel_->SetCapturer(last_ssrc_, &capturer));
-  parameters.options.is_screencast = rtc::Optional<bool>(true);
-  EXPECT_TRUE(channel_->SetSendParameters(parameters));
+  VideoOptions screencast_options;
+  screencast_options.is_screencast = rtc::Optional<bool>(true);
+  EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, &screencast_options));
   EXPECT_TRUE(capturer.CaptureFrame());
   // Send stream not recreated after option change.
   ASSERT_EQ(send_stream, fake_call_->GetVideoSendStreams().front());
@@ -1642,11 +1644,12 @@ TEST_F(WebRtcVideoChannel2Test,
   static const int kConferenceScreencastTemporalBitrateBps =
       ScreenshareLayerConfig::GetDefault().tl0_bitrate_kbps * 1000;
   send_parameters_.conference_mode = true;
-  send_parameters_.options.is_screencast = rtc::Optional<bool>(true);
   channel_->SetSendParameters(send_parameters_);
 
   AddSendStream();
-
+  VideoOptions options;
+  options.is_screencast = rtc::Optional<bool>(true);
+  channel_->SetVideoSend(last_ssrc_, true, &options);
   cricket::FakeVideoCapturer capturer;
   EXPECT_TRUE(channel_->SetCapturer(last_ssrc_, &capturer));
   cricket::VideoFormat capture_format_hd =
@@ -1730,14 +1733,14 @@ TEST_F(WebRtcVideoChannel2Test, VerifyVp8SpecificSettings) {
   EXPECT_TRUE(vp8_settings.denoisingOn)
       << "VP8 denoising should be on by default.";
 
-  stream = SetDenoisingOption(parameters, &capturer, false);
+  stream = SetDenoisingOption(last_ssrc_, &capturer, false);
 
   ASSERT_TRUE(stream->GetVp8Settings(&vp8_settings)) << "No VP8 config set.";
   EXPECT_FALSE(vp8_settings.denoisingOn);
   EXPECT_TRUE(vp8_settings.automaticResizeOn);
   EXPECT_TRUE(vp8_settings.frameDroppingOn);
 
-  stream = SetDenoisingOption(parameters, &capturer, true);
+  stream = SetDenoisingOption(last_ssrc_, &capturer, true);
 
   ASSERT_TRUE(stream->GetVp8Settings(&vp8_settings)) << "No VP8 config set.";
   EXPECT_TRUE(vp8_settings.denoisingOn);
@@ -1757,10 +1760,11 @@ TEST_F(WebRtcVideoChannel2Test, VerifyVp8SpecificSettings) {
   EXPECT_TRUE(vp8_settings.frameDroppingOn);
 
   // In screen-share mode, denoising is forced off and simulcast disabled.
-  parameters.options.is_screencast = rtc::Optional<bool>(true);
-  EXPECT_TRUE(channel_->SetSendParameters(parameters));
+  VideoOptions options;
+  options.is_screencast = rtc::Optional<bool>(true);
+  EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, &options));
 
-  stream = SetDenoisingOption(parameters, &capturer, false);
+  stream = SetDenoisingOption(last_ssrc_, &capturer, false);
 
   EXPECT_EQ(1, stream->GetVideoStreams().size());
   ASSERT_TRUE(stream->GetVp8Settings(&vp8_settings)) << "No VP8 config set.";
@@ -1769,7 +1773,7 @@ TEST_F(WebRtcVideoChannel2Test, VerifyVp8SpecificSettings) {
   EXPECT_FALSE(vp8_settings.automaticResizeOn);
   EXPECT_FALSE(vp8_settings.frameDroppingOn);
 
-  stream = SetDenoisingOption(parameters, &capturer, true);
+  stream = SetDenoisingOption(last_ssrc_, &capturer, true);
 
   ASSERT_TRUE(stream->GetVp8Settings(&vp8_settings)) << "No VP8 config set.";
   EXPECT_FALSE(vp8_settings.denoisingOn);
@@ -1822,31 +1826,32 @@ TEST_F(Vp9SettingsTest, VerifyVp9SpecificSettings) {
   EXPECT_FALSE(vp9_settings.denoisingOn)
       << "VP9 denoising should be off by default.";
 
-  stream = SetDenoisingOption(parameters, &capturer, false);
+  stream = SetDenoisingOption(last_ssrc_, &capturer, false);
 
   ASSERT_TRUE(stream->GetVp9Settings(&vp9_settings)) << "No VP9 config set.";
   EXPECT_FALSE(vp9_settings.denoisingOn);
   // Frame dropping always on for real time video.
   EXPECT_TRUE(vp9_settings.frameDroppingOn);
 
-  stream = SetDenoisingOption(parameters, &capturer, true);
+  stream = SetDenoisingOption(last_ssrc_, &capturer, true);
 
   ASSERT_TRUE(stream->GetVp9Settings(&vp9_settings)) << "No VP9 config set.";
   EXPECT_TRUE(vp9_settings.denoisingOn);
   EXPECT_TRUE(vp9_settings.frameDroppingOn);
 
   // In screen-share mode, denoising is forced off.
-  parameters.options.is_screencast = rtc::Optional<bool>(true);
-  EXPECT_TRUE(channel_->SetSendParameters(parameters));
+  VideoOptions options;
+  options.is_screencast = rtc::Optional<bool>(true);
+  EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, &options));
 
-  stream = SetDenoisingOption(parameters, &capturer, false);
+  stream = SetDenoisingOption(last_ssrc_, &capturer, false);
 
   ASSERT_TRUE(stream->GetVp9Settings(&vp9_settings)) << "No VP9 config set.";
   EXPECT_FALSE(vp9_settings.denoisingOn);
   // Frame dropping always off for screen sharing.
   EXPECT_FALSE(vp9_settings.frameDroppingOn);
 
-  stream = SetDenoisingOption(parameters, &capturer, false);
+  stream = SetDenoisingOption(last_ssrc_, &capturer, false);
 
   ASSERT_TRUE(stream->GetVp9Settings(&vp9_settings)) << "No VP9 config set.";
   EXPECT_FALSE(vp9_settings.denoisingOn);
@@ -1946,11 +1951,14 @@ void WebRtcVideoChannel2Test::TestCpuAdaptation(bool enable_overuse,
   channel_.reset(
       engine_.CreateChannel(fake_call_.get(), media_config, VideoOptions()));
 
-  parameters.options.is_screencast = rtc::Optional<bool>(is_screenshare);
   EXPECT_TRUE(channel_->SetSendParameters(parameters));
 
   AddSendStream();
 
+  VideoOptions options;
+  options.is_screencast = rtc::Optional<bool>(is_screenshare);
+  EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, &options));
+
   cricket::FakeVideoCapturer capturer;
   EXPECT_TRUE(channel_->SetCapturer(last_ssrc_, &capturer));
   EXPECT_EQ(cricket::CS_RUNNING,
@@ -3299,4 +3307,3 @@ TEST_F(WebRtcVideoChannel2SimulcastTest, SetSendCodecsWithOddSizeInSimulcast) {
   VerifySimulcastSettings(codec, 2, 2);
 }
 }  // namespace cricket
-

webrtc/pc/channel.cc

@@ -133,10 +133,10 @@ void RtpParametersFromMediaDescription(
   params->rtcp.reduced_size = desc->rtcp_reduced_size();
 }
 
-template <class Codec, class Options>
+template <class Codec>
 void RtpSendParametersFromMediaDescription(
     const MediaContentDescriptionImpl<Codec>* desc,
-    RtpSendParameters<Codec, Options>* send_params) {
+    RtpSendParameters<Codec>* send_params) {
   RtpParametersFromMediaDescription(desc, send_params);
   send_params->max_bandwidth_bps = desc->bandwidth();
 }