The clipping in accelerated Canvas2D is not as fast as it could be. It clears the stencil buffer when removing clipping paths, when it would probably be faster to re-draw the path with a decrement stencil op.
Created attachment 87552 [details] Patch
Comment on attachment 87552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87552&action=review The idea is good but the implementation needs cleanup. > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:72 > + Path m_path; > + AffineTransform m_transform; In WebKit style the m_ prefix isn't needed for members of structs. > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:81 > + , m_numClippingPaths(0) Please add a method to this struct like "bool clippingEnabled()" which does the test of m_numClippingPaths > 0. The tests strewn throughout this patch are gross and will be fragile in the long run. Even better, make the method call m_clippingPaths.size() and remove the duplicated m_numClippingPaths member entirely. > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:221 > + if (m_state->m_ctm.isIdentity() && !m_state->m_numClippingPaths) { Replace the test of m_numClippingPaths with a method call. > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:254 > + applyClipping(m_state->m_numClippingPaths > 0); Here and below, please replace the test of "m_state->m_numClippingPaths > 0" with a method call. > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:355 > + m_state->m_clippingPaths.append(PathAndTransform(path, m_state->m_ctm)); > + m_state->m_numClippingPaths++; This looks like replicated data. m_numClippingPaths == m_clippingPaths.size().
Comment on attachment 87552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87552&action=review Thanks for the review. >> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:72 >> + AffineTransform m_transform; > > In WebKit style the m_ prefix isn't needed for members of structs. Fixed. >> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:81 >> + , m_numClippingPaths(0) > > Please add a method to this struct like "bool clippingEnabled()" which does the test of m_numClippingPaths > 0. The tests strewn throughout this patch are gross and will be fragile in the long run. Even better, make the method call m_clippingPaths.size() and remove the duplicated m_numClippingPaths member entirely. No, that won't work. m_numClippingPaths is the total number of paths, in all States. m_clippingPaths contains only the paths in the current State (see the comment in the GLES2Canvas copy constructor). >> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:221 >> + if (m_state->m_ctm.isIdentity() && !m_state->m_numClippingPaths) { > > Replace the test of m_numClippingPaths with a method call. Done. >> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:254 >> + applyClipping(m_state->m_numClippingPaths > 0); > > Here and below, please replace the test of "m_state->m_numClippingPaths > 0" with a method call. Done. >> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:355 >> + m_state->m_numClippingPaths++; > > This looks like replicated data. m_numClippingPaths == m_clippingPaths.size(). Nope (see above).
Created attachment 87620 [details] Patch
Comment on attachment 87620 [details] Patch Thanks for doing the cleanups. I understand now the difference between m_clippingPaths' length and m_numClippingPaths. r=me
Committed r82502: <http://trac.webkit.org/changeset/82502>