Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871623002:
Update video adaptation stats to support degradations in both resolution and framerate. (Closed)
|
Created:
3 years, 7 months ago by åsapersson Modified:
3 years, 7 months 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. |
DescriptionUpdate adaptation stats to support degradations in both resolution and framerate.
Add AdaptCounter class which holds the number of downgrade counts per degradation way (resolution/fps) and reason (cpu/quality).
BUG=webrtc:7607
Review-Url: https://codereview.webrtc.org/2871623002
Cr-Commit-Position: refs/heads/master@{#18156}
Committed: https://chromium.googlesource.com/external/webrtc/+/09f05616757ed8d6215ad5dd5b1eb270625c1a7c
Patch Set 1 #
Total comments: 23
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : address comment #
Total comments: 2
Patch Set 4 : address comments #
Messages
Total messages: 33 (19 generated)
Description was changed from ========== Update adaptation stats... BUG= ========== to ========== Update adaptation stats to support both degradations in resolution and framerate. Add AdaptCounter class which holds the number of downgrade counts per degradation way (resolution/fps) and reason (cpu/quality). BUG= ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
asapersson@webrtc.org changed reviewers: + kthelgason@webrtc.org, sprang@webrtc.org
Description was changed from ========== Update adaptation stats to support both degradations in resolution and framerate. Add AdaptCounter class which holds the number of downgrade counts per degradation way (resolution/fps) and reason (cpu/quality). BUG= ========== to ========== Update adaptation stats to support both degradations in resolution and framerate. Add AdaptCounter class which holds the number of downgrade counts per degradation way (resolution/fps) and reason (cpu/quality). BUG=webrtc:7607 ==========
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Update adaptation stats to support both degradations in resolution and framerate. Add AdaptCounter class which holds the number of downgrade counts per degradation way (resolution/fps) and reason (cpu/quality). BUG=webrtc:7607 ========== to ========== Update adaptation stats to support degradations in both resolution and framerate. Add AdaptCounter class which holds the number of downgrade counts per degradation way (resolution/fps) and reason (cpu/quality). BUG=webrtc:7607 ==========
https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.h (right): https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.h:67: const ViEEncoder::AdaptCounts& quality_counts); It seems wrong to me that both these methods take cpu_counts AND quality_counts. Shouldn't they just need one each? https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy_unittest.cc (right): https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:404: statistics_proxy_->SetAdaptationStats(quality_counts, quality_counts); first param should be `cpu_counts` https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:432: EXPECT_EQ(1, statistics_proxy_->GetStats().number_of_cpu_adapt_changes); Why is this 1? https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:439: EXPECT_EQ(2, statistics_proxy_->GetStats().number_of_cpu_adapt_changes); is this expected behavior? If the number of fps adaptations goes from 0 to 3 is that just a single "change"? https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:451: EXPECT_EQ(1, statistics_proxy_->GetStats().number_of_quality_adapt_changes); same here, should this be 1? Nothing has changes AFAIK https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:458: EXPECT_EQ(2, statistics_proxy_->GetStats().number_of_quality_adapt_changes); ditto https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:1006: } are these two methods necessary? We have the AdaptCounter available and can just call IncrementResolution or Framerate directly whenever we use this. https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:1066: return sum; I'd prefer `return std::accumulate(counters.begin(), counters.end(), 0);` for the sake of simplicity. https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:1071: std::stringstream ss; There has been some discussion about not adding new code that uses std::stringstream, but I don't remember the conclusion. I defer to the judgement of my seniors here! https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.h:220: EXCLUSIVE_LOCKS_REQUIRED(&encoder_queue_); I've been using the RUN_ON macro for asserting that something runs on a specific queue. They do the same thing but I find RUN_ON to be more readable, since we're dealing with a queue, not a lock.
Patchset #2 (id:80001) has been deleted
https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.h (right): https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.h:67: const ViEEncoder::AdaptCounts& quality_counts); On 2017/05/09 11:50:14, kthelgason wrote: > It seems wrong to me that both these methods take cpu_counts AND quality_counts. > Shouldn't they just need one each? Right, only one is needed for this cl but it was done to prepare for balanced mode. E.g. if adapting down due to cpu and quality and the adapt up is in a different order: Adapt down resolution (cpu): cpu={res:1,fps:0}, quality={res:0,fps:0} Adapt down fps (quality): cpu={res:1,fps:0}, quality={res:0,fps:1} Adapt up fps (cpu): cpu={res:0,fps:0}, quality={res:1,fps:0} then both the cpu and quality counts can change. https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy_unittest.cc (right): https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:404: statistics_proxy_->SetAdaptationStats(quality_counts, quality_counts); On 2017/05/09 11:50:15, kthelgason wrote: > first param should be `cpu_counts` thanks, done. https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:432: EXPECT_EQ(1, statistics_proxy_->GetStats().number_of_cpu_adapt_changes); On 2017/05/09 11:50:15, kthelgason wrote: > Why is this 1? OnCpuAdaptationChanged is supposed to be called on every adaptation change (and one of the counters should be changed by one). The number_of_cpu_adapt_changes is incremented on every call and the counters are just updated with the current counts. I have modified the test to have realistic values for the counters. https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:439: EXPECT_EQ(2, statistics_proxy_->GetStats().number_of_cpu_adapt_changes); On 2017/05/09 11:50:15, kthelgason wrote: > is this expected behavior? If the number of fps adaptations goes from 0 to 3 is > that just a single "change"? see comment above https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:451: EXPECT_EQ(1, statistics_proxy_->GetStats().number_of_quality_adapt_changes); On 2017/05/09 11:50:15, kthelgason wrote: > same here, should this be 1? Nothing has changes AFAIK see comment above https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy_unittest.cc:458: EXPECT_EQ(2, statistics_proxy_->GetStats().number_of_quality_adapt_changes); On 2017/05/09 11:50:15, kthelgason wrote: > ditto see comment above https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:1006: } On 2017/05/09 11:50:15, kthelgason wrote: > are these two methods necessary? We have the AdaptCounter available and can just > call IncrementResolution or Framerate directly whenever we use this. Removed these methods and added a non const GetAdaptCounter. https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:1066: return sum; On 2017/05/09 11:50:15, kthelgason wrote: > I'd prefer `return std::accumulate(counters.begin(), counters.end(), 0);` for > the sake of simplicity. Done. https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.h:220: EXCLUSIVE_LOCKS_REQUIRED(&encoder_queue_); On 2017/05/09 11:50:15, kthelgason wrote: > I've been using the RUN_ON macro for asserting that something runs on a specific > queue. They do the same thing but I find RUN_ON to be more readable, since we're > dealing with a queue, not a lock. Done.
lgtm https://codereview.webrtc.org/2871623002/diff/100001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2871623002/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.h:220: ViEEncoder::AdaptCounts GetActiveCounts(AdaptReason reason) this scope resolution isn't needed, right?
https://codereview.webrtc.org/2871623002/diff/100001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2871623002/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.h:220: ViEEncoder::AdaptCounts GetActiveCounts(AdaptReason reason) On 2017/05/10 08:32:46, kthelgason wrote: > this scope resolution isn't needed, right? Done.
lgtm with nits https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.h:68: }; Maybe this can be declared together with the counter class down below? https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.h:187: ~AdaptCounter(); nit: virtual destructor or final class https://codereview.webrtc.org/2871623002/diff/120001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2871623002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.h:68: }; Maybe add a comment what the numbers mean, especially -1 which means disabled?
https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.h:68: }; On 2017/05/10 09:34:23, sprang_webrtc wrote: > Maybe this can be declared together with the counter class down below? The struct is public. https://codereview.webrtc.org/2871623002/diff/60001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.h:187: ~AdaptCounter(); On 2017/05/10 09:34:24, sprang_webrtc wrote: > nit: virtual destructor or final class Done. https://codereview.webrtc.org/2871623002/diff/120001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2871623002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.h:68: }; On 2017/05/10 09:34:24, sprang_webrtc wrote: > Maybe add a comment what the numbers mean, especially -1 which means disabled? Done.
asapersson@webrtc.org changed reviewers: + mflodman@webrtc.org
Magnus, can you have a look at webrtc/video_send_stream.h
The CQ bit was checked by asapersson@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.
lgtm
video_send_stream.h lgtm
The CQ bit was checked by asapersson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org Link to the patchset: https://codereview.webrtc.org/2871623002/#ps140001 (title: "address comments")
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: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/24608)
The CQ bit was checked by asapersson@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": 140001, "attempt_start_ts": 1494916431235770,
"parent_rev": "9a6f4d4316af2307ac6699525559ecea3500f75f", "commit_rev":
"09f05616757ed8d6215ad5dd5b1eb270625c1a7c"}
Message was sent while issue was closed.
Description was changed from ========== Update adaptation stats to support degradations in both resolution and framerate. Add AdaptCounter class which holds the number of downgrade counts per degradation way (resolution/fps) and reason (cpu/quality). BUG=webrtc:7607 ========== to ========== Update adaptation stats to support degradations in both resolution and framerate. Add AdaptCounter class which holds the number of downgrade counts per degradation way (resolution/fps) and reason (cpu/quality). BUG=webrtc:7607 Review-Url: https://codereview.webrtc.org/2871623002 Cr-Commit-Position: refs/heads/master@{#18156} Committed: https://chromium.googlesource.com/external/webrtc/+/09f05616757ed8d6215ad5dd5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/09f05616757ed8d6215ad5dd5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
