Forward fill and stroke styles from 2d canvas to canvas filters

Forward fill and stroke styles from 2d canvas to canvas filters

This forwards the fill and stroke style from 2d canvas to the
FillPaint and StrokePaint inputs of canvas filters.

BUG=502877
Committed: https://crrev.com/435e3bbf39d8799289f0f0435e7065f841a702ac
Cr-Commit-Position: refs/heads/master@{#368726}

[ajuma, 2015-12-21 16:39:49]

Description was changed from

==========
Forward fill and stroke styles from 2d canvas to filters

This forwards the fill and stroke style from 2d canvas to the
FillPaint and StrokePaint inputs of filters.

BUG=502877
==========

to

==========
Forward fill and stroke styles from 2d canvas to canvas filters

This forwards the fill and stroke style from 2d canvas to the
FillPaint and StrokePaint inputs of canvas filters.

BUG=502877
==========

[ajuma, 2015-12-21 16:41:08]

ajuma@chromium.org changed reviewers:
+ junov@chromium.org, senorblanco@chromium.org

[ajuma, 2015-12-21 16:41:57]

This depends on skia CL http://crrev.com/1543583002

[Stephen White, 2015-12-21 16:47:42]

Cool stuff!

Could you break out SVG changes from the canvas changes, so we can land them
separately?

Could you also add some layout tests for the same functionality on the SVG side?
(I'm assuming it works in SVG too.) svg/filters is probably the best place.

[ajuma, 2015-12-21 16:58:55]

On 2015/12/21 16:47:42, Stephen White wrote:
> Cool stuff!
> 
> Could you break out SVG changes from the canvas changes, so we can land them
> separately?
> 
> Could you also add some layout tests for the same functionality on the SVG
side?
> (I'm assuming it works in SVG too.) svg/filters is probably the best place.

I haven't implemented the SVG side in this CL (was planning to leave that for a
separate CL).

[Justin Novosad, 2015-12-21 18:10:26]

junov@chromium.org changed reviewers:
+ fmalita@chromium.org

[Justin Novosad, 2015-12-21 18:10:27]

+fmalita for expertise on skia usage in blink.

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.h (right):

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.h:57: static
PassRefPtrWillBeRawPtr<Filter> build(float zoom, Element*, FilterEffect*
previousEffect, const ReferenceFilterOperation&, PassRefPtr<SkShader> fillShader
= nullptr, PassRefPtr<SkShader> strokeShader = nullptr);
Are we sure SkShader is the right abstraction? I am concerned that we might end
up needing special behaviors that do map directly to skia (e.g. pattern tiling
modes). Possible alternatives that come to mind:
* blink:Image (which can be a GeneratedImage), 
* SkPaint, which can efficiently represent solid color styles, and can still
encapsulate an SkShader.  Also, are we comfortable with using SkShader directly
in core/layout/svg?

[f(malita), 2015-12-21 19:01:04]

fmalita@chromium.org changed reviewers:
+ fs@opera.com

[f(malita), 2015-12-21 19:01:05]

+fs

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.h (right):

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.h:57: static
PassRefPtrWillBeRawPtr<Filter> build(float zoom, Element*, FilterEffect*
previousEffect, const ReferenceFilterOperation&, PassRefPtr<SkShader> fillShader
= nullptr, PassRefPtr<SkShader> strokeShader = nullptr);
On 2015/12/21 18:10:27, Justin Novosad wrote:
> Are we sure SkShader is the right abstraction? I am concerned that we might
end
> up needing special behaviors that do map directly to skia (e.g. pattern tiling
> modes). Possible alternatives that come to mind:
> * blink:Image (which can be a GeneratedImage), 
> * SkPaint, which can efficiently represent solid color styles, and can still
> encapsulate an SkShader.

+1 to making PaintFilterEffect SkPaint-based.

Then instead of CanvasStyle::asShader() we could have something like
applyToSkPaint (better aligned with SVGPaintServer::applyToSkPaint).

Maybe have a generalized PaintServer usable by both SVG & C2D?

[fs, 2015-12-21 19:34:20]

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.h (right):

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.h:57: static
PassRefPtrWillBeRawPtr<Filter> build(float zoom, Element*, FilterEffect*
previousEffect, const ReferenceFilterOperation&, PassRefPtr<SkShader> fillShader
= nullptr, PassRefPtr<SkShader> strokeShader = nullptr);
On 2015/12/21 at 19:01:05, f(malita) wrote:
> On 2015/12/21 18:10:27, Justin Novosad wrote:
> > Are we sure SkShader is the right abstraction? I am concerned that we might
end
> > up needing special behaviors that do map directly to skia (e.g. pattern
tiling
> > modes). Possible alternatives that come to mind:
> > * blink:Image (which can be a GeneratedImage), 
> > * SkPaint, which can efficiently represent solid color styles, and can still
> > encapsulate an SkShader.
> 
> +1 to making PaintFilterEffect SkPaint-based.
> 
> Then instead of CanvasStyle::asShader() we could have something like
applyToSkPaint (better aligned with SVGPaintServer::applyToSkPaint).

Yes, SkPaint would mesh slightly better with the (current) structure of SVG
paint servers - not that using SkShader would be overly problematic either
though.

> Maybe have a generalized PaintServer usable by both SVG & C2D?

I think I would off on that for now at least - where I see paint server setup
moving we'll likely not need that middle-man.

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/FilterEffectBuilder.cpp (right):

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FilterEffectBuilder.cpp:143:
RefPtrWillBeRawPtr<Filter> referenceFilter = ReferenceFilterBuilder::build(zoom,
element, previousEffect.get(), toReferenceFilterOperation(*filterOperation),
fillShader, strokeShader);
Won't this "lose" the shaders if there were more than one "reference" filters in
the same chain? (PassRefPtr copying semantics.)

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.cpp
(right):

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.cpp:91: ,
m_paintFilter(Filter::create(1.0))
Could the "paint" inputs use the Filter from, say, sourceGraphic instead? IIRC,
FilterEffects don't have strong references to the Filter, so this might run the
risk of dangling references since the lifetime of this might be shorter than
that of the entire chain.

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h
(right):

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h:21:
static const AtomicString& fillEffectName();
I think these could just be local to the SVGFilterBuilder - and the same
treatment given to SourceGraphic::effectName and SourceAlpha::effectName rather.
Maybe a local "FilterInputKeywords" with some methods or something.

[ajuma, 2016-01-11 19:14:08]

Sorry for the delay (this was blocked on getting an SkPaintImageFilter into
skia).

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.h (right):

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.h:57: static
PassRefPtrWillBeRawPtr<Filter> build(float zoom, Element*, FilterEffect*
previousEffect, const ReferenceFilterOperation&, PassRefPtr<SkShader> fillShader
= nullptr, PassRefPtr<SkShader> strokeShader = nullptr);
On 2015/12/21 18:10:27, Justin Novosad wrote:
> Are we sure SkShader is the right abstraction? I am concerned that we might
end
> up needing special behaviors that do map directly to skia (e.g. pattern tiling
> modes). Possible alternatives that come to mind:
> * blink:Image (which can be a GeneratedImage), 
> * SkPaint, which can efficiently represent solid color styles, and can still
> encapsulate an SkShader.  Also, are we comfortable with using SkShader
directly
> in core/layout/svg?

This now uses SkPaint instead of SkShader.

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/FilterEffectBuilder.cpp (right):

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FilterEffectBuilder.cpp:143:
RefPtrWillBeRawPtr<Filter> referenceFilter = ReferenceFilterBuilder::build(zoom,
element, previousEffect.get(), toReferenceFilterOperation(*filterOperation),
fillShader, strokeShader);
On 2015/12/21 19:34:20, fs wrote:
> Won't this "lose" the shaders if there were more than one "reference" filters
in
> the same chain? (PassRefPtr copying semantics.)

Good catch, thanks! This now uses a raw pointer to an SkPaint (rather than a
PassRefPtr to a shader) so this is no longer a problem.

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.cpp
(right):

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.cpp:91: ,
m_paintFilter(Filter::create(1.0))
On 2015/12/21 19:34:20, fs wrote:
> Could the "paint" inputs use the Filter from, say, sourceGraphic instead?
IIRC,
> FilterEffects don't have strong references to the Filter, so this might run
the
> risk of dangling references since the lifetime of this might be shorter than
> that of the entire chain.

Done.

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h
(right):

https://codereview.chromium.org/1543593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h:21:
static const AtomicString& fillEffectName();
On 2015/12/21 19:34:20, fs wrote:
> I think these could just be local to the SVGFilterBuilder - and the same
> treatment given to SourceGraphic::effectName and SourceAlpha::effectName
rather.
> Maybe a local "FilterInputKeywords" with some methods or something.

Done.

[Justin Novosad, 2016-01-11 19:52:42]

https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp
(right):

https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:359:
SkPaint fillPaint;
Is there a reason why we need to create a new paint rather than use m_fillPaint
and m_strokePaint? Assuming there is a good reason, could you put that reason in
a comment, and use a name that is more specific, like fillPaintForFilter to
avoid reader's confusion with m_fillPaint.  Same thing for strokePaint.

[fs, 2016-01-11 20:19:24]

core/ bits LGTM

(Don't forget s/2015/2016/ in new files BTW =))

https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h
(right):

https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h:23: bool
affectsTransparentPixels() override { return true; }
Since this is an input I'm sure if this feels a bit weird to me (and will be
redundant with the !size part of the condition where this is used.) I'll defer
to whoever signs off on he platform/ bits though.

[ajuma, 2016-01-11 21:33:08]

https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp
(right):

https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:359:
SkPaint fillPaint;
On 2016/01/11 19:52:42, Justin Novosad wrote:
> Is there a reason why we need to create a new paint rather than use
m_fillPaint
> and m_strokePaint? Assuming there is a good reason, could you put that reason
in
> a comment, and use a name that is more specific, like fillPaintForFilter to
> avoid reader's confusion with m_fillPaint.  Same thing for strokePaint.

Added a comment and changed the variable names.

https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h
(right):

https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h:23: bool
affectsTransparentPixels() override { return true; }
On 2016/01/11 20:19:24, fs wrote:
> Since this is an input I'm sure if this feels a bit weird to me (and will be
> redundant with the !size part of the condition where this is used.) I'll defer
> to whoever signs off on he platform/ bits though.

I interpreted "affectsTransparentPixes" as "produces non-transparent input even
when it doesn't have non-transparent input", but I'm not sure if this is the
intended interpretation. As you say, it doesn't affect current behavior because
of the !size part of the condition where it's used.

[ajuma, 2016-01-11 21:34:18]

https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h
(right):

https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h:23: bool
affectsTransparentPixels() override { return true; }
On 2016/01/11 21:33:08, ajuma wrote:
> On 2016/01/11 20:19:24, fs wrote:
> > Since this is an input I'm sure if this feels a bit weird to me (and will be
> > redundant with the !size part of the condition where this is used.) I'll
defer
> > to whoever signs off on he platform/ bits though.
> 
> I interpreted "affectsTransparentPixes" as "produces non-transparent input
even
> when it doesn't have non-transparent input", but I'm not sure if this is the
> intended interpretation. As you say, it doesn't affect current behavior
because
> of the !size part of the condition where it's used.

Sorry, I meant "produces non-transparent *output* even when it doesn't have
non-transparent input"

[Stephen White, 2016-01-11 21:36:36]

On 2016/01/11 21:34:18, ajuma wrote:
>
https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h
> (right):
> 
>
https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h:23:
bool
> affectsTransparentPixels() override { return true; }
> On 2016/01/11 21:33:08, ajuma wrote:
> > On 2016/01/11 20:19:24, fs wrote:
> > > Since this is an input I'm sure if this feels a bit weird to me (and will
be
> > > redundant with the !size part of the condition where this is used.) I'll
> defer
> > > to whoever signs off on he platform/ bits though.
> > 
> > I interpreted "affectsTransparentPixes" as "produces non-transparent input
> even
> > when it doesn't have non-transparent input", but I'm not sure if this is the
> > intended interpretation. As you say, it doesn't affect current behavior
> because
> > of the !size part of the condition where it's used.
> 
> Sorry, I meant "produces non-transparent *output* even when it doesn't have
> non-transparent input"

Since this filter doesn't have an input, I don't think this should be necessary.
SourceGraphic and
SourceAlpha don't do this, for example, even though they can produce transparent
output.

[ajuma, 2016-01-11 21:41:46]

On 2016/01/11 21:36:36, Stephen White wrote:
> On 2016/01/11 21:34:18, ajuma wrote:
> >
>
https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h:23:
> bool
> > affectsTransparentPixels() override { return true; }
> > On 2016/01/11 21:33:08, ajuma wrote:
> > > On 2016/01/11 20:19:24, fs wrote:
> > > > Since this is an input I'm sure if this feels a bit weird to me (and
will
> be
> > > > redundant with the !size part of the condition where this is used.) I'll
> > defer
> > > > to whoever signs off on he platform/ bits though.
> > > 
> > > I interpreted "affectsTransparentPixes" as "produces non-transparent input
> > even
> > > when it doesn't have non-transparent input", but I'm not sure if this is
the
> > > intended interpretation. As you say, it doesn't affect current behavior
> > because
> > > of the !size part of the condition where it's used.
> > 
> > Sorry, I meant "produces non-transparent *output* even when it doesn't have
> > non-transparent input"
> 
> Since this filter doesn't have an input, I don't think this should be
necessary.
> SourceGraphic and
> SourceAlpha don't do this, for example, even though they can produce
transparent
> output.

Thanks, removed now.

[Stephen White, 2016-01-11 21:50:28]

LGTM

[fs, 2016-01-11 21:55:27]

On 2016/01/11 at 21:36:36, senorblanco wrote:
> On 2016/01/11 21:34:18, ajuma wrote:
> >
https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h
> > (right):
> > 
> >
https://codereview.chromium.org/1543593002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/graphics/filters/PaintFilterEffect.h:23:
bool
> > affectsTransparentPixels() override { return true; }
> > On 2016/01/11 21:33:08, ajuma wrote:
> > > On 2016/01/11 20:19:24, fs wrote:
> > > > Since this is an input I'm sure if this feels a bit weird to me (and
will be
> > > > redundant with the !size part of the condition where this is used.) I'll
> > defer
> > > > to whoever signs off on he platform/ bits though.
> > > 
> > > I interpreted "affectsTransparentPixes" as "produces non-transparent input
> > even
> > > when it doesn't have non-transparent input", but I'm not sure if this is
the
> > > intended interpretation. As you say, it doesn't affect current behavior
> > because
> > > of the !size part of the condition where it's used.
> > 
> > Sorry, I meant "produces non-transparent *output* even when it doesn't have
> > non-transparent input"
> 
> Since this filter doesn't have an input, I don't think this should be
necessary. SourceGraphic and
> SourceAlpha don't do this, for example, even though they can produce
transparent output.

FEFlood is another example (which is very similar to PaintFilterEffect.)

[Justin Novosad, 2016-01-11 22:28:55]

Your interpretation of affectsTransparentPixels is correct. In any case,
returning true conservatively is always safe though possibly sub-optimal.

lgtm

[ajuma, 2016-01-11 22:34:37]

The CQ bit was checked by ajuma@chromium.org

[ajuma, 2016-01-11 22:34:38]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1543593002/#ps120001
(title: "Remove PaintFilterEffect::affectsTransparentPixels")

[commit-bot: I haz the power, 2016-01-11 22:36:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1543593002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1543593002/120001

[commit-bot: I haz the power, 2016-01-12 00:01:39]

Description was changed from

==========
Forward fill and stroke styles from 2d canvas to canvas filters

This forwards the fill and stroke style from 2d canvas to the
FillPaint and StrokePaint inputs of canvas filters.

BUG=502877
==========

to

==========
Forward fill and stroke styles from 2d canvas to canvas filters

This forwards the fill and stroke style from 2d canvas to the
FillPaint and StrokePaint inputs of canvas filters.

BUG=502877
==========

[commit-bot: I haz the power, 2016-01-12 00:01:40]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-12 00:03:35]

Description was changed from

==========
Forward fill and stroke styles from 2d canvas to canvas filters

This forwards the fill and stroke style from 2d canvas to the
FillPaint and StrokePaint inputs of canvas filters.

BUG=502877
==========

to

==========
Forward fill and stroke styles from 2d canvas to canvas filters

This forwards the fill and stroke style from 2d canvas to the
FillPaint and StrokePaint inputs of canvas filters.

BUG=502877

Committed: https://crrev.com/435e3bbf39d8799289f0f0435e7065f841a702ac
Cr-Commit-Position: refs/heads/master@{#368726}
==========

[commit-bot: I haz the power, 2016-01-12 00:03:36]

Patchset 7 (id:??) landed as
https://crrev.com/435e3bbf39d8799289f0f0435e7065f841a702ac
Cr-Commit-Position: refs/heads/master@{#368726}