Bug 52627 - Accelerated canvas2D has bad clipping performance
Summary: Accelerated canvas2D has bad clipping performance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-18 07:55 PST by Stephen White
Modified: 2011-01-20 08:46 PST (History)
8 users (show)

See Also:


Attachments
Patch (46.11 KB, patch)
2011-01-18 08:03 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch (47.10 KB, patch)
2011-01-18 08:59 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch (47.13 KB, patch)
2011-01-18 09:03 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch (50.33 KB, patch)
2011-01-18 10:59 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch (50.56 KB, patch)
2011-01-18 11:56 PST, Stephen White
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2011-01-18 07:55:27 PST
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
Comment 1 Stephen White 2011-01-18 08:03:55 PST
Created attachment 79278 [details]
Patch
Comment 2 WebKit Review Bot 2011-01-18 08:05:40 PST
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.
Comment 3 Stephen White 2011-01-18 08:09:03 PST
FYI:  the style error is in thirdparty (ie., non-WebKit) code, and is expected.
Comment 4 WebKit Review Bot 2011-01-18 08:38:14 PST
Attachment 79278 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7518181
Comment 5 Stephen White 2011-01-18 08:59:27 PST
Created attachment 79282 [details]
Patch
Comment 6 WebKit Review Bot 2011-01-18 09:02:12 PST
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.
Comment 7 Stephen White 2011-01-18 09:03:25 PST
Created attachment 79283 [details]
Patch
Comment 8 WebKit Review Bot 2011-01-18 09:04:58 PST
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.
Comment 9 WebKit Review Bot 2011-01-18 10:00:19 PST
Attachment 79278 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7562179
Comment 10 James Robinson 2011-01-18 10:04:18 PST
Please file a bug on check-webkit-style to have it skip the third party directory.  It shouldn't have false positives.
Comment 11 Stephen White 2011-01-18 10:11:44 PST
(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
Comment 12 James Robinson 2011-01-18 10:23:22 PST
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
Comment 13 Kenneth Russell 2011-01-18 10:44:39 PST
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?
Comment 14 Stephen White 2011-01-18 10:59:40 PST
Created attachment 79296 [details]
Patch
Comment 15 WebKit Review Bot 2011-01-18 11:01:54 PST
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.
Comment 16 Stephen White 2011-01-18 11:16:18 PST
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.
Comment 17 WebKit Review Bot 2011-01-18 11:19:59 PST
Attachment 79283 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7559201
Comment 18 Stephen White 2011-01-18 11:56:42 PST
Created attachment 79303 [details]
Patch
Comment 19 WebKit Review Bot 2011-01-18 11:58:54 PST
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.
Comment 20 Kenneth Russell 2011-01-18 12:06:19 PST
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.
Comment 21 Stephen White 2011-01-18 14:20:26 PST
cr-mac is green.. i can haz r+?
Comment 22 James Robinson 2011-01-19 14:06:15 PST
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
Comment 23 Stephen White 2011-01-19 14:12:38 PST
Committed r76159: <http://trac.webkit.org/changeset/76159>
Comment 24 Dmitry Titov 2011-01-19 18:00:12 PST
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....
Comment 25 Dmitry Titov 2011-01-19 18:28:55 PST
Revert landed: http://trac.webkit.org/changeset/76189
Comment 26 James Robinson 2011-01-19 18:52:00 PST
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.
Comment 27 WebKit Review Bot 2011-01-19 19:14:23 PST
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
Comment 28 Stephen White 2011-01-19 20:29:52 PST
(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.
Comment 29 Stephen White 2011-01-20 08:46:24 PST
Re-landed as http://trac.webkit.org/changeset/76235 w/shlib fix.