From 05103314e52e1b21e17dd4f0b28d038ed8942e2f Mon Sep 17 00:00:00 2001 From: nisse Date: Wed, 16 Mar 2016 02:22:50 -0700 Subject: [PATCH] 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 | 7 ++- webrtc/media/base/mediachannel.h | 32 +++++----- webrtc/media/engine/webrtcvideoengine2.cc | 32 +++------- webrtc/media/engine/webrtcvideoengine2.h | 3 +- .../engine/webrtcvideoengine2_unittest.cc | 59 +++++++++++-------- webrtc/pc/channel.cc | 4 +- 6 files changed, 69 insertions(+), 68 deletions(-) diff --git a/webrtc/media/base/fakemediaengine.h b/webrtc/media/base/fakemediaengine.h index c3f16603ca..8869667d7c 100644 --- a/webrtc/media/base/fakemediaengine.h +++ b/webrtc/media/base/fakemediaengine.h @@ -441,8 +441,7 @@ class FakeVideoMediaChannel : public RtpHelper { 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 { int max_bps_; }; +// Dummy option class, needed for the DataTraits abstraction in +// channel_unittest.c. +class DataOptions {}; + class FakeDataMediaChannel : public RtpHelper { public: explicit FakeDataMediaChannel(void* unused, const DataOptions& options) diff --git a/webrtc/media/base/mediachannel.h b/webrtc/media/base/mediachannel.h index d7aefe54ec..68efbe7984 100644 --- a/webrtc/media/base/mediachannel.h +++ b/webrtc/media/base/mediachannel.h @@ -842,8 +842,22 @@ struct RtpParameters { RtcpParameters rtcp; }; -template +template struct RtpSendParameters : RtpParameters { + 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 { std::string ToString() const override { std::ostringstream ost; ost << "{"; @@ -855,11 +869,7 @@ struct RtpSendParameters : RtpParameters { return ost.str(); } - int max_bandwidth_bps = -1; - Options options; -}; - -struct AudioSendParameters : RtpSendParameters { + AudioOptions options; }; struct AudioRecvParameters : RtpParameters { @@ -929,7 +939,7 @@ class VoiceMediaChannel : public MediaChannel { std::unique_ptr sink) = 0; }; -struct VideoSendParameters : RtpSendParameters { +struct VideoSendParameters : RtpSendParameters { // 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 { +struct DataSendParameters : RtpSendParameters { std::string ToString() const { std::ostringstream ost; // Options and extensions aren't used. diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 5f84887051..6442613081 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/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(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(new_options); - } - // Handle RTCP mode. if (params.rtcp.reduced_size != send_params_.rtcp.reduced_size) { changed_params->rtcp_mode = rtc::Optional( @@ -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) { diff --git a/webrtc/media/engine/webrtcvideoengine2.h b/webrtc/media/engine/webrtcvideoengine2.h index ccf753e9fc..4461027e91 100644 --- a/webrtc/media/engine/webrtcvideoengine2.h +++ b/webrtc/media/engine/webrtcvideoengine2.h @@ -190,7 +190,6 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { rtc::Optional> rtp_header_extensions; rtc::Optional max_bandwidth_bps; rtc::Optional conference_mode; - rtc::Optional options; rtc::Optional 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_; }; diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index f3f1ee855e..f5e4cff94d 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/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(enabled); - // TODO(nisse): Switch to using SetOptions? - channel_->SetSendParameters(params); + cricket::VideoOptions options; + options.video_noise_reduction = rtc::Optional(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(kScreenshareMinBitrateKbps); EXPECT_TRUE(channel_->SetSendParameters(parameters)); - AddSendStream(); + VideoOptions min_bitrate_options; + min_bitrate_options.screencast_min_bitrate_kbps = + rtc::Optional(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(true); - EXPECT_TRUE(channel_->SetSendParameters(parameters)); + VideoOptions screencast_options; + screencast_options.is_screencast = rtc::Optional(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(true); channel_->SetSendParameters(send_parameters_); AddSendStream(); - + VideoOptions options; + options.is_screencast = rtc::Optional(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(true); - EXPECT_TRUE(channel_->SetSendParameters(parameters)); + VideoOptions options; + options.is_screencast = rtc::Optional(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(true); - EXPECT_TRUE(channel_->SetSendParameters(parameters)); + VideoOptions options; + options.is_screencast = rtc::Optional(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(is_screenshare); EXPECT_TRUE(channel_->SetSendParameters(parameters)); AddSendStream(); + VideoOptions options; + options.is_screencast = rtc::Optional(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 - diff --git a/webrtc/pc/channel.cc b/webrtc/pc/channel.cc index e5658092c8..2920af53d0 100644 --- a/webrtc/pc/channel.cc +++ b/webrtc/pc/channel.cc @@ -133,10 +133,10 @@ void RtpParametersFromMediaDescription( params->rtcp.reduced_size = desc->rtcp_reduced_size(); } -template +template void RtpSendParametersFromMediaDescription( const MediaContentDescriptionImpl* desc, - RtpSendParameters* send_params) { + RtpSendParameters* send_params) { RtpParametersFromMediaDescription(desc, send_params); send_params->max_bandwidth_bps = desc->bandwidth(); }