Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2018983003:
cc: Use FilterOperations::MapRect in RenderSurfaceImpl::DrawableContentRect. (Closed)
Descriptioncc: Use FilterOperations::MapRect in RenderSurfaceImpl::DrawableContentRect.
GetOutsets provided imprecise -- or possibly incorrect -- outsets for certain
reference filters, such as <feOffset>. Instead RenderSurfaceImpl should
explicitly specify that it wants forward mapping here.
Also fix this to incorporate scale factors in the filter rect calculations
(e.g. due to a high DPI display), and add unit tests.
BUG=600821
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/1c44d5b5ff7b1c422b75f2d68e409583db0a71bc
Cr-Commit-Position: refs/heads/master@{#398126}
Patch Set 1 #
Total comments: 5
Patch Set 2 : high-dpi #
Total comments: 1
Patch Set 3 : adjust LayerTreeHostCommonTest.RenderSurfaceListForFilter #
Total comments: 1
Patch Set 4 : remove logging #
Total comments: 1
Messages
Total messages: 23 (7 generated)
Description was changed from ========== cc: Use FilterOperations::MapRect in RenderSurfaceImpl::DrawableContentRect. GetOutsets provided imprecise -- or possibly incorrect -- outsets for certain reference filters, such as <feOffset>. Instead RenderSurfaceImpl should explicitly specify that it wants forward mapping here. BUG=600821 ========== to ========== cc: Use FilterOperations::MapRect in RenderSurfaceImpl::DrawableContentRect. GetOutsets provided imprecise -- or possibly incorrect -- outsets for certain reference filters, such as <feOffset>. Instead RenderSurfaceImpl should explicitly specify that it wants forward mapping here. BUG=600821 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
jbroman@chromium.org changed reviewers: + senorblanco@chromium.org
On the continuing quest to kill GetOutsets. If this LGTY, I'll grab a cc owner to review.
https://codereview.chromium.org/2018983003/diff/1/cc/layers/render_surface_im... File cc/layers/render_surface_impl.cc (left): https://codereview.chromium.org/2018983003/diff/1/cc/layers/render_surface_im... cc/layers/render_surface_impl.cc:80: surface_content_rect.Inset(-left, -top, -right, -bottom); Output of the test before this patch: [ RUN ] LayerTreeHostCommonTest.DrawableContentRectForReferenceFilter ../../cc/trees/layer_tree_host_common_unittest.cc:1439: Failure Value of: child->render_surface()->DrawableContentRect() Actual: -50.000000,-50.000000 75.000000x75.000000 Expected: gfx::RectF(50, 50, 25, 25) Which is: 50.000000,50.000000 25.000000x25.000000 [ FAILED ] LayerTreeHostCommonTest.DrawableContentRectForReferenceFilter (7 ms) Note that not only was the previous rect large, but it didn't even enclose (50,50) 25x25, because GetOutsets was asking for the reverse mapping from Skia, even though (I believe) the forward mapping is desired here.
ping
LGTM
https://codereview.chromium.org/2018983003/diff/1/cc/layers/render_surface_im... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2018983003/diff/1/cc/layers/render_surface_im... cc/layers/render_surface_impl.cc:79: owning_layer_->filters().MapRect(surface_content_rect, SkMatrix::I()); Er, wait. Shouldn't we be using the filters_scale here for the matrix? (E.g., for hiDPI?)
https://codereview.chromium.org/2018983003/diff/1/cc/layers/render_surface_im... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2018983003/diff/1/cc/layers/render_surface_im... cc/layers/render_surface_impl.cc:79: owning_layer_->filters().MapRect(surface_content_rect, SkMatrix::I()); On 2016/06/01 at 17:17:38, Stephen White wrote: > Er, wait. Shouldn't we be using the filters_scale here for the matrix? > > (E.g., for hiDPI?) I don't think so; I'd expect that the scale of filter dimensions (blur radii, etc.) would be in the coordinate space of the surface_content_rect (and, of course, the existing outsets were not accounting for filters_scale). I'd expect the MathUtil::MapClippedRect(draw_transform(), ...) to deal with the appropriate transformations to put the rectangle in target space. But I admit to being a little out of my depth here.
https://codereview.chromium.org/2018983003/diff/1/cc/layers/render_surface_im... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2018983003/diff/1/cc/layers/render_surface_im... cc/layers/render_surface_impl.cc:79: owning_layer_->filters().MapRect(surface_content_rect, SkMatrix::I()); On 2016/06/01 19:38:00, jbroman wrote: > On 2016/06/01 at 17:17:38, Stephen White wrote: > > Er, wait. Shouldn't we be using the filters_scale here for the matrix? > > > > (E.g., for hiDPI?) > > I don't think so; I'd expect that the scale of filter dimensions (blur radii, > etc.) would be in the coordinate space of the surface_content_rect (and, of > course, the existing outsets were not accounting for filters_scale). > > I'd expect the MathUtil::MapClippedRect(draw_transform(), ...) to deal with the > appropriate transformations to put the rectangle in target space. > > But I admit to being a little out of my depth here. It's not the rectangle that's the problem; it's the filter parameters. If the rectangle is in device space (e.g., has hiDPI applied), then the filter parameters have to be transformed to device space via a scale in the matrix to apply hiDPI to the filter parameters as well. It's entirely possible that this path has bugs wrt hiDPI; IIRC, it's only relevant when nested filters are applied (e.g., filters on both parent and child layers). Unless I'm misremembering when this is used... Anyway I'm fine if you want to land this and investigate the HiDPI case separately, since as you say, the old code path wasn't applying the scale either.
Description was changed from ========== cc: Use FilterOperations::MapRect in RenderSurfaceImpl::DrawableContentRect. GetOutsets provided imprecise -- or possibly incorrect -- outsets for certain reference filters, such as <feOffset>. Instead RenderSurfaceImpl should explicitly specify that it wants forward mapping here. BUG=600821 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Use FilterOperations::MapRect in RenderSurfaceImpl::DrawableContentRect. GetOutsets provided imprecise -- or possibly incorrect -- outsets for certain reference filters, such as <feOffset>. Instead RenderSurfaceImpl should explicitly specify that it wants forward mapping here. Also fix this to incorporate scale factors in the filter rect calculations (e.g. due to a high DPI display), and add unit tests. BUG=600821 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
jbroman@chromium.org changed reviewers: + ajuma@chromium.org
Ah, you're right, that rect does already have the DSF applied. Adjusted, with a unit test. +ajuma for cc/OWNERS https://codereview.chromium.org/2018983003/diff/1/cc/layers/render_surface_im... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2018983003/diff/1/cc/layers/render_surface_im... cc/layers/render_surface_impl.cc:79: owning_layer_->filters().MapRect(surface_content_rect, SkMatrix::I()); On 2016/06/01 at 22:13:12, Stephen White wrote: > On 2016/06/01 19:38:00, jbroman wrote: > > On 2016/06/01 at 17:17:38, Stephen White wrote: > > > Er, wait. Shouldn't we be using the filters_scale here for the matrix? > > > > > > (E.g., for hiDPI?) > > > > I don't think so; I'd expect that the scale of filter dimensions (blur radii, > > etc.) would be in the coordinate space of the surface_content_rect (and, of > > course, the existing outsets were not accounting for filters_scale). > > > > I'd expect the MathUtil::MapClippedRect(draw_transform(), ...) to deal with the > > appropriate transformations to put the rectangle in target space. > > > > But I admit to being a little out of my depth here. > > It's not the rectangle that's the problem; it's the filter parameters. If the rectangle is in device space (e.g., has hiDPI > applied), then the filter parameters have to be transformed to device > space via a scale in the matrix to apply hiDPI to the filter > parameters as well. Yes, but the rectangle isn't in device space yet at this point; I think it's the MapClippedRect below that sends it to device space. > It's entirely possible that this path has bugs wrt hiDPI; IIRC, it's > only relevant when nested filters are applied (e.g., filters on > both parent and child layers). Unless I'm misremembering when this > is used... > > Anyway I'm fine if you want to land this and investigate the HiDPI > case separately, since as you say, the old code path wasn't applying > the scale either. https://codereview.chromium.org/2018983003/diff/20001/cc/layers/render_surfac... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2018983003/diff/20001/cc/layers/render_surfac... cc/layers/render_surface_impl.cc:79: owning_layer_->DrawTransform(); using the owning layer's draw transform is how RenderSurfaceImpl::AppendQuads computes filter scale
On 2016/06/06 at 17:17:00, jbroman wrote: > Yes, but the rectangle isn't in device space yet at this point; I think it's the MapClippedRect below that sends it to device space. ^ Ignore this comment; I wrote it before I realized that was not the case, and accidentally left it in drafts.
LGTM https://codereview.chromium.org/2018983003/diff/40001/cc/layers/render_surfac... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2018983003/diff/40001/cc/layers/render_surfac... cc/layers/render_surface_impl.cc:78: LOG(ERROR) << "content_rect " << content_rect().ToString(); I'm assuming this is safe to leave in, perf-wise. I have no idea how heavyweight logging is in Chrome.
On 2016/06/06 at 17:27:03, senorblanco wrote: > LGTM > > https://codereview.chromium.org/2018983003/diff/40001/cc/layers/render_surfac... > File cc/layers/render_surface_impl.cc (right): > > https://codereview.chromium.org/2018983003/diff/40001/cc/layers/render_surfac... > cc/layers/render_surface_impl.cc:78: LOG(ERROR) << "content_rect " << content_rect().ToString(); > I'm assuming this is safe to leave in, perf-wise. I have no idea how heavyweight logging is in Chrome. Dammit, I need to stop leaving logs in my CLs. That shouldn't be there, regardless (ERROR would be a stupidly high log level). Will remove.
lgtm https://codereview.chromium.org/2018983003/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/2018983003/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:1414: // (-60,-60 220x220). Thanks for adding this comment (and for the similar comments in your new tests).
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from senorblanco@chromium.org Link to the patchset: https://codereview.chromium.org/2018983003/#ps60001 (title: "remove logging")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018983003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== cc: Use FilterOperations::MapRect in RenderSurfaceImpl::DrawableContentRect. GetOutsets provided imprecise -- or possibly incorrect -- outsets for certain reference filters, such as <feOffset>. Instead RenderSurfaceImpl should explicitly specify that it wants forward mapping here. Also fix this to incorporate scale factors in the filter rect calculations (e.g. due to a high DPI display), and add unit tests. BUG=600821 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Use FilterOperations::MapRect in RenderSurfaceImpl::DrawableContentRect. GetOutsets provided imprecise -- or possibly incorrect -- outsets for certain reference filters, such as <feOffset>. Instead RenderSurfaceImpl should explicitly specify that it wants forward mapping here. Also fix this to incorporate scale factors in the filter rect calculations (e.g. due to a high DPI display), and add unit tests. BUG=600821 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/1c44d5b5ff7b1c422b75f2d68e409583db0a71bc Cr-Commit-Position: refs/heads/master@{#398126} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1c44d5b5ff7b1c422b75f2d68e409583db0a71bc Cr-Commit-Position: refs/heads/master@{#398126} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
