Reland of Add framerate to VideoSinkWants and ability to signal on overuse (patchset #1 id:1 of https://codereview.webrtc.org/2783183003/ )
Reason for revert:
Seem to be a flaky test rather than an issue with this cl. Creating reland, will add code to reduce flakiness to that test.
Original issue's description:
> Revert of Add framerate to VideoSinkWants and ability to signal on overuse (patchset #8 id:410001 of https://codereview.webrtc.org/2781433002/ )
>
> Reason for revert:
> This has resulted in failure of CallPerfTest.ReceivesCpuOveruseAndUnderuse test on the Win7 build bot https://build.chromium.org/p/client.webrtc.perf/builders/Win7/builds/1780
>
> Original issue's description:
> > Reland of Add framerate to VideoSinkWants and ability to signal on overuse (patchset #1 id:1 of https://codereview.webrtc.org/2764133002/ )
> >
> > Reason for revert:
> > Found issue with test case, will add fix to reland cl.
> >
> > Original issue's description:
> > > Revert of Add framerate to VideoSinkWants and ability to signal on overuse (patchset #14 id:250001 of https://codereview.webrtc.org/2716643002/ )
> > >
> > > Reason for revert:
> > > Breaks perf tests:
> > > https://build.chromium.org/p/client.webrtc.perf/builders/Win7/builds/1679
> > > https://build.chromium.org/p/client.webrtc.perf/builders/Android32%20Tests%20%28L%20Nexus5%29/builds/2325
> > >
> > > Original issue's description:
> > > > Add framerate to VideoSinkWants and ability to signal on overuse
> > > >
> > > > In ViEEncoder, try to reduce framerate instead of resolution if the
> > > > current degradation preference is maintain-resolution rather than
> > > > balanced.
> > > >
> > > > BUG=webrtc:4172
> > > >
> > > > Review-Url: https://codereview.webrtc.org/2716643002
> > > > Cr-Commit-Position: refs/heads/master@{#17327}
> > > > Committed: https://chromium.googlesource.com/external/webrtc/+/72acf2526177bb4dbb5103cd6e165eb4361a5ae6
> > >
> > > TBR=nisse@webrtc.org,magjed@webrtc.org,kthelgason@webrtc.org,ilnik@webrtc.org,stefan@webrtc.org,sprang@webrtc.org
> > > # Skipping CQ checks because original CL landed less than 1 days ago.
> > > NOPRESUBMIT=true
> > > NOTREECHECKS=true
> > > NOTRY=true
> > > BUG=webrtc:4172
> > >
> > > Review-Url: https://codereview.webrtc.org/2764133002
> > > Cr-Commit-Position: refs/heads/master@{#17331}
> > > Committed: https://chromium.googlesource.com/external/webrtc/+/8b45b11144c968b4173215c76f78c710c9a2ed0b
> >
> > TBR=nisse@webrtc.org,magjed@webrtc.org,kthelgason@webrtc.org,ilnik@webrtc.org,stefan@webrtc.org,skvlad@webrtc.org
> > # Not skipping CQ checks because original CL landed more than 1 days ago.
> > BUG=webrtc:4172
> >
> > Review-Url: https://codereview.webrtc.org/2781433002
> > Cr-Commit-Position: refs/heads/master@{#17474}
> > Committed: https://chromium.googlesource.com/external/webrtc/+/3ea3c77e93121b1ab9d5e46641e6764f2cca0d51
>
> TBR=ilnik@webrtc.org,stefan@webrtc.org,asapersson@webrtc.org,sprang@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:4172
>
> Review-Url: https://codereview.webrtc.org/2783183003
> Cr-Commit-Position: refs/heads/master@{#17477}
> Committed: https://chromium.googlesource.com/external/webrtc/+/f9ed235c9b7248694edcb46feb1f29ce7456ab59
R=ilnik@webrtc.org,stefan@webrtc.org
BUG=webrtc:4172
Review-Url: https://codereview.webrtc.org/2789823002
Cr-Original-Commit-Position: refs/heads/master@{#17498}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: c5d62e29ca21da19a89c0a287c2d5231afa6465a
diff --git a/media/engine/fakewebrtccall.cc b/media/engine/fakewebrtccall.cc
index f442a3c..b539665 100644
--- a/media/engine/fakewebrtccall.cc
+++ b/media/engine/fakewebrtccall.cc
@@ -108,6 +108,7 @@
config_(std::move(config)),
codec_settings_set_(false),
resolution_scaling_enabled_(false),
+ framerate_scaling_enabled_(false),
source_(nullptr),
num_swapped_frames_(0) {
RTC_DCHECK(config.encoder_settings.encoder != NULL);
@@ -252,9 +253,24 @@
if (source_)
source_->RemoveSink(this);
source_ = source;
- resolution_scaling_enabled_ =
- degradation_preference !=
- webrtc::VideoSendStream::DegradationPreference::kMaintainResolution;
+ switch (degradation_preference) {
+ case DegradationPreference::kMaintainFramerate:
+ resolution_scaling_enabled_ = true;
+ framerate_scaling_enabled_ = false;
+ break;
+ case DegradationPreference::kMaintainResolution:
+ resolution_scaling_enabled_ = false;
+ framerate_scaling_enabled_ = true;
+ break;
+ case DegradationPreference::kBalanced:
+ resolution_scaling_enabled_ = true;
+ framerate_scaling_enabled_ = true;
+ break;
+ case DegradationPreference::kDegradationDisabled:
+ resolution_scaling_enabled_ = false;
+ framerate_scaling_enabled_ = false;
+ break;
+ }
if (source)
source->AddOrUpdateSink(this, resolution_scaling_enabled_
? sink_wants_
@@ -333,7 +349,9 @@
audio_network_state_(webrtc::kNetworkUp),
video_network_state_(webrtc::kNetworkUp),
num_created_send_streams_(0),
- num_created_receive_streams_(0) {}
+ num_created_receive_streams_(0),
+ audio_transport_overhead_(0),
+ video_transport_overhead_(0) {}
FakeCall::~FakeCall() {
EXPECT_EQ(0u, video_send_streams_.size());
diff --git a/media/engine/fakewebrtccall.h b/media/engine/fakewebrtccall.h
index 1c9212d..6b25422 100644
--- a/media/engine/fakewebrtccall.h
+++ b/media/engine/fakewebrtccall.h
@@ -138,6 +138,7 @@
bool resolution_scaling_enabled() const {
return resolution_scaling_enabled_;
}
+ bool framerate_scaling_enabled() const { return framerate_scaling_enabled_; }
void InjectVideoSinkWants(const rtc::VideoSinkWants& wants);
rtc::VideoSourceInterface<webrtc::VideoFrame>* source() const {
@@ -169,6 +170,7 @@
webrtc::VideoCodecVP9 vp9;
} vpx_settings_;
bool resolution_scaling_enabled_;
+ bool framerate_scaling_enabled_;
rtc::VideoSourceInterface<webrtc::VideoFrame>* source_;
int num_swapped_frames_;
rtc::Optional<webrtc::VideoFrame> last_frame_;
diff --git a/media/engine/webrtcvideoengine2.cc b/media/engine/webrtcvideoengine2.cc
index 8c3f958..742b34c 100644
--- a/media/engine/webrtcvideoengine2.cc
+++ b/media/engine/webrtcvideoengine2.cc
@@ -38,9 +38,10 @@
#include "webrtc/video_decoder.h"
#include "webrtc/video_encoder.h"
+using DegradationPreference = webrtc::VideoSendStream::DegradationPreference;
+
namespace cricket {
namespace {
-
// If this field trial is enabled, we will enable sending FlexFEC and disable
// sending ULPFEC whenever the former has been negotiated. Receiving FlexFEC
// is enabled whenever FlexFEC has been negotiated.
@@ -1637,26 +1638,35 @@
}
if (source_ && stream_) {
- stream_->SetSource(
- nullptr, webrtc::VideoSendStream::DegradationPreference::kBalanced);
+ stream_->SetSource(nullptr, DegradationPreference::kDegradationDisabled);
}
// Switch to the new source.
source_ = source;
if (source && stream_) {
- // Do not adapt resolution for screen content as this will likely
- // result in blurry and unreadable text.
- // |this| acts like a VideoSource to make sure SinkWants are handled on the
- // correct thread.
- stream_->SetSource(
- this, enable_cpu_overuse_detection_ &&
- !parameters_.options.is_screencast.value_or(false)
- ? webrtc::VideoSendStream::DegradationPreference::kBalanced
- : webrtc::VideoSendStream::DegradationPreference::
- kMaintainResolution);
+ stream_->SetSource(this, GetDegradationPreference());
}
return true;
}
+webrtc::VideoSendStream::DegradationPreference
+WebRtcVideoChannel2::WebRtcVideoSendStream::GetDegradationPreference() const {
+ // Do not adapt resolution for screen content as this will likely
+ // result in blurry and unreadable text.
+ // |this| acts like a VideoSource to make sure SinkWants are handled on the
+ // correct thread.
+ DegradationPreference degradation_preference;
+ if (!enable_cpu_overuse_detection_) {
+ degradation_preference = DegradationPreference::kDegradationDisabled;
+ } else {
+ if (parameters_.options.is_screencast.value_or(false)) {
+ degradation_preference = DegradationPreference::kMaintainResolution;
+ } else {
+ degradation_preference = DegradationPreference::kMaintainFramerate;
+ }
+ }
+ return degradation_preference;
+}
+
const std::vector<uint32_t>&
WebRtcVideoChannel2::WebRtcVideoSendStream::GetSsrcs() const {
return ssrcs_;
@@ -2095,16 +2105,7 @@
parameters_.encoder_config.encoder_specific_settings = NULL;
if (source_) {
- // Do not adapt resolution for screen content as this will likely result in
- // blurry and unreadable text.
- // |this| acts like a VideoSource to make sure SinkWants are handled on the
- // correct thread.
- stream_->SetSource(
- this, enable_cpu_overuse_detection_ &&
- !parameters_.options.is_screencast.value_or(false)
- ? webrtc::VideoSendStream::DegradationPreference::kBalanced
- : webrtc::VideoSendStream::DegradationPreference::
- kMaintainResolution);
+ stream_->SetSource(this, GetDegradationPreference());
}
// Call stream_->Start() if necessary conditions are met.
diff --git a/media/engine/webrtcvideoengine2.h b/media/engine/webrtcvideoengine2.h
index 309a84d..0a2f4a5 100644
--- a/media/engine/webrtcvideoengine2.h
+++ b/media/engine/webrtcvideoengine2.h
@@ -324,6 +324,9 @@
// and whether or not the encoding in |rtp_parameters_| is active.
void UpdateSendState();
+ webrtc::VideoSendStream::DegradationPreference GetDegradationPreference()
+ const EXCLUSIVE_LOCKS_REQUIRED(&thread_checker_);
+
rtc::ThreadChecker thread_checker_;
rtc::AsyncInvoker invoker_;
rtc::Thread* worker_thread_;
diff --git a/media/engine/webrtcvideoengine2_unittest.cc b/media/engine/webrtcvideoengine2_unittest.cc
index 0589610..1868834 100644
--- a/media/engine/webrtcvideoengine2_unittest.cc
+++ b/media/engine/webrtcvideoengine2_unittest.cc
@@ -2115,8 +2115,8 @@
// Trigger overuse.
rtc::VideoSinkWants wants;
- wants.max_pixel_count = rtc::Optional<int>(
- send_stream->GetLastWidth() * send_stream->GetLastHeight() - 1);
+ wants.max_pixel_count =
+ send_stream->GetLastWidth() * send_stream->GetLastHeight() - 1;
send_stream->InjectVideoSinkWants(wants);
EXPECT_TRUE(capturer.CaptureCustomFrame(1280, 720, cricket::FOURCC_I420));
EXPECT_EQ(2, send_stream->GetNumberOfSwappedFrames());
@@ -2124,8 +2124,8 @@
EXPECT_EQ(720 * 3 / 4, send_stream->GetLastHeight());
// Trigger overuse again.
- wants.max_pixel_count = rtc::Optional<int>(
- send_stream->GetLastWidth() * send_stream->GetLastHeight() - 1);
+ wants.max_pixel_count =
+ send_stream->GetLastWidth() * send_stream->GetLastHeight() - 1;
send_stream->InjectVideoSinkWants(wants);
EXPECT_TRUE(capturer.CaptureCustomFrame(1280, 720, cricket::FOURCC_I420));
EXPECT_EQ(3, send_stream->GetNumberOfSwappedFrames());
@@ -2143,7 +2143,7 @@
send_stream->GetLastWidth() * send_stream->GetLastHeight();
// Cap the max to 4x the pixel count (assuming max 1/2 x 1/2 scale downs)
// of the current stream, so we don't take too large steps.
- wants.max_pixel_count = rtc::Optional<int>(current_pixel_count * 4);
+ wants.max_pixel_count = current_pixel_count * 4;
// Default step down is 3/5 pixel count, so go up by 5/3.
wants.target_pixel_count = rtc::Optional<int>((current_pixel_count * 5) / 3);
send_stream->InjectVideoSinkWants(wants);
@@ -2155,7 +2155,7 @@
// Trigger underuse again, should go back up to full resolution.
current_pixel_count =
send_stream->GetLastWidth() * send_stream->GetLastHeight();
- wants.max_pixel_count = rtc::Optional<int>(current_pixel_count * 4);
+ wants.max_pixel_count = current_pixel_count * 4;
wants.target_pixel_count = rtc::Optional<int>((current_pixel_count * 5) / 3);
send_stream->InjectVideoSinkWants(wants);
EXPECT_TRUE(capturer.CaptureCustomFrame(1284, 724, cricket::FOURCC_I420));
@@ -2199,8 +2199,8 @@
// Trigger overuse.
rtc::VideoSinkWants wants;
- wants.max_pixel_count = rtc::Optional<int>(
- send_stream->GetLastWidth() * send_stream->GetLastHeight() - 1);
+ wants.max_pixel_count =
+ send_stream->GetLastWidth() * send_stream->GetLastHeight() - 1;
send_stream->InjectVideoSinkWants(wants);
EXPECT_TRUE(capturer.CaptureCustomFrame(1280, 720, cricket::FOURCC_I420));
EXPECT_EQ(2, send_stream->GetNumberOfSwappedFrames());
@@ -2242,6 +2242,7 @@
void WebRtcVideoChannel2Test::TestCpuAdaptation(bool enable_overuse,
bool is_screenshare) {
+ const int kDefaultFps = 30;
cricket::VideoCodec codec = GetEngineCodec("VP8");
cricket::VideoSendParameters parameters;
parameters.codecs.push_back(codec);
@@ -2263,14 +2264,17 @@
options.is_screencast = rtc::Optional<bool>(is_screenshare);
EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, &options, &capturer));
cricket::VideoFormat capture_format = capturer.GetSupportedFormats()->front();
+ capture_format.interval = rtc::kNumNanosecsPerSec / kDefaultFps;
EXPECT_EQ(cricket::CS_RUNNING, capturer.Start(capture_format));
EXPECT_TRUE(channel_->SetSend(true));
FakeVideoSendStream* send_stream = fake_call_->GetVideoSendStreams().front();
- if (!enable_overuse || is_screenshare) {
+ if (!enable_overuse) {
EXPECT_FALSE(send_stream->resolution_scaling_enabled());
+ EXPECT_FALSE(send_stream->framerate_scaling_enabled());
+ EXPECT_EQ(is_screenshare, send_stream->framerate_scaling_enabled());
EXPECT_TRUE(capturer.CaptureFrame());
EXPECT_EQ(1, send_stream->GetNumberOfSwappedFrames());
@@ -2282,33 +2286,59 @@
return;
}
- EXPECT_TRUE(send_stream->resolution_scaling_enabled());
+ if (is_screenshare) {
+ EXPECT_FALSE(send_stream->resolution_scaling_enabled());
+ EXPECT_TRUE(send_stream->framerate_scaling_enabled());
+ } else {
+ EXPECT_TRUE(send_stream->resolution_scaling_enabled());
+ EXPECT_FALSE(send_stream->framerate_scaling_enabled());
+ }
+
// Trigger overuse.
ASSERT_EQ(1u, fake_call_->GetVideoSendStreams().size());
rtc::VideoSinkWants wants;
- wants.max_pixel_count =
- rtc::Optional<int>(capture_format.width * capture_format.height - 1);
+ if (is_screenshare) {
+ wants.max_framerate_fps = (kDefaultFps * 2) / 3;
+ } else {
+ wants.max_pixel_count = capture_format.width * capture_format.height - 1;
+ }
send_stream->InjectVideoSinkWants(wants);
- EXPECT_TRUE(capturer.CaptureFrame());
- EXPECT_EQ(1, send_stream->GetNumberOfSwappedFrames());
+ for (int i = 0; i < kDefaultFps; ++i)
+ EXPECT_TRUE(capturer.CaptureFrame());
- EXPECT_TRUE(capturer.CaptureFrame());
- EXPECT_EQ(2, send_stream->GetNumberOfSwappedFrames());
-
- EXPECT_LT(send_stream->GetLastWidth(), capture_format.width);
- EXPECT_LT(send_stream->GetLastHeight(), capture_format.height);
+ if (is_screenshare) {
+ // Drops every third frame.
+ EXPECT_EQ(kDefaultFps * 2 / 3, send_stream->GetNumberOfSwappedFrames());
+ EXPECT_EQ(send_stream->GetLastWidth(), capture_format.width);
+ EXPECT_EQ(send_stream->GetLastHeight(), capture_format.height);
+ } else {
+ EXPECT_EQ(kDefaultFps, send_stream->GetNumberOfSwappedFrames());
+ EXPECT_LT(send_stream->GetLastWidth(), capture_format.width);
+ EXPECT_LT(send_stream->GetLastHeight(), capture_format.height);
+ }
// Trigger underuse which should go back to normal resolution.
int last_pixel_count =
send_stream->GetLastWidth() * send_stream->GetLastHeight();
- wants.max_pixel_count = rtc::Optional<int>(last_pixel_count * 4);
- wants.target_pixel_count = rtc::Optional<int>((last_pixel_count * 5) / 3);
+ if (is_screenshare) {
+ wants.max_framerate_fps = kDefaultFps;
+ } else {
+ wants.max_pixel_count = last_pixel_count * 4;
+ wants.target_pixel_count.emplace((last_pixel_count * 5) / 3);
+ }
send_stream->InjectVideoSinkWants(wants);
- EXPECT_TRUE(capturer.CaptureFrame());
- EXPECT_EQ(3, send_stream->GetNumberOfSwappedFrames());
+ for (int i = 0; i < kDefaultFps; ++i)
+ EXPECT_TRUE(capturer.CaptureFrame());
+
+ if (is_screenshare) {
+ EXPECT_EQ(kDefaultFps + (kDefaultFps * 2 / 3),
+ send_stream->GetNumberOfSwappedFrames());
+ } else {
+ EXPECT_EQ(kDefaultFps * 2, send_stream->GetNumberOfSwappedFrames());
+ }
EXPECT_EQ(capture_format.width, send_stream->GetLastWidth());
EXPECT_EQ(capture_format.height, send_stream->GetLastHeight());
