Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2681683003:
Added Vp9 simulcast tests. (Closed)
DescriptionAdded Vp9 simulcast tests.
For them implemeted upscaling in libyuv metrics calculation.
Updated maximum number of SL in vp9 encoder to 3.
Refactored names of some fields in Video_quality_check analyzer.
BUG=webrtc:7095
Review-Url: https://codereview.webrtc.org/2681683003
Cr-Commit-Position: refs/heads/master@{#16625}
Committed: https://chromium.googlesource.com/external/webrtc/+/2a8c2f589aa5e757cdb5d5cbb13f4494250ddf11
Patch Set 1 #Patch Set 2 : Rebased on latest commits and resolved conflicts #
Total comments: 12
Patch Set 3 : Implemented sprang@ comments #
Total comments: 8
Patch Set 4 : fixed latest comments by sprang@. #Patch Set 5 : rebase #Patch Set 6 : rebase #
Total comments: 6
Patch Set 7 : Implemented Magjed@ comments, fixed potential memory corruption #Patch Set 8 : oops #Patch Set 9 : fixed memory leak #
Total comments: 4
Patch Set 10 : implemented magjed comments #Patch Set 11 : forgot to remove unneeded dchecks in SSIM function #Patch Set 12 : rebase #
Messages
Total messages: 82 (53 generated)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/21023) android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
ilnik@webrtc.org changed reviewers: + sprang@webrtc.org, stefan@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ilnik@webrtc.org changed reviewers: + asapersson@webrtc.org, mflodman@webrtc.org
https://codereview.webrtc.org/2681683003/diff/20001/webrtc/common_video/libyu... File webrtc/common_video/libyuv/webrtc_libyuv.cc (right): https://codereview.webrtc.org/2681683003/diff/20001/webrtc/common_video/libyu... webrtc/common_video/libyuv/webrtc_libyuv.cc:301: rtc::scoped_refptr<I420Buffer> upscaled_buffer = Maybe you can move this declaration up to method scope and create a VideoFrameBuffer* that initially points to test_buffer, then just update those here so that you don't have to duplicate the call to I420Psnr and the capping of the result? https://codereview.webrtc.org/2681683003/diff/20001/webrtc/common_video/libyu... webrtc/common_video/libyuv/webrtc_libyuv.cc:303: int result = libyuv::I420Scale( Don't need this temporary https://codereview.webrtc.org/2681683003/diff/20001/webrtc/common_video/libyu... webrtc/common_video/libyuv/webrtc_libyuv.cc:356: rtc::scoped_refptr<I420Buffer> upscaled_buffer = dito https://codereview.webrtc.org/2681683003/diff/20001/webrtc/video/full_stack_t... File webrtc/video/full_stack_tests.cc (right): https://codereview.webrtc.org/2681683003/diff/20001/webrtc/video/full_stack_t... webrtc/video/full_stack_tests.cc:367: simulcast.video = {true, 1280, 720, 5, You probably want more than 5 fps for this test. Try with 30? https://codereview.webrtc.org/2681683003/diff/20001/webrtc/video/full_stack_t... webrtc/video/full_stack_tests.cc:371: simulcast.analyzer = {"simulcast_vp9_2sl", 0.0, 0.0, Update "simulcast_vp9_2sl". This will be the key used to identify the metrics in the logs, so it needs to be different for the three versions as well. https://codereview.webrtc.org/2681683003/diff/20001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2681683003/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1355: SetupScreenshareOrSimulcast(); Why remove if (params_.screenshare.enabled) ?
https://codereview.webrtc.org/2681683003/diff/20001/webrtc/common_video/libyu... File webrtc/common_video/libyuv/webrtc_libyuv.cc (right): https://codereview.webrtc.org/2681683003/diff/20001/webrtc/common_video/libyu... webrtc/common_video/libyuv/webrtc_libyuv.cc:301: rtc::scoped_refptr<I420Buffer> upscaled_buffer = On 2017/02/09 17:10:39, språng wrote: > Maybe you can move this declaration up to method scope and create a > VideoFrameBuffer* that initially points to test_buffer, then just update those > here so that you don't have to duplicate the call to I420Psnr and the capping of > the result? Done. https://codereview.webrtc.org/2681683003/diff/20001/webrtc/common_video/libyu... webrtc/common_video/libyuv/webrtc_libyuv.cc:303: int result = libyuv::I420Scale( On 2017/02/09 17:10:39, språng wrote: > Don't need this temporary Done. https://codereview.webrtc.org/2681683003/diff/20001/webrtc/common_video/libyu... webrtc/common_video/libyuv/webrtc_libyuv.cc:356: rtc::scoped_refptr<I420Buffer> upscaled_buffer = On 2017/02/09 17:10:39, språng wrote: > dito Done. https://codereview.webrtc.org/2681683003/diff/20001/webrtc/video/full_stack_t... File webrtc/video/full_stack_tests.cc (right): https://codereview.webrtc.org/2681683003/diff/20001/webrtc/video/full_stack_t... webrtc/video/full_stack_tests.cc:367: simulcast.video = {true, 1280, 720, 5, On 2017/02/09 17:10:39, språng wrote: > You probably want more than 5 fps for this test. Try with 30? Done. https://codereview.webrtc.org/2681683003/diff/20001/webrtc/video/full_stack_t... webrtc/video/full_stack_tests.cc:371: simulcast.analyzer = {"simulcast_vp9_2sl", 0.0, 0.0, On 2017/02/09 17:10:39, språng wrote: > Update "simulcast_vp9_2sl". This will be the key used to identify the metrics in > the logs, so it needs to be different for the three versions as well. Done. https://codereview.webrtc.org/2681683003/diff/20001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2681683003/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1355: SetupScreenshareOrSimulcast(); On 2017/02/09 17:10:39, språng wrote: > Why remove if (params_.screenshare.enabled) ? Because this function now setups not only screenshare but also a simulcast. Both checks are moved inside. This could be two separate functions (screencast ans simulcast) but they will do very similar things so it looks logical to me to leave it like that.
looks good, just a few nits https://codereview.webrtc.org/2681683003/diff/40001/webrtc/video/full_stack_t... File webrtc/video/full_stack_tests.cc (right): https://codereview.webrtc.org/2681683003/diff/40001/webrtc/video/full_stack_t... webrtc/video/full_stack_tests.cc:364: TEST_F(FullStackTest, SimulcastVP9_3SL_High) { Rename these VP9 SVC instead. https://codereview.webrtc.org/2681683003/diff/40001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2681683003/diff/40001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1193: void VideoQualityTest::SetupScreenshareOrSimulcast() { Looks like it's screenshare or svc, rather than simulcast? https://codereview.webrtc.org/2681683003/diff/40001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1200: VideoEncoderConfig::ContentType::kScreen; Copy/pastepaste? :) https://codereview.webrtc.org/2681683003/diff/40001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1254: } else if (params_.ss.num_spatial_layers > 1) { Add comment this is for the non-screenshare case.
https://codereview.webrtc.org/2681683003/diff/40001/webrtc/video/full_stack_t... File webrtc/video/full_stack_tests.cc (right): https://codereview.webrtc.org/2681683003/diff/40001/webrtc/video/full_stack_t... webrtc/video/full_stack_tests.cc:364: TEST_F(FullStackTest, SimulcastVP9_3SL_High) { On 2017/02/10 10:34:27, språng wrote: > Rename these VP9 SVC instead. Done. https://codereview.webrtc.org/2681683003/diff/40001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2681683003/diff/40001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1193: void VideoQualityTest::SetupScreenshareOrSimulcast() { On 2017/02/10 10:34:27, språng wrote: > Looks like it's screenshare or svc, rather than simulcast? Acknowledged. https://codereview.webrtc.org/2681683003/diff/40001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1200: VideoEncoderConfig::ContentType::kScreen; On 2017/02/10 10:34:27, språng wrote: > Copy/pastepaste? :) Acknowledged. https://codereview.webrtc.org/2681683003/diff/40001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1254: } else if (params_.ss.num_spatial_layers > 1) { On 2017/02/10 10:34:27, språng wrote: > Add comment this is for the non-screenshare case. Acknowledged.
lgtm
The CQ bit was checked by ilnik@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/13288)
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2681683003/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/13294)
ilnik@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, magjed@webrtc.org
ilnik@webrtc.org changed reviewers: - henrik.lundin@webrtc.org
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/13377)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/10 14:31:31, ilnik wrote: Magjed, when you have time could you please check webrtc/common_video/libyuv/webrtc_libyuv.cc changes? I've only added upscaling there. Sprang reviewed the changes, but he doesn't have ownership of that file.
https://codereview.webrtc.org/2681683003/diff/100001/webrtc/common_video/liby... File webrtc/common_video/libyuv/webrtc_libyuv.cc (right): https://codereview.webrtc.org/2681683003/diff/100001/webrtc/common_video/liby... webrtc/common_video/libyuv/webrtc_libyuv.cc:295: if ((ref_buffer.width() < test_buffer.width()) || I don't think we need these checks. Can we simply do: if (ref_buffer.width() != test_buffer.width() || ref_buffer.height() != test_buffer.height()) { ...scale... } instead? https://codereview.webrtc.org/2681683003/diff/100001/webrtc/common_video/liby... webrtc/common_video/libyuv/webrtc_libyuv.cc:304: if (libyuv::I420Scale( Can you use 'scaled_buffer.ScaleFrom(test_buffer)' instead?
https://codereview.webrtc.org/2681683003/diff/100001/webrtc/common_video/liby... File webrtc/common_video/libyuv/webrtc_libyuv.cc (right): https://codereview.webrtc.org/2681683003/diff/100001/webrtc/common_video/liby... webrtc/common_video/libyuv/webrtc_libyuv.cc:295: if ((ref_buffer.width() < test_buffer.width()) || On 2017/02/13 15:35:11, magjed_webrtc wrote: > I don't think we need these checks. Can we simply do: > if (ref_buffer.width() != test_buffer.width() || > ref_buffer.height() != test_buffer.height()) { > ...scale... > } > instead? Scaling down is not an intended behavior here, so I guess we should not do it like you suggest. Or we can do it like you ask, but include RTC_CHECKs. What do you think? https://codereview.webrtc.org/2681683003/diff/100001/webrtc/common_video/liby... webrtc/common_video/libyuv/webrtc_libyuv.cc:304: if (libyuv::I420Scale( On 2017/02/13 15:35:11, magjed_webrtc wrote: > Can you use 'scaled_buffer.ScaleFrom(test_buffer)' instead? Yes, it turns out, it will be equivalent. Will change to that. https://codereview.webrtc.org/2681683003/diff/100001/webrtc/common_video/liby... webrtc/common_video/libyuv/webrtc_libyuv.cc:358: temp_buffer = static_cast<VideoFrameBuffer*>(scaled_buffer.release()); Fixed potential memory corruption. If no scaling were done temp_buffer acquired responsibility for test_buffer and deleted it at the end of function.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/5178)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by ilnik@webrtc.org
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2681683003/diff/100001/webrtc/common_video/liby... File webrtc/common_video/libyuv/webrtc_libyuv.cc (right): https://codereview.webrtc.org/2681683003/diff/100001/webrtc/common_video/liby... webrtc/common_video/libyuv/webrtc_libyuv.cc:295: if ((ref_buffer.width() < test_buffer.width()) || On 2017/02/13 16:54:25, ilnik wrote: > On 2017/02/13 15:35:11, magjed_webrtc wrote: > > I don't think we need these checks. Can we simply do: > > if (ref_buffer.width() != test_buffer.width() || > > ref_buffer.height() != test_buffer.height()) { > > ...scale... > > } > > instead? > > Scaling down is not an intended behavior here, so I guess we should not do it > like you suggest. Or we can do it like you ask, but include RTC_CHECKs. What do > you think? I don't see the problem of making the code more general if it works and simplifies the code, but sure, keep the RTC_DCHECKS if you want to catch these cases. https://codereview.webrtc.org/2681683003/diff/160001/webrtc/common_video/liby... File webrtc/common_video/libyuv/webrtc_libyuv.cc (right): https://codereview.webrtc.org/2681683003/diff/160001/webrtc/common_video/liby... webrtc/common_video/libyuv/webrtc_libyuv.cc:294: rtc::scoped_refptr<const VideoFrameBuffer> temp_buffer(&test_buffer); Taking the address of a const-ref argument is sketchy, you are not supposed to know/care about the lifetime management of such an argument. I would recommend removing |temp_buffer|, and do: return I420PSNR(ref_buffer, *scaled_buffer); inside the if-statement instead. An alternative is that you factor out the common code in I420PSNR and I420SSIM in a way that doesn't require you taking the address of the const-ref argument. https://codereview.webrtc.org/2681683003/diff/160001/webrtc/common_video/liby... webrtc/common_video/libyuv/webrtc_libyuv.cc:297: RTC_DCHECK_GE(ref_buffer.width(), 0); This check is really paranoid. Width/height is never supposed to be negative and we don't check it in other places. Just let it crash for these cases instead.
https://codereview.webrtc.org/2681683003/diff/160001/webrtc/common_video/liby... File webrtc/common_video/libyuv/webrtc_libyuv.cc (right): https://codereview.webrtc.org/2681683003/diff/160001/webrtc/common_video/liby... webrtc/common_video/libyuv/webrtc_libyuv.cc:294: rtc::scoped_refptr<const VideoFrameBuffer> temp_buffer(&test_buffer); On 2017/02/14 12:47:49, magjed_webrtc wrote: > Taking the address of a const-ref argument is sketchy, you are not supposed to > know/care about the lifetime management of such an argument. I would recommend > removing |temp_buffer|, and do: > return I420PSNR(ref_buffer, *scaled_buffer); > inside the if-statement instead. An alternative is that you factor out the > common code in I420PSNR and I420SSIM in a way that doesn't require you taking > the address of the const-ref argument. That is a great idea! https://codereview.webrtc.org/2681683003/diff/160001/webrtc/common_video/liby... webrtc/common_video/libyuv/webrtc_libyuv.cc:297: RTC_DCHECK_GE(ref_buffer.width(), 0); On 2017/02/14 12:47:49, magjed_webrtc wrote: > This check is really paranoid. Width/height is never supposed to be negative and > we don't check it in other places. Just let it crash for these cases instead. Acknowledged.
webrtc_libyuv.cc lgtm
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2681683003/#ps190001 (title: "forgot to remove unneeded dchecks in SSIM function")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/21139)
The CQ bit was checked by ilnik@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/5195)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...) android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/21292)
The CQ bit was checked by ilnik@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
The CQ bit was checked by ilnik@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for webrtc/video/video_quality_test.cc:
While running git apply --index -p1;
error: patch failed: webrtc/video/video_quality_test.cc:135
error: webrtc/video/video_quality_test.cc: patch does not apply
Patch: webrtc/video/video_quality_test.cc
Index: webrtc/video/video_quality_test.cc
diff --git a/webrtc/video/video_quality_test.cc
b/webrtc/video/video_quality_test.cc
index
23487f49e8deb8e4affce33e7c353396785414ef..662245043d23476285a1b85540603df7271e2cfa
100644
--- a/webrtc/video/video_quality_test.cc
+++ b/webrtc/video/video_quality_test.cc
@@ -135,8 +135,8 @@ class VideoAnalyzer : public PacketReceiver,
const std::string& graph_title,
uint32_t ssrc_to_analyze,
uint32_t rtx_ssrc_to_analyze,
- uint32_t selected_width,
- uint32_t selected_height)
+ uint32_t selected_stream_width,
+ uint32_t selected_stream_height)
: transport_(transport),
receiver_(nullptr),
send_stream_(nullptr),
@@ -147,8 +147,8 @@ class VideoAnalyzer : public PacketReceiver,
graph_title_(graph_title),
ssrc_to_analyze_(ssrc_to_analyze),
rtx_ssrc_to_analyze_(rtx_ssrc_to_analyze),
- selected_width_(selected_width),
- selected_height_(selected_height),
+ selected_stream_width_(selected_stream_width),
+ selected_stream_height_(selected_stream_height),
pre_encode_proxy_(this),
encode_timing_proxy_(this),
frames_to_process_(duration_frames),
@@ -228,7 +228,6 @@ class VideoAnalyzer : public PacketReceiver,
if (RtpHeaderParser::IsRtcp(packet, length)) {
return receiver_->DeliverPacket(media_type, packet, length, packet_time);
}
-
RtpUtility::RtpHeaderParser parser(packet, length);
RTPHeader header;
parser.Parse(&header);
@@ -270,8 +269,8 @@ class VideoAnalyzer : public PacketReceiver,
void PostEncodeFrameCallback(const EncodedFrame& encoded_frame) {
rtc::CritScope lock(&crit_);
if (!first_sent_timestamp_ &&
- encoded_frame.encoded_width_ == selected_width_ &&
- encoded_frame.encoded_height_ == selected_height_) {
+ encoded_frame.encoded_width_ == selected_stream_width_ &&
+ encoded_frame.encoded_height_ == selected_stream_height_) {
first_sent_timestamp_ =
rtc::Optional<uint32_t>(encoded_frame.timestamp_);
}
}
@@ -285,6 +284,7 @@ class VideoAnalyzer : public PacketReceiver,
int64_t current_time =
Clock::GetRealTimeClock()->CurrentNtpInMilliseconds();
+
bool result = transport_->SendRtp(packet, length, options);
{
rtc::CritScope lock(&crit_);
@@ -850,8 +850,8 @@ class VideoAnalyzer : public PacketReceiver,
const std::string graph_title_;
const uint32_t ssrc_to_analyze_;
const uint32_t rtx_ssrc_to_analyze_;
- const uint32_t selected_width_;
- const uint32_t selected_height_;
+ const uint32_t selected_stream_width_;
+ const uint32_t selected_stream_height_;
PreEncodeProxy pre_encode_proxy_;
OnEncodeTimingProxy encode_timing_proxy_;
std::vector<Sample> samples_ GUARDED_BY(comparison_lock_);
@@ -1198,25 +1198,67 @@ void VideoQualityTest::SetupVideo(Transport*
send_transport,
}
}
-void VideoQualityTest::SetupScreenshare() {
- RTC_CHECK(params_.screenshare.enabled);
-
- // Fill out codec settings.
- video_encoder_config_.content_type =
VideoEncoderConfig::ContentType::kScreen;
- degradation_preference_ =
- VideoSendStream::DegradationPreference::kMaintainResolution;
- if (params_.video.codec == "VP8") {
- VideoCodecVP8 vp8_settings = VideoEncoder::GetDefaultVp8Settings();
- vp8_settings.denoisingOn = false;
- vp8_settings.frameDroppingOn = false;
- vp8_settings.numberOfTemporalLayers =
- static_cast<unsigned char>(params_.video.num_temporal_layers);
- video_encoder_config_.encoder_specific_settings = new
rtc::RefCountedObject<
- VideoEncoderConfig::Vp8EncoderSpecificSettings>(vp8_settings);
- } else if (params_.video.codec == "VP9") {
+void VideoQualityTest::SetupScreenshareOrSVC() {
+ if (params_.screenshare.enabled) {
+ // Fill out codec settings.
+ video_encoder_config_.content_type =
+ VideoEncoderConfig::ContentType::kScreen;
+ degradation_preference_ =
+ VideoSendStream::DegradationPreference::kMaintainResolution;
+ if (params_.video.codec == "VP8") {
+ VideoCodecVP8 vp8_settings = VideoEncoder::GetDefaultVp8Settings();
+ vp8_settings.denoisingOn = false;
+ vp8_settings.frameDroppingOn = false;
+ vp8_settings.numberOfTemporalLayers =
+ static_cast<unsigned char>(params_.video.num_temporal_layers);
+ video_encoder_config_.encoder_specific_settings =
+ new rtc::RefCountedObject<
+ VideoEncoderConfig::Vp8EncoderSpecificSettings>(vp8_settings);
+ } else if (params_.video.codec == "VP9") {
+ VideoCodecVP9 vp9_settings = VideoEncoder::GetDefaultVp9Settings();
+ vp9_settings.denoisingOn = false;
+ vp9_settings.frameDroppingOn = false;
+ vp9_settings.numberOfTemporalLayers =
+ static_cast<unsigned char>(params_.video.num_temporal_layers);
+ vp9_settings.numberOfSpatialLayers =
+ static_cast<unsigned char>(params_.ss.num_spatial_layers);
+ video_encoder_config_.encoder_specific_settings =
+ new rtc::RefCountedObject<
+ VideoEncoderConfig::Vp9EncoderSpecificSettings>(vp9_settings);
+ }
+ // Setup frame generator.
+ const size_t kWidth = 1850;
+ const size_t kHeight = 1110;
+ std::vector<std::string> slides;
+ slides.push_back(test::ResourcePath("web_screenshot_1850_1110", "yuv"));
+ slides.push_back(test::ResourcePath("presentation_1850_1110", "yuv"));
+ slides.push_back(test::ResourcePath("photo_1850_1110", "yuv"));
+ slides.push_back(test::ResourcePath("difficult_photo_1850_1110", "yuv"));
+
+ if (params_.screenshare.scroll_duration == 0) {
+ // Cycle image every slide_change_interval seconds.
+ frame_generator_.reset(test::FrameGenerator::CreateFromYuvFile(
+ slides, kWidth, kHeight,
+ params_.screenshare.slide_change_interval * params_.video.fps));
+ } else {
+ RTC_CHECK_LE(params_.video.width, kWidth);
+ RTC_CHECK_LE(params_.video.height, kHeight);
+ RTC_CHECK_GT(params_.screenshare.slide_change_interval, 0);
+ const int kPauseDurationMs = (params_.screenshare.slide_change_interval -
+ params_.screenshare.scroll_duration) *
+ 1000;
+ RTC_CHECK_LE(params_.screenshare.scroll_duration,
+ params_.screenshare.slide_change_interval);
+
+ frame_generator_.reset(
+ test::FrameGenerator::CreateScrollingInputFromYuvFiles(
+ clock_, slides, kWidth, kHeight, params_.video.width,
+ params_.video.height, params_.screenshare.scroll_duration * 1000,
+ kPauseDurationMs));
+ }
+ } else if (params_.ss.num_spatial_layers > 1) { // For non-screenshare case.
+ RTC_CHECK(params_.video.codec == "VP9");
VideoCodecVP9 vp9_settings = VideoEncoder::GetDefaultVp9Settings();
- vp9_settings.denoisingOn = false;
- vp9_settings.frameDroppingOn = false;
vp9_settings.numberOfTemporalLayers =
static_cast<unsigned char>(params_.video.num_temporal_layers);
vp9_settings.numberOfSpatialLayers =
@@ -1224,37 +1266,6 @@ void VideoQualityTest::SetupScreenshare() {
video_encoder_config_.encoder_specific_settings = new
rtc::RefCountedObject<
VideoEncoderConfig::Vp9EncoderSpecificSettings>(vp9_settings);
}
-
- // Setup frame generator.
- const size_t kWidth = 1850;
- const size_t kHeight = 1110;
- std::vector<std::string> slides;
- slides.push_back(test::ResourcePath("web_screenshot_1850_1110", "yuv"));
- slides.push_back(test::ResourcePath("presentation_1850_1110", "yuv"));
- slides.push_back(test::ResourcePath("photo_1850_1110", "yuv"));
- slides.push_back(test::ResourcePath("difficult_photo_1850_1110", "yuv"));
-
- if (params_.screenshare.scroll_duration == 0) {
- // Cycle image every slide_change_interval seconds.
- frame_generator_.reset(test::FrameGenerator::CreateFromYuvFile(
- slides, kWidth, kHeight,
- params_.screenshare.slide_change_interval * params_.video.fps));
- } else {
- RTC_CHECK_LE(params_.video.width, kWidth);
- RTC_CHECK_LE(params_.video.height, kHeight);
- RTC_CHECK_GT(params_.screenshare.slide_change_interval, 0);
- const int kPauseDurationMs = (params_.screenshare.slide_change_interval -
- params_.screenshare.scroll_duration) *
- 1000;
- RTC_CHECK_LE(params_.screenshare.scroll_duration,
- params_.screenshare.slide_change_interval);
-
- frame_generator_.reset(
- test::FrameGenerator::CreateScrollingInputFromYuvFiles(
- clock_, slides, kWidth, kHeight, params_.video.width,
- params_.video.height, params_.screenshare.scroll_duration * 1000,
- kPauseDurationMs));
- }
}
void VideoQualityTest::CreateCapturer() {
@@ -1324,25 +1335,10 @@ void VideoQualityTest::RunWithAnalyzer(const Params&
params) {
// 0.0 by default. Setting the thresholds to -1.1 prevents the unnecessary
// abort.
VideoStream& selected_stream =
params_.ss.streams[params_.ss.selected_stream];
- int selected_sl = params_.ss.selected_sl != -1
- ? params_.ss.selected_sl
- : params_.ss.num_spatial_layers - 1;
- bool disable_quality_check =
- selected_stream.width != params_.video.width ||
- selected_stream.height != params_.video.height ||
- (!params_.ss.spatial_layers.empty() &&
- params_.ss.spatial_layers[selected_sl].scaling_factor_num !=
- params_.ss.spatial_layers[selected_sl].sc…
(message too large)
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2681683003/#ps210001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 210001, "attempt_start_ts": 1487149616631730,
"parent_rev": "27260ced9fc3e57e51b6be7ca42ae71b28d742e7", "commit_rev":
"2a8c2f589aa5e757cdb5d5cbb13f4494250ddf11"}
Message was sent while issue was closed.
Description was changed from ========== Added Vp9 simulcast tests. For them implemeted upscaling in libyuv metrics calculation. Updated maximum number of SL in vp9 encoder to 3. Refactored names of some fields in Video_quality_check analyzer. BUG=webrtc:7095 ========== to ========== Added Vp9 simulcast tests. For them implemeted upscaling in libyuv metrics calculation. Updated maximum number of SL in vp9 encoder to 3. Refactored names of some fields in Video_quality_check analyzer. BUG=webrtc:7095 Review-Url: https://codereview.webrtc.org/2681683003 Cr-Commit-Position: refs/heads/master@{#16625} Committed: https://chromium.googlesource.com/external/webrtc/+/2a8c2f589aa5e757cdb5d5cbb... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:210001) as https://chromium.googlesource.com/external/webrtc/+/2a8c2f589aa5e757cdb5d5cbb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
