Add canvas blending modes under a runtime flag

Add canvas blending modes behind a runtime flag

BUG=229111
TEST=fast/canvas/canvas-blend-*

[rosca, 2013-04-08 15:51:48]

I merged the proposed changes from
https://bugs.webkit.org/show_bug.cgi?id=100071. Please review.

[Stephen Chennney, 2013-04-08 16:07:02]

https://codereview.chromium.org/13637006/diff/1/Source/WebCore/platform/graph...
File Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp (right):

https://codereview.chromium.org/13637006/diff/1/Source/WebCore/platform/graph...
Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:274:
context->drawImage(image.get(), styleColorSpace, destRect, srcRect, op,
blendMode, DoNotRespectImageOrientation, useLowQualityScale);
Does this change make some version of drawImage no-longer-used? If so, the
no-longer-used version should be removed.

https://codereview.chromium.org/13637006/diff/1/Source/WebCore/platform/graph...
File Source/WebCore/platform/graphics/skia/SkiaUtils.cpp (right):

https://codereview.chromium.org/13637006/diff/1/Source/WebCore/platform/graph...
Source/WebCore/platform/graphics/skia/SkiaUtils.cpp:82: };
This should be implemented as an array. If the BlendModeMultiply is not
zero-based, you can always index the array by (blendMode - BlendModeMultiply).
You are only ever converting in one direction, so the loop is not needed.

https://codereview.chromium.org/13637006/diff/1/Source/WebCore/platform/graph...
Source/WebCore/platform/graphics/skia/SkiaUtils.cpp:86: if (blendMode !=
BlendModeNormal) {
Is BlendModeNormal == 0? If so, suitably adjust my statements above.

[rosca, 2013-04-08 19:53:25]

https://codereview.chromium.org/13637006/diff/1/Source/WebCore/platform/graph...
File Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp (right):

https://codereview.chromium.org/13637006/diff/1/Source/WebCore/platform/graph...
Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:274:
context->drawImage(image.get(), styleColorSpace, destRect, srcRect, op,
blendMode, DoNotRespectImageOrientation, useLowQualityScale);
On 2013/04/08 16:07:02, Stephen Chenney wrote:
> Does this change make some version of drawImage no-longer-used? If so, the
> no-longer-used version should be removed.
I removed one of the GraphicsContext::drawImage methods.

https://codereview.chromium.org/13637006/diff/1/Source/WebCore/platform/graph...
File Source/WebCore/platform/graphics/skia/SkiaUtils.cpp (right):

https://codereview.chromium.org/13637006/diff/1/Source/WebCore/platform/graph...
Source/WebCore/platform/graphics/skia/SkiaUtils.cpp:82: };
On 2013/04/08 16:07:02, Stephen Chenney wrote:
> This should be implemented as an array. If the BlendModeMultiply is not
> zero-based, you can always index the array by (blendMode - BlendModeMultiply).
> You are only ever converting in one direction, so the loop is not needed.

I followed the example of CompositOpToXfermodeMode. I changed it to an array
with the last upload. Should I change the CompositeOpToXfermodeMode also with
this review?

[Stephen Chennney, 2013-04-08 20:45:07]

Only one more step. This should be run through try jobs just to make sure it
doesn't break the build.

Run this in the third_party/WebKit directory of Chromium, or wherever you have
Blink:
  git wktry

You should get email, sooner or later, with results of a try job (like WebKit
EWS).

[RikC, 2013-04-08 21:33:10]

On 2013/04/08 20:45:07, Stephen Chenney wrote:
> Only one more step. This should be run through try jobs just to make sure it
> doesn't break the build.
> 
> Run this in the third_party/WebKit directory of Chromium, or wherever you have
> Blink:
>   git wktry
> 
> You should get email, sooner or later, with results of a try job (like WebKit
> EWS).

Do we have to be chromium committer to do this?

[Stephen Chennney, 2013-04-08 22:46:32]

On 2013/04/08 21:33:10, RikC wrote:
> On 2013/04/08 20:45:07, Stephen Chenney wrote:
> > Only one more step. This should be run through try jobs just to make sure it
> > doesn't break the build.
> > 
> > Run this in the third_party/WebKit directory of Chromium, or wherever you
have
> > Blink:
> >   git wktry
> > 
> > You should get email, sooner or later, with results of a try job (like
WebKit
> > EWS).
> 
> Do we have to be chromium committer to do this?

Maybe you do. Tomorrow I'll apply the patch and try it. Then I can land it for
you if it passes.

[RikC, 2013-04-08 22:59:32]

> Maybe you do. Tomorrow I'll apply the patch and try it. Then I can land it for
> you if it passes.

Thanks!
Without committer access, we can't pull the tree down and have git hooked up.
(Anonymous pulls are not supported yet)

[eseidel, 2013-04-09 07:26:34]

Are we just implementing part of the existing <canvas> spec, or is this a new
feature?

[rosca, 2013-04-09 08:18:19]

On 2013/04/09 07:26:34, Eric Seidel (Google) wrote:
> Are we just implementing part of the existing <canvas> spec, or is this a new
> feature?

This is a part of Compositing and Blending[1] spec. It extends Canvas
globalCompositeOperation to accept blend modes.
[1]https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#canvascompositingandblending

[eseidel, 2013-04-09 08:20:19]

This was not mentioned in the change description, and sounds like something
which should go through feature review?

Are we the first browser to implement this in-progress spec?

[rosca, 2013-04-09 08:22:28]

Nightly Firefox and WebKit already have this feature in place
(http://blogs.adobe.com/webplatform/2013/01/28/blending-features-in-canvas/)

[eseidel, 2013-04-09 08:25:21]

That sounds like a good argument for it's inclusion in Blink, but it also seems
like it's still possibly a non-trivial platform addition, and worthy of wider
discussion, per:
http://www.chromium.org/blink#new-features

I'm not really trying to create process... so much as I'm trying to make sure
we're aware of what we're adding/removing to the Web Platform and making sure
we're doing so in the most web-friendly, long-term-focused way possible. :)

[Stephen Chennney, 2013-04-09 13:24:54]

On 2013/04/09 08:25:21, Eric Seidel (Google) wrote:
> That sounds like a good argument for it's inclusion in Blink, but it also
seems
> like it's still possibly a non-trivial platform addition, and worthy of wider
> discussion, per:
> http://www.chromium.org/blink#new-features
> 
> I'm not really trying to create process... so much as I'm trying to make sure
> we're aware of what we're adding/removing to the Web Platform and making sure
> we're doing so in the most web-friendly, long-term-focused way possible. :)

Eric makes a good point. The various pieces of the blending/compositing spec
should appear on the feature dashboard (http://www.chromestatus.com/features)
and go through API review (although it seems a bit of a no-brainer right now).

I'll get the conversation started with the people who deal with the dashboard
and API review.

[rosca, 2013-04-25 13:54:22]

I adapted the patch to the latest blink and put blending modes under a runtime
flag.
Please review.

[eseidel, 2013-04-26 18:11:35]

I'm confused as to the runtime flag?  Which RuntimeEnabledFeature flag does this
use?  How would DRT enable this code and shipping Content Layer code disable it?

[Rik, 2013-04-26 19:50:33]

On 2013/04/26 18:11:35, Eric Seidel (Google) wrote:
> I'm confused as to the runtime flag?  Which RuntimeEnabledFeature flag does
this
> use?  How would DRT enable this code and shipping Content Layer code disable
it?

See Source/core/html/canvas/CanvasRenderingContext2D.cpp
This is toggled by the "enable experimental features" flag.

[Rik, 2013-04-26 22:38:56]

On 2013/04/26 18:11:35, Eric Seidel (Google) wrote:
> I'm confused as to the runtime flag?  Which RuntimeEnabledFeature flag does
this
> use?  How would DRT enable this code and shipping Content Layer code disable
it?

Is your question about how the try servers enable this flag? If so, how can we
turn it on?

[Rik, 2013-04-26 22:51:47]

On 2013/04/25 13:54:22, Rosca wrote:
> I adapted the patch to the latest blink and put blending modes under a runtime
> flag.
> Please review.

GraphicsContextSkia.cpp was removed so this patch is no longer valid.

[eseidel, 2013-04-27 03:57:41]

https://codereview.chromium.org/13637006/diff/22001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/13637006/diff/22001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:613: if
(!canvas()->document()->cssCompositingEnabled() && blendMode != BlendModeNormal)
Is cassCompositing a per-document setting?  Most code like this would read from
RuntimeEnabledFeatures::cssCompositingEnabled().

But maybe this can be per-document?

[rosca, 2013-04-30 21:06:27]

I uploaded a new patch.

Tests are skipped because the blending&compositing feature is still under a
compile time flag [Issue 14295007] and the runtime flag is going to be turned on
by the experimental WebKit features [Issue 12681024].
Currently DRT doesn't have a way to run tests with blending enabled. I think
this should come with another review as the blending & compositing runtime flag
is not canvas specific.

https://codereview.chromium.org/13637006/diff/22001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/13637006/diff/22001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:613: if
(!canvas()->document()->cssCompositingEnabled() && blendMode != BlendModeNormal)
On 2013/04/27 03:57:42, Eric Seidel (Google) wrote:
> Is cassCompositing a per-document setting?  Most code like this would read
from
> RuntimeEnabledFeatures::cssCompositingEnabled().
> 
> But maybe this can be per-document?

This setting shouldn't be different among different document objects. I changed
it to query the setting from RuntimeEnabledFeatures.

[RikC, 2013-05-01 04:07:10]

> This setting shouldn't be different among different document objects. I
changed
> it to query the setting from RuntimeEnabledFeatures.

The latest patch stopped working because of other changes so it can't be
submitted yet :-(

[rosca, 2013-05-08 19:30:40]

I uploaded a new patch adapted to the latest blink changes. Eric, could you
review it?

[eseidel, 2013-05-08 19:34:44]

https://codereview.chromium.org/13637006/diff/79001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/13637006/diff/79001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:615: if
(!RuntimeEnabledFeatures::cssCompositingEnabled() && blendMode !=
BlendModeNormal)
I'm confused.  Did we change the parsing?  Was it already possible to pass a
non-normal blend mode?  Did it do anything?

https://codereview.chromium.org/13637006/diff/79001/Source/core/platform/grap...
File Source/core/platform/graphics/skia/SkiaUtils.cpp (right):

https://codereview.chromium.org/13637006/diff/79001/Source/core/platform/grap...
Source/core/platform/graphics/skia/SkiaUtils.cpp:64: // keep this array in sync
with BlendMode enum in GraphicsTypes.h
We can COMPILE_ASSERT That these match, no?

[rosca, 2013-05-08 21:37:17]

https://codereview.chromium.org/13637006/diff/79001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/13637006/diff/79001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:615: if
(!RuntimeEnabledFeatures::cssCompositingEnabled() && blendMode !=
BlendModeNormal)
As described in
https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#canvascomposit...,
globalCompositeOperation has been extended with blend modes. When
globalCompositeOperation is a blend mode, composition operator is suposed to be
'source over'. Parsing is done in parseCompositeAndBlendOperator.

On 2013/05/08 19:34:44, Eric Seidel (Google) wrote:
> I'm confused.  Did we change the parsing?  Was it already possible to pass a
> non-normal blend mode?  Did it do anything?

https://codereview.chromium.org/13637006/diff/79001/Source/core/platform/grap...
File Source/core/platform/graphics/skia/SkiaUtils.cpp (right):

https://codereview.chromium.org/13637006/diff/79001/Source/core/platform/grap...
Source/core/platform/graphics/skia/SkiaUtils.cpp:64: // keep this array in sync
with BlendMode enum in GraphicsTypes.h
On 2013/05/08 19:34:44, Eric Seidel wrote:
> We can COMPILE_ASSERT That these match, no?
Yes, we can compile_assert, but I'm not sure it worths, these values are not
likely to change in the future.

[Rik, 2013-05-09 00:34:30]

> I'm confused.  Did we change the parsing?  Was it already possible to pass a
> non-normal blend mode?  
Yes, this landed a couple of months ago in WebKit. There were no runtime flags.

>Did it do anything?
You can set and query it. Blend modes acted like regular source-over

[eseidel, 2013-05-09 07:34:35]

I see.  So we've long supported parsing of these modes, but that wasn't
detectable because we didnt' do anything with them?

[rosca, 2013-05-09 07:41:45]

On 2013/05/09 07:34:35, Eric Seidel wrote:
> I see.  So we've long supported parsing of these modes, but that wasn't
> detectable because we didnt' do anything with them?

Yes, that's correct. Thanks for reviewing this. Rik started a parallel review
for this issue which already got committed.

[rosca, 2013-05-09 07:42:26]

> Rik started a parallel review
> for this issue which already got committed.

This is the issue https://codereview.chromium.org/14679003

[Rik, 2013-05-09 19:45:36]

On 2013/05/09 07:34:35, Eric Seidel wrote:
> I see.  So we've long supported parsing of these modes, but that wasn't
> detectable because we didnt' do anything with them?

That's correct.

[Rik, 2013-05-09 19:46:27]

On 2013/05/09 07:42:26, Rosca wrote:
> > Rik started a parallel review
> > for this issue which already got committed.
> 
> This is the issue https://codereview.chromium.org/14679003

We should make sure that Mihai gets 'credit' for this patch since all I did was
send it through the system.

[Rik, 2013-05-09 19:47:07]

On 2013/05/09 19:46:27, Rik wrote:
> On 2013/05/09 07:42:26, Rosca wrote:
> > > Rik started a parallel review
> > > for this issue which already got committed.
> > 
> > This is the issue https://codereview.chromium.org/14679003
> 
> We should make sure that Mihai gets 'credit' for this patch since all I did
was
> send it through the system.

Sorry, I meant Ion.

[eseidel, 2013-05-09 20:00:39]

lgtm