Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1489153002:
DirectRenderer allows empty swap rects for CommitOverlayPlanes (Closed)
DescriptionReland: DirectRenderer allows empty swap rects for CommitOverlayPlanes
Previously DirectRenderer would expand the damage rect to the full
output when partial swap buffers was not supported, because it had to
swap the whole buffer. Now, with the availability of CommitOverlayPlanes
it's possible that the surface has capability "allow_empty_swap", which
means swapping an empty rect is valid regardless of whether partial
swap is available. The surface is responsible for translating an empty
swap rect into a call to CommitOverlayPlanes.
BUG=533630
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/c7589a8de1e21cc5a02279ddffe945af1409a655
Cr-Commit-Position: refs/heads/master@{#363106}
Patch Set 1 #Patch Set 2 : Fix tests #
Total comments: 4
Patch Set 3 : piman's comment #
Total comments: 1
Patch Set 4 : rebase only #Patch Set 5 : Initialize allow_empty_swap #
Messages
Total messages: 39 (18 generated)
Description was changed from ========== DirectRenderer allows empty swap rects with CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 ========== to ========== DirectRenderer allows empty swap rects with CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== DirectRenderer allows empty swap rects with CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== DirectRenderer allows empty swap rects for CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
watk@chromium.org changed reviewers: + ccameron@chromium.org, piman@chromium.org
watk@chromium.org changed required reviewers: + ccameron@chromium.org
Hi ccameron, piman, kalyan. This CL is the evolution of https://codereview.chromium.org/1454323002/ but using CommitOverlayPlanes. PTAL https://codereview.chromium.org/1489153002/diff/20001/cc/output/overlay_unitt... File cc/output/overlay_unittest.cc (left): https://codereview.chromium.org/1489153002/diff/20001/cc/output/overlay_unitt... cc/output/overlay_unittest.cc:1747: TEST_F(GLRendererWithOverlaysTest, OccludedQuadNotDrawn) { I had to modify this test because this CL breaks the optimization this tests for: when the whole framebuffer is obscured by overlays, and !allow_empty_swap and !supports_partial_swap. You can skip drawing, and swap garbage, because it won't be visible. I wanted to avoid adding any more logic to DirectRenderer to handle this. It seems easier to support one of CommitOverlayPlanes or PostSubBuffer. But if this is a useful use case, we could special case it in DirectRenderer::DrawFrame.
LGTM + nit https://codereview.chromium.org/1489153002/diff/20001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/1489153002/diff/20001/cc/output/gl_renderer.c... cc/output/gl_renderer.cc:2628: compositor_frame.gl_frame_data->sub_buffer_rect = swap_buffer_rect_ = nit: only one statement per line. I think you can simply reset swap_buffer_rect_ to gfx::Rect(surface_size) and compositor_frame.gl_frame_data->sub_buffer_rect will be taken care of by l.2631 https://codereview.chromium.org/1489153002/diff/20001/cc/output/overlay_unitt... File cc/output/overlay_unittest.cc (left): https://codereview.chromium.org/1489153002/diff/20001/cc/output/overlay_unitt... cc/output/overlay_unittest.cc:1747: TEST_F(GLRendererWithOverlaysTest, OccludedQuadNotDrawn) { On 2015/12/02 01:05:01, watk wrote: > I had to modify this test because this CL breaks the optimization this tests > for: when the whole framebuffer is obscured by overlays, and !allow_empty_swap > and !supports_partial_swap. You can skip drawing, and swap garbage, because it > won't be visible. > > I wanted to avoid adding any more logic to DirectRenderer to handle this. It > seems easier to support one of CommitOverlayPlanes or PostSubBuffer. But if this > is a useful use case, we could special case it in DirectRenderer::DrawFrame. I think it's ok for now. Currently all platforms that have overlays also have at least partial swaps, so we're not losing anything currently useful.
Thanks! https://codereview.chromium.org/1489153002/diff/20001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/1489153002/diff/20001/cc/output/gl_renderer.c... cc/output/gl_renderer.cc:2628: compositor_frame.gl_frame_data->sub_buffer_rect = swap_buffer_rect_ = On 2015/12/02 01:23:05, piman (slow to review) wrote: > nit: only one statement per line. > > I think you can simply reset swap_buffer_rect_ to gfx::Rect(surface_size) and > compositor_frame.gl_frame_data->sub_buffer_rect will be taken care of by l.2631 Done.
The CQ bit was checked by watk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/1489153002/#ps40001 (title: "piman's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1489153002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1489153002/40001
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Oh, I forgot I starred you ccameron. I would appreciate if you could take a look as well, but if you're too busy let me know and I can unstar you.
This lgtm -- I'll probably look at using this for Mac as well.
On 2015/12/02 21:10:42, ccameron wrote: > This lgtm -- I'll probably look at using this for Mac as well. Great, thanks
The CQ bit was checked by watk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1489153002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1489153002/40001
Message was sent while issue was closed.
Description was changed from ========== DirectRenderer allows empty swap rects for CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== DirectRenderer allows empty swap rects for CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== DirectRenderer allows empty swap rects for CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== DirectRenderer allows empty swap rects for CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/581da2687c9e3031d07f2e0c28e32283b9b3e770 Cr-Commit-Position: refs/heads/master@{#362812} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/581da2687c9e3031d07f2e0c28e32283b9b3e770 Cr-Commit-Position: refs/heads/master@{#362812}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1491013005/ by watk@chromium.org. The reason for reverting is: This probably broken the msan bots, e.g., https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20....
Message was sent while issue was closed.
https://codereview.chromium.org/1489153002/diff/40001/cc/output/renderer.h File cc/output/renderer.h (right): https://codereview.chromium.org/1489153002/diff/40001/cc/output/renderer.h#ne... cc/output/renderer.h:39: bool allow_empty_swap; New field needs new initializer.
Message was sent while issue was closed.
On 2015/12/03 22:51:23, piman (slow to review) wrote: > https://codereview.chromium.org/1489153002/diff/40001/cc/output/renderer.h > File cc/output/renderer.h (right): > > https://codereview.chromium.org/1489153002/diff/40001/cc/output/renderer.h#ne... > cc/output/renderer.h:39: bool allow_empty_swap; > New field needs new initializer. wow, nice catch, thanks
Message was sent while issue was closed.
Description was changed from ========== DirectRenderer allows empty swap rects for CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/581da2687c9e3031d07f2e0c28e32283b9b3e770 Cr-Commit-Position: refs/heads/master@{#362812} ========== to ========== Reland: DirectRenderer allows empty swap rects for CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
watk@chromium.org changed required reviewers: - ccameron@chromium.org
Added the fix to initialize allow_empty_swap.
lgtm
The CQ bit was checked by watk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ccameron@chromium.org Link to the patchset: https://codereview.chromium.org/1489153002/#ps80001 (title: "Initialize allow_empty_swap")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1489153002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1489153002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by watk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1489153002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1489153002/80001
Message was sent while issue was closed.
Description was changed from ========== Reland: DirectRenderer allows empty swap rects for CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Reland: DirectRenderer allows empty swap rects for CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 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 ========== Reland: DirectRenderer allows empty swap rects for CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Reland: DirectRenderer allows empty swap rects for CommitOverlayPlanes Previously DirectRenderer would expand the damage rect to the full output when partial swap buffers was not supported, because it had to swap the whole buffer. Now, with the availability of CommitOverlayPlanes it's possible that the surface has capability "allow_empty_swap", which means swapping an empty rect is valid regardless of whether partial swap is available. The surface is responsible for translating an empty swap rect into a call to CommitOverlayPlanes. BUG=533630 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/c7589a8de1e21cc5a02279ddffe945af1409a655 Cr-Commit-Position: refs/heads/master@{#363106} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c7589a8de1e21cc5a02279ddffe945af1409a655 Cr-Commit-Position: refs/heads/master@{#363106} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
