Bug 52627 – Accelerated canvas2D has bad clipping performance

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Stephen White]

Since our Canvas2D path does not implement clipping, all draws are done in software and uploaded, which is mighty slow for sites with lots of clipped, composited objects.

See also http://crbug.com/68735

[Stephen White]

Created attachment 79278 [details]
Patch

[WebKit Review Bot]

Attachment 79278 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/thirdparty/glu/libtess/sweep.c:256:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen White]

FYI:  the style error is in thirdparty (ie., non-WebKit) code, and is expected.

[WebKit Review Bot]

Attachment 79278 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7518181

[Stephen White]

Created attachment 79282 [details]
Patch

[WebKit Review Bot]

Attachment 79282 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/thirdparty/glu/libtess/sweep.c:256:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen White]

Created attachment 79283 [details]
Patch

[WebKit Review Bot]

Attachment 79283 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/thirdparty/glu/libtess/sweep.c:256:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 79278 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7562179

[James Robinson]

Please file a bug on check-webkit-style to have it skip the third party directory.  It shouldn't have false positives.

[Stephen White]

(In reply to comment #10)
> Please file a bug on check-webkit-style to have it skip the third party directory.  It shouldn't have false positives.

Done.  https://bugs.webkit.org/show_bug.cgi?id=52636

[James Robinson]

Comment on attachment 79283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79283&action=review

R- for a printf() left in, but looks very cool.  I think Ken or Chris should sanity check the libtess usage as that part's all greek to me.

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:57
> +const int pathTesselation = 30;

nit: static. What does this value (30) mean?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:74
> +        , m_clippingPaths()

why doesn't copying a State object copy the clipping paths as well?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:86
> +static inline FloatPoint operator*(const FloatPoint& f, float scale)

It's kind of weird to define these here.  Why not in FloatPoint/FloatSize.h?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:106
> +class Quadratic {

should this go in its own file?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:127
> +class Cubic {

should this go in its own file?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:254
> +    Color red(255, 0, 0, 255);
> +    fillPath(path, red);

why red?

looking at beginStencilDraw() this doesn't seem to actually put red anywhere, so maybe make this a more descriptively named constant would be clearer

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:261
> +    printf("clipOut\n");

do you mean to check this in?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:285
> +            for (pathIter = clippingPaths.begin(); pathIter < clippingPaths.end();
> +                 ++pathIter) {

nit: odd wrapping

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:312
> +void GLES2Canvas::drawTexturedRect(Texture* texture, const FloatRect& srcRect, const FloatRect& dstRect, const AffineTransform& transform, float alpha, ColorSpace colorSpace, CompositeOperator compositeOp, bool clip)

enum preferred for the 'clip' parameter as drawTextureRect(..., Clip) / drawTexturedRect(..., NoClip) or some better names are easier to grok at the callsites.

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:380
> +static void interpolateQuadratic(FloatVector* vertices, const SkPoint p0, const SkPoint& p1, const SkPoint& p2)

these helpers also feel out of place in this file

[Kenneth Russell]

Comment on attachment 79283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79283&action=review

Looks good overall. There is a problem with the GLU tessellator usage. I am pretty sure the vertices need to be allocated out of non-moving storage. Perhaps there is a way to make it work correctly with a WTF::Vector<float>; the initial vertices could be allocated out of one FloatVector and those created by the combine callback could be added to another. A couple of additional questions.

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:425
> +    static_cast<PolygonData*>(data)->m_indices->append(reinterpret_cast<long>(vertexData));

Why is this reinterpret_cast to long and not short?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:450
> +    checkGLError("createVertexBufferFromPath, createBuffer");
> +    *indexBuffer = m_context->graphicsContext3D()->createBuffer();
> +    checkGLError("createVertexBufferFromPath, createBuffer");

You might want to use different descriptive strings for these two potential errors.

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:506
> +            c[0] = vertices[i * 3];
> +            c[1] = vertices[i * 3 + 1];
> +            c[2] = 1.0f;
> +            internal_gluTessVertex(tess, c, reinterpret_cast<void*>(i));

It's illegal to use a local variable to store the vertex data passed to gluTessVertex. See http://www.opengl.org/sdk/docs/man/xhtml/gluTessVertex.xml .

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:391
> +    if (platformContext()->useGPU())
> +        platformContext()->gpuCanvas()->clipPath(path);
> +

What about GraphicsContext::clipOut(const IntRect& r)? Is that code path used for Canvas' rect(x, y, w, h) plus clip()? Since it hasn't been modified in these changes it looks to me like things will be broken if the clip region is the intersection of a rectangle and a path.

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:418
> +    if (platformContext()->useGPU())
> +        platformContext()->gpuCanvas()->clipOut(p);
> +

Am I reading these correctly that they fall through and perform the software clipping against the backing Skia canvas? If so, is there any way to cache the clipping commands so that in the common case you can avoid executing them in the software rasterizer altogether?

[Stephen White]

Created attachment 79296 [details]
Patch

[WebKit Review Bot]

Attachment 79296 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/thirdparty/glu/libtess/sweep.c:256:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/thirdparty/glu/libtess/render.c:153:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/thirdparty/glu/libtess/render.c:157:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/thirdparty/glu/libtess/render.c:157:  Missing space before ( in while(  [whitespace/parens] [5]
Source/WebCore/thirdparty/glu/libtess/priorityq.c:95:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/thirdparty/glu/libtess/priorityq.c:95:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/thirdparty/glu/libtess/priorityq.c:95:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/thirdparty/glu/libtess/geom.c:208:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/thirdparty/glu/libtess/geom.c:208:  Missing space before ( in while(  [whitespace/parens] [5]
Source/WebCore/thirdparty/glu/libtess/geom.c:208:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 10 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen White]

Comment on attachment 79283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79283&action=review

>> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:57
>> +const int pathTesselation = 30;
> 
> nit: static. What does this value (30) mean?

Fixed:  I've put in a comment to document it.

>> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:74
>> +        , m_clippingPaths()
> 
> why doesn't copying a State object copy the clipping paths as well?

Added a comment.  Clipping paths are saved separately in each state, partly to save memory, and partly because we need to know if there were any clipping paths in the current state in restore() (in order to know which ones to redraw).

>> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:86
>> +static inline FloatPoint operator*(const FloatPoint& f, float scale)
> 
> It's kind of weird to define these here.  Why not in FloatPoint/FloatSize.h?

Since multiplying a point by a scalar is "illegal" (according to the strict mathematical definition), I decided to keep it here to avoid a math nerd war.  :)  (Personally, I hate these kind of artificial distinctions, but that's a flamewar for another day.)

>> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:106
>> +class Quadratic {
> 
> should this go in its own file?

Probably should at some point, but perhaps we could extract it later?

>> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:127
>> +class Cubic {
> 
> should this go in its own file?

Ibid.

>> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:254
>> +    fillPath(path, red);
> 
> why red?
> 
> looking at beginStencilDraw() this doesn't seem to actually put red anywhere, so maybe make this a more descriptively named constant would be clearer

Added a comment.  It actually doesn't matter what color is used (the stencil buffer doesn't read it), but this way if we accidentally draw to the color buffer instead, we can see it.

>> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:261
>> +    printf("clipOut\n");
> 
> do you mean to check this in?

This should never get called from Canvas2D.  Changed it to an assert.

>> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:285
>> +                 ++pathIter) {
> 
> nit: odd wrapping

Fixed.

>> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:312
>> +void GLES2Canvas::drawTexturedRect(Texture* texture, const FloatRect& srcRect, const FloatRect& dstRect, const AffineTransform& transform, float alpha, ColorSpace colorSpace, CompositeOperator compositeOp, bool clip)
> 
> enum preferred for the 'clip' parameter as drawTextureRect(..., Clip) / drawTexturedRect(..., NoClip) or some better names are easier to grok at the callsites.

Will fix in a future patch.

>> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:425
>> +    static_cast<PolygonData*>(data)->m_indices->append(reinterpret_cast<long>(vertexData));
> 
> Why is this reinterpret_cast to long and not short?

It actually has to go to long, then to short, in order to please all compilers.

>> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:506
>> +            internal_gluTessVertex(tess, c, reinterpret_cast<void*>(i));
> 
> It's illegal to use a local variable to store the vertex data passed to gluTessVertex. See http://www.opengl.org/sdk/docs/man/xhtml/gluTessVertex.xml .

The value used for vertexData is not really a local variable:  I'm just placing the vertex index into the pointer (kind of gross, but it saves me having to allocate anything).

You're right about the vertex parameter, though.  Hmm.  Strange -- this code worked for lots of things I've thrown at it (including those with a combine callback).  I guess I'll have to change all my vertices to doubles.  :(

>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:391
>> +
> 
> What about GraphicsContext::clipOut(const IntRect& r)? Is that code path used for Canvas' rect(x, y, w, h) plus clip()? Since it hasn't been modified in these changes it looks to me like things will be broken if the clip region is the intersection of a rectangle and a path.

clipOut() isn't called by canvas2D.  I've put an ASSERT to that effect for the clipOut(Path&) version; will do the same for the rect version.

>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:418
>> +
> 
> Am I reading these correctly that they fall through and perform the software clipping against the backing Skia canvas? If so, is there any way to cache the clipping commands so that in the common case you can avoid executing them in the software rasterizer altogether?

No, it's not possible, since we don't know at this point which types of primitives will be drawn, and some of them are still not accelerated, so we need software clipping in that case.

It's actually ok perf-wise, since the software rasterizer doesn't do anything with the paths until it actually goes to draw a primitive, so the extra cost is just to add the path to a list.

[WebKit Review Bot]

Attachment 79283 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7559201

[Stephen White]

Created attachment 79303 [details]
Patch

[WebKit Review Bot]

Attachment 79303 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/thirdparty/glu/libtess/sweep.c:256:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/thirdparty/glu/libtess/render.c:153:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/thirdparty/glu/libtess/render.c:157:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/thirdparty/glu/libtess/render.c:157:  Missing space before ( in while(  [whitespace/parens] [5]
Source/WebCore/thirdparty/glu/libtess/priorityq.c:95:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/thirdparty/glu/libtess/priorityq.c:95:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/thirdparty/glu/libtess/priorityq.c:95:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/thirdparty/glu/libtess/geom.c:208:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/thirdparty/glu/libtess/geom.c:208:  Missing space before ( in while(  [whitespace/parens] [5]
Source/WebCore/thirdparty/glu/libtess/geom.c:208:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 10 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Russell]

Comment on attachment 79303 [details]
Patch

Thanks for the answers to my previous questions. The revised code which calls the GLU tessellator looks good to me.

[Stephen White]

cr-mac is green.. i can haz r+?

[James Robinson]

Comment on attachment 79303 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79303&action=review

R=me

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:289
> +            for (pathIter = clippingPaths.begin();
> +                 pathIter < clippingPaths.end();
> +                 ++pathIter) {

nit: why wrap this line at all?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:317
> +void GLES2Canvas::drawTexturedRect(Texture* texture, const FloatRect& srcRect, const FloatRect& dstRect, const AffineTransform& transform, float alpha, ColorSpace colorSpace, CompositeOperator compositeOp, bool clip)

nit: file a bug, or better yet just change this to an enum now

[Stephen White]

Committed r76159: <http://trac.webkit.org/changeset/76159>

[Dmitry Titov]

This change broke compile on Chromium Linux-shlib bot: http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28dbg-shlib%29/builds/5757

Chromium does not have trybots or canary for this type of build yet.

Reverting....

[Dmitry Titov]

Revert landed: http://trac.webkit.org/changeset/76189

[James Robinson]

I guess we don't want to put priorityq.c and priorityq-heap.c in the .gypi since priorityq.c does #include "priorityq-heap.c".  I don't know why that would only break this one builder, though.  Here's how to reproduce that build environment locally: http://www.chromium.org/developers/testing/chromium-build-infrastructure/tour-of-the-chromium-buildbot/linux-builder-dbg-shlib.

[WebKit Review Bot]

http://trac.webkit.org/changeset/76189 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
fast/forms/input-text-scroll-left-on-blur.html

[Stephen White]

(In reply to comment #26)
> I guess we don't want to put priorityq.c and priorityq-heap.c in the .gypi since priorityq.c does #include "priorityq-heap.c".

Ugh.  Thanks.

> I don't know why that would only break this one builder, though.

My guess is that it's a difference in behaviour (or our linker flags) between linking an executable vs linking a shared library with all symbols exported: the linker is confident enough to eliminate the duplicate symbol in the former case, but not in the latter.

[Stephen White]

Re-landed as http://trac.webkit.org/changeset/76235 w/shlib fix.