From 6bb1b6e7fe5631e9f218b80292df5b64623c5616 Mon Sep 17 00:00:00 2001 From: pbos Date: Fri, 24 Jul 2015 07:10:18 -0700 Subject: [PATCH] Control combined_audio_video_bwe with config bool. Permits setting RTP extensions for AudioReceiveStream without enabling combined A/V BWE. This prevents spamming the log with "Failed to find extension id:". BUG=webrtc:4870 R=mflodman@webrtc.org, stefan@webrtc.org Review URL: https://codereview.webrtc.org/1256803004 Cr-Commit-Position: refs/heads/master@{#9633} --- talk/media/webrtc/webrtcvoiceengine.cc | 6 +- .../webrtc/webrtcvoiceengine_unittest.cc | 55 +++++++++---------- webrtc/audio_receive_stream.h | 3 + webrtc/video/audio_receive_stream.cc | 6 +- webrtc/video/bitrate_estimator_tests.cc | 1 + 5 files changed, 37 insertions(+), 34 deletions(-) diff --git a/talk/media/webrtc/webrtcvoiceengine.cc b/talk/media/webrtc/webrtcvoiceengine.cc index 6710539aed..6a448c8311 100644 --- a/talk/media/webrtc/webrtcvoiceengine.cc +++ b/talk/media/webrtc/webrtcvoiceengine.cc @@ -3637,9 +3637,9 @@ void WebRtcVoiceMediaChannel::TryAddAudioRecvStream(uint32 ssrc) { webrtc::AudioReceiveStream::Config config; config.rtp.remote_ssrc = ssrc; // Only add RTP extensions if we support combined A/V BWE. - if (options_.combined_audio_video_bwe.GetWithDefaultIfUnset(false)) { - config.rtp.extensions = recv_rtp_extensions_; - } + config.rtp.extensions = recv_rtp_extensions_; + config.combined_audio_video_bwe = + options_.combined_audio_video_bwe.GetWithDefaultIfUnset(false); config.voe_channel_id = channel->channel(); config.sync_group = receive_stream_params_[ssrc].sync_label; webrtc::AudioReceiveStream* s = call_->CreateAudioReceiveStream(config); diff --git a/talk/media/webrtc/webrtcvoiceengine_unittest.cc b/talk/media/webrtc/webrtcvoiceengine_unittest.cc index 77f437c60a..c559e39f83 100644 --- a/talk/media/webrtc/webrtcvoiceengine_unittest.cc +++ b/talk/media/webrtc/webrtcvoiceengine_unittest.cc @@ -3381,7 +3381,7 @@ TEST_F(WebRtcVoiceEngineTestFake, SetsSyncGroupFromSyncLabel) { media_channel->SetCall(nullptr); } -TEST_F(WebRtcVoiceEngineTestFake, ChangeCombinedBweOption_Call) { +TEST_F(WebRtcVoiceEngineTestFake, CanChangeCombinedBweOption) { // Test that changing the combined_audio_video_bwe option results in the // expected state changes on an associated Call. cricket::FakeCall call(webrtc::Call::Config(nullptr)); @@ -3399,42 +3399,42 @@ TEST_F(WebRtcVoiceEngineTestFake, ChangeCombinedBweOption_Call) { EXPECT_TRUE(media_channel->AddRecvStream( cricket::StreamParams::CreateLegacy(kAudioSsrc2))); - // Combined BWE should not be set up yet (no RTP extensions). + // Combined BWE should not be set up yet. EXPECT_EQ(2, call.GetAudioReceiveStreams().size()); - EXPECT_TRUE(call.GetAudioReceiveStream(kAudioSsrc1) - ->GetConfig() - .rtp.extensions.empty()); - EXPECT_TRUE(call.GetAudioReceiveStream(kAudioSsrc2) - ->GetConfig() - .rtp.extensions.empty()); + EXPECT_FALSE(call.GetAudioReceiveStream(kAudioSsrc1) + ->GetConfig() + .combined_audio_video_bwe); + EXPECT_FALSE(call.GetAudioReceiveStream(kAudioSsrc2) + ->GetConfig() + .combined_audio_video_bwe); // Enable combined BWE option - now it should be set up. cricket::AudioOptions options; options.combined_audio_video_bwe.Set(true); EXPECT_TRUE(media_channel->SetOptions(options)); EXPECT_EQ(2, call.GetAudioReceiveStreams().size()); - EXPECT_FALSE(call.GetAudioReceiveStream(kAudioSsrc1) - ->GetConfig() - .rtp.extensions.empty()); - EXPECT_FALSE(call.GetAudioReceiveStream(kAudioSsrc2) - ->GetConfig() - .rtp.extensions.empty()); + EXPECT_TRUE(call.GetAudioReceiveStream(kAudioSsrc1) + ->GetConfig() + .combined_audio_video_bwe); + EXPECT_TRUE(call.GetAudioReceiveStream(kAudioSsrc2) + ->GetConfig() + .combined_audio_video_bwe); // Disable combined BWE option - should be disabled again. options.combined_audio_video_bwe.Set(false); EXPECT_TRUE(media_channel->SetOptions(options)); EXPECT_EQ(2, call.GetAudioReceiveStreams().size()); - EXPECT_TRUE(call.GetAudioReceiveStream(kAudioSsrc1) - ->GetConfig() - .rtp.extensions.empty()); - EXPECT_TRUE(call.GetAudioReceiveStream(kAudioSsrc2) - ->GetConfig() - .rtp.extensions.empty()); + EXPECT_FALSE(call.GetAudioReceiveStream(kAudioSsrc1) + ->GetConfig() + .combined_audio_video_bwe); + EXPECT_FALSE(call.GetAudioReceiveStream(kAudioSsrc2) + ->GetConfig() + .combined_audio_video_bwe); media_channel->SetCall(nullptr); } -TEST_F(WebRtcVoiceEngineTestFake, ConfigureCombinedBwe_Call) { +TEST_F(WebRtcVoiceEngineTestFake, SetCallConfiguresAudioReceiveChannels) { // Test that calling SetCall() on the voice media channel results in the // expected state changes in Call. cricket::FakeCall call(webrtc::Call::Config(nullptr)); @@ -3445,9 +3445,6 @@ TEST_F(WebRtcVoiceEngineTestFake, ConfigureCombinedBwe_Call) { EXPECT_TRUE(SetupEngine()); cricket::WebRtcVoiceMediaChannel* media_channel = static_cast(channel_); - cricket::AudioOptions options; - options.combined_audio_video_bwe.Set(true); - EXPECT_TRUE(media_channel->SetOptions(options)); EXPECT_TRUE(media_channel->AddRecvStream( cricket::StreamParams::CreateLegacy(kAudioSsrc1))); EXPECT_TRUE(media_channel->AddRecvStream( @@ -3474,7 +3471,7 @@ TEST_F(WebRtcVoiceEngineTestFake, ConfigureCombinedBwe_Call) { EXPECT_EQ(0, call.GetAudioReceiveStreams().size()); } -TEST_F(WebRtcVoiceEngineTestFake, ConfigureCombinedBweForNewRecvStreams_Call) { +TEST_F(WebRtcVoiceEngineTestFake, ConfigureCombinedBweForNewRecvStreams) { // Test that adding receive streams after enabling combined bandwidth // estimation will correctly configure each channel. cricket::FakeCall call(webrtc::Call::Config(nullptr)); @@ -3492,6 +3489,9 @@ TEST_F(WebRtcVoiceEngineTestFake, ConfigureCombinedBweForNewRecvStreams_Call) { EXPECT_TRUE(media_channel->AddRecvStream( cricket::StreamParams::CreateLegacy(kSsrcs[i]))); EXPECT_NE(nullptr, call.GetAudioReceiveStream(kSsrcs[i])); + EXPECT_TRUE(call.GetAudioReceiveStream(kSsrcs[i]) + ->GetConfig() + .combined_audio_video_bwe); } EXPECT_EQ(ARRAY_SIZE(kSsrcs), call.GetAudioReceiveStreams().size()); @@ -3499,7 +3499,7 @@ TEST_F(WebRtcVoiceEngineTestFake, ConfigureCombinedBweForNewRecvStreams_Call) { EXPECT_EQ(0, call.GetAudioReceiveStreams().size()); } -TEST_F(WebRtcVoiceEngineTestFake, ConfigureCombinedBweExtensions_Call) { +TEST_F(WebRtcVoiceEngineTestFake, ConfiguresAudioReceiveStreamRtpExtensions) { // Test that setting the header extensions results in the expected state // changes on an associated Call. cricket::FakeCall call(webrtc::Call::Config(nullptr)); @@ -3511,9 +3511,6 @@ TEST_F(WebRtcVoiceEngineTestFake, ConfigureCombinedBweExtensions_Call) { cricket::WebRtcVoiceMediaChannel* media_channel = static_cast(channel_); media_channel->SetCall(&call); - cricket::AudioOptions options; - options.combined_audio_video_bwe.Set(true); - EXPECT_TRUE(media_channel->SetOptions(options)); for (uint32 ssrc : ssrcs) { EXPECT_TRUE(media_channel->AddRecvStream( cricket::StreamParams::CreateLegacy(ssrc))); diff --git a/webrtc/audio_receive_stream.h b/webrtc/audio_receive_stream.h index 44e232c34c..9a8601de9b 100644 --- a/webrtc/audio_receive_stream.h +++ b/webrtc/audio_receive_stream.h @@ -59,6 +59,9 @@ class AudioReceiveStream : public ReceiveStream { // Call::CreateReceiveStream(). // TODO(solenberg): Use unique_ptr<> once our std lib fully supports C++11. std::map decoder_map; + + // TODO(pbos): Remove config option once combined A/V BWE is always on. + bool combined_audio_video_bwe = false; }; virtual Stats GetStats() const = 0; diff --git a/webrtc/video/audio_receive_stream.cc b/webrtc/video/audio_receive_stream.cc index 88fd431f82..9f8bebe462 100644 --- a/webrtc/video/audio_receive_stream.cc +++ b/webrtc/video/audio_receive_stream.cc @@ -86,13 +86,15 @@ bool AudioReceiveStream::DeliverRtcp(const uint8_t* packet, size_t length) { bool AudioReceiveStream::DeliverRtp(const uint8_t* packet, size_t length) { RTPHeader header; + if (!rtp_header_parser_->Parse(packet, length, &header)) { return false; } - // Only forward if the parsed header has absolute sender time. RTP time stamps + // Only forward if the parsed header has absolute sender time. RTP timestamps // may have different rates for audio and video and shouldn't be mixed. - if (header.extension.hasAbsoluteSendTime) { + if (config_.combined_audio_video_bwe && + header.extension.hasAbsoluteSendTime) { int64_t arrival_time_ms = TickTime::MillisecondTimestamp(); size_t payload_size = length - header.headerLength; remote_bitrate_estimator_->IncomingPacket(arrival_time_ms, payload_size, diff --git a/webrtc/video/bitrate_estimator_tests.cc b/webrtc/video/bitrate_estimator_tests.cc index 872fe44803..cb6ef0ebf8 100644 --- a/webrtc/video/bitrate_estimator_tests.cc +++ b/webrtc/video/bitrate_estimator_tests.cc @@ -210,6 +210,7 @@ class BitrateEstimatorTest : public test::CallTest { receive_config.voe_channel_id = 0; receive_config.rtp.extensions.push_back( RtpExtension(RtpExtension::kAbsSendTime, kASTExtensionId)); + receive_config.combined_audio_video_bwe = true; audio_receive_stream_ = test_->receiver_call_->CreateAudioReceiveStream( receive_config); } else {