|
|
DescriptionAvoid using SkClipOp::kReplace_deprecated in software renderer
Currently, the software renderer always uses kReplace_deprecated when
applying clips. This is necessary, as we never save/restore around
clips, never cleaning up the previous clips we've used.
This change causes software renderer to avoid the replace behavior
by saving/restoring around clipping changes in DrawRenderPassQuad.
With this change, any clipped DrawRenderPassQuad will add a
save/restore. Additionally, calling EnsureScissorTestEnabled or
SetScissorTestRect has no immediate impact, instead it just queues
up the clip for the next DrawRenderPassQuad.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2867913002
Cr-Commit-Position: refs/heads/master@{#470981}
Committed: https://chromium.googlesource.com/chromium/src/+/22e6f3e097117be4c680285f58c25c72e6d643b0
Patch Set 1 #Patch Set 2 : rebase + cleanup #Patch Set 3 : add extra robustness to ScopedSaveCanvas #
Total comments: 8
Patch Set 4 : feedback #
Messages
Total messages: 34 (24 generated)
Description was changed from ========== Avoid using SkClipOp::kReplace_deprecated in software renderer Currently, the software renderer always uses kReplace_deprecated when applying clips. This is necessary, as we never save/restore around clips, never cleaning up the previous clips we've used. This change causes software renderer to avoid the replace behavior by saving/restoring around clipping changes in DrawRenderPassQuad. With this change, any clipped DrawRenderPassQuad will add a save/restore. Additionally, calling EnsureScissorTestEnabled or SetScissorTestRect has no immediate impact, instead it just queues up the clip for the next DrawRenderPassQuad. ========== to ========== Avoid using SkClipOp::kReplace_deprecated in software renderer Currently, the software renderer always uses kReplace_deprecated when applying clips. This is necessary, as we never save/restore around clips, never cleaning up the previous clips we've used. This change causes software renderer to avoid the replace behavior by saving/restoring around clipping changes in DrawRenderPassQuad. With this change, any clipped DrawRenderPassQuad will add a save/restore. Additionally, calling EnsureScissorTestEnabled or SetScissorTestRect has no immediate impact, instead it just queues up the clip for the next DrawRenderPassQuad. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Avoid using SkClipOp::kReplace_deprecated in software renderer Currently, the software renderer always uses kReplace_deprecated when applying clips. This is necessary, as we never save/restore around clips, never cleaning up the previous clips we've used. This change causes software renderer to avoid the replace behavior by saving/restoring around clipping changes in DrawRenderPassQuad. With this change, any clipped DrawRenderPassQuad will add a save/restore. Additionally, calling EnsureScissorTestEnabled or SetScissorTestRect has no immediate impact, instead it just queues up the clip for the next DrawRenderPassQuad. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Avoid using SkClipOp::kReplace_deprecated in software renderer Currently, the software renderer always uses kReplace_deprecated when applying clips. This is necessary, as we never save/restore around clips, never cleaning up the previous clips we've used. This change causes software renderer to avoid the replace behavior by saving/restoring around clipping changes in DrawRenderPassQuad. With this change, any clipped DrawRenderPassQuad will add a save/restore. Additionally, calling EnsureScissorTestEnabled or SetScissorTestRect has no immediate impact, instead it just queues up the clip for the next DrawRenderPassQuad. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by ericrk@chromium.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.chromium.or...
The CQ bit was unchecked by ericrk@chromium.org
The CQ bit was checked by ericrk@chromium.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.chromium.or...
The CQ bit was checked by ericrk@chromium.org to run a CQ dry run
ericrk@chromium.org changed reviewers: + enne@chromium.org, reed@google.com
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2867913002/diff/40001/cc/output/software_rend... File cc/output/software_renderer.cc (right): https://codereview.chromium.org/2867913002/diff/40001/cc/output/software_rend... cc/output/software_renderer.cc:54: class ScopedSaveCanvas { I think this is SkAutoCanvasRestore. You can even pass a doSave to it if you don't want to make it optional.
The CQ bit was unchecked by ericrk@chromium.org
That's great news that you found an alternate pattern. https://codereview.chromium.org/2867913002/diff/40001/cc/output/software_rend... File cc/output/software_renderer.cc (right): https://codereview.chromium.org/2867913002/diff/40001/cc/output/software_rend... cc/output/software_renderer.cc:217: base::Optional<ScopedSaveCanvas> scoped_save_canvas_; Can this section just be: SkAutoCanvasRestore acr(current_canvas_, draw_region); SetClipRect(scissor_rect_); ?
https://codereview.chromium.org/2867913002/diff/40001/cc/output/software_rend... File cc/output/software_renderer.cc (right): https://codereview.chromium.org/2867913002/diff/40001/cc/output/software_rend... cc/output/software_renderer.cc:157: current_canvas_->resetMatrix(); // SetClipRect is assumed to be applied temporarily, on an otherwise-unclipped canvas. DCHECK_EQ(current_canvas_->getDeviceClipBounds().width(), current_canvas_->imageInfo().width()); DCHECK_EQ(current_canvas_->getDeviceClipBounds().height(), current_canvas_->imageInfo().height()); ? https://codereview.chromium.org/2867913002/diff/40001/cc/output/software_rend... cc/output/software_renderer.cc:158: current_canvas_->clipRect(gfx::RectToSkRect(rect), SkClipOp::kIntersect); nit: you can omit the clipOp - it defaults to intersect.
The CQ bit was checked by ericrk@chromium.org to run a CQ dry run
Thanks for the feedback. Updated. https://codereview.chromium.org/2867913002/diff/40001/cc/output/software_rend... File cc/output/software_renderer.cc (right): https://codereview.chromium.org/2867913002/diff/40001/cc/output/software_rend... cc/output/software_renderer.cc:54: class ScopedSaveCanvas { On 2017/05/09 01:02:11, enne wrote: > I think this is SkAutoCanvasRestore. You can even pass a doSave to it if you > don't want to make it optional. Yay! I couldn't find this earlier. https://codereview.chromium.org/2867913002/diff/40001/cc/output/software_rend... cc/output/software_renderer.cc:157: current_canvas_->resetMatrix(); On 2017/05/09 13:24:03, f(malita) wrote: > // SetClipRect is assumed to be applied temporarily, on an otherwise-unclipped > canvas. > DCHECK_EQ(current_canvas_->getDeviceClipBounds().width(), > current_canvas_->imageInfo().width()); > DCHECK_EQ(current_canvas_->getDeviceClipBounds().height(), > current_canvas_->imageInfo().height()); > > ? Good suggestion. done. https://codereview.chromium.org/2867913002/diff/40001/cc/output/software_rend... cc/output/software_renderer.cc:158: current_canvas_->clipRect(gfx::RectToSkRect(rect), SkClipOp::kIntersect); On 2017/05/09 13:24:03, f(malita) wrote: > nit: you can omit the clipOp - it defaults to intersect. Done. https://codereview.chromium.org/2867913002/diff/40001/cc/output/software_rend... cc/output/software_renderer.cc:217: base::Optional<ScopedSaveCanvas> scoped_save_canvas_; On 2017/05/09 11:50:46, reed1 wrote: > Can this section just be: > > SkAutoCanvasRestore acr(current_canvas_, draw_region); > SetClipRect(scissor_rect_); > > ? Refactored - I think we still only want to save/clip if we have a scissor rect (not always).
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
very clean looking. lgtm
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 ericrk@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by reed@google.com 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.chromium.or...
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 ericrk@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494521650963640, "parent_rev": "d6449a52e98ecf9dcc888c0868935d9944375e7a", "commit_rev": "22e6f3e097117be4c680285f58c25c72e6d643b0"}
Message was sent while issue was closed.
Description was changed from ========== Avoid using SkClipOp::kReplace_deprecated in software renderer Currently, the software renderer always uses kReplace_deprecated when applying clips. This is necessary, as we never save/restore around clips, never cleaning up the previous clips we've used. This change causes software renderer to avoid the replace behavior by saving/restoring around clipping changes in DrawRenderPassQuad. With this change, any clipped DrawRenderPassQuad will add a save/restore. Additionally, calling EnsureScissorTestEnabled or SetScissorTestRect has no immediate impact, instead it just queues up the clip for the next DrawRenderPassQuad. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Avoid using SkClipOp::kReplace_deprecated in software renderer Currently, the software renderer always uses kReplace_deprecated when applying clips. This is necessary, as we never save/restore around clips, never cleaning up the previous clips we've used. This change causes software renderer to avoid the replace behavior by saving/restoring around clipping changes in DrawRenderPassQuad. With this change, any clipped DrawRenderPassQuad will add a save/restore. Additionally, calling EnsureScissorTestEnabled or SetScissorTestRect has no immediate impact, instead it just queues up the clip for the next DrawRenderPassQuad. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2867913002 Cr-Commit-Position: refs/heads/master@{#470981} Committed: https://chromium.googlesource.com/chromium/src/+/22e6f3e097117be4c680285f58c2... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/22e6f3e097117be4c680285f58c2... |