Use an up-to-date canvas size when building canvas filters

Use an up-to-date canvas size when building canvas filters

When canvas filters specify a filter primitive subregion
(e.g. by specifying a width or height), filter building
depends on having an up to date canvas size; otherwise,
the crop rect computed for the filter is incorrect.

This CL directly passes the canvas size to filter building
code, rather than having the filter builder use a size
from the layout tree, since the latter might be stale.

BUG=561045
Committed: https://crrev.com/d69e12d35e3b986fe0ba40e04368f85cda57b99c
Cr-Commit-Position: refs/heads/master@{#370047}

[ajuma, 2016-01-14 23:06:34]

Description was changed from

==========
WIP: Make sure layout is up to date before building canvas filters

BUG=
==========

to

==========
Make sure layout is up to date before building canvas filters

When canvas filters specify a filter primitive subregion
(e.g. by specifying a width or height), filter building
depends on having an up to date physical bounding box
for the canvas' layoutObject.

BUG=561045
==========

[ajuma, 2016-01-14 23:07:12]

Description was changed from

==========
Make sure layout is up to date before building canvas filters

When canvas filters specify a filter primitive subregion
(e.g. by specifying a width or height), filter building
depends on having an up to date physical bounding box
for the canvas' layoutObject.

BUG=561045
==========

to

==========
Make sure layout is up to date before building canvas filters

When canvas filters specify a filter primitive subregion
(e.g. by specifying a width or height), filter building
depends on having an up to date physical bounding box
for the canvas' LayoutObject; otherwise, the crop rect
computed for the filter is incorrect.

BUG=561045
==========

[ajuma, 2016-01-15 18:49:58]

ajuma@chromium.org changed reviewers:
+ junov@chromium.org

[Justin Novosad, 2016-01-15 21:16:28]

https://codereview.chromium.org/1583103007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1583103007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:884:
canvas()->document().updateLayoutIgnorePendingStylesheets();
This is a big hammer that may affect performance if we call it when not
necessary. You can make the layout conditional on the canvas's style being
dirty. Basically you want to avoid recomputing style when the style recalc would
only affect objects that are not in the canvas's hierarchy. Use this instead:
canvas()->document().updateLayoutTreeForNodeIfNeeded(canvas());

[ajuma, 2016-01-18 14:34:16]

https://codereview.chromium.org/1583103007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1583103007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:884:
canvas()->document().updateLayoutIgnorePendingStylesheets();
On 2016/01/15 21:16:28, Justin Novosad wrote:
> This is a big hammer that may affect performance if we call it when not
> necessary. You can make the layout conditional on the canvas's style being
> dirty. Basically you want to avoid recomputing style when the style recalc
would
> only affect objects that are not in the canvas's hierarchy. Use this instead:
> canvas()->document().updateLayoutTreeForNodeIfNeeded(canvas());

Turns out that updateLayoutTreeForNodeIfNeeded isn't sufficient (at least not
for the test case); document()->needsLayoutTreeUpdate() is false, so
updateLayoutTreeForNodeIfNeeded() earlies-out without doing any useful work. But
updateLayoutIgnorePendingStylesheets works because the FrameView needs layout
(since it hasn't done any layout yet -- FrameView::didFirstLayout is false at
this point), and the call to FrameView::layout leads to
FrameView::layoutFromRootObject which does layout on the whole tree.

Maybe a better approach is to pass the canvas rect directly to the filter
builder rather than having the builder retrieve the rect from the layout tree.
Let me see if that works.

[Justin Novosad, 2016-01-18 16:10:49]

On 2016/01/18 14:34:16, ajuma wrote:
>
https://codereview.chromium.org/1583103007/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
> (right):
> 
>
https://codereview.chromium.org/1583103007/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:884:
> canvas()->document().updateLayoutIgnorePendingStylesheets();
> On 2016/01/15 21:16:28, Justin Novosad wrote:
> > This is a big hammer that may affect performance if we call it when not
> > necessary. You can make the layout conditional on the canvas's style being
> > dirty. Basically you want to avoid recomputing style when the style recalc
> would
> > only affect objects that are not in the canvas's hierarchy. Use this
instead:
> > canvas()->document().updateLayoutTreeForNodeIfNeeded(canvas());
> 
> Turns out that updateLayoutTreeForNodeIfNeeded isn't sufficient (at least not
> for the test case); document()->needsLayoutTreeUpdate() is false, so
> updateLayoutTreeForNodeIfNeeded() earlies-out without doing any useful work.
But
> updateLayoutIgnorePendingStylesheets works because the FrameView needs layout
> (since it hasn't done any layout yet -- FrameView::didFirstLayout is false at
> this point), and the call to FrameView::layout leads to
> FrameView::layoutFromRootObject which does layout on the whole tree.
> 
> Maybe a better approach is to pass the canvas rect directly to the filter
> builder rather than having the builder retrieve the rect from the layout tree.
> Let me see if that works.

Or maybe there is a more complex set of conditions we can use to trigger layout?

No matter how you do it, the bar to meet IMHO is that following use case should
not compute layout more than once per frame: A rAF loop touches the style of an
element that is unrelated to the canvas, then draws to the canvas with a filter,
then touches the style of another element. If you have a solution that would do
layout twice for the first frame, but just once after that, I think that would
be acceptable too.

I just had an idea (extra credit): it might be interesting to have a layout
counter in blink::Document, and an API on window.internals to peek that counter.
 I can think of a few benchmarks I would retire in favor of layout tests if we
had something like that.

[ajuma, 2016-01-18 18:34:45]

Description was changed from

==========
Make sure layout is up to date before building canvas filters

When canvas filters specify a filter primitive subregion
(e.g. by specifying a width or height), filter building
depends on having an up to date physical bounding box
for the canvas' LayoutObject; otherwise, the crop rect
computed for the filter is incorrect.

BUG=561045
==========

to

==========
Use an up-to-date canvas size when building canvas filters

When canvas filters specify a filter primitive subregion
(e.g. by specifying a width or height), filter building
depends on having an up to date canvas size; otherwise,
the crop rect computed for the filter is incorrect.

BUG=561045
==========

[ajuma, 2016-01-18 18:45:29]

On 2016/01/18 16:10:49, Justin Novosad wrote:
> On 2016/01/18 14:34:16, ajuma wrote:
> >
>
https://codereview.chromium.org/1583103007/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/1583103007/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:884:
> > canvas()->document().updateLayoutIgnorePendingStylesheets();
> > On 2016/01/15 21:16:28, Justin Novosad wrote:
> > > This is a big hammer that may affect performance if we call it when not
> > > necessary. You can make the layout conditional on the canvas's style being
> > > dirty. Basically you want to avoid recomputing style when the style recalc
> > would
> > > only affect objects that are not in the canvas's hierarchy. Use this
> instead:
> > > canvas()->document().updateLayoutTreeForNodeIfNeeded(canvas());
> > 
> > Turns out that updateLayoutTreeForNodeIfNeeded isn't sufficient (at least
not
> > for the test case); document()->needsLayoutTreeUpdate() is false, so
> > updateLayoutTreeForNodeIfNeeded() earlies-out without doing any useful work.
> But
> > updateLayoutIgnorePendingStylesheets works because the FrameView needs
layout
> > (since it hasn't done any layout yet -- FrameView::didFirstLayout is false
at
> > this point), and the call to FrameView::layout leads to
> > FrameView::layoutFromRootObject which does layout on the whole tree.
> > 
> > Maybe a better approach is to pass the canvas rect directly to the filter
> > builder rather than having the builder retrieve the rect from the layout
tree.
> > Let me see if that works.
> 
> Or maybe there is a more complex set of conditions we can use to trigger
layout?
> 
> No matter how you do it, the bar to meet IMHO is that following use case
should
> not compute layout more than once per frame: A rAF loop touches the style of
an
> element that is unrelated to the canvas, then draws to the canvas with a
filter,
> then touches the style of another element. If you have a solution that would
do
> layout twice for the first frame, but just once after that, I think that would
> be acceptable too.
> 
> I just had an idea (extra credit): it might be interesting to have a layout
> counter in blink::Document, and an API on window.internals to peek that
counter.
>  I can think of a few benchmarks I would retire in favor of layout tests if we
> had something like that.

I've updated this to pass the canvas size directly to the filter builder.
Looking around at other code, it seems that forcing layout to happen before the
FrameView's first layout is not a good idea (e.g. see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

[Justin Novosad, 2016-01-18 18:54:50]

It's more complexity, but it looks like the right thing to do.  Besides, the
canvas intrinsic size is what you really want to use here.
lgtm

[ajuma, 2016-01-18 18:59:26]

Description was changed from

==========
Use an up-to-date canvas size when building canvas filters

When canvas filters specify a filter primitive subregion
(e.g. by specifying a width or height), filter building
depends on having an up to date canvas size; otherwise,
the crop rect computed for the filter is incorrect.

BUG=561045
==========

to

==========
Use an up-to-date canvas size when building canvas filters

When canvas filters specify a filter primitive subregion
(e.g. by specifying a width or height), filter building
depends on having an up to date canvas size; otherwise,
the crop rect computed for the filter is incorrect.

This CL directly passes the canvas size to filter building
code, rather than having the filter builder use a size
from the layout tree, since the latter might be stale.

BUG=561045
==========

[ajuma, 2016-01-18 19:02:11]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2016-01-18 19:02:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1583103007/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1583103007/60001

[Justin Novosad, 2016-01-18 19:37:38]

Just had a thought. Could you follow-up with additional layout tests that do
essentially the same thing as this one, but with hidpi and CSS scaling?

[ajuma, 2016-01-18 19:38:28]

On 2016/01/18 19:37:38, Justin Novosad wrote:
> Just had a thought. Could you follow-up with additional layout tests that do
> essentially the same thing as this one, but with hidpi and CSS scaling?

Sure, good idea, will do.

[commit-bot: I haz the power, 2016-01-18 20:39:09]

Description was changed from

==========
Use an up-to-date canvas size when building canvas filters

When canvas filters specify a filter primitive subregion
(e.g. by specifying a width or height), filter building
depends on having an up to date canvas size; otherwise,
the crop rect computed for the filter is incorrect.

This CL directly passes the canvas size to filter building
code, rather than having the filter builder use a size
from the layout tree, since the latter might be stale.

BUG=561045
==========

to

==========
Use an up-to-date canvas size when building canvas filters

When canvas filters specify a filter primitive subregion
(e.g. by specifying a width or height), filter building
depends on having an up to date canvas size; otherwise,
the crop rect computed for the filter is incorrect.

This CL directly passes the canvas size to filter building
code, rather than having the filter builder use a size
from the layout tree, since the latter might be stale.

BUG=561045
==========

[commit-bot: I haz the power, 2016-01-18 20:39:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-18 20:39:31]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-01-18 20:40:08]

Description was changed from

==========
Use an up-to-date canvas size when building canvas filters

When canvas filters specify a filter primitive subregion
(e.g. by specifying a width or height), filter building
depends on having an up to date canvas size; otherwise,
the crop rect computed for the filter is incorrect.

This CL directly passes the canvas size to filter building
code, rather than having the filter builder use a size
from the layout tree, since the latter might be stale.

BUG=561045
==========

to

==========
Use an up-to-date canvas size when building canvas filters

When canvas filters specify a filter primitive subregion
(e.g. by specifying a width or height), filter building
depends on having an up to date canvas size; otherwise,
the crop rect computed for the filter is incorrect.

This CL directly passes the canvas size to filter building
code, rather than having the filter builder use a size
from the layout tree, since the latter might be stale.

BUG=561045

Committed: https://crrev.com/d69e12d35e3b986fe0ba40e04368f85cda57b99c
Cr-Commit-Position: refs/heads/master@{#370047}
==========

[commit-bot: I haz the power, 2016-01-18 20:40:09]

Patchset 4 (id:??) landed as
https://crrev.com/d69e12d35e3b986fe0ba40e04368f85cda57b99c
Cr-Commit-Position: refs/heads/master@{#370047}

[Stephen White, 2016-01-21 14:49:49]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2016-01-21 14:49:50]

https://codereview.chromium.org/1583103007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
(right):

https://codereview.chromium.org/1583103007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:104: if
(referenceBoxSize) {
Post-commit drive-by: should we just do this check outside the element layout
checks? I'm guessing it's valid even for canvas elements which aren't in the DOM
and won't get layout.

[Justin Novosad, 2016-01-21 15:35:08]

On 2016/01/21 14:49:50, Stephen White wrote:
>
https://codereview.chromium.org/1583103007/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
> (right):
> 
>
https://codereview.chromium.org/1583103007/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:104: if
> (referenceBoxSize) {
> Post-commit drive-by: should we just do this check outside the element layout
> checks? I'm guessing it's valid even for canvas elements which aren't in the
DOM
> and won't get layout.

That is a really good point. Ali, we need to support the case where the canvas
is not attached to the DOM (document.createElement('canvas')...)

[ajuma, 2016-01-21 15:58:29]

On 2016/01/21 15:35:08, Justin Novosad wrote:
> On 2016/01/21 14:49:50, Stephen White wrote:
> >
>
https://codereview.chromium.org/1583103007/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/1583103007/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:104: if
> > (referenceBoxSize) {
> > Post-commit drive-by: should we just do this check outside the element
layout
> > checks? I'm guessing it's valid even for canvas elements which aren't in the
> DOM
> > and won't get layout.
> 
> That is a really good point. Ali, we need to support the case where the canvas
> is not attached to the DOM (document.createElement('canvas')...)

Thanks for spotting this! Fix posted here:
https://codereview.chromium.org/1613193002/