Bug 57464 – Clipping in accelerated Canvas2D should be faster

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Stephen White]

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.

[Stephen White]

Created attachment 87552 [details]
Patch

[Kenneth Russell]

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().

[Stephen White]

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).

[Stephen White]

Created attachment 87620 [details]
Patch

[Kenneth Russell]

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

[Stephen White]

Committed r82502: <http://trac.webkit.org/changeset/82502>