From a9b4c32052fd55df7e1d02e846fbea3178bebf71 Mon Sep 17 00:00:00 2001 From: Peter Thatcher Date: Thu, 16 Jul 2015 03:47:28 -0700 Subject: [PATCH] Nuke buffered latency mode. It's not actually working, and it's not used. It's just dead code complexity. R=pbos@webrtc.org Review URL: https://codereview.webrtc.org/1226093010 . Cr-Commit-Position: refs/heads/master@{#9593} --- talk/app/webrtc/webrtcsdp.cc | 27 ------------ talk/app/webrtc/webrtcsdp_unittest.cc | 41 ------------------- talk/media/base/mediachannel.h | 6 +-- .../webrtc/webrtcvideoengine2_unittest.cc | 2 + talk/session/media/channel.cc | 3 -- talk/session/media/channel_unittest.cc | 28 ------------- 6 files changed, 3 insertions(+), 104 deletions(-) diff --git a/talk/app/webrtc/webrtcsdp.cc b/talk/app/webrtc/webrtcsdp.cc index 30db437b81..5518233f2f 100644 --- a/talk/app/webrtc/webrtcsdp.cc +++ b/talk/app/webrtc/webrtcsdp.cc @@ -164,8 +164,6 @@ static const char kAttributeSctpPort[] = "sctp-port"; // Experimental flags static const char kAttributeXGoogleFlag[] = "x-google-flag"; static const char kValueConference[] = "conference"; -static const char kAttributeXGoogleBufferLatency[] = - "x-google-buffer-latency"; // Candidate static const char kCandidateHost[] = "host"; @@ -1427,15 +1425,6 @@ void BuildRtpContentAttributes( // [/] BuildRtpMap(media_desc, media_type, message); - // Specify latency for buffered mode. - // a=x-google-buffer-latency: - if (media_desc->buffered_mode_latency() != cricket::kBufferedModeDisabled) { - std::ostringstream os; - InitAttrLine(kAttributeXGoogleBufferLatency, &os); - os << kSdpDelimiterColon << media_desc->buffered_mode_latency(); - AddLine(os.str(), message); - } - for (StreamParamsVec::const_iterator track = media_desc->streams().begin(); track != media_desc->streams().end(); ++track) { // Require that the track belongs to a media stream, @@ -2631,22 +2620,6 @@ bool ParseContent(const std::string& message, } if (flag_value.compare(kValueConference) == 0) media_desc->set_conference_mode(true); - } else if (HasAttribute(line, kAttributeXGoogleBufferLatency)) { - // Experimental attribute. - // TODO: expose API to set this directly. - std::string flag_value; - if (!GetValue(line, kAttributeXGoogleBufferLatency, &flag_value, - error)) { - return false; - } - int buffer_latency = 0; - if (!GetValueFromString(line, flag_value, &buffer_latency, error)) { - return false; - } - if (buffer_latency < 0) { - return ParseFailed(line, "Buffer latency less than 0.", error); - } - media_desc->set_buffered_mode_latency(buffer_latency); } } else { // Only parse lines that we are interested of. diff --git a/talk/app/webrtc/webrtcsdp_unittest.cc b/talk/app/webrtc/webrtcsdp_unittest.cc index b657768204..869c3ecabd 100644 --- a/talk/app/webrtc/webrtcsdp_unittest.cc +++ b/talk/app/webrtc/webrtcsdp_unittest.cc @@ -778,9 +778,6 @@ class WebRtcSdpTest : public testing::Test { EXPECT_EQ(ext1.uri, ext2.uri); EXPECT_EQ(ext1.id, ext2.id); } - - // buffered mode latency - EXPECT_EQ(cd1->buffered_mode_latency(), cd2->buffered_mode_latency()); } @@ -1721,22 +1718,6 @@ TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithExtmap) { EXPECT_EQ(sdp_with_extmap, message); } -TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithBufferLatency) { - VideoContentDescription* vcd = static_cast( - GetFirstVideoContent(&desc_)->description); - vcd->set_buffered_mode_latency(128); - - ASSERT_TRUE(jdesc_.Initialize(desc_.Copy(), - jdesc_.session_id(), - jdesc_.session_version())); - std::string message = webrtc::SdpSerialize(jdesc_); - std::string sdp_with_buffer_latency = kSdpFullString; - InjectAfter("a=rtpmap:120 VP8/90000\r\n", - "a=x-google-buffer-latency:128\r\n", - &sdp_with_buffer_latency); - EXPECT_EQ(sdp_with_buffer_latency, message); -} - TEST_F(WebRtcSdpTest, SerializeCandidates) { std::string message = webrtc::SdpSerializeCandidate(*jcandidate_); EXPECT_EQ(std::string(kRawCandidate), message); @@ -1962,24 +1943,6 @@ TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithUfragPwd) { EXPECT_TRUE(CompareSessionDescription(jdesc_, jdesc_with_ufrag_pwd)); } -TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithBufferLatency) { - JsepSessionDescription jdesc_with_buffer_latency(kDummyString); - std::string sdp_with_buffer_latency = kSdpFullString; - InjectAfter("a=rtpmap:120 VP8/90000\r\n", - "a=x-google-buffer-latency:128\r\n", - &sdp_with_buffer_latency); - - EXPECT_TRUE( - SdpDeserialize(sdp_with_buffer_latency, &jdesc_with_buffer_latency)); - VideoContentDescription* vcd = static_cast( - GetFirstVideoContent(&desc_)->description); - vcd->set_buffered_mode_latency(128); - ASSERT_TRUE(jdesc_.Initialize(desc_.Copy(), - jdesc_.session_id(), - jdesc_.session_version())); - EXPECT_TRUE(CompareSessionDescription(jdesc_, jdesc_with_buffer_latency)); -} - TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithRecvOnlyContent) { EXPECT_TRUE(TestDeserializeDirection(cricket::MD_RECVONLY)); } @@ -2438,10 +2401,6 @@ TEST_F(WebRtcSdpTest, DeserializeSdpWithInvalidAttributeValue) { ExpectParseFailureWithNewLines("a=mid:video_content_name\r\n", "a=extmap:badvalue http://example.com\r\n", "a=extmap:badvalue http://example.com"); - // x-google-buffer-latency - ExpectParseFailureWithNewLines("a=mid:video_content_name\r\n", - "a=x-google-buffer-latency:badvalue\r\n", - "a=x-google-buffer-latency:badvalue"); } TEST_F(WebRtcSdpTest, DeserializeSdpWithReorderedPltypes) { diff --git a/talk/media/base/mediachannel.h b/talk/media/base/mediachannel.h index e7af7a76aa..492f136798 100644 --- a/talk/media/base/mediachannel.h +++ b/talk/media/base/mediachannel.h @@ -327,7 +327,6 @@ struct VideoOptions { change.system_low_adaptation_threshhold); system_high_adaptation_threshhold.SetFrom( change.system_high_adaptation_threshhold); - buffered_mode_latency.SetFrom(change.buffered_mode_latency); dscp.SetFrom(change.dscp); suspend_below_min_bitrate.SetFrom(change.suspend_below_min_bitrate); unsignalled_recv_stream_limit.SetFrom(change.unsignalled_recv_stream_limit); @@ -356,7 +355,7 @@ struct VideoOptions { o.system_low_adaptation_threshhold && system_high_adaptation_threshhold == o.system_high_adaptation_threshhold && - buffered_mode_latency == o.buffered_mode_latency && dscp == o.dscp && + dscp == o.dscp && suspend_below_min_bitrate == o.suspend_below_min_bitrate && unsignalled_recv_stream_limit == o.unsignalled_recv_stream_limit && use_simulcast_adapter == o.use_simulcast_adapter && @@ -385,7 +384,6 @@ struct VideoOptions { ost << ToStringIfSet("process", process_adaptation_threshhold); ost << ToStringIfSet("low", system_low_adaptation_threshhold); ost << ToStringIfSet("high", system_high_adaptation_threshhold); - ost << ToStringIfSet("buffered mode latency", buffered_mode_latency); ost << ToStringIfSet("dscp", dscp); ost << ToStringIfSet("suspend below min bitrate", suspend_below_min_bitrate); @@ -439,8 +437,6 @@ struct VideoOptions { Settable system_low_adaptation_threshhold; // High threshhold for cpu adaptation. (Adapt down) Settable system_high_adaptation_threshhold; - // Specify buffered mode latency in milliseconds. - Settable buffered_mode_latency; // Set DSCP value for packet sent from video channel. Settable dscp; // Enable WebRTC suspension of video. No video frames will be sent when the diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc index 745a0c4ade..8875994fb8 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc @@ -2156,6 +2156,8 @@ TEST_F(WebRtcVideoChannel2Test, TestSetDscpOptions) { new cricket::FakeNetworkInterface); channel_->SetInterface(network_interface.get()); cricket::VideoOptions options; + EXPECT_TRUE(channel_->SetOptions(options)); + EXPECT_EQ(rtc::DSCP_NO_CHANGE, network_interface->dscp()); options.dscp.Set(true); EXPECT_TRUE(channel_->SetOptions(options)); EXPECT_EQ(rtc::DSCP_AF41, network_interface->dscp()); diff --git a/talk/session/media/channel.cc b/talk/session/media/channel.cc index d30972db06..4aa2d7be70 100644 --- a/talk/session/media/channel.cc +++ b/talk/session/media/channel.cc @@ -1858,8 +1858,6 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, if (action != CA_UPDATE) { VideoOptions video_options; media_channel()->GetOptions(&video_options); - video_options.buffered_mode_latency.Set(video->buffered_mode_latency()); - if (!media_channel()->SetOptions(video_options)) { // Log an error on failure, but don't abort the call. LOG(LS_ERROR) << "Failed to set video channel options"; @@ -1911,7 +1909,6 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, if (video->conference_mode()) { video_options.conference_mode.Set(true); } - video_options.buffered_mode_latency.Set(video->buffered_mode_latency()); if (!media_channel()->SetOptions(video_options)) { // Log an error on failure, but don't abort the call. diff --git a/talk/session/media/channel_unittest.cc b/talk/session/media/channel_unittest.cc index 2573454b22..76cabd60e2 100644 --- a/talk/session/media/channel_unittest.cc +++ b/talk/session/media/channel_unittest.cc @@ -618,30 +618,6 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { EXPECT_TRUE(channel2_->rtcp_transport_channel() != NULL); } - // Test that SetLocalContent and SetRemoteContent properly set - // video options to the media channel. - void TestSetContentsVideoOptions() { - CreateChannels(0, 0); - typename T::Content content; - CreateContent(0, kPcmuCodec, kH264Codec, &content); - content.set_buffered_mode_latency(101); - EXPECT_TRUE(channel1_->SetLocalContent(&content, CA_OFFER, NULL)); - EXPECT_EQ(0U, media_channel1_->codecs().size()); - cricket::VideoOptions options; - ASSERT_TRUE(media_channel1_->GetOptions(&options)); - int latency = 0; - EXPECT_TRUE(options.buffered_mode_latency.Get(&latency)); - EXPECT_EQ(101, latency); - content.set_buffered_mode_latency(102); - EXPECT_TRUE(channel1_->SetRemoteContent(&content, CA_ANSWER, NULL)); - ASSERT_EQ(1U, media_channel1_->codecs().size()); - EXPECT_TRUE(CodecMatches(content.codecs()[0], - media_channel1_->codecs()[0])); - ASSERT_TRUE(media_channel1_->GetOptions(&options)); - EXPECT_TRUE(options.buffered_mode_latency.Get(&latency)); - EXPECT_EQ(102, latency); - } - // Test that SetRemoteContent properly deals with a content update. void TestSetRemoteContentUpdate() { CreateChannels(0, 0); @@ -2410,10 +2386,6 @@ TEST_F(VideoChannelTest, TestSetContentsRtcpMuxWithPrAnswer) { Base::TestSetContentsRtcpMux(); } -TEST_F(VideoChannelTest, TestSetContentsVideoOptions) { - Base::TestSetContentsVideoOptions(); -} - TEST_F(VideoChannelTest, TestSetRemoteContentUpdate) { Base::TestSetRemoteContentUpdate(); }