Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2310853002:
Refactor QualityScaler and MovingAverage (Closed)
|
Created:
4 years, 3 months ago by kthelgason Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRefactor QualityScaler and MovingAverage
The MovingAverage class was very specific to the QualityScaler. This
commit generalizes the MovingAverage class to be useful in other
situations as well, and adapts the QualityScaler to use the new
MovingAverage.
BUG=webrtc:6304
Committed: https://crrev.com/194f40a2e77481b727e6656a6945bc23180eb9ff
Cr-Commit-Position: refs/heads/master@{#14207}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Refactor according to review #Patch Set 3 : add missing include #Patch Set 4 : update gyp files #
Total comments: 12
Patch Set 5 : Review round 2 #
Total comments: 1
Patch Set 6 : fix nit #Patch Set 7 : fix build error #Messages
Total messages: 55 (33 generated)
Description was changed from ========== refactor QualityScaler and MovingAverage The MovingAverage class was very specific to the QualityScaler. This commit generalizes the MovingAverage class to be useful in other situations as well, and adapts the QualityScaler to use the new MovingAverage. BUG=webrtc:6304 ========== to ========== refactor QualityScaler and MovingAverage The MovingAverage class was very specific to the QualityScaler. This commit generalizes the MovingAverage class to be useful in other situations as well, and adapts the QualityScaler to use the new MovingAverage. BUG=webrtc:6304 BUG=webrtc:5557 ==========
kthelgason@webrtc.org changed reviewers: + magjed@webrtc.org, pbos@webrtc.org
kthelgason@webrtc.org changed reviewers: + sakal@webrtc.org
https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... File webrtc/modules/video_coding/utility/moving_average.cc (right): https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. You probably mean 2016. https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.cc:9: */ nit: add an empty line here https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.cc:10: #include "webrtc/modules/video_coding/utility/moving_average.h" nit: add an empty line here https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.cc:15: MovingAverage::MovingAverage() : MovingAverage(1024) {} I would leave out the default constructor. I don't feel like there is a sensible default for the size of the ring buffer so I would leave it to the user to decide. https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.cc:17: MovingAverage::MovingAverage(size_t s) : samples_(s, 0) {} nit: samples(s) would be enough, values are initialized to 0 by default https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.cc:26: return GetAverage(count_ + 1); Why +1? I saw you commented it is an array index on the other CL. However, this is 1 after adding one sample though? https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.cc:29: int MovingAverage::GetAverage(size_t num_samples) const { I would rather this method return rtc::Optional in case there are not enough samples. https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.cc:33: return sum / num_samples; This will always round down. Is this the desired behavior? Maybe also worth considering returning double instead? https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.cc:39: std::fill(samples_.begin(), samples_.end(), 0); nit: This is not necessarily required because we will overwrite the values before we use them. So, just samples_[0] = 0; would be enough. I am okay with keeping it if you feel like it though. https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.cc:44: } nit: add an empty line here https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... File webrtc/modules/video_coding/utility/moving_average.h (right): https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.h:32: std::vector<int> samples_; This vector does not contain samples. Maybe sum_history_ would be more descriptive.
Closing this for now. Going to go another route and use ExpFilter instead and delete the MovingAverage class.
Message was sent while issue was closed.
Write the bugs as BUG=webrtc:6304,webrtc:5557 https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... File webrtc/modules/video_coding/utility/moving_average.cc (right): https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.cc:26: return GetAverage(count_ + 1); On 2016/09/05 12:28:10, sakal wrote: > Why +1? I saw you commented it is an array index on the other CL. However, this > is 1 after adding one sample though? Yes, I'm also pretty sure this is a bug. |count_| is the number of samples added, so there should be no +1. In size() you use it correctly without +1. Actually, you should consider using size() here instead of |count_|. https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.cc:30: num_samples = std::min(num_samples, this->size()); Is this check really needed if you use size() in the other GetAverage() function? https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/moving_average.cc:32: int sum = sum_ - samples_[(count_ - num_samples) % samples_.size()]; nit: Use const. https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/quality_scaler.cc:90: if (fast_rampup_) I would prefer to write this as: num_samples_upscale_ = framerate * (fast_rampup_ ? kMeasureSecondsFastUpscale : kMeasureSecondsUpscale); https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/quality_scaler.cc:118: // too much framedrop or too high QP. We want to scale down. nit: Start with capital letter in comments. https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/quality_scaler.cc:136: int frame_width = res_.width >> downscale_shift_; Use const (or inline these). https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/quality_scaler.cc:162: std::log2(std::min(width, height)/kMinDownscaleDimension)); nit: formatting. Use git cl format. https://codereview.webrtc.org/2310853002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/quality_scaler.cc:164: res_ = Resolution{width, height}; Maybe this variable should be called target_resolution_ instead.
Description was changed from ========== refactor QualityScaler and MovingAverage The MovingAverage class was very specific to the QualityScaler. This commit generalizes the MovingAverage class to be useful in other situations as well, and adapts the QualityScaler to use the new MovingAverage. BUG=webrtc:6304 BUG=webrtc:5557 ========== to ========== refactor QualityScaler and MovingAverage The MovingAverage class was very specific to the QualityScaler. This commit generalizes the MovingAverage class to be useful in other situations as well, and adapts the QualityScaler to use the new MovingAverage. BUG=webrtc:6304,webrtc:5557 ==========
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_gyp_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gyp_rel/builds/326)
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: ios64_gyp_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_rel/builds/229)
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_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/16428)
This is ready for review. The android trybot failure is unrelated.
https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/moving_average.cc (right): https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/moving_average.cc:35: if (sum == 0) What is this special case for? Looks like a bug to me. https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:69: ClearSamples(); This is not necessary since ReportFramerate will clear the samples anyway, right? https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:134: // Check if we should scale up or down based on QP. We never scale up here.
https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/moving_average.cc (right): https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/moving_average.cc:33: return rtc::Optional<int>(0); I find it more intuitive if we return rtc::Optional<int>() in the case num_samples==0. You would need to change the return type of the the other GetAverage() function for that change as well. https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/moving_average_unittest.cc (right): https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/moving_average_unittest.cc:15: TEST(MovingAverageTest, GetAverage) { It's really good that you have added tests! I would like to add some really basic tests as well: // Test empty container. TEST(MovingAverageTest, GetAverage) { webrtc::MovingAverage moving_average(1); EXPECT_EQ(0u, moving_average.size()); EXPECT(!moving_average.GetAverage()); EXPECT(!moving_average.GetAverage(0)); } // Test single value. TEST(MovingAverageTest, GetAverage) { webrtc::MovingAverage moving_average(1); moving_average.AddSample(3); EXPECT_EQ(1u, moving_average.size()); EXPECT_EQ(3, *moving_average.GetAverage()); EXPECT_EQ(3, *moving_average.GetAverage(1)); EXPECT(!moving_average.GetAverage(2)); } https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:96: num_samples_upscale_ = framerate * kMeasureSecondsFastUpscale; nit: I would like to write this as a single assignment of num_samples_upscale_ using ternary to select FastUpscale vs Upscale. https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:126: if (frames_seen_ >= num_samples_downscale_) { Do we really need this check now that we have rtc::Optional as a return value of GetAverage? I expected this code to be: const rtc::Optional<int> drop_rate = framedrop_percent_.GetAverage(num_samples_downscale_); const rtc::Optional<int> avg_qp_down = average_qp_.GetAverage(num_samples_downscale_); if ((drop_rate && *drop_rate >= kFramedropPercentThreshold) || (avg_qp_down && *avg_qp_down > high_qp_threshold_)) { ScaleDown(); return; } https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:141: if (frames_seen_ >= num_samples_upscale_) { ditto: the if-statement seems redundant.
https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/moving_average.cc (right): https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/moving_average.cc:35: if (sum == 0) On 2016/09/07 09:47:21, sakal wrote: > What is this special case for? Looks like a bug to me. Well spotted. That was left in by accident when I was making some changes. https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/moving_average_unittest.cc (right): https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/moving_average_unittest.cc:15: TEST(MovingAverageTest, GetAverage) { On 2016/09/07 11:55:16, magjed_webrtc wrote: > It's really good that you have added tests! > > I would like to add some really basic tests as well: > // Test empty container. > TEST(MovingAverageTest, GetAverage) { > webrtc::MovingAverage moving_average(1); > EXPECT_EQ(0u, moving_average.size()); > EXPECT(!moving_average.GetAverage()); > EXPECT(!moving_average.GetAverage(0)); > } > > // Test single value. > TEST(MovingAverageTest, GetAverage) { > webrtc::MovingAverage moving_average(1); > moving_average.AddSample(3); > EXPECT_EQ(1u, moving_average.size()); > EXPECT_EQ(3, *moving_average.GetAverage()); > EXPECT_EQ(3, *moving_average.GetAverage(1)); > EXPECT(!moving_average.GetAverage(2)); > } Acknowledged. https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:126: if (frames_seen_ >= num_samples_downscale_) { On 2016/09/07 11:55:16, magjed_webrtc wrote: > Do we really need this check now that we have rtc::Optional as a return value of > GetAverage? Yeah, you're right that it's not necessary. I thought it might be better to be explicit about when we try to scale up, but looking at it now it's probably clear just from the usage of MovingAverage here. Definitely not worth the added code anyway. https://codereview.webrtc.org/2310853002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:134: // Check if we should scale up or down based on QP. On 2016/09/07 09:47:21, sakal wrote: > We never scale up here. The comment applies to the following two branches, at least that's how it was intended. If it's not clear I will fix it.
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_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/16301)
lgtm https://codereview.webrtc.org/2310853002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/2310853002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:95: if (fast_rampup_) nit: num_samples_upscale_ = framerate * (fast_rampup_ ? kMeasureSecondsFastUpscale : kMeasureSecondsUpscale);
Description was changed from ========== refactor QualityScaler and MovingAverage The MovingAverage class was very specific to the QualityScaler. This commit generalizes the MovingAverage class to be useful in other situations as well, and adapts the QualityScaler to use the new MovingAverage. BUG=webrtc:6304,webrtc:5557 ========== to ========== Refactor QualityScaler and MovingAverage The MovingAverage class was very specific to the QualityScaler. This commit generalizes the MovingAverage class to be useful in other situations as well, and adapts the QualityScaler to use the new MovingAverage. BUG=webrtc:6304,webrtc:5557 ==========
lgtm Is issue webrtc:5557 related to this CL? It looks like that issue was already fixed in https://codereview.webrtc.org/1971693003.
On 2016/09/08 10:08:36, magjed_webrtc wrote: > lgtm > > Is issue webrtc:5557 related to this CL? It looks like that issue was already > fixed in https://codereview.webrtc.org/1971693003. Oh, you're right, sorry. I just saw that it was still open in the tracker, and that this implementation does not suffer from this problem. But it was clearly fixed in the CL you linked to. I will remove it from the description.
Description was changed from ========== Refactor QualityScaler and MovingAverage The MovingAverage class was very specific to the QualityScaler. This commit generalizes the MovingAverage class to be useful in other situations as well, and adapts the QualityScaler to use the new MovingAverage. BUG=webrtc:6304,webrtc:5557 ========== to ========== Refactor QualityScaler and MovingAverage The MovingAverage class was very specific to the QualityScaler. This commit generalizes the MovingAverage class to be useful in other situations as well, and adapts the QualityScaler to use the new MovingAverage. BUG=webrtc:6304 ==========
The CQ bit was checked by kthelgason@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, sakal@webrtc.org Link to the patchset: https://codereview.webrtc.org/2310853002/#ps100001 (title: "fix nit")
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: ios64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/343) ios64_gyp_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_rel/builds/343) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/14202) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/18081)
The CQ bit was checked by kthelgason@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, sakal@webrtc.org Link to the patchset: https://codereview.webrtc.org/2310853002/#ps120001 (title: "fix build error")
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/8224)
On 2016/09/09 09:39:23, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > presubmit on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8224) Whoops, still missing comments from pbos@ :)
On 2016/09/09 10:13:55, kthelgason wrote: > On 2016/09/09 09:39:23, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > presubmit on master.tryserver.webrtc (JOB_FAILED, > > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8224) > > Whoops, still missing comments from pbos@ :) rs-lgtm, I trust you to own this code :)
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
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/...
Message was sent while issue was closed.
Description was changed from ========== Refactor QualityScaler and MovingAverage The MovingAverage class was very specific to the QualityScaler. This commit generalizes the MovingAverage class to be useful in other situations as well, and adapts the QualityScaler to use the new MovingAverage. BUG=webrtc:6304 ========== to ========== Refactor QualityScaler and MovingAverage The MovingAverage class was very specific to the QualityScaler. This commit generalizes the MovingAverage class to be useful in other situations as well, and adapts the QualityScaler to use the new MovingAverage. BUG=webrtc:6304 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Refactor QualityScaler and MovingAverage The MovingAverage class was very specific to the QualityScaler. This commit generalizes the MovingAverage class to be useful in other situations as well, and adapts the QualityScaler to use the new MovingAverage. BUG=webrtc:6304 ========== to ========== Refactor QualityScaler and MovingAverage The MovingAverage class was very specific to the QualityScaler. This commit generalizes the MovingAverage class to be useful in other situations as well, and adapts the QualityScaler to use the new MovingAverage. BUG=webrtc:6304 Committed: https://crrev.com/194f40a2e77481b727e6656a6945bc23180eb9ff Cr-Commit-Position: refs/heads/master@{#14207} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/194f40a2e77481b727e6656a6945bc23180eb9ff Cr-Commit-Position: refs/heads/master@{#14207} |
