Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2887303003:
Implement kBalanced degradation preference. (Closed)
|
Created:
3 years, 7 months ago by åsapersson Modified:
3 years, 6 months ago 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. |
DescriptionImplement kBalanced degradation preference.
A balance of framerate reduction and resolution down-scaling is used on degrades.
BUG=webrtc:7607
Review-Url: https://codereview.webrtc.org/2887303003
Cr-Commit-Position: refs/heads/master@{#18583}
Committed: https://chromium.googlesource.com/external/webrtc/+/f7e294d5687cd3c5851dfd01ee8d44eb4c3bde46
Patch Set 1 #
Total comments: 12
Patch Set 2 : compare request with sink_wants_ in VideoSourceProxy methods and merge with new methods #Patch Set 3 #Patch Set 4 : address comments #
Total comments: 4
Patch Set 5 : rebase #Patch Set 6 : address comments #Patch Set 7 : rebase #
Total comments: 4
Patch Set 8 : fix issue #Patch Set 9 : remove bitrate limits #Patch Set 10 : address comments #Patch Set 11 : rebase #
Messages
Total messages: 34 (25 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Implement kBalanced degradation preference. A balance of framerate reduction and resolution down-scaling is used on degrades. BUG=webrtc:7607 ========== to ========== Implement kBalanced degradation preference. A balance of framerate reduction and resolution down-scaling is used on degrades. BUG=webrtc:7607 ==========
asapersson@webrtc.org changed reviewers: + kthelgason@webrtc.org, sprang@webrtc.org
Patchset #1 (id:20001) has been deleted
First pass. This is getting quite complicated, perhaps this logic should be moved out of ViEEncoder eventually? https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:82: // kbps: 0 60 130 500 | Just out of curiosity, how are these determined? https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:363: // Called on the encoder task queue. Do you want to add a thread_checker here to make this assumption explicit? https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:371: return true; I don't really like this weird way of signalling what changed. Can we just return a bit-vector that's 1 if fps changed, 2 if resolution changed and 0 otherwise? Or perhaps we can do something else like return a struct or enum or something. https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:386: if (source_) Maybe we should just bail out right at the start if there is no source. Isn't all of this pointless if there is no source that can be adapted? https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:1059: switch (degradation_preference_) { Why do we have these two identical switch statements right after each other? https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:1195: // E.g. framerate adapt down: quality, framerate adapt up: cpu. Why is this the case? This seems very confusing.
https://codereview.webrtc.org/2887303003/diff/100001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2887303003/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:1061: RTC_DCHECK_EQ(fps_counters_.size(), 2) << "Update MoveCount()"; Perhaps it would be better to add a `static_assert` on `kScaleReasonSize` so this would fail at compile time rather than runtime. https://codereview.webrtc.org/2887303003/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:1148: RTC_DCHECK_GE((*counters)[from_reason], 0); This postcondition is not at all clear to me. If this is only intended to be called with (*counters)[from_reason] >= 1, perhaps that should be documented in a comment and a precondition rather than a postcondition.
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 #6 (id:140001) 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: Try jobs failed on following builders: win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/14223) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/26792)
https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:82: // kbps: 0 60 130 500 | On 2017/05/19 08:34:14, kthelgason wrote: > Just out of curiosity, how are these determined? The bitrate limits were set roughly to a value when bits per pixel were below some threshold (around 0.07). https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:363: // Called on the encoder task queue. On 2017/05/19 08:34:15, kthelgason wrote: > Do you want to add a thread_checker here to make this assumption explicit? Can maybe do it in a separate cl. https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:371: return true; On 2017/05/19 08:34:14, kthelgason wrote: > I don't really like this weird way of signalling what changed. Can we just > return a bit-vector that's 1 if fps changed, 2 if resolution changed and 0 > otherwise? Or perhaps we can do something else like return a struct or enum or > something. This part has been changed in https://codereview.webrtc.org/2903563002/. https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:386: if (source_) On 2017/05/19 08:34:14, kthelgason wrote: > Maybe we should just bail out right at the start if there is no source. Isn't > all of this pointless if there is no source that can be adapted? Done in https://codereview.webrtc.org/2903563002/. https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:1059: switch (degradation_preference_) { On 2017/05/19 08:34:14, kthelgason wrote: > Why do we have these two identical switch statements right after each other? Removed the one above. https://codereview.webrtc.org/2887303003/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:1195: // E.g. framerate adapt down: quality, framerate adapt up: cpu. On 2017/05/19 08:34:14, kthelgason wrote: > Why is this the case? This seems very confusing. Updated comment. https://codereview.webrtc.org/2887303003/diff/100001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2887303003/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:1061: RTC_DCHECK_EQ(fps_counters_.size(), 2) << "Update MoveCount()"; On 2017/05/30 11:17:06, kthelgason wrote: > Perhaps it would be better to add a `static_assert` on `kScaleReasonSize` so > this would fail at compile time rather than runtime. Done. https://codereview.webrtc.org/2887303003/diff/100001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:1148: RTC_DCHECK_GE((*counters)[from_reason], 0); On 2017/05/30 11:17:06, kthelgason wrote: > This postcondition is not at all clear to me. If this is only intended to be > called with (*counters)[from_reason] >= 1, perhaps that should be documented in > a comment and a precondition rather than a postcondition. Done.
Patchset #9 (id:220001) has been deleted
Patchset #9 (id:240001) has been deleted
Patchset #9 (id:260001) has been deleted
Patchset #9 (id:280001) has been deleted
I don't really know how the framerate scaling stuff is supposed to work, so I can't comment on that. The code lgtm.
lgtm with nits Would be great if this was documented in a short design doc. https://codereview.webrtc.org/2887303003/diff/180001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2887303003/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:77: // >640x480: | max | I think there could be a case for 24fps for 720p as well? But maybe in followup in that case, this is getting complex. https://codereview.webrtc.org/2887303003/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:92: return std::numeric_limits<int>::max(); nit: please use brackets for if/else statements
https://codereview.webrtc.org/2887303003/diff/180001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2887303003/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:77: // >640x480: | max | On 2017/06/13 11:58:29, sprang_webrtc wrote: > I think there could be a case for 24fps for 720p as well? But maybe in followup > in that case, this is getting complex. Agree, these are initial limits. https://codereview.webrtc.org/2887303003/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:92: return std::numeric_limits<int>::max(); On 2017/06/13 11:58:29, sprang_webrtc wrote: > nit: please use brackets for if/else statements Done.
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.
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, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2887303003/#ps340001 (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": 340001, "attempt_start_ts": 1497421397263560,
"parent_rev": "b749e5e1f591fa804fb3493aec819ff39a881fc5", "commit_rev":
"f7e294d5687cd3c5851dfd01ee8d44eb4c3bde46"}
Message was sent while issue was closed.
Description was changed from ========== Implement kBalanced degradation preference. A balance of framerate reduction and resolution down-scaling is used on degrades. BUG=webrtc:7607 ========== to ========== Implement kBalanced degradation preference. A balance of framerate reduction and resolution down-scaling is used on degrades. BUG=webrtc:7607 Review-Url: https://codereview.webrtc.org/2887303003 Cr-Commit-Position: refs/heads/master@{#18583} Committed: https://chromium.googlesource.com/external/webrtc/+/f7e294d5687cd3c5851dfd01e... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:340001) as https://chromium.googlesource.com/external/webrtc/+/f7e294d5687cd3c5851dfd01e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
