| Summary: | Accelerated canvas2D has bad clipping performance | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Stephen White <senorblanco> | ||||||||||||
| Component: | Layout and Rendering | Assignee: | Stephen White <senorblanco> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | abarth, cmarrin, dglazkov, dimich, eric, jamesr, mdelaney7, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Stephen White
2011-01-18 07:55:27 PST
Created attachment 79278 [details]
Patch
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.
FYI: the style error is in thirdparty (ie., non-WebKit) code, and is expected. Attachment 79278 [details] did not build on mac: Build output: http://queues.webkit.org/results/7518181 Created attachment 79282 [details]
Patch
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.
Created attachment 79283 [details]
Patch
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.
Attachment 79278 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7562179 Please file a bug on check-webkit-style to have it skip the third party directory. It shouldn't have false positives. (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 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 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? Created attachment 79296 [details]
Patch
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 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. Attachment 79283 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7559201 Created attachment 79303 [details]
Patch
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 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.
cr-mac is green.. i can haz r+? 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 Committed r76159: <http://trac.webkit.org/changeset/76159> 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.... Revert landed: http://trac.webkit.org/changeset/76189 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. 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 (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. Re-landed as http://trac.webkit.org/changeset/76235 w/shlib fix. |