Bug 79659 – Optimize Canvas fill and drawImage with SourceIn, DestinationIn, SourceOut, and DestinationAtop using transparencyLayer.

• Component: Canvas
• Status: RESOLVED FIXED

[Dongseong Hwang]

SourceIn, DestinationIn, SourceOut, and DestinationAtop need to update an
entire canvas, and transparencyLayer can handle well if amending little.
I represented it using GraphicsContextQt that was easy to change.
The previous implementation requires new graphicsContext creation. It is better to
delegate jobs like this to each platform. Especially, the previous implementation
causes large performance hit to GPU implementation (i.e. ACCELERATED_2D_CANVAS).

If other ports implement endTransparencyLayer like Qt, fullCanvasCompositedFill and
fullCanvasCompositedDrawImage can be removed.

Following test cases cover this patch.
    fast/canvas/canvas-composite-alpha.html
    fast/canvas/canvas-composite-canvas.html
    fast/canvas/canvas-composite-fill-repaint.html
    fast/canvas/canvas-composite-image.html
    fast/canvas/canvas-composite-transformclip.html
    fast/canvas/canvas-composite.html

[Dongseong Hwang]

Created attachment 129010 [details]
patch

[WebKit Review Bot]

Attachment 129010 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/GraphicsContext.h:369:  The parameter name "op" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/GraphicsContext.h:547:  The parameter name "op" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dongseong Hwang]

Created attachment 129011 [details]
patch v.2

[Philippe Normand]

Comment on attachment 129011 [details]
patch v.2

Attachment 129011 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11640014

[Gyuyoung Kim]

Comment on attachment 129011 [details]
patch v.2

Attachment 129011 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11628988

[Simon Fraser (smfr)]

Comment on attachment 129011 [details]
patch v.2

View in context: https://bugs.webkit.org/attachment.cgi?id=129011&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:935
> +#if PLATFORM(QT)

We try to discourage platform #ifdefs like this in common code. It would be much better if the #ifdef was something like
TRANSPARENCYLAYER_USES_COMPOSITING_OPERATOR
or something.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:990
> -void GraphicsContext::endPlatformTransparencyLayer()
> +void GraphicsContext::endPlatformTransparencyLayer(CompositeOperator)

I think this patch should at least add an assertion that the operator is srcOver on all platforms where you leave it unimplemented.

[Dongseong Hwang]

Thank for guide.
Can I define guard in wtf/Platform.h ?

[Dongseong Hwang]

Created attachment 129122 [details]
patch v.3

[Dongseong Hwang]

Defined ENABLE_TRANSPARENCYLAYER_USES_COMPOSITING_OPERATOR.
Build fix for cairo.

[Simon Fraser (smfr)]

Comment on attachment 129122 [details]
patch v.3

View in context: https://bugs.webkit.org/attachment.cgi?id=129122&action=review

> Source/JavaScriptCore/wtf/Platform.h:824
> +#if !defined(ENABLE_TRANSPARENCYLAYER_USES_COMPOSITING_OPERATOR)
> +#define ENABLE_TRANSPARENCYLAYER_USES_COMPOSITING_OPERATOR 0
> +#endif

Ugh, this will force a world rebuild for everyone. Just put this in GraphicsContext.h or somewhere.

[Dongseong Hwang]

Created attachment 129128 [details]
patch v.4

[Dongseong Hwang]

I hesitated that it was right, also.

[Dongseong Hwang]

This patch is applied to only Qt.
However, Qt will do correctly after Bug 79658 is committed.

[Dirk Schulze]

Comment on attachment 129128 [details]
patch v.4

View in context: https://bugs.webkit.org/attachment.cgi?id=129128&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:943
>              fullCanvasCompositedFill(m_path);

Can't this logic go to fullCanvasCompositedFill instead?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1064
>          fullCanvasCompositedFill(rect);

Ditto.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1305
>          fullCanvasCompositedDrawImage(cachedImage->imageForRenderer(image->renderer()), ColorSpaceDeviceRGB, normalizedDstRect, normalizedSrcRect, op);

fullCanvasCompositedDrawImage here.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1397
>          fullCanvasCompositedDrawImage(buffer, ColorSpaceDeviceRGB, bufferSrcRect, srcRect, state().m_globalComposite);

Ditto.

[Renata Hodovan]

What is the current status of this bug?

[Allan Sandfeld Jensen]

(In reply to comment #15)
> What is the current status of this bug?

Does it apply and work on the qt5x2 branch https://gitorious.org/qtwebkit/qt5x2/ ?

For webkit trunk I guess it would need one of the other ports to be using the code before it would be accepted.

[Darin Adler]

Comment on attachment 129128 [details]
patch v.4

The current patch is a Qt-only optimization, and that port is no longer in TOT WebKit. It would be OK to do that optimization for some other platform, or for all platforms.

[Dirk Schulze]

Created attachment 228730 [details]
Patch

[Andreas Kling]

Comment on attachment 228730 [details]
Patch

r=me, awesome!

Maybe you could check in the test first and let the bot chew on it for one round, so we can see the needle jump? ;)

[Dirk Schulze]

Committed r167123: <http://trac.webkit.org/changeset/167123>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 131538

[Dirk Schulze]

Committed r167124: <http://trac.webkit.org/changeset/167124>

[Dirk Schulze]

Created attachment 229128 [details]
Patch for landing

[Dirk Schulze]

Created attachment 229271 [details]
Patch

[Darin Adler]

Comment on attachment 229271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229271&action=review

> Source/WebCore/ChangeLog:18
> +        An inline function will create a transparency layer for CG. Cairo graphics
> +        does not composite correctly when a transparency layer gets created. 
> +        The inline function is just a NOOP for Cairo.

Why is this OK for Cairo? Does this behave correctly on Cairo?

[Dirk Schulze]

(In reply to comment #25)
> (From update of attachment 229271 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229271&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        An inline function will create a transparency layer for CG. Cairo graphics
> > +        does not composite correctly when a transparency layer gets created. 
> > +        The inline function is just a NOOP for Cairo.
> 
> Why is this OK for Cairo? Does this behave correctly on Cairo?

Yes, I build it with Cairo as well. The existing tests pass now which were skipped before.

[WebKit Commit Bot]

Comment on attachment 229271 [details]
Patch

Clearing flags on attachment: 229271

Committed r167248: <http://trac.webkit.org/changeset/167248>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.