Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2564373002:
Properly report number of quality downscales in stats. (Closed)
|
Created:
4 years ago by kthelgason Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionProperly report number of quality downscales in stats.
A regression was introduced in 876222f that caused these stats to
be reported incorrectly. This used to be only implemented for VP8
but should now be available for all codecs.
BUG=webrtc:6860
Review-Url: https://codereview.webrtc.org/2564373002
Cr-Commit-Position: refs/heads/master@{#15673}
Committed: https://chromium.googlesource.com/external/webrtc/+/0c8c5388355f5df085595d9ea24fa38995708120
Patch Set 1 #Patch Set 2 : add missing header #Patch Set 3 : switch strategy to avoid data race #Patch Set 4 : Fix bug with incorrect initialization #Patch Set 5 : unused include #
Total comments: 14
Patch Set 6 : Code review comments #Patch Set 7 : tests passing #
Total comments: 5
Patch Set 8 : Factor ScalingSettings into decision on whether to report scaling stats #Patch Set 9 : fix tests #
Total comments: 5
Patch Set 10 : code review #Patch Set 11 : clarify check for initialized encoder in simulcast adapter #
Messages
Total messages: 57 (43 generated)
The CQ bit was checked by kthelgason@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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/13333) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15548) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/19634) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/20228)
The CQ bit was checked by kthelgason@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: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/20229)
The CQ bit was checked by kthelgason@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: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/20989)
The CQ bit was checked by kthelgason@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/...
kthelgason@webrtc.org changed reviewers: + asapersson@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:484: report_quality_scales = simulcast_idx == 0; Scaling could be disabled for simulcast_idx == 0? Maybe initialize quality_downscales_ to -1 and use on line 513. https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:511: stats_.bw_limited_resolution || quality_downscales_ > 0; Why remove encoded_image.adapt_reason_.bw_resolutions_disabled > 0. https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:514: if (quality_downscales_ >= 0) { always >= 0? https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy_unittest.cc (left): https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:726: EXPECT_FALSE(statistics_proxy_->GetStats().bw_limited_resolution); I think this should still be tested and not removed. https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:728: encoded_image.adapt_reason_.bw_resolutions_disabled = 1; ditto https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy_unittest.cc (right): https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:684: QualityLimitedHistogramsUpdatedWhenEnabled_NoResolutionDownscale) { Seems like the stats can also be reported if scaling is disabled. https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:738: scale_counter_[reason]); -1? Maybe add a test.
Thanks for the review, I've uploaded a new patchset. Please take a look when you have time. https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:484: report_quality_scales = simulcast_idx == 0; On 2016/12/14 09:25:13, åsapersson wrote: > Scaling could be disabled for simulcast_idx == 0? Maybe initialize > quality_downscales_ to -1 and use on line 513. Done. https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:511: stats_.bw_limited_resolution || quality_downscales_ > 0; On 2016/12/14 09:25:13, åsapersson wrote: > Why remove encoded_image.adapt_reason_.bw_resolutions_disabled > 0. Sorry, that should not have been removed. Fixed! https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:514: if (quality_downscales_ >= 0) { On 2016/12/14 09:25:13, åsapersson wrote: > always >= 0? Acknowledged. https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy_unittest.cc (left): https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:726: EXPECT_FALSE(statistics_proxy_->GetStats().bw_limited_resolution); On 2016/12/14 09:25:13, åsapersson wrote: > I think this should still be tested and not removed. Acknowledged. https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:728: encoded_image.adapt_reason_.bw_resolutions_disabled = 1; On 2016/12/14 09:25:13, åsapersson wrote: > ditto Acknowledged. https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy_unittest.cc (right): https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:684: QualityLimitedHistogramsUpdatedWhenEnabled_NoResolutionDownscale) { On 2016/12/14 09:25:13, åsapersson wrote: > Seems like the stats can also be reported if scaling is disabled. Right. I fixed this and added a test for that case. https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2564373002/diff/80001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:738: scale_counter_[reason]); On 2016/12/14 09:25:13, åsapersson wrote: > -1? Maybe add a test. Wow, good catch, thanks!
The CQ bit was checked by kthelgason@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: win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/21485)
The CQ bit was checked by kthelgason@webrtc.org
The CQ bit was unchecked by kthelgason@webrtc.org
The CQ bit was checked by kthelgason@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/2564373002/diff/120001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/2564373002/diff/120001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:484: report_quality_scales &= simulcast_idx == 0; Shouldn't this be decided from the ScalingSettings? https://codereview.webrtc.org/2564373002/diff/120001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.h (right): https://codereview.webrtc.org/2564373002/diff/120001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.h:159: int quality_downscales_ GUARDED_BY(crit_) = 0; -1
The CQ bit was checked by kthelgason@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...) linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/21453) linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
The CQ bit was checked by kthelgason@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/...
Patchset #9 (id:160001) has been deleted
The CQ bit was checked by kthelgason@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/...
I made some changes that make sure `scaling_enabled` passed to the stats proxy depends on both degradation preference and ScalingSettings as set by the encoder. https://codereview.webrtc.org/2564373002/diff/120001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/2564373002/diff/120001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:484: report_quality_scales &= simulcast_idx == 0; On 2016/12/16 14:45:14, åsapersson wrote: > Shouldn't this be decided from the ScalingSettings? Yes, you're absolutely right, quality scaling is turned off for simulcast and we should be able to remove this. https://codereview.webrtc.org/2564373002/diff/120001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.h (right): https://codereview.webrtc.org/2564373002/diff/120001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.h:159: int quality_downscales_ GUARDED_BY(crit_) = 0; On 2016/12/16 14:45:14, åsapersson wrote: > -1 Actually this makes sense as 0. When scaling is disabled this is set to -1 explicitly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2564373002/diff/120001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.h (right): https://codereview.webrtc.org/2564373002/diff/120001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.h:61: void OnQualityRestrictedResolutionChanged(int nr_of_downscales); s/nr_of_downscales/num_downscales, same for nr_of_quality_downscales. I think that's a more common name for this type of thing. Btw, should this say num_resolution_downscales? https://codereview.webrtc.org/2564373002/diff/180001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc (right): https://codereview.webrtc.org/2564373002/diff/180001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:467: if (NumberOfStreams(codec_) != 1 || streaminfos_.size() != 1) Not obvious to me why this is needed. Could you elaborate? https://codereview.webrtc.org/2564373002/diff/180001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/2564373002/diff/180001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:591: if (!scaling_enabled) { Either remove the negation by swapping the blocks, if (scaling_enabled) { ... } else { ... } or do: if (!scaling_enabled) { ... return; } ... }
Thanks for the comments Stefan, fixed. https://codereview.webrtc.org/2564373002/diff/180001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc (right): https://codereview.webrtc.org/2564373002/diff/180001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:467: if (NumberOfStreams(codec_) != 1 || streaminfos_.size() != 1) On 2016/12/19 12:12:00, stefan-webrtc (holmer) wrote: > Not obvious to me why this is needed. Could you elaborate? This is actually not needed currently, but I ran into a crash here when testing and decided to add a guard. If this method is called before the encoder is initialized |streaminfos_| will have 0 size. https://codereview.webrtc.org/2564373002/diff/180001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/2564373002/diff/180001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:591: if (!scaling_enabled) { On 2016/12/19 12:12:00, stefan-webrtc (holmer) wrote: > Either remove the negation by swapping the blocks, > > if (scaling_enabled) { > ... > } else { > ... > } > > or do: > > if (!scaling_enabled) { > ... > return; > } > ... > } Done.
https://codereview.webrtc.org/2564373002/diff/180001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc (right): https://codereview.webrtc.org/2564373002/diff/180001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:467: if (NumberOfStreams(codec_) != 1 || streaminfos_.size() != 1) On 2016/12/19 12:18:18, kthelgason wrote: > On 2016/12/19 12:12:00, stefan-webrtc (holmer) wrote: > > Not obvious to me why this is needed. Could you elaborate? > > This is actually not needed currently, but I ran into a crash here when testing > and decided to add a guard. If this method is called before the encoder is > initialized |streaminfos_| will have 0 size. Ok, maybe we should write if (!Initialized() || NumberOfStreams(codec)_ != 1) so that it's clear why we have to check streaminfos?
On 2016/12/19 12:21:26, stefan-webrtc (holmer) wrote: > https://codereview.webrtc.org/2564373002/diff/180001/webrtc/modules/video_cod... > File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc > (right): > > https://codereview.webrtc.org/2564373002/diff/180001/webrtc/modules/video_cod... > webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:467: if > (NumberOfStreams(codec_) != 1 || streaminfos_.size() != 1) > On 2016/12/19 12:18:18, kthelgason wrote: > > On 2016/12/19 12:12:00, stefan-webrtc (holmer) wrote: > > > Not obvious to me why this is needed. Could you elaborate? > > > > This is actually not needed currently, but I ran into a crash here when > testing > > and decided to add a guard. If this method is called before the encoder is > > initialized |streaminfos_| will have 0 size. > > Ok, maybe we should write > > if (!Initialized() || NumberOfStreams(codec)_ != 1) > > so that it's clear why we have to check streaminfos? SGTM. Fixed!
The CQ bit was checked by kthelgason@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/...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by kthelgason@webrtc.org
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": 220001, "attempt_start_ts": 1482152557190790,
"parent_rev": "df6075a77ffae5e0de7c272abc56183faf04814e", "commit_rev":
"0c8c5388355f5df085595d9ea24fa38995708120"}
Message was sent while issue was closed.
Description was changed from ========== Properly report number of quality downscales in stats. A regression was introduced in 876222f that caused these stats to be reported incorrectly. This used to be only implemented for VP8 but should now be available for all codecs. BUG=webrtc:6860 ========== to ========== Properly report number of quality downscales in stats. A regression was introduced in 876222f that caused these stats to be reported incorrectly. This used to be only implemented for VP8 but should now be available for all codecs. BUG=webrtc:6860 Review-Url: https://codereview.webrtc.org/2564373002 Cr-Commit-Position: refs/heads/master@{#15673} Committed: https://chromium.googlesource.com/external/webrtc/+/0c8c5388355f5df085595d9ea... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/external/webrtc/+/0c8c5388355f5df085595d9ea...
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:220001) has been created in https://codereview.webrtc.org/2586783003/ by kthelgason@webrtc.org. The reason for reverting is: Breaks perf tests. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
