Bug 57464 - Clipping in accelerated Canvas2D should be faster
Summary: Clipping in accelerated Canvas2D should be faster
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-30 09:20 PDT by Stephen White
Modified: 2011-03-30 14:50 PDT (History)
3 users (show)

See Also:


Attachments
Patch (11.83 KB, patch)
2011-03-30 09:38 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (12.07 KB, patch)
2011-03-30 14:41 PDT, Stephen White
kbr: 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-03-30 09:20:33 PDT
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.
Comment 1 Stephen White 2011-03-30 09:38:56 PDT
Created attachment 87552 [details]
Patch
Comment 2 Kenneth Russell 2011-03-30 14:20:28 PDT
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 3 Stephen White 2011-03-30 14:34:46 PDT
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).
Comment 4 Stephen White 2011-03-30 14:41:03 PDT
Created attachment 87620 [details]
Patch
Comment 5 Kenneth Russell 2011-03-30 14:45:06 PDT
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
Comment 6 Stephen White 2011-03-30 14:50:04 PDT
Committed r82502: <http://trac.webkit.org/changeset/82502>