diff --git a/talk/app/webrtc/mediaconstraintsinterface.cc b/talk/app/webrtc/mediaconstraintsinterface.cc index d0e5ee371c..cd9bdc1c30 100644 --- a/talk/app/webrtc/mediaconstraintsinterface.cc +++ b/talk/app/webrtc/mediaconstraintsinterface.cc @@ -97,6 +97,8 @@ const char MediaConstraintsInterface::kEnableRtpDataChannels[] = "RtpDataChannels"; const char MediaConstraintsInterface::kEnableDscp[] = "googDscp"; const char MediaConstraintsInterface::kEnableIPv6[] = "googIPv6"; +const char MediaConstraintsInterface::kEnableVideoSuspendBelowMinBitrate[] = + "googSuspendBelowMinBitrate"; // Set |value| to the value associated with the first appearance of |key|, or // return false if |key| is not found. diff --git a/talk/app/webrtc/mediaconstraintsinterface.h b/talk/app/webrtc/mediaconstraintsinterface.h index d60cabb86d..69d49e8673 100644 --- a/talk/app/webrtc/mediaconstraintsinterface.h +++ b/talk/app/webrtc/mediaconstraintsinterface.h @@ -114,6 +114,8 @@ class MediaConstraintsInterface { static const char kEnableDscp[]; // Constraint to enable IPv6 through JS. static const char kEnableIPv6[]; + // Temporary constraint to enable suspend below min bitrate feature. + static const char kEnableVideoSuspendBelowMinBitrate[]; // The prefix of internal-only constraints whose JS set values should be // stripped by Chrome before passed down to Libjingle. diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc index 8d985835b7..f04baeaf59 100644 --- a/talk/app/webrtc/webrtcsession.cc +++ b/talk/app/webrtc/webrtcsession.cc @@ -434,7 +434,6 @@ WebRtcSession::WebRtcSession( ice_connection_state_(PeerConnectionInterface::kIceConnectionNew), older_version_remote_peer_(false), dtls_enabled_(false), - dscp_enabled_(false), data_channel_type_(cricket::DCT_NONE), ice_restart_latch_(new IceRestartAnswerLatch) { } @@ -505,7 +504,17 @@ bool WebRtcSession::Initialize( constraints, MediaConstraintsInterface::kEnableDscp, &value, NULL)) { - dscp_enabled_ = value; + audio_options_.dscp.Set(value); + video_options_.dscp.Set(value); + } + + // Find Suspend Below Min Bitrate constraint. + if (FindConstraint( + constraints, + MediaConstraintsInterface::kEnableVideoSuspendBelowMinBitrate, + &value, + NULL)) { + video_options_.suspend_below_min_bitrate.Set(value); } const cricket::VideoCodec default_codec( @@ -1386,11 +1395,7 @@ bool WebRtcSession::CreateVoiceChannel(const cricket::ContentInfo* content) { if (!voice_channel_.get()) return false; - if (dscp_enabled_) { - cricket::AudioOptions options; - options.dscp.Set(true); - voice_channel_->SetChannelOptions(options); - } + voice_channel_->SetChannelOptions(audio_options_); return true; } @@ -1400,11 +1405,7 @@ bool WebRtcSession::CreateVideoChannel(const cricket::ContentInfo* content) { if (!video_channel_.get()) return false; - if (dscp_enabled_) { - cricket::VideoOptions options; - options.dscp.Set(true); - video_channel_->SetChannelOptions(options); - } + video_channel_->SetChannelOptions(video_options_); return true; } diff --git a/talk/app/webrtc/webrtcsession.h b/talk/app/webrtc/webrtcsession.h index f3d50faede..98effa6818 100644 --- a/talk/app/webrtc/webrtcsession.h +++ b/talk/app/webrtc/webrtcsession.h @@ -318,8 +318,6 @@ class WebRtcSession : public cricket::BaseSession, // If the remote peer is using a older version of implementation. bool older_version_remote_peer_; bool dtls_enabled_; - // Flag will be set based on the constraint value. - bool dscp_enabled_; // Specifies which kind of data channel is allowed. This is controlled // by the chrome command-line flag and constraints: // 1. If chrome command-line switch 'enable-sctp-data-channels' is enabled, @@ -337,6 +335,10 @@ class WebRtcSession : public cricket::BaseSession, sigslot::signal0<> SignalVideoChannelDestroyed; sigslot::signal0<> SignalDataChannelDestroyed; + // Member variables for caching global options. + cricket::AudioOptions audio_options_; + cricket::VideoOptions video_options_; + DISALLOW_COPY_AND_ASSIGN(WebRtcSession); }; } // namespace webrtc diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc index b30ab3ef13..79ea2a508e 100644 --- a/talk/app/webrtc/webrtcsession_unittest.cc +++ b/talk/app/webrtc/webrtcsession_unittest.cc @@ -3120,6 +3120,26 @@ TEST_F(WebRtcSessionTest, TestDscpConstraint) { EXPECT_TRUE(video_options.dscp.GetWithDefaultIfUnset(false)); } +TEST_F(WebRtcSessionTest, TestSuspendBelowMinBitrateConstraint) { + constraints_.reset(new FakeConstraints()); + constraints_->AddOptional( + webrtc::MediaConstraintsInterface::kEnableVideoSuspendBelowMinBitrate, + true); + Init(NULL); + mediastream_signaling_.SendAudioVideoStream1(); + SessionDescriptionInterface* offer = CreateOffer(NULL); + + SetLocalDescriptionWithoutError(offer); + + video_channel_ = media_engine_->GetVideoChannel(0); + + ASSERT_TRUE(video_channel_ != NULL); + cricket::VideoOptions video_options; + EXPECT_TRUE(video_channel_->GetOptions(&video_options)); + EXPECT_TRUE( + video_options.suspend_below_min_bitrate.GetWithDefaultIfUnset(false)); +} + // TODO(bemasc): Add a TestIceStatesBundle with BUNDLE enabled. That test // currently fails because upon disconnection and reconnection OnIceComplete is // called more than once without returning to IceGatheringGathering. diff --git a/talk/examples/ios/AppRTCDemo/VideoView.h b/talk/examples/ios/AppRTCDemo/VideoView.h new file mode 100755 index 0000000000..e69de29bb2 diff --git a/talk/examples/ios/AppRTCDemo/VideoView.m b/talk/examples/ios/AppRTCDemo/VideoView.m new file mode 100755 index 0000000000..e69de29bb2 diff --git a/talk/media/base/videoengine_unittest.h b/talk/media/base/videoengine_unittest.h index d3f01ccab7..54b55005ac 100644 --- a/talk/media/base/videoengine_unittest.h +++ b/talk/media/base/videoengine_unittest.h @@ -1667,33 +1667,39 @@ class VideoMediaChannelTest : public testing::Test, EXPECT_EQ(1, renderer2_.num_rendered_frames()); } - // Disconnect the first stream and re-use it with another SSRC + // Set up 2 streams where the first stream uses the default channel. + // Then disconnect the first stream and verify default channel becomes + // available. + // Then add a new stream with |new_ssrc|. The new stream should re-use the + // default channel. void TwoStreamsReUseFirstStream(const cricket::VideoCodec& codec) { SetUpSecondStream(); + // Default channel used by the first stream. + EXPECT_EQ(kSsrc, channel_->GetDefaultChannelSsrc()); EXPECT_TRUE(channel_->RemoveRecvStream(kSsrc)); EXPECT_FALSE(channel_->RemoveRecvStream(kSsrc)); - // SSRC 0 should map to the "default" stream. I.e. the first added stream. - EXPECT_TRUE(channel_->RemoveSendStream(0)); - // Make sure that the first added stream was indeed the "default" stream. + EXPECT_TRUE(channel_->RemoveSendStream(kSsrc)); EXPECT_FALSE(channel_->RemoveSendStream(kSsrc)); - // Make sure that the "default" stream is indeed removed and that removing - // the default stream has an effect. - EXPECT_FALSE(channel_->RemoveSendStream(0)); - + // Default channel is no longer used by a stream. + EXPECT_EQ(0u, channel_->GetDefaultChannelSsrc()); SetRendererAsDefault(); + uint32 new_ssrc = kSsrc + 100; EXPECT_TRUE(channel_->AddSendStream( - cricket::StreamParams::CreateLegacy(kSsrc))); + cricket::StreamParams::CreateLegacy(new_ssrc))); + // Re-use default channel. + EXPECT_EQ(new_ssrc, channel_->GetDefaultChannelSsrc()); EXPECT_FALSE(channel_->AddSendStream( - cricket::StreamParams::CreateLegacy(kSsrc))); + cricket::StreamParams::CreateLegacy(new_ssrc))); EXPECT_TRUE(channel_->AddRecvStream( - cricket::StreamParams::CreateLegacy(kSsrc))); + cricket::StreamParams::CreateLegacy(new_ssrc))); EXPECT_FALSE(channel_->AddRecvStream( - cricket::StreamParams::CreateLegacy(kSsrc))); + cricket::StreamParams::CreateLegacy(new_ssrc))); - EXPECT_TRUE(channel_->SetCapturer(kSsrc, video_capturer_.get())); + EXPECT_TRUE(channel_->SetCapturer(new_ssrc, video_capturer_.get())); SendAndReceive(codec); - EXPECT_TRUE(channel_->RemoveSendStream(0)); + EXPECT_TRUE(channel_->RemoveSendStream(new_ssrc)); + EXPECT_EQ(0u, channel_->GetDefaultChannelSsrc()); } // Tests that we can send and receive frames with early receive. diff --git a/talk/media/webrtc/fakewebrtcvideoengine.h b/talk/media/webrtc/fakewebrtcvideoengine.h index 05edd792e2..209c8fc497 100644 --- a/talk/media/webrtc/fakewebrtcvideoengine.h +++ b/talk/media/webrtc/fakewebrtcvideoengine.h @@ -299,7 +299,9 @@ class FakeWebRtcVideoEngine send_fec_bitrate_(0), send_nack_bitrate_(0), send_bandwidth_(0), - receive_bandwidth_(0) { + receive_bandwidth_(0), + suspend_below_min_bitrate_(false), + overuse_observer_(NULL) { ssrcs_[0] = 0; // default ssrc. memset(&send_codec, 0, sizeof(send_codec)); } @@ -338,6 +340,8 @@ class FakeWebRtcVideoEngine unsigned int send_nack_bitrate_; unsigned int send_bandwidth_; unsigned int receive_bandwidth_; + bool suspend_below_min_bitrate_; + webrtc::CpuOveruseObserver* overuse_observer_; }; class Capturer : public webrtc::ViEExternalCapture { public: @@ -535,6 +539,10 @@ class FakeWebRtcVideoEngine WEBRTC_ASSERT_CHANNEL(channel); return channels_.find(channel)->second->can_transmit_; } + webrtc::CpuOveruseObserver* GetCpuOveruseObserver(int channel) const { + WEBRTC_ASSERT_CHANNEL(channel); + return channels_.find(channel)->second->overuse_observer_; + } int GetRtxSsrc(int channel, int simulcast_idx) const { WEBRTC_ASSERT_CHANNEL(channel); if (channels_.find(channel)->second->rtx_ssrcs_.find(simulcast_idx) == @@ -611,6 +619,10 @@ class FakeWebRtcVideoEngine WEBRTC_CHECK_CHANNEL(channel); return channels_.find(channel)->second->remote_rtx_ssrc_; } + bool GetSuspendBelowMinBitrateStatus(int channel) { + WEBRTC_ASSERT_CHANNEL(channel); + return channels_.find(channel)->second->suspend_below_min_bitrate_; + } WEBRTC_STUB(Release, ()); @@ -651,8 +663,12 @@ class FakeWebRtcVideoEngine channels_.erase(channel); return 0; } - WEBRTC_STUB(RegisterCpuOveruseObserver, - (int channel, webrtc::CpuOveruseObserver* observer)); + WEBRTC_FUNC(RegisterCpuOveruseObserver, + (int channel, webrtc::CpuOveruseObserver* observer)) { + WEBRTC_CHECK_CHANNEL(channel); + channels_[channel]->overuse_observer_ = observer; + return 0; + } WEBRTC_STUB(CpuOveruseMeasures, (int, int*, int*, int*, int*)); WEBRTC_STUB(ConnectAudioChannel, (const int, const int)); WEBRTC_STUB(DisconnectAudioChannel, (const int)); @@ -771,7 +787,10 @@ class FakeWebRtcVideoEngine WEBRTC_STUB(WaitForFirstKeyFrame, (const int, const bool)); WEBRTC_STUB(StartDebugRecording, (int, const char*)); WEBRTC_STUB(StopDebugRecording, (int)); - WEBRTC_VOID_STUB(SuspendBelowMinBitrate, (int)); + WEBRTC_VOID_FUNC(SuspendBelowMinBitrate, (int channel)) { + WEBRTC_ASSERT_CHANNEL(channel); + channels_[channel]->suspend_below_min_bitrate_ = true; + } // webrtc::ViECapture WEBRTC_STUB(NumberOfCaptureDevices, ()); diff --git a/talk/media/webrtc/webrtcvideoengine.cc b/talk/media/webrtc/webrtcvideoengine.cc index 385b19e6d2..1537f40b96 100644 --- a/talk/media/webrtc/webrtcvideoengine.cc +++ b/talk/media/webrtc/webrtcvideoengine.cc @@ -1570,7 +1570,7 @@ WebRtcVideoMediaChannel::~WebRtcVideoMediaChannel() { // Remove all receive streams and the default channel. while (!recv_channels_.empty()) { - RemoveRecvStream(recv_channels_.begin()->first); + RemoveRecvStreamInternal(recv_channels_.begin()->first); } // Unregister the channel from the engine. @@ -1778,6 +1778,11 @@ bool WebRtcVideoMediaChannel::SetSend(bool send) { } bool WebRtcVideoMediaChannel::AddSendStream(const StreamParams& sp) { + if (sp.first_ssrc() == 0) { + LOG(LS_ERROR) << "AddSendStream with 0 ssrc is not supported."; + return false; + } + LOG(LS_INFO) << "AddSendStream " << sp.ToString(); if (!IsOneSsrcStream(sp) && !IsSimulcastStream(sp)) { @@ -1859,6 +1864,11 @@ bool WebRtcVideoMediaChannel::AddSendStream(const StreamParams& sp) { } bool WebRtcVideoMediaChannel::RemoveSendStream(uint32 ssrc) { + if (ssrc == 0) { + LOG(LS_ERROR) << "RemoveSendStream with 0 ssrc is not supported."; + return false; + } + uint32 ssrc_key; if (!GetSendChannelKey(ssrc, &ssrc_key)) { LOG(LS_WARNING) << "Try to remove stream with ssrc " << ssrc @@ -1899,6 +1909,11 @@ bool WebRtcVideoMediaChannel::RemoveSendStream(uint32 ssrc) { } bool WebRtcVideoMediaChannel::AddRecvStream(const StreamParams& sp) { + if (sp.first_ssrc() == 0) { + LOG(LS_ERROR) << "AddRecvStream with 0 ssrc is not supported."; + return false; + } + // TODO(zhurunz) Remove this once BWE works properly across different send // and receive channels. // Reuse default channel for recv stream in 1:1 call. @@ -1981,8 +1996,15 @@ bool WebRtcVideoMediaChannel::AddRecvStream(const StreamParams& sp) { } bool WebRtcVideoMediaChannel::RemoveRecvStream(uint32 ssrc) { - RecvChannelMap::iterator it = recv_channels_.find(ssrc); + if (ssrc == 0) { + LOG(LS_ERROR) << "RemoveRecvStream with 0 ssrc is not supported."; + return false; + } + return RemoveRecvStreamInternal(ssrc); +} +bool WebRtcVideoMediaChannel::RemoveRecvStreamInternal(uint32 ssrc) { + RecvChannelMap::iterator it = recv_channels_.find(ssrc); if (it == recv_channels_.end()) { // TODO(perkj): Remove this once BWE works properly across different send // and receive channels. @@ -3405,6 +3427,11 @@ bool WebRtcVideoMediaChannel::ConfigureSending(int channel_id, LOG_RTCERR2(SetSenderBufferingMode, channel_id, buffer_latency); } } + + if (options_.suspend_below_min_bitrate.GetWithDefaultIfUnset(false)) { + engine()->vie()->codec()->SuspendBelowMinBitrate(channel_id); + } + // The remb status direction correspond to the RTP stream (and not the RTCP // stream). I.e. if send remb is enabled it means it is receiving remote // rembs and should use them to estimate bandwidth. Receive remb mean that @@ -3739,12 +3766,20 @@ bool WebRtcVideoMediaChannel::MaybeResetVieSendCodec( vie_codec.height = target_height; vie_codec.maxFramerate = target_codec.maxFramerate; vie_codec.startBitrate = target_codec.startBitrate; +#ifdef USE_WEBRTC_DEV_BRANCH + vie_codec.targetBitrate = 0; +#endif vie_codec.codecSpecific.VP8.automaticResizeOn = automatic_resize; vie_codec.codecSpecific.VP8.denoisingOn = denoising; vie_codec.codecSpecific.VP8.frameDroppingOn = vp8_frame_dropping; - // TODO(mflodman): Remove 'is_screencast' check when screen cast settings - // are treated correctly in WebRTC. - if (!is_screencast) + bool maybe_change_start_bitrate = !is_screencast; +#ifdef USE_WEBRTC_DEV_BRANCH + // TODO(pbos): When USE_WEBRTC_DEV_BRANCH is removed, remove + // maybe_change_start_bitrate as well. MaybeChangeStartBitrate should be + // called for all content. + maybe_change_start_bitrate = true; +#endif + if (maybe_change_start_bitrate) MaybeChangeStartBitrate(channel_id, &vie_codec); if (engine()->vie()->codec()->SetSendCodec(channel_id, vie_codec) != 0) { diff --git a/talk/media/webrtc/webrtcvideoengine.h b/talk/media/webrtc/webrtcvideoengine.h index b818b0d143..1d36ab19a3 100644 --- a/talk/media/webrtc/webrtcvideoengine.h +++ b/talk/media/webrtc/webrtcvideoengine.h @@ -246,6 +246,9 @@ class WebRtcVideoMediaChannel : public talk_base::MessageHandler, int video_channel() const { return vie_channel_; } bool sending() const { return sending_; } + // Public for testing purpose. + uint32 GetDefaultChannelSsrc(); + // VideoMediaChannel implementation virtual bool SetRecvCodecs(const std::vector &codecs); virtual bool SetSendCodecs(const std::vector &codecs); @@ -378,7 +381,6 @@ class WebRtcVideoMediaChannel : public talk_base::MessageHandler, bool IsDefaultChannel(int channel_id) const { return channel_id == vie_channel_; } - uint32 GetDefaultChannelSsrc(); bool DeleteSendChannel(uint32 ssrc_key); @@ -414,6 +416,8 @@ class WebRtcVideoMediaChannel : public talk_base::MessageHandler, // to one send channel, i.e. the last send channel. void MaybeDisconnectCapturer(VideoCapturer* capturer); + bool RemoveRecvStreamInternal(uint32 ssrc); + // Global state. WebRtcVideoEngine* engine_; VoiceMediaChannel* voice_channel_; diff --git a/talk/media/webrtc/webrtcvideoengine_unittest.cc b/talk/media/webrtc/webrtcvideoengine_unittest.cc index 4c4bdedf77..d305756fa2 100644 --- a/talk/media/webrtc/webrtcvideoengine_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine_unittest.cc @@ -824,6 +824,28 @@ TEST_F(WebRtcVideoEngineTestFake, LeakyBucketTest) { EXPECT_TRUE(vie_.GetTransmissionSmoothingStatus(second_send_channel)); } +// Verify that SuspendBelowMinBitrate is enabled if it is set in the options. +TEST_F(WebRtcVideoEngineTestFake, SuspendBelowMinBitrateTest) { + EXPECT_TRUE(SetupEngine()); + + // Verify this is off by default. + EXPECT_TRUE(channel_->AddSendStream(cricket::StreamParams::CreateLegacy(1))); + int first_send_channel = vie_.GetLastChannel(); + EXPECT_FALSE(vie_.GetSuspendBelowMinBitrateStatus(first_send_channel)); + + // Enable the experiment and verify. + cricket::VideoOptions options; + options.suspend_below_min_bitrate.Set(true); + EXPECT_TRUE(channel_->SetOptions(options)); + EXPECT_TRUE(vie_.GetSuspendBelowMinBitrateStatus(first_send_channel)); + + // Add a new send stream and verify suspend_below_min_bitrate is enabled. + EXPECT_TRUE(channel_->AddSendStream(cricket::StreamParams::CreateLegacy(2))); + int second_send_channel = vie_.GetLastChannel(); + EXPECT_NE(first_send_channel, second_send_channel); + EXPECT_TRUE(vie_.GetSuspendBelowMinBitrateStatus(second_send_channel)); +} + TEST_F(WebRtcVideoEngineTestFake, BufferedModeLatency) { EXPECT_TRUE(SetupEngine()); @@ -1057,6 +1079,24 @@ TEST_F(WebRtcVideoEngineTestFake, AddRemoveRecvStreamConference) { EXPECT_FALSE(vie_.IsChannel(receive_channel_num)); } +// Test that adding/removing stream with 0 ssrc should fail (and not crash). +// For crbug/351699 and 350988. +TEST_F(WebRtcVideoEngineTestFake, AddRemoveRecvStreamWith0Ssrc) { + EXPECT_TRUE(SetupEngine()); + EXPECT_TRUE(channel_->AddRecvStream(cricket::StreamParams::CreateLegacy(1))); + EXPECT_FALSE(channel_->AddRecvStream(cricket::StreamParams::CreateLegacy(0))); + EXPECT_FALSE(channel_->RemoveRecvStream(0)); + EXPECT_TRUE(channel_->RemoveRecvStream(1)); +} + +TEST_F(WebRtcVideoEngineTestFake, AddRemoveSendStreamWith0Ssrc) { + EXPECT_TRUE(SetupEngine()); + EXPECT_TRUE(channel_->AddSendStream(cricket::StreamParams::CreateLegacy(1))); + EXPECT_FALSE(channel_->AddSendStream(cricket::StreamParams::CreateLegacy(0))); + EXPECT_FALSE(channel_->RemoveSendStream(0)); + EXPECT_TRUE(channel_->RemoveSendStream(1)); +} + // Test that we can create a channel and start/stop rendering out on it. TEST_F(WebRtcVideoEngineTestFake, SetRender) { EXPECT_TRUE(SetupEngine());