Don't store an SkPicture in the SourceGraphic FilterEffect

Don't store an SkPicture in the SourceGraphic FilterEffect

Instead of storing the SkPicture in the SourceGraphic FilterEffect, just
create a filter and pre-populate all the image filter "slots" when we've
recorded the content that should be filtered.
This avoids keeping an explicit reference to the SkPicture, and thus
avoids keeping this object alive when the Filter and it's associated
filter-chain is in limbo waiting for a Oilpan GC sweep.

BUG=610158
Committed: https://crrev.com/589ab53f384e1e8a0df92eb868acf3ea35839a2e
Cr-Commit-Position: refs/heads/master@{#393280}

[fs, 2016-05-10 15:35:45]

Description was changed from

==========
Don't store SkPicture in SourceGraphic

Instead of storing the SkPicture in the SourceGraphic FilterEffect, just
create a filter and pre-populate all the image filter "slots" when we've
recorded the content that should be filtered.

BUG=610158
==========

to

==========
Don't store an SkPicture in the SourceGraphic FilterEffect

Instead of storing the SkPicture in the SourceGraphic FilterEffect, just
create a filter and pre-populate all the image filter "slots" when we've
recorded the content that should be filtered.

BUG=610158
==========

[Stephen White, 2016-05-10 18:24:01]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2016-05-10 18:24:02]

https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp
(right):

https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp:59:
ASSERT_NOT_REACHED();
I think this is problematic because there are some valid cases where we don't
have an SkPicture (and that createImageFilter() should return null): accelerated
CSS filters, and canvas filters.

But it should be ok to just remove the assert and return nullptr here.

[fs, 2016-05-11 10:54:23]

https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp
(right):

https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp:59:
ASSERT_NOT_REACHED();
On 2016/05/10 at 18:24:02, Stephen White wrote:
> I think this is problematic because there are some valid cases where we don't
have an SkPicture (and that createImageFilter() should return null): accelerated
CSS filters, and canvas filters.
> 
> But it should be ok to just remove the assert and return nullptr here.

Yes, doing that would reflect the old behavior better... so done.

[fs, 2016-05-11 11:33:11]

Description was changed from

==========
Don't store an SkPicture in the SourceGraphic FilterEffect

Instead of storing the SkPicture in the SourceGraphic FilterEffect, just
create a filter and pre-populate all the image filter "slots" when we've
recorded the content that should be filtered.

BUG=610158
==========

to

==========
Don't store an SkPicture in the SourceGraphic FilterEffect

Instead of storing the SkPicture in the SourceGraphic FilterEffect, just
create a filter and pre-populate all the image filter "slots" when we've
recorded the content that should be filtered.
This avoids keeping an explicit reference to the SkPicture, and thus
avoids keeping this object alive when the Filter and it's associated
filter-chain is in limbo waiting for a Oilpan GC sweep.

BUG=610158
==========

[fs, 2016-05-11 17:08:12]

https://codereview.chromium.org/1961083006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp
(right):

https://codereview.chromium.org/1961083006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp:57:
FloatRect FilterEffect::determineAbsolutePaintRect(const FloatRect&
originalRequestedRect)
(Tangent: Do we still need this mechanism, or could we rely on Skia now?)

[fs, 2016-05-12 12:23:51]

On 2016/05/11 at 10:54:23, fs wrote:
>
https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp
(right):
> 
>
https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp:59:
ASSERT_NOT_REACHED();
> On 2016/05/10 at 18:24:02, Stephen White wrote:
> > I think this is problematic because there are some valid cases where we
don't have an SkPicture (and that createImageFilter() should return null):
accelerated CSS filters, and canvas filters.
> > 
> > But it should be ok to just remove the assert and return nullptr here.
> 
> Yes, doing that would reflect the old behavior better... so done.

Ping.

[Stephen White, 2016-05-12 15:47:03]

On 2016/05/12 12:23:51, fs wrote:
> On 2016/05/11 at 10:54:23, fs wrote:
> >
>
https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/p...
> > File third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/p...
> > third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp:59:
> ASSERT_NOT_REACHED();
> > On 2016/05/10 at 18:24:02, Stephen White wrote:
> > > I think this is problematic because there are some valid cases where we
> don't have an SkPicture (and that createImageFilter() should return null):
> accelerated CSS filters, and canvas filters.
> > > 
> > > But it should be ok to just remove the assert and return nullptr here.
> > 
> > Yes, doing that would reflect the old behavior better... so done.
> 
> Ping.

LGTM

Sorry, I don't seem to be getting review notification emails.

[Stephen White, 2016-05-12 15:49:19]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2016-05-12 15:49:30]

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

[commit-bot: I haz the power, 2016-05-12 17:27:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-12 17:29:28]

Description was changed from

==========
Don't store an SkPicture in the SourceGraphic FilterEffect

Instead of storing the SkPicture in the SourceGraphic FilterEffect, just
create a filter and pre-populate all the image filter "slots" when we've
recorded the content that should be filtered.
This avoids keeping an explicit reference to the SkPicture, and thus
avoids keeping this object alive when the Filter and it's associated
filter-chain is in limbo waiting for a Oilpan GC sweep.

BUG=610158
==========

to

==========
Don't store an SkPicture in the SourceGraphic FilterEffect

Instead of storing the SkPicture in the SourceGraphic FilterEffect, just
create a filter and pre-populate all the image filter "slots" when we've
recorded the content that should be filtered.
This avoids keeping an explicit reference to the SkPicture, and thus
avoids keeping this object alive when the Filter and it's associated
filter-chain is in limbo waiting for a Oilpan GC sweep.

BUG=610158

Committed: https://crrev.com/589ab53f384e1e8a0df92eb868acf3ea35839a2e
Cr-Commit-Position: refs/heads/master@{#393280}
==========

[commit-bot: I haz the power, 2016-05-12 17:29:30]

Patchset 3 (id:??) landed as
https://crrev.com/589ab53f384e1e8a0df92eb868acf3ea35839a2e
Cr-Commit-Position: refs/heads/master@{#393280}

[Stephen White, 2016-05-12 18:42:30]

https://codereview.chromium.org/1961083006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp
(right):

https://codereview.chromium.org/1961083006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp:57:
FloatRect FilterEffect::determineAbsolutePaintRect(const FloatRect&
originalRequestedRect)
On 2016/05/11 17:08:11, fs wrote:
> (Tangent: Do we still need this mechanism, or could we rely on Skia now?)

We're almost there. The one remaining place that I know of where this
optimization is better than what Skia does is in FEComposite, where it reduces
the rect based on the compositing mode.

[fs, 2016-05-13 08:05:31]

On 2016/05/12 at 18:42:30, senorblanco wrote:
>
https://codereview.chromium.org/1961083006/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp
(right):
> 
>
https://codereview.chromium.org/1961083006/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp:57:
FloatRect FilterEffect::determineAbsolutePaintRect(const FloatRect&
originalRequestedRect)
> On 2016/05/11 17:08:11, fs wrote:
> > (Tangent: Do we still need this mechanism, or could we rely on Skia now?)
> 
> We're almost there. The one remaining place that I know of where this
optimization is better than what Skia does is in FEComposite, where it reduces
the rect based on the compositing mode.

Is there a bug, or should I file on? I'll file one regardless, and then we dupe
it...