From 742386a13670337db6e3bbf4cf54e7cb24a9b717 Mon Sep 17 00:00:00 2001 From: "stefan@webrtc.org" Date: Fri, 19 Dec 2014 15:33:17 +0000 Subject: [PATCH] Enable payload-based padding by default and remove the API. BUG=1812 R=mflodman@webrtc.org, pbos@webrtc.org, perkj@webrtc.org Review URL: https://webrtc-codereview.appspot.com/31319004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@7964 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/app/webrtc/webrtcsession.cc | 5 --- talk/media/base/mediachannel.h | 7 +--- talk/media/webrtc/fakewebrtcvideoengine.h | 8 ---- talk/media/webrtc/webrtcvideoengine.cc | 39 +------------------ talk/media/webrtc/webrtcvideoengine2.cc | 4 -- .../webrtc/webrtcvideoengine2_unittest.cc | 26 ------------- webrtc/video/end_to_end_tests.cc | 1 - webrtc/video/rampup_tests.cc | 2 - webrtc/video/video_send_stream.cc | 5 --- webrtc/video_engine/include/vie_rtp_rtcp.h | 9 ----- webrtc/video_engine/vie_channel.cc | 26 +------------ webrtc/video_engine/vie_channel.h | 3 -- webrtc/video_engine/vie_rtp_rtcp_impl.cc | 15 ------- webrtc/video_engine/vie_rtp_rtcp_impl.h | 1 - webrtc/video_send_stream.h | 6 +-- 15 files changed, 5 insertions(+), 152 deletions(-) diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc index 683efac13f..7d4174f971 100644 --- a/talk/app/webrtc/webrtcsession.cc +++ b/talk/app/webrtc/webrtcsession.cc @@ -593,11 +593,6 @@ bool WebRtcSession::Initialize( MediaConstraintsInterface::kCpuOveruseEncodeRsdThreshold, &video_options_.cpu_overuse_encode_rsd_threshold); - // Find payload padding constraint. - SetOptionFromOptionalConstraint(constraints, - MediaConstraintsInterface::kPayloadPadding, - &video_options_.use_payload_padding); - SetOptionFromOptionalConstraint(constraints, MediaConstraintsInterface::kNumUnsignalledRecvStreams, &video_options_.unsignalled_recv_stream_limit); diff --git a/talk/media/base/mediachannel.h b/talk/media/base/mediachannel.h index cb50490c02..e3454c41a1 100644 --- a/talk/media/base/mediachannel.h +++ b/talk/media/base/mediachannel.h @@ -314,7 +314,6 @@ struct VideoOptions { unsignalled_recv_stream_limit.SetFrom(change.unsignalled_recv_stream_limit); use_simulcast_adapter.SetFrom(change.use_simulcast_adapter); screencast_min_bitrate.SetFrom(change.screencast_min_bitrate); - use_payload_padding.SetFrom(change.use_payload_padding); } bool operator==(const VideoOptions& o) const { @@ -342,8 +341,7 @@ struct VideoOptions { 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 && - screencast_min_bitrate == o.screencast_min_bitrate && - use_payload_padding == o.use_payload_padding; + screencast_min_bitrate == o.screencast_min_bitrate; } std::string ToString() const { @@ -376,7 +374,6 @@ struct VideoOptions { unsignalled_recv_stream_limit); ost << ToStringIfSet("use simulcast adapter", use_simulcast_adapter); ost << ToStringIfSet("screencast min bitrate", screencast_min_bitrate); - ost << ToStringIfSet("payload padding", use_payload_padding); ost << "}"; return ost.str(); } @@ -436,8 +433,6 @@ struct VideoOptions { Settable use_simulcast_adapter; // Force screencast to use a minimum bitrate Settable screencast_min_bitrate; - // Enable payload padding. - Settable use_payload_padding; }; // A class for playing out soundclips. diff --git a/talk/media/webrtc/fakewebrtcvideoengine.h b/talk/media/webrtc/fakewebrtcvideoengine.h index e8a3f09844..aaa2f3bc0b 100644 --- a/talk/media/webrtc/fakewebrtcvideoengine.h +++ b/talk/media/webrtc/fakewebrtcvideoengine.h @@ -689,11 +689,7 @@ class FakeWebRtcVideoEngine channels_[channel]->overuse_observer_ = observer; return 0; } -#ifdef USE_WEBRTC_DEV_BRANCH WEBRTC_STUB(GetCpuOveruseMetrics, (int, webrtc::CpuOveruseMetrics*)); -#else - WEBRTC_STUB(CpuOveruseMeasures, (int, int*, int*, int*, int*)); -#endif WEBRTC_FUNC(SetCpuOveruseOptions, (int channel, const webrtc::CpuOveruseOptions& options)) { WEBRTC_CHECK_CHANNEL(channel); @@ -1017,10 +1013,6 @@ class FakeWebRtcVideoEngine return 0; } -#ifdef USE_WEBRTC_DEV_BRANCH - WEBRTC_STUB(SetPadWithRedundantPayloads, (int, bool)); -#endif - WEBRTC_FUNC(SetRtxReceivePayloadType, (const int channel, const uint8 payload_type)) { WEBRTC_CHECK_CHANNEL(channel); diff --git a/talk/media/webrtc/webrtcvideoengine.cc b/talk/media/webrtc/webrtcvideoengine.cc index 1785cc265a..689aef35d3 100644 --- a/talk/media/webrtc/webrtcvideoengine.cc +++ b/talk/media/webrtc/webrtcvideoengine.cc @@ -1078,7 +1078,7 @@ static bool GetCpuOveruseOptions(const VideoOptions& options, // Use method based on encode usage. overuse_options->low_encode_usage_threshold_percent = underuse_threshold; overuse_options->high_encode_usage_threshold_percent = overuse_threshold; -#ifdef USE_WEBRTC_DEV_BRANCH + // Set optional thresholds, if configured. int underuse_rsd_threshold = 0; if (options.cpu_underuse_encode_rsd_threshold.Get( @@ -1089,7 +1089,6 @@ static bool GetCpuOveruseOptions(const VideoOptions& options, if (options.cpu_overuse_encode_rsd_threshold.Get(&overuse_rsd_threshold)) { overuse_options->high_encode_time_rsd_threshold = overuse_rsd_threshold; } -#endif } else { // Use default method based on capture jitter. overuse_options->low_capture_jitter_threshold_ms = @@ -2653,35 +2652,12 @@ bool WebRtcVideoMediaChannel::GetStats(const StatsOptions& options, sinfo.adapt_reason = send_channel->CurrentAdaptReason(); sinfo.adapt_changes = send_channel->AdaptChanges(); -#ifdef USE_WEBRTC_DEV_BRANCH webrtc::CpuOveruseMetrics metrics; engine()->vie()->base()->GetCpuOveruseMetrics(channel_id, &metrics); sinfo.capture_jitter_ms = metrics.capture_jitter_ms; sinfo.avg_encode_ms = metrics.avg_encode_time_ms; sinfo.encode_usage_percent = metrics.encode_usage_percent; sinfo.capture_queue_delay_ms_per_s = metrics.capture_queue_delay_ms_per_s; -#else - sinfo.capture_jitter_ms = -1; - sinfo.avg_encode_ms = -1; - sinfo.encode_usage_percent = -1; - sinfo.capture_queue_delay_ms_per_s = -1; - - int capture_jitter_ms = 0; - int avg_encode_time_ms = 0; - int encode_usage_percent = 0; - int capture_queue_delay_ms_per_s = 0; - if (engine()->vie()->base()->CpuOveruseMeasures( - channel_id, - &capture_jitter_ms, - &avg_encode_time_ms, - &encode_usage_percent, - &capture_queue_delay_ms_per_s) == 0) { - sinfo.capture_jitter_ms = capture_jitter_ms; - sinfo.avg_encode_ms = avg_encode_time_ms; - sinfo.encode_usage_percent = encode_usage_percent; - sinfo.capture_queue_delay_ms_per_s = capture_queue_delay_ms_per_s; - } -#endif webrtc::RtcpPacketTypeCounter rtcp_sent; webrtc::RtcpPacketTypeCounter rtcp_received; @@ -3192,19 +3168,6 @@ bool WebRtcVideoMediaChannel::SetOptions(const VideoOptions &options) { } } -#ifdef USE_WEBRTC_DEV_BRANCH - bool use_payload_padding; - if (Changed(options.use_payload_padding, - original.use_payload_padding, - &use_payload_padding)) { - LOG(LS_INFO) << "Payload-based padding called."; - for (SendChannelMap::iterator it = send_channels_.begin(); - it != send_channels_.end(); ++it) { - engine()->vie()->rtp()->SetPadWithRedundantPayloads( - it->second->channel_id(), use_payload_padding); - } - } -#endif webrtc::CpuOveruseOptions overuse_options; if (GetCpuOveruseOptions(options_, &overuse_options)) { for (SendChannelMap::iterator it = send_channels_.begin(); diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc index b30c46793a..0b72fdf0ee 100644 --- a/talk/media/webrtc/webrtcvideoengine2.cc +++ b/talk/media/webrtc/webrtcvideoengine2.cc @@ -606,7 +606,6 @@ void WebRtcVideoChannel2::SetDefaultOptions() { options_.cpu_overuse_detection.Set(false); options_.dscp.Set(false); options_.suspend_below_min_bitrate.Set(false); - options_.use_payload_padding.Set(false); options_.video_noise_reduction.Set(true); options_.screencast_min_bitrate.Set(0); } @@ -1595,9 +1594,6 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetCodecAndOptions( // Set RTX payload type if RTX is enabled. if (!parameters_.config.rtp.rtx.ssrcs.empty()) { parameters_.config.rtp.rtx.payload_type = codec_settings.rtx_payload_type; - - options.use_payload_padding.Get( - ¶meters_.config.rtp.rtx.pad_with_redundant_payloads); } if (IsNackEnabled(codec_settings.codec)) { diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc index 1248b316d2..cc7c2f9847 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc @@ -1387,32 +1387,6 @@ TEST_F(WebRtcVideoChannel2Test, SetOptionsWithSuspendBelowMinBitrate) { EXPECT_FALSE(stream->GetConfig().suspend_below_min_bitrate); } -TEST_F(WebRtcVideoChannel2Test, RedundantPayloadsDisabledByDefault) { - const std::vector ssrcs = MAKE_VECTOR(kSsrcs1); - const std::vector rtx_ssrcs = MAKE_VECTOR(kRtxSsrcs1); - FakeVideoSendStream* stream = AddSendStream( - cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs)); - EXPECT_FALSE(stream->GetConfig().rtp.rtx.pad_with_redundant_payloads); -} - -TEST_F(WebRtcVideoChannel2Test, SetOptionsWithPayloadPadding) { - VideoOptions options; - options.use_payload_padding.Set(true); - channel_->SetOptions(options); - - const std::vector ssrcs = MAKE_VECTOR(kSsrcs1); - const std::vector rtx_ssrcs = MAKE_VECTOR(kRtxSsrcs1); - FakeVideoSendStream* stream = AddSendStream( - cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs)); - EXPECT_TRUE(stream->GetConfig().rtp.rtx.pad_with_redundant_payloads); - - options.use_payload_padding.Set(false); - channel_->SetOptions(options); - - stream = fake_call_->GetVideoSendStreams()[0]; - EXPECT_FALSE(stream->GetConfig().rtp.rtx.pad_with_redundant_payloads); -} - TEST_F(WebRtcVideoChannel2Test, Vp8DenoisingEnabledByDefault) { FakeVideoSendStream* stream = AddSendStream(); webrtc::VideoCodecVP8 vp8_settings; diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index 51b5bf782f..f82055456e 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -1765,7 +1765,6 @@ TEST_F(EndToEndTest, DISABLED_RedundantPayloadsTransmittedOnAllSsrcs) { } send_config->rtp.rtx.payload_type = kSendRtxPayloadType; - send_config->rtp.rtx.pad_with_redundant_payloads = true; for (size_t i = 0; i < kNumSsrcs; ++i) send_config->rtp.rtx.ssrcs.push_back(kSendRtxSsrcs[i]); diff --git a/webrtc/video/rampup_tests.cc b/webrtc/video/rampup_tests.cc index 704115a609..1d530aac5a 100644 --- a/webrtc/video/rampup_tests.cc +++ b/webrtc/video/rampup_tests.cc @@ -421,7 +421,6 @@ void RampUpTest::RunRampUpTest(bool rtx, if (rtx) { send_config_.rtp.rtx.payload_type = kSendRtxPayloadType; send_config_.rtp.rtx.ssrcs = rtx_ssrcs; - send_config_.rtp.rtx.pad_with_redundant_payloads = true; } if (num_streams == 1) { @@ -467,7 +466,6 @@ void RampUpTest::RunRampUpDownUpTest(size_t number_of_streams, bool rtx) { if (rtx) { send_config_.rtp.rtx.payload_type = kSendRtxPayloadType; send_config_.rtp.rtx.ssrcs = GenerateSsrcs(number_of_streams, 200); - send_config_.rtp.rtx.pad_with_redundant_payloads = true; } CreateStreams(); diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index cd8554e910..b8ef4d3496 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -458,7 +458,6 @@ void VideoSendStream::ConfigureSsrcs() { } if (config_.rtp.rtx.ssrcs.empty()) { - assert(!config_.rtp.rtx.pad_with_redundant_payloads); return; } @@ -475,10 +474,6 @@ void VideoSendStream::ConfigureSsrcs() { rtp_rtcp_->SetRtpStateForSsrc(channel_, ssrc, it->second); } - if (config_.rtp.rtx.pad_with_redundant_payloads) { - rtp_rtcp_->SetPadWithRedundantPayloads(channel_, true); - } - assert(config_.rtp.rtx.payload_type >= 0); rtp_rtcp_->SetRtxSendPayloadType(channel_, config_.rtp.rtx.payload_type); } diff --git a/webrtc/video_engine/include/vie_rtp_rtcp.h b/webrtc/video_engine/include/vie_rtp_rtcp.h index f704f741c5..103a196f63 100644 --- a/webrtc/video_engine/include/vie_rtp_rtcp.h +++ b/webrtc/video_engine/include/vie_rtp_rtcp.h @@ -116,15 +116,6 @@ class WEBRTC_DLLEXPORT ViERTP_RTCP { virtual int SetRtxSendPayloadType(const int video_channel, const uint8_t payload_type) = 0; - // This enables sending redundant payloads when padding up the bitrate instead - // of sending dummy padding packets. This feature is off by default and will - // only have an effect if RTX is also enabled. - // TODO(holmer): Remove default implementation once this has been implemented - // in libjingle. - virtual int SetPadWithRedundantPayloads(int video_channel, bool enable) { - return 0; - } - virtual int SetRtxReceivePayloadType(const int video_channel, const uint8_t payload_type) = 0; diff --git a/webrtc/video_engine/vie_channel.cc b/webrtc/video_engine/vie_channel.cc index 6708eadfc5..24805ae9f0 100644 --- a/webrtc/video_engine/vie_channel.cc +++ b/webrtc/video_engine/vie_channel.cc @@ -136,7 +136,6 @@ ViEChannel::ViEChannel(int32_t channel_id, intra_frame_observer_(intra_frame_observer), rtt_stats_(rtt_stats), paced_sender_(paced_sender), - pad_with_redundant_payloads_(false), bandwidth_observer_(bandwidth_observer), send_timestamp_extension_id_(kInvalidRtpExtensionId), absolute_send_time_extension_id_(kInvalidRtpExtensionId), @@ -937,22 +936,6 @@ int32_t ViEChannel::GetRemoteCSRC(uint32_t CSRCs[kRtpCsrcSize]) { return 0; } -void ViEChannel::SetPadWithRedundantPayloads(bool enable) { - { - CriticalSectionScoped cs(callback_cs_.get()); - pad_with_redundant_payloads_ = enable; - } - int mode; - uint32_t ssrc; - int payload_type; - rtp_rtcp_->RTXSendStatus(&mode, &ssrc, &payload_type); - if (mode != kRtxOff) { - // Since RTX was already enabled we have to reset it with payload-based - // padding on. - SetRtxSendStatus(true); - } -} - int ViEChannel::SetRtxSendPayloadType(int payload_type) { rtp_rtcp_->SetRtxSendPayloadType(payload_type); for (std::list::iterator it = simulcast_rtp_rtcp_.begin(); @@ -964,13 +947,8 @@ int ViEChannel::SetRtxSendPayloadType(int payload_type) { } void ViEChannel::SetRtxSendStatus(bool enable) { - int rtx_settings = kRtxOff; - if (enable) { - CriticalSectionScoped cs(callback_cs_.get()); - rtx_settings = kRtxRetransmitted; - if (pad_with_redundant_payloads_) - rtx_settings |= kRtxRedundantPayloads; - } + int rtx_settings = + enable ? kRtxRetransmitted | kRtxRedundantPayloads : kRtxOff; rtp_rtcp_->SetRTXSendStatus(rtx_settings); CriticalSectionScoped cs(rtp_rtcp_cs_.get()); for (std::list::iterator it = simulcast_rtp_rtcp_.begin(); diff --git a/webrtc/video_engine/vie_channel.h b/webrtc/video_engine/vie_channel.h index d3cc3665dd..5c85b5b42c 100644 --- a/webrtc/video_engine/vie_channel.h +++ b/webrtc/video_engine/vie_channel.h @@ -145,8 +145,6 @@ class ViEChannel int32_t GetRemoteCSRC(uint32_t CSRCs[kRtpCsrcSize]); int SetRtxSendPayloadType(int payload_type); - // Only has an effect once RTX is enabled. - void SetPadWithRedundantPayloads(bool enable); void SetRtxReceivePayloadType(int payload_type); // Sets the starting sequence number, must be called before StartSend. @@ -490,7 +488,6 @@ class ViEChannel RtcpIntraFrameObserver* intra_frame_observer_; RtcpRttStats* rtt_stats_; PacedSender* paced_sender_; - bool pad_with_redundant_payloads_; scoped_ptr bandwidth_observer_; int send_timestamp_extension_id_; diff --git a/webrtc/video_engine/vie_rtp_rtcp_impl.cc b/webrtc/video_engine/vie_rtp_rtcp_impl.cc index 49999a982a..ae213079d3 100644 --- a/webrtc/video_engine/vie_rtp_rtcp_impl.cc +++ b/webrtc/video_engine/vie_rtp_rtcp_impl.cc @@ -207,21 +207,6 @@ int ViERTP_RTCPImpl::SetRtxSendPayloadType(const int video_channel, return 0; } -int ViERTP_RTCPImpl::SetPadWithRedundantPayloads(int video_channel, - bool enable) { - LOG_F(LS_INFO) << "channel: " << video_channel - << " pad with redundant payloads: " << (enable ? "enable" : - "disable"); - ViEChannelManagerScoped cs(*(shared_data_->channel_manager())); - ViEChannel* vie_channel = cs.Channel(video_channel); - if (!vie_channel) { - shared_data_->SetLastError(kViERtpRtcpInvalidChannelId); - return -1; - } - vie_channel->SetPadWithRedundantPayloads(enable); - return 0; -} - int ViERTP_RTCPImpl::SetRtxReceivePayloadType(const int video_channel, const uint8_t payload_type) { LOG_F(LS_INFO) << "channel: " << video_channel diff --git a/webrtc/video_engine/vie_rtp_rtcp_impl.h b/webrtc/video_engine/vie_rtp_rtcp_impl.h index 99b97583ef..8e9f09735c 100644 --- a/webrtc/video_engine/vie_rtp_rtcp_impl.h +++ b/webrtc/video_engine/vie_rtp_rtcp_impl.h @@ -41,7 +41,6 @@ class ViERTP_RTCPImpl unsigned int CSRCs[kRtpCsrcSize]) const; virtual int SetRtxSendPayloadType(const int video_channel, const uint8_t payload_type); - virtual int SetPadWithRedundantPayloads(int video_channel, bool enable); virtual int SetRtxReceivePayloadType(const int video_channel, const uint8_t payload_type); virtual int SetStartSequenceNumber(const int video_channel, diff --git a/webrtc/video_send_stream.h b/webrtc/video_send_stream.h index 712b16dfa8..269fda0276 100644 --- a/webrtc/video_send_stream.h +++ b/webrtc/video_send_stream.h @@ -95,17 +95,13 @@ class VideoSendStream { // Settings for RTP retransmission payload format, see RFC 4588 for // details. struct Rtx { - Rtx() : payload_type(-1), pad_with_redundant_payloads(false) {} + Rtx() : payload_type(-1) {} std::string ToString() const; // SSRCs to use for the RTX streams. std::vector ssrcs; // Payload type to use for the RTX stream. int payload_type; - // Use redundant payloads to pad the bitrate. Instead of padding with - // randomized packets, we will preemptively retransmit media packets on - // the RTX stream. - bool pad_with_redundant_payloads; } rtx; // RTCP CNAME, see RFC 3550.