Rounded background image fast path

Rounded background image fast path

The current box background paint logic relies on clipping for drawing
rounded-rect background images.  This can yield some pathological
rasterization slowdowns on the GPU backend.

In the case where a single image tile covers the whole box (no fancy
tiling involved/required), a more efficient way to draw is to use a
Skia shader and just fill the rounded rect shape (skipping the
unnecessary clip).

This CL implements such an optimization:

1) Introduce a new Image virtual (applyShader) which configures the
  paint for filling arbitrary shapes with the image content.

2) Add a new GraphicsContext method for drawing rounded-rect images
  (drawRRectImage) using shaders build via #1.

3) Expand BoxPainter's fillFastBottomLayer() fast path to detect
  when tiling is not required and use #2 for painting  rounded-rect
  background images.

This avoids clipping for simple rounded-rect background
images/gradients, and improves GPU rasterization times significantly
in some cases.

In local testing, Animometer/CSS-bouncing-gradient-circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 14X score improvement with Ganesh (27 -> 380).

BUG=603966
R=fs@opera.com,chrishtr@chromium.org,schenney@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/4c1400381d16b6dcd8921a8f19a2ae9af4aac4cf
Cr-Commit-Position: refs/heads/master@{#395492}

[f(malita), 2016-05-05 16:39:28]

Description was changed from

==========
Rounded-rect image fast path

Experimental/WIP

BUG=603966
==========

to

==========
Rounded-rect image fast path

Experimental/WIP

BUG=603966
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[f(malita), 2016-05-19 13:14:46]

Description was changed from

==========
Rounded-rect image fast path

Experimental/WIP

BUG=603966
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Rounded-rect image fast path

[Experimental/WIP]

Expand the color-only BoxPainter::paintFillLayer() fast path to handle
image backgrounds also.

Add a rounded-rect variant of GraphicsContext::drawImage(), based on
native Skia shaders.

This avoids clipping for simple rounded-rect backgrounds, and improves
GPU rasterization times significantly in some cases.

In local testing, Animometer/CSS bouncing gradient circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 12X score improvement with Ganesh (27 -> 380).


BUG=603966
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[f(malita), 2016-05-19 20:06:08]

Description was changed from

==========
Rounded-rect image fast path

[Experimental/WIP]

Expand the color-only BoxPainter::paintFillLayer() fast path to handle
image backgrounds also.

Add a rounded-rect variant of GraphicsContext::drawImage(), based on
native Skia shaders.

This avoids clipping for simple rounded-rect backgrounds, and improves
GPU rasterization times significantly in some cases.

In local testing, Animometer/CSS bouncing gradient circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 12X score improvement with Ganesh (27 -> 380).


BUG=603966
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Rounded border background image fast path

[Experimental/WIP]

Expand the color-only BoxPainter::paintFastBottomLayer() to handle
image backgrounds also.

Add a rounded-rect variant of GraphicsContext::drawImage(), based on
native Skia shaders.

This avoids clipping for simple rounded-rect backgrounds, and improves
GPU rasterization times significantly in some cases.

In local testing, Animometer/CSS bouncing gradient circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 12X score improvement with Ganesh (27 -> 380).


BUG=603966
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[f(malita), 2016-05-19 20:19:48]

Description was changed from

==========
Rounded border background image fast path

[Experimental/WIP]

Expand the color-only BoxPainter::paintFastBottomLayer() to handle
image backgrounds also.

Add a rounded-rect variant of GraphicsContext::drawImage(), based on
native Skia shaders.

This avoids clipping for simple rounded-rect backgrounds, and improves
GPU rasterization times significantly in some cases.

In local testing, Animometer/CSS bouncing gradient circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 12X score improvement with Ganesh (27 -> 380).


BUG=603966
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Rounded background image fast path

[Experimental/WIP]

Expand the color-only BoxPainter::paintFastBottomLayer() to handle
image backgrounds also.

Add a rounded-rect variant of GraphicsContext::drawImage(), based on
native Skia shaders.

This avoids clipping for simple rounded-rect backgrounds, and improves
GPU rasterization times significantly in some cases.

In local testing, Animometer/CSS bouncing gradient circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 12X score improvement with Ganesh (27 -> 380).


BUG=603966
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[f(malita), 2016-05-20 00:17:59]

Description was changed from

==========
Rounded background image fast path

[Experimental/WIP]

Expand the color-only BoxPainter::paintFastBottomLayer() to handle
image backgrounds also.

Add a rounded-rect variant of GraphicsContext::drawImage(), based on
native Skia shaders.

This avoids clipping for simple rounded-rect backgrounds, and improves
GPU rasterization times significantly in some cases.

In local testing, Animometer/CSS bouncing gradient circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 12X score improvement with Ganesh (27 -> 380).


BUG=603966
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Rounded background image fast path

The current box background paint logic relies on clipping for drawing
rounded-rect background images.  This can yield some pathological
rasterization slowdowns on the GPU backend.

In the case when a single image tile covers the whole box (no fancy
tiling involved/required), a more efficient way to draw is to use a
Skia shader and just fill the rounded rect shape (skipping the
unnecessary clip).

This CL implements such an optimization:

1) Introduce a new Image virtual (applyShader) which configures the
  paint for filling arbitrary shapes with the image content.

2) Add a new GraphicsContext method for drawing rounded-rect images
  (drawRRectImage) using shaders build via #1.

3) Expand BoxPainter's fillFastBottomLayer() fast path to detect
  when tiling is not required and use #2 for painting  rounded-rect
  background images.


This avoids clipping for simple rounded-rect background
images/gradients, and improves GPU rasterization times significantly
in some cases.

In local testing, Animometer/CSS-bouncing-gradient-circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 14X score improvement with Ganesh (27 -> 380).


BUG=603966
R=fs@opera.com,chrishtr@chromium.org,schenney@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[f(malita), 2016-05-20 00:18:00]

fmalita@chromium.org changed reviewers:
+ chrishtr@chromium.org, fs@opera.com, schenney@chromium.org

[f(malita), 2016-05-20 00:19:14]

Description was changed from

==========
Rounded background image fast path

The current box background paint logic relies on clipping for drawing
rounded-rect background images.  This can yield some pathological
rasterization slowdowns on the GPU backend.

In the case when a single image tile covers the whole box (no fancy
tiling involved/required), a more efficient way to draw is to use a
Skia shader and just fill the rounded rect shape (skipping the
unnecessary clip).

This CL implements such an optimization:

1) Introduce a new Image virtual (applyShader) which configures the
  paint for filling arbitrary shapes with the image content.

2) Add a new GraphicsContext method for drawing rounded-rect images
  (drawRRectImage) using shaders build via #1.

3) Expand BoxPainter's fillFastBottomLayer() fast path to detect
  when tiling is not required and use #2 for painting  rounded-rect
  background images.


This avoids clipping for simple rounded-rect background
images/gradients, and improves GPU rasterization times significantly
in some cases.

In local testing, Animometer/CSS-bouncing-gradient-circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 14X score improvement with Ganesh (27 -> 380).


BUG=603966
R=fs@opera.com,chrishtr@chromium.org,schenney@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Rounded background image fast path

The current box background paint logic relies on clipping for drawing
rounded-rect background images.  This can yield some pathological
rasterization slowdowns on the GPU backend.

In the case where a single image tile covers the whole box (no fancy
tiling involved/required), a more efficient way to draw is to use a
Skia shader and just fill the rounded rect shape (skipping the
unnecessary clip).

This CL implements such an optimization:

1) Introduce a new Image virtual (applyShader) which configures the
  paint for filling arbitrary shapes with the image content.

2) Add a new GraphicsContext method for drawing rounded-rect images
  (drawRRectImage) using shaders build via #1.

3) Expand BoxPainter's fillFastBottomLayer() fast path to detect
  when tiling is not required and use #2 for painting  rounded-rect
  background images.


This avoids clipping for simple rounded-rect background
images/gradients, and improves GPU rasterization times significantly
in some cases.

In local testing, Animometer/CSS-bouncing-gradient-circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 14X score improvement with Ganesh (27 -> 380).


BUG=603966
R=fs@opera.com,chrishtr@chromium.org,schenney@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[fs, 2016-05-20 12:47:35]

That's some persuasive numbers you've got there =)

The BoxPainter code isn't particularly easy to read in diff-form, but it looked
ok to me. I'll hold for more eyes.

https://codereview.chromium.org/1949253004/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1949253004/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:394: // When the layer has
an image, figure whether it is covered by a single tile.
Nit: figure out

https://codereview.chromium.org/1949253004/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GradientGeneratedImage.cpp
(right):

https://codereview.chromium.org/1949253004/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GradientGeneratedImage.cpp:67:
m_gradient->setGradientSpaceTransform(transform);
Could this end up affecting the result of a draw() if we have a sequence
"applyShader(...)", "draw()" on the same GradientGeneratedImage? I.e
state-pollution.

[Stephen Chennney, 2016-05-20 13:57:20]

I'm off today but I want to understand this. Please hold off on committing until
I can look Monday.

[f(malita), 2016-05-20 13:57:57]

https://codereview.chromium.org/1949253004/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1949253004/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:394: // When the layer has
an image, figure whether it is covered by a single tile.
On 2016/05/20 12:47:35, fs wrote:
> Nit: figure out

Done.

https://codereview.chromium.org/1949253004/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GradientGeneratedImage.cpp
(right):

https://codereview.chromium.org/1949253004/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GradientGeneratedImage.cpp:67:
m_gradient->setGradientSpaceTransform(transform);
On 2016/05/20 12:47:35, fs wrote:
> Could this end up affecting the result of a draw() if we have a sequence
> "applyShader(...)", "draw()" on the same GradientGeneratedImage? I.e
> state-pollution.

Hmm, good question.  I thought we always update the gradient transform when
painting (SVG at least seems to do that), but yeah - the draw() path above
pushes the transform to the canvas, not to the gradient.  I guess for CSS
gradients we never touch the transform?

Updated to preserve the original gradient transform (always identity for
GradientGeneratedImage?).

(follow-up idea: I think in the long run it would be nicer to remove the
transform from the gradient/pattern state altogether and instead pass an
optional matrix to applyToPaint();  then the matrix is part of the key for the
cached shader, but no longer exposed/manipulated independently on the gradient
object)

[f(malita), 2016-05-20 14:01:04]

On 2016/05/20 13:57:20, Stephen Chennney wrote:
> I'm off today but I want to understand this. Please hold off on committing
until
> I can look Monday.

Ack.

[fs, 2016-05-20 14:18:57]

https://codereview.chromium.org/1949253004/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GradientGeneratedImage.cpp
(right):

https://codereview.chromium.org/1949253004/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GradientGeneratedImage.cpp:67:
m_gradient->setGradientSpaceTransform(transform);
On 2016/05/20 at 13:57:57, f(malita) wrote:
> On 2016/05/20 12:47:35, fs wrote:
> > Could this end up affecting the result of a draw() if we have a sequence
> > "applyShader(...)", "draw()" on the same GradientGeneratedImage? I.e
> > state-pollution.
> 
> Hmm, good question.  I thought we always update the gradient transform when
painting (SVG at least seems to do that), but yeah - the draw() path above
pushes the transform to the canvas, not to the gradient.  I guess for CSS
gradients we never touch the transform?

The transform is never touch explicitly (and this is where I usually go
"oh..."), but createShader does mutate the gradient-space-transform to achieve
elliptical gradients. (Should be easy to fix that oddity though...)
 
> Updated to preserve the original gradient transform (always identity for
GradientGeneratedImage?).
> 
> (follow-up idea: I think in the long run it would be nicer to remove the
transform from the gradient/pattern state altogether and instead pass an
optional matrix to applyToPaint();  then the matrix is part of the key for the
cached shader, but no longer exposed/manipulated independently on the gradient
object)

Yes, that'd be nice (I think we'd like that for SVG gradients too.)

[chrishtr, 2016-05-20 15:44:13]

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:401: if
(!geometry->destRect().isEmpty()) {
It's not so good that this code duplicates information that is
implicitly already in Image. It seems there should be a method
there called something like computeTiles, and another one called
drawWithTiles that uses the results of computeTiles. And it
should be unittested.

WDYT?

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:820: if
(!dest.isRounded() || !dest.isRenderable()) {
should isRenderable() be considered illegal?

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsContext.h:165: void
drawImageRRect(Image*, const FloatRoundedRect& dest, const FloatRect* srcRect =
nullptr,
Make the srcRect parameter required.

[f(malita), 2016-05-20 19:17:12]

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:401: if
(!geometry->destRect().isEmpty()) {
On 2016/05/20 15:44:13, chrishtr wrote:
> It's not so good that this code duplicates information that is
> implicitly already in Image. It seems there should be a method
> there called something like computeTiles, and another one called
> drawWithTiles that uses the results of computeTiles. And it
> should be unittested.
> 
> WDYT?

Agreed.

The current API is convoluted and hard to follow, and my dirty little secret is
I'm planning to take a stab at ripping all the image drawing code out of Blink
in favor of a specialized DrawImageDisplayItem (essentially move all the image
draw/tiling logic to CC/display item impl).  So I'm not terribly excited about
spending too much time refactoring image tiling in Blink :)

Would it be OK to just share the existing code from Image via helpers for now? 
PTAL at PS#25

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:820: if
(!dest.isRounded() || !dest.isRenderable()) {
On 2016/05/20 15:44:13, chrishtr wrote:
> should isRenderable() be considered illegal?

Done.

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsContext.h:165: void
drawImageRRect(Image*, const FloatRoundedRect& dest, const FloatRect* srcRect =
nullptr,
On 2016/05/20 15:44:13, chrishtr wrote:
> Make the srcRect parameter required.

Done.

[chrishtr, 2016-05-20 19:52:35]

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:401: if
(!geometry->destRect().isEmpty()) {
On 2016/05/20 at 19:17:11, f(malita) wrote:
> On 2016/05/20 15:44:13, chrishtr wrote:
> > It's not so good that this code duplicates information that is
> > implicitly already in Image. It seems there should be a method
> > there called something like computeTiles, and another one called
> > drawWithTiles that uses the results of computeTiles. And it
> > should be unittested.
> > 
> > WDYT?
> 
> Agreed.
> 
> The current API is convoluted and hard to follow, and my dirty little secret
is I'm planning to take a stab at ripping all the image drawing code out of
Blink in favor of a specialized DrawImageDisplayItem (essentially move all the
image draw/tiling logic to CC/display item impl).  So I'm not terribly excited
about spending too much time refactoring image tiling in Blink :)

Sounds like a lot of work. Have you sketched it out? We should VC to talk about
it some before investing that much work.

> 
> Would it be OK to just share the existing code from Image via helpers for now?
 PTAL at PS#25

Ok.

https://codereview.chromium.org/1949253004/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/1949253004/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Image.h:158: static FloatRect
computeTileContaining(const FloatPoint&, const FloatSize& tileSize,
Also comment: input point and return value is in destination grid space.

https://codereview.chromium.org/1949253004/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Image.h:163: static FloatRect
computeSubsetForTile(const FloatRect& tile, const FloatRect& dest,
Also comment: tile rect is in destination grid space, and return value is in
the space of the image.

[f(malita), 2016-05-22 14:04:07]

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:401: if
(!geometry->destRect().isEmpty()) {
On 2016/05/20 19:52:35, chrishtr wrote:
> On 2016/05/20 at 19:17:11, f(malita) wrote:
> > On 2016/05/20 15:44:13, chrishtr wrote:
> > > It's not so good that this code duplicates information that is
> > > implicitly already in Image. It seems there should be a method
> > > there called something like computeTiles, and another one called
> > > drawWithTiles that uses the results of computeTiles. And it
> > > should be unittested.
> > > 
> > > WDYT?
> > 
> > Agreed.
> > 
> > The current API is convoluted and hard to follow, and my dirty little secret
> is I'm planning to take a stab at ripping all the image drawing code out of
> Blink in favor of a specialized DrawImageDisplayItem (essentially move all the
> image draw/tiling logic to CC/display item impl).  So I'm not terribly excited
> about spending too much time refactoring image tiling in Blink :)
> 
> Sounds like a lot of work. Have you sketched it out? We should VC to talk
about
> it some before investing that much work.

I do expect it to be quite a bit of work, but a) I think the end result will be
worth it
and b) I'm hoping once we have a basic skeleton in place we can transition
gradually
(no mega-CL to break the world).

We can certainly VC, on we can talk about it at BlinkOn (planning to put
together a rough
plan and prototype by then).

https://codereview.chromium.org/1949253004/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/1949253004/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Image.h:158: static FloatRect
computeTileContaining(const FloatPoint&, const FloatSize& tileSize,
On 2016/05/20 19:52:35, chrishtr wrote:
> Also comment: input point and return value is in destination grid space.

Done.

https://codereview.chromium.org/1949253004/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Image.h:163: static FloatRect
computeSubsetForTile(const FloatRect& tile, const FloatRect& dest,
On 2016/05/20 19:52:35, chrishtr wrote:
> Also comment: tile rect is in destination grid space, and return value is in
> the space of the image.

Done.

[chrishtr, 2016-05-23 19:59:22]

I assume there are already layout tests that exercise this? Which ones?

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1949253004/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:401: if
(!geometry->destRect().isEmpty()) {
On 2016/05/22 at 14:04:06, f(malita) wrote:
> On 2016/05/20 19:52:35, chrishtr wrote:
> > On 2016/05/20 at 19:17:11, f(malita) wrote:
> > > On 2016/05/20 15:44:13, chrishtr wrote:
> > > > It's not so good that this code duplicates information that is
> > > > implicitly already in Image. It seems there should be a method
> > > > there called something like computeTiles, and another one called
> > > > drawWithTiles that uses the results of computeTiles. And it
> > > > should be unittested.
> > > > 
> > > > WDYT?
> > > 
> > > Agreed.
> > > 
> > > The current API is convoluted and hard to follow, and my dirty little
secret
> > is I'm planning to take a stab at ripping all the image drawing code out of
> > Blink in favor of a specialized DrawImageDisplayItem (essentially move all
the
> > image draw/tiling logic to CC/display item impl).  So I'm not terribly
excited
> > about spending too much time refactoring image tiling in Blink :)
> > 
> > Sounds like a lot of work. Have you sketched it out? We should VC to talk
about
> > it some before investing that much work.
> 
> I do expect it to be quite a bit of work, but a) I think the end result will
be worth it
> and b) I'm hoping once we have a basic skeleton in place we can transition
gradually
> (no mega-CL to break the world).
> 
> We can certainly VC, on we can talk about it at BlinkOn (planning to put
together a rough
> plan and prototype by then).

Talking about it at BlinkOn sounds good.

https://codereview.chromium.org/1949253004/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1949253004/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:428: //
RoundedInnerRectClipper handles this case via discrete, corner-wise clipping.
What cases is this fallback necessary for?

[Stephen Chennney, 2016-05-23 20:21:15]

I'm fine with this. It's a bit harder to follow but more speed is good.

You may now have the information necessary to fix FillLayer::imageTilesLayer so
that it considers a large image that isn't tiled as sufficient to cover the
background. There's a TODO there.

Chris should indicate satisfaction.

[f(malita), 2016-05-23 21:12:23]

https://codereview.chromium.org/1949253004/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1949253004/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:428: //
RoundedInnerRectClipper handles this case via discrete, corner-wise clipping.
On 2016/05/23 19:59:22, chrishtr wrote:
> What cases is this fallback necessary for?

Degenerate rounded rects, where the radii sum is > side, presumably.

I know we run into non-renderable rrects when computing the inner shape for
borders, but TBH I'm not convinced we ever get non-renderable rrects for the
outer border rect on this code path: I can't produce any interesting results
with degenerate border radii, it looks like we're rationalizing the values
before we hit this code: http://jsbin.com/viqopi/1/edit?html,css,output

Probably thanks to 
https://www.w3.org/TR/css3-background/#corner-overlap


Still, it's a pre-existing fallback, so I'm hesitant to change in this CL. 
Maybe come back and replace with an assert later?

[chrishtr, 2016-05-23 21:18:04]

On 2016/05/23 at 21:12:23, fmalita wrote:
>
https://codereview.chromium.org/1949253004/diff/500001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):
> 
>
https://codereview.chromium.org/1949253004/diff/500001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/BoxPainter.cpp:428: //
RoundedInnerRectClipper handles this case via discrete, corner-wise clipping.
> On 2016/05/23 19:59:22, chrishtr wrote:
> > What cases is this fallback necessary for?
> 
> Degenerate rounded rects, where the radii sum is > side, presumably.
> 
> I know we run into non-renderable rrects when computing the inner shape for
borders, but TBH I'm not convinced we ever get non-renderable rrects for the
outer border rect on this code path: I can't produce any interesting results
with degenerate border radii, it looks like we're rationalizing the values
before we hit this code: http://jsbin.com/viqopi/1/edit?html,css,output
> 
> Probably thanks to 
> https://www.w3.org/TR/css3-background/#corner-overlap
> 
> 
> Still, it's a pre-existing fallback, so I'm hesitant to change in this CL. 
Maybe come back and replace with an assert later?

Ok, please add a TODO though.

[chrishtr, 2016-05-23 21:18:19]

lgtm

[f(malita), 2016-05-23 21:39:04]

On 2016/05/23 21:18:04, chrishtr wrote:
> On 2016/05/23 at 21:12:23, fmalita wrote:
> > Still, it's a pre-existing fallback, so I'm hesitant to change in this CL. 
> Maybe come back and replace with an assert later?
> 
> Ok, please add a TODO though.

Done.

[chrishtr, 2016-05-23 21:44:40]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-05-23 21:44:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1949253004/#ps520001
(title: "todo")

[commit-bot: I haz the power, 2016-05-23 21:45:05]

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

[commit-bot: I haz the power, 2016-05-24 00:53:18]

Description was changed from

==========
Rounded background image fast path

The current box background paint logic relies on clipping for drawing
rounded-rect background images.  This can yield some pathological
rasterization slowdowns on the GPU backend.

In the case where a single image tile covers the whole box (no fancy
tiling involved/required), a more efficient way to draw is to use a
Skia shader and just fill the rounded rect shape (skipping the
unnecessary clip).

This CL implements such an optimization:

1) Introduce a new Image virtual (applyShader) which configures the
  paint for filling arbitrary shapes with the image content.

2) Add a new GraphicsContext method for drawing rounded-rect images
  (drawRRectImage) using shaders build via #1.

3) Expand BoxPainter's fillFastBottomLayer() fast path to detect
  when tiling is not required and use #2 for painting  rounded-rect
  background images.


This avoids clipping for simple rounded-rect background
images/gradients, and improves GPU rasterization times significantly
in some cases.

In local testing, Animometer/CSS-bouncing-gradient-circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 14X score improvement with Ganesh (27 -> 380).


BUG=603966
R=fs@opera.com,chrishtr@chromium.org,schenney@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Rounded background image fast path

The current box background paint logic relies on clipping for drawing
rounded-rect background images.  This can yield some pathological
rasterization slowdowns on the GPU backend.

In the case where a single image tile covers the whole box (no fancy
tiling involved/required), a more efficient way to draw is to use a
Skia shader and just fill the rounded rect shape (skipping the
unnecessary clip).

This CL implements such an optimization:

1) Introduce a new Image virtual (applyShader) which configures the
  paint for filling arbitrary shapes with the image content.

2) Add a new GraphicsContext method for drawing rounded-rect images
  (drawRRectImage) using shaders build via #1.

3) Expand BoxPainter's fillFastBottomLayer() fast path to detect
  when tiling is not required and use #2 for painting  rounded-rect
  background images.


This avoids clipping for simple rounded-rect background
images/gradients, and improves GPU rasterization times significantly
in some cases.

In local testing, Animometer/CSS-bouncing-gradient-circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 14X score improvement with Ganesh (27 -> 380).


BUG=603966
R=fs@opera.com,chrishtr@chromium.org,schenney@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-05-24 00:53:20]

Committed patchset #27 (id:520001)

[commit-bot: I haz the power, 2016-05-24 00:54:38]

Description was changed from

==========
Rounded background image fast path

The current box background paint logic relies on clipping for drawing
rounded-rect background images.  This can yield some pathological
rasterization slowdowns on the GPU backend.

In the case where a single image tile covers the whole box (no fancy
tiling involved/required), a more efficient way to draw is to use a
Skia shader and just fill the rounded rect shape (skipping the
unnecessary clip).

This CL implements such an optimization:

1) Introduce a new Image virtual (applyShader) which configures the
  paint for filling arbitrary shapes with the image content.

2) Add a new GraphicsContext method for drawing rounded-rect images
  (drawRRectImage) using shaders build via #1.

3) Expand BoxPainter's fillFastBottomLayer() fast path to detect
  when tiling is not required and use #2 for painting  rounded-rect
  background images.


This avoids clipping for simple rounded-rect background
images/gradients, and improves GPU rasterization times significantly
in some cases.

In local testing, Animometer/CSS-bouncing-gradient-circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 14X score improvement with Ganesh (27 -> 380).


BUG=603966
R=fs@opera.com,chrishtr@chromium.org,schenney@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Rounded background image fast path

The current box background paint logic relies on clipping for drawing
rounded-rect background images.  This can yield some pathological
rasterization slowdowns on the GPU backend.

In the case where a single image tile covers the whole box (no fancy
tiling involved/required), a more efficient way to draw is to use a
Skia shader and just fill the rounded rect shape (skipping the
unnecessary clip).

This CL implements such an optimization:

1) Introduce a new Image virtual (applyShader) which configures the
  paint for filling arbitrary shapes with the image content.

2) Add a new GraphicsContext method for drawing rounded-rect images
  (drawRRectImage) using shaders build via #1.

3) Expand BoxPainter's fillFastBottomLayer() fast path to detect
  when tiling is not required and use #2 for painting  rounded-rect
  background images.

This avoids clipping for simple rounded-rect background
images/gradients, and improves GPU rasterization times significantly
in some cases.

In local testing, Animometer/CSS-bouncing-gradient-circles shows a
~26% score improvement with the software backend (300 -> 370) and
a 14X score improvement with Ganesh (27 -> 380).

BUG=603966
R=fs@opera.com,chrishtr@chromium.org,schenney@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/4c1400381d16b6dcd8921a8f19a2ae9af4aac4cf
Cr-Commit-Position: refs/heads/master@{#395492}
==========

[commit-bot: I haz the power, 2016-05-24 00:54:40]

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