Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1493013002:
cc: Force commit on the frame after a copy request (Closed)
|
Created:
5 years ago by ccameron Modified:
5 years ago CC:
chromium-reviews, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Force commit on the frame after a copy request
This ensures that the next frame will remove any extra render surfaces
that were created to handle the copy request. Persisting extra render
surfaces would cause extra GPU work and prevent using overlays.
BUG=564832
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/0d063c3894993a381e5f018092351172ccfb3b78
Cr-Commit-Position: refs/heads/master@{#363062}
Patch Set 1 #Patch Set 2 : Update comments #
Total comments: 3
Patch Set 3 : Move to LayerTreeHost #
Total comments: 3
Patch Set 4 : Incorporate review feedback #Patch Set 5 : s/CommitAndDrawFrame/Commit/g #Messages
Total messages: 25 (14 generated)
Description was changed from ========== DelegatedFrameHost: Force full commit on the frame after a copy request This ensures that the next frame will remove any extra render surfaces that were created to handle the copy request. Persisting extra render surfaces would cause extra GPU work and prevent using overlays. BUG=561672 ========== to ========== DelegatedFrameHost: Force full commit on the frame after a copy request This ensures that the next frame will remove any extra render surfaces that were created to handle the copy request. Persisting extra render surfaces would cause extra GPU work and prevent using overlays. BUG=564832 ==========
ccameron@chromium.org changed reviewers: + enne@chromium.org
Turned out to be simple enough. Not the most robust thing in the world, but it fixes the issue for my particular situation.
miu@chromium.org changed reviewers: + miu@chromium.org
One thing for consideration, but otherwise lgtm: https://codereview.chromium.org/1493013002/diff/20001/content/browser/composi... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/1493013002/diff/20001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:1044: force_commit_for_next_frame_ = true; Idea: Should you also call compositor_->ScheduleDraw() here too? This might solve a problem I was mentioning in our meeting, where the compositor can decide to skip a frame. It's always kept our "smoothness" performance for tab capture from being perfect.
danakj@chromium.org changed reviewers: + danakj@chromium.org
Why is this in DelegatedFrameHost and not in LayerTreeHostImpl? Maybe somewhere like https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... Like, this doesn't seem embedder specific?
Description was changed from ========== DelegatedFrameHost: Force full commit on the frame after a copy request This ensures that the next frame will remove any extra render surfaces that were created to handle the copy request. Persisting extra render surfaces would cause extra GPU work and prevent using overlays. BUG=564832 ========== to ========== DelegatedFrameHost: Force full commit on the frame after a copy request This ensures that the next frame will remove any extra render surfaces that were created to handle the copy request. Persisting extra render surfaces would cause extra GPU work and prevent using overlays. BUG=564832 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
My concern there was that this would cause an extra frame whenever a copy request was completed -- I preferred the idea of requesting the commit when we submit the frame, soas to hopefully piggy-back on the existing call. But ... I went and I implemented both ways, and, actually, putting this in the LTHI produces more reliable behavior. PTAL the updated patch. https://codereview.chromium.org/1493013002/diff/20001/content/browser/composi... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/1493013002/diff/20001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:1044: force_commit_for_next_frame_ = true; On 2015/12/02 20:32:57, miu wrote: > Idea: Should you also call compositor_->ScheduleDraw() here too? This might > solve a problem I was mentioning in our meeting, where the compositor can decide > to skip a frame. It's always kept our "smoothness" performance for tab capture > from being perfect. That would seem like a separate bug if we're not getting a commit when we request a copy of the output -- the source of my problem is that that causes a commit which changes the tree, but there is subsequent commit to un-do that change. So I'd prefer to keep that to separate changes.
Thx for the test. LGTM https://codereview.chromium.org/1493013002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/1493013002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1072: // web contents, which will persist until a new commit removes it. Force a s/web contents/layer/ https://codereview.chromium.org/1493013002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1074: // http://crbug.com/564832 prefix this whole comment with TODO(crbug.com/564832): ? https://codereview.chromium.org/1493013002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_copyrequest.cc (right): https://codereview.chromium.org/1493013002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_copyrequest.cc:178: class LayerTreeHostCopyRequestCompletionCausesCommit can you leave a comment to the bug here too so it's obvious this test can go away if we fix said bug, and test for something else instead?
still lgtm, but you'll want to update the change description. https://codereview.chromium.org/1493013002/diff/20001/content/browser/composi... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/1493013002/diff/20001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:1044: force_commit_for_next_frame_ = true; On 2015/12/02 23:03:22, ccameron wrote: > On 2015/12/02 20:32:57, miu wrote: > > Idea: Should you also call compositor_->ScheduleDraw() here too? This might > > solve a problem I was mentioning in our meeting, where the compositor can > decide > > to skip a frame. It's always kept our "smoothness" performance for tab > capture > > from being perfect. > > That would seem like a separate bug if we're not getting a commit when we > request a copy of the output -- the source of my problem is that that causes a > commit which changes the tree, but there is subsequent commit to un-do that > change. So I'd prefer to keep that to separate changes. Acknowledged.
Description was changed from ========== DelegatedFrameHost: Force full commit on the frame after a copy request This ensures that the next frame will remove any extra render surfaces that were created to handle the copy request. Persisting extra render surfaces would cause extra GPU work and prevent using overlays. BUG=564832 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Force commit on the frame after a copy request This ensures that the next frame will remove any extra render surfaces that were created to handle the copy request. Persisting extra render surfaces would cause extra GPU work and prevent using overlays. BUG=564832 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by ccameron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1493013002/#ps60001 (title: "Incorporate review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1493013002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1493013002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by ccameron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/1493013002/#ps80001 (title: "s/CommitAndDrawFrame/Commit/g")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1493013002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1493013002/80001
Message was sent while issue was closed.
Description was changed from ========== cc: Force commit on the frame after a copy request This ensures that the next frame will remove any extra render surfaces that were created to handle the copy request. Persisting extra render surfaces would cause extra GPU work and prevent using overlays. BUG=564832 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Force commit on the frame after a copy request This ensures that the next frame will remove any extra render surfaces that were created to handle the copy request. Persisting extra render surfaces would cause extra GPU work and prevent using overlays. BUG=564832 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== cc: Force commit on the frame after a copy request This ensures that the next frame will remove any extra render surfaces that were created to handle the copy request. Persisting extra render surfaces would cause extra GPU work and prevent using overlays. BUG=564832 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Force commit on the frame after a copy request This ensures that the next frame will remove any extra render surfaces that were created to handle the copy request. Persisting extra render surfaces would cause extra GPU work and prevent using overlays. BUG=564832 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/0d063c3894993a381e5f018092351172ccfb3b78 Cr-Commit-Position: refs/heads/master@{#363062} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0d063c3894993a381e5f018092351172ccfb3b78 Cr-Commit-Position: refs/heads/master@{#363062}
Message was sent while issue was closed.
Patchset #6 (id:100001) has been deleted |
