Bug 79659

Summary: Optimize Canvas fill and drawImage with SourceIn, DestinationIn, SourceOut, and DestinationAtop using transparencyLayer.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: CanvasAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, allan.jensen, benwells, commit-queue, darin, esprehn+autocc, gns, gyuyoung.kim, hausmann, kbr, kenneth, kling, krit, mdelaney7, pnormand, rhodovan.u-szeged, rniwa, simon.fraser, webkit.review.bot, xan.lopez, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 79658, 131538    
Bug Blocks: 131303    
Attachments:
Description Flags
patch
none
patch v.2
simon.fraser: review-, pnormand: commit-queue-
patch v.3
simon.fraser: review-, simon.fraser: commit-queue-
patch v.4
darin: review-, darin: commit-queue-
Patch
kling: review+
Patch for landing
none
Patch none

Description Dongseong Hwang 2012-02-27 04:10:50 PST
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
Comment 1 Dongseong Hwang 2012-02-27 04:15:22 PST
Created attachment 129010 [details]
patch
Comment 2 WebKit Review Bot 2012-02-27 04:18:54 PST
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.
Comment 3 Dongseong Hwang 2012-02-27 04:21:16 PST
Created attachment 129011 [details]
patch v.2
Comment 4 Philippe Normand 2012-02-27 04:39:30 PST
Comment on attachment 129011 [details]
patch v.2

Attachment 129011 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11640014
Comment 5 Gyuyoung Kim 2012-02-27 04:40:26 PST
Comment on attachment 129011 [details]
patch v.2

Attachment 129011 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11628988
Comment 6 Simon Fraser (smfr) 2012-02-27 09:52:34 PST
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.
Comment 7 Dongseong Hwang 2012-02-27 15:41:38 PST
Thank for guide.
Can I define guard in wtf/Platform.h ?
Comment 8 Dongseong Hwang 2012-02-27 16:16:46 PST
Created attachment 129122 [details]
patch v.3
Comment 9 Dongseong Hwang 2012-02-27 16:17:39 PST
Defined ENABLE_TRANSPARENCYLAYER_USES_COMPOSITING_OPERATOR.
Build fix for cairo.
Comment 10 Simon Fraser (smfr) 2012-02-27 16:28:38 PST
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.
Comment 11 Dongseong Hwang 2012-02-27 16:42:06 PST
Created attachment 129128 [details]
patch v.4
Comment 12 Dongseong Hwang 2012-02-27 16:43:55 PST
I hesitated that it was right, also.
Comment 13 Dongseong Hwang 2012-02-27 17:01:00 PST
This patch is applied to only Qt.
However, Qt will do correctly after Bug 79658 is committed.
Comment 14 Dirk Schulze 2013-02-16 00:30:14 PST
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.
Comment 15 Renata Hodovan 2013-11-04 06:33:11 PST
What is the current status of this bug?
Comment 16 Allan Sandfeld Jensen 2013-11-04 06:52:39 PST
(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.
Comment 17 Darin Adler 2013-11-04 09:41:48 PST
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.
Comment 18 Dirk Schulze 2014-04-07 06:01:31 PDT
Created attachment 228730 [details]
Patch
Comment 19 Andreas Kling 2014-04-07 10:03:12 PDT
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? ;)
Comment 20 Dirk Schulze 2014-04-11 05:37:58 PDT
Committed r167123: <http://trac.webkit.org/changeset/167123>
Comment 21 WebKit Commit Bot 2014-04-11 05:41:48 PDT
Re-opened since this is blocked by bug 131538
Comment 22 Dirk Schulze 2014-04-11 05:45:30 PDT
Committed r167124: <http://trac.webkit.org/changeset/167124>
Comment 23 Dirk Schulze 2014-04-11 05:57:48 PDT
Created attachment 229128 [details]
Patch for landing
Comment 24 Dirk Schulze 2014-04-14 01:00:58 PDT
Created attachment 229271 [details]
Patch
Comment 25 Darin Adler 2014-04-14 08:31:51 PDT
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?
Comment 26 Dirk Schulze 2014-04-14 08:40:46 PDT
(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.
Comment 27 WebKit Commit Bot 2014-04-14 09:02:31 PDT
Comment on attachment 229271 [details]
Patch

Clearing flags on attachment: 229271

Committed r167248: <http://trac.webkit.org/changeset/167248>
Comment 28 WebKit Commit Bot 2014-04-14 09:02:45 PDT
All reviewed patches have been landed.  Closing bug.