diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 5e25a6587a..383f29a922 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -781,7 +781,7 @@ bool WebRtcVideoChannel2::GetChangedSendParameters( } // Handle max bitrate. - if (params.max_bandwidth_bps != bitrate_config_.max_bitrate_bps && + if (params.max_bandwidth_bps != send_params_.max_bandwidth_bps && params.max_bandwidth_bps >= 0) { // 0 uncaps max bitrate (-1). changed_params->max_bandwidth_bps = rtc::Optional( @@ -816,39 +816,38 @@ bool WebRtcVideoChannel2::SetSendParameters(const VideoSendParameters& params) { return false; } - bool bitrate_config_changed = false; - if (changed_params.codec) { const VideoCodecSettings& codec_settings = *changed_params.codec; send_codec_ = rtc::Optional(codec_settings); - LOG(LS_INFO) << "Using codec: " << codec_settings.codec.ToString(); - // TODO(holmer): Changing the codec parameters shouldn't necessarily mean - // that we change the min/max of bandwidth estimation. Reevaluate this. - bitrate_config_ = GetBitrateConfigForCodec(codec_settings.codec); - bitrate_config_changed = true; } if (changed_params.rtp_header_extensions) { send_rtp_extensions_ = *changed_params.rtp_header_extensions; } - if (changed_params.max_bandwidth_bps) { - // TODO(pbos): Figure out whether b=AS means max bitrate for this - // WebRtcVideoChannel2 (in which case we're good), or per sender (SSRC), in - // which case this should not set a Call::BitrateConfig but rather - // reconfigure all senders. - int max_bitrate_bps = *changed_params.max_bandwidth_bps; - bitrate_config_.start_bitrate_bps = -1; - bitrate_config_.max_bitrate_bps = max_bitrate_bps; - if (max_bitrate_bps > 0 && - bitrate_config_.min_bitrate_bps > max_bitrate_bps) { - bitrate_config_.min_bitrate_bps = max_bitrate_bps; + if (changed_params.codec || changed_params.max_bandwidth_bps) { + if (send_codec_) { + // TODO(holmer): Changing the codec parameters shouldn't necessarily mean + // that we change the min/max of bandwidth estimation. Reevaluate this. + bitrate_config_ = GetBitrateConfigForCodec(send_codec_->codec); + if (!changed_params.codec) { + // If the codec isn't changing, set the start bitrate to -1 which means + // "unchanged" so that BWE isn't affected. + bitrate_config_.start_bitrate_bps = -1; + } + } + if (params.max_bandwidth_bps >= 0) { + // Note that max_bandwidth_bps intentionally takes priority over the + // bitrate config for the codec. This allows FEC to be applied above the + // codec target bitrate. + // TODO(pbos): Figure out whether b=AS means max bitrate for this + // WebRtcVideoChannel2 (in which case we're good), or per sender (SSRC), + // in which case this should not set a Call::BitrateConfig but rather + // reconfigure all senders. + bitrate_config_.max_bitrate_bps = + params.max_bandwidth_bps == 0 ? -1 : params.max_bandwidth_bps; } - bitrate_config_changed = true; - } - - if (bitrate_config_changed) { call_->SetBitrateConfig(bitrate_config_); } diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index b202207f21..0a04fc78ca 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -2390,6 +2390,43 @@ TEST_F(WebRtcVideoChannel2Test, SetSendCodecsRejectsMaxLessThanMinBitrate) { EXPECT_FALSE(channel_->SetSendParameters(send_parameters_)); } +// Test that when both the codec-specific bitrate params and max_bandwidth_bps +// are present in the same send parameters, the settings are combined correctly. +TEST_F(WebRtcVideoChannel2Test, SetSendCodecsWithBitratesAndMaxSendBandwidth) { + send_parameters_.codecs[0].params[kCodecParamMinBitrate] = "100"; + send_parameters_.codecs[0].params[kCodecParamStartBitrate] = "200"; + send_parameters_.codecs[0].params[kCodecParamMaxBitrate] = "300"; + send_parameters_.max_bandwidth_bps = 400000; + EXPECT_TRUE(channel_->SetSendParameters(send_parameters_)); + EXPECT_EQ(100000, fake_call_->GetConfig().bitrate_config.min_bitrate_bps); + EXPECT_EQ(200000, fake_call_->GetConfig().bitrate_config.start_bitrate_bps); + // We expect max_bandwidth_bps to take priority, if set. + EXPECT_EQ(400000, fake_call_->GetConfig().bitrate_config.max_bitrate_bps); + + // Decrease max_bandwidth_bps. + send_parameters_.max_bandwidth_bps = 350000; + EXPECT_TRUE(channel_->SetSendParameters(send_parameters_)); + EXPECT_EQ(100000, fake_call_->GetConfig().bitrate_config.min_bitrate_bps); + // Since the codec isn't changing, start_bitrate_bps should be -1. + EXPECT_EQ(-1, fake_call_->GetConfig().bitrate_config.start_bitrate_bps); + EXPECT_EQ(350000, fake_call_->GetConfig().bitrate_config.max_bitrate_bps); + + // Now try again with the values flipped around. + send_parameters_.codecs[0].params[kCodecParamMaxBitrate] = "400"; + send_parameters_.max_bandwidth_bps = 300000; + EXPECT_TRUE(channel_->SetSendParameters(send_parameters_)); + EXPECT_EQ(100000, fake_call_->GetConfig().bitrate_config.min_bitrate_bps); + EXPECT_EQ(200000, fake_call_->GetConfig().bitrate_config.start_bitrate_bps); + EXPECT_EQ(300000, fake_call_->GetConfig().bitrate_config.max_bitrate_bps); + + // If we change the codec max, max_bandwidth_bps should still apply. + send_parameters_.codecs[0].params[kCodecParamMaxBitrate] = "350"; + EXPECT_TRUE(channel_->SetSendParameters(send_parameters_)); + EXPECT_EQ(100000, fake_call_->GetConfig().bitrate_config.min_bitrate_bps); + EXPECT_EQ(200000, fake_call_->GetConfig().bitrate_config.start_bitrate_bps); + EXPECT_EQ(300000, fake_call_->GetConfig().bitrate_config.max_bitrate_bps); +} + TEST_F(WebRtcVideoChannel2Test, SetMaxSendBandwidthShouldPreserveOtherBitrates) { SetSendCodecsShouldWorkForBitrates("100", 100000, "150", 150000, "200",