Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2800403002:
Scale counter and resolution downgrade stats may be incorrect. (Closed)
|
Created:
3 years, 8 months ago by åsapersson Modified:
3 years, 8 months ago Reviewers:
kthelgason CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionOnly increment scale counter and adaptation stats (and store last_adaptation_request_) if sink_wants_ is updated.
BUG=webrtc:7492
Review-Url: https://codereview.webrtc.org/2800403002
Cr-Commit-Position: refs/heads/master@{#17808}
Committed: https://chromium.googlesource.com/external/webrtc/+/d0de295119a32fc994b18e49e1629d88a9d4e386
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #
Total comments: 8
Patch Set 4 : address comments #Messages
Total messages: 37 (31 generated)
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: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15977)
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: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15991)
Description was changed from ========== Adaptation stats... BUG= ========== to ========== Only increment scale counter and adaptation stats (and store last_adaptation_request_) if sink_wants_ is updated. BUG= ==========
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: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/19560) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24862) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/23699)
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: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/16187)
Description was changed from ========== Only increment scale counter and adaptation stats (and store last_adaptation_request_) if sink_wants_ is updated. BUG= ========== to ========== Only increment scale counter and adaptation stats (and store last_adaptation_request_) if sink_wants_ is updated. BUG=webrtc:7492 ==========
Patchset #3 (id:40001) has been deleted
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.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
asapersson@webrtc.org changed reviewers: + kthelgason@webrtc.org
Minor comments, thanks! https://codereview.webrtc.org/2800403002/diff/100001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2800403002/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:839: int cpu_scale_counter = GetScaleCounters()[reason]; this can be const right? https://codereview.webrtc.org/2800403002/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:957: IncrementScaleCounter(reason, -1); Why move this below the switch-case? IMO it's less intuitive to check if the sum is 1, but no big deal. The stats update is better after the scale counter has been updated though. https://codereview.webrtc.org/2800403002/diff/100001/webrtc/video/vie_encoder... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2800403002/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder_unittest.cc:1292: // Expect nothing to change, still no scaling accident? https://codereview.webrtc.org/2800403002/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder_unittest.cc:1525: sink_.WaitForEncodedFrame(960, 540); I've tried to avoid making assumptions about the scale factors in these tests, as they should be unrelated. If it's impossible to test the desired logic without it I think it's ok, but it would be better if we could avoid it.
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
https://codereview.webrtc.org/2800403002/diff/100001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2800403002/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:839: int cpu_scale_counter = GetScaleCounters()[reason]; On 2017/04/19 08:23:04, kthelgason wrote: > this can be const right? Done. https://codereview.webrtc.org/2800403002/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:957: IncrementScaleCounter(reason, -1); On 2017/04/19 08:23:04, kthelgason wrote: > Why move this below the switch-case? IMO it's less intuitive to check if the sum > is 1, but no big deal. The stats update is better after the scale counter has > been updated though. Moved this up. https://codereview.webrtc.org/2800403002/diff/100001/webrtc/video/vie_encoder... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2800403002/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder_unittest.cc:1292: // Expect nothing to change, still no scaling On 2017/04/19 08:23:04, kthelgason wrote: > accident? yes https://codereview.webrtc.org/2800403002/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder_unittest.cc:1525: sink_.WaitForEncodedFrame(960, 540); On 2017/04/19 08:23:04, kthelgason wrote: > I've tried to avoid making assumptions about the scale factors in these tests, > as they should be unrelated. If it's impossible to test the desired logic > without it I think it's ok, but it would be better if we could avoid it. Done.
lgtm
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": 160001, "attempt_start_ts": 1492762344989290,
"parent_rev": "b04646f38bef1715fc1d8788f7343125ad3f2990", "commit_rev":
"d0de295119a32fc994b18e49e1629d88a9d4e386"}
Message was sent while issue was closed.
Description was changed from ========== Only increment scale counter and adaptation stats (and store last_adaptation_request_) if sink_wants_ is updated. BUG=webrtc:7492 ========== to ========== Only increment scale counter and adaptation stats (and store last_adaptation_request_) if sink_wants_ is updated. BUG=webrtc:7492 Review-Url: https://codereview.webrtc.org/2800403002 Cr-Commit-Position: refs/heads/master@{#17808} Committed: https://chromium.googlesource.com/external/webrtc/+/d0de295119a32fc994b18e49e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/d0de295119a32fc994b18e49e... |
