Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(538)

Issue 1961083006: Don't store an SkPicture in the SourceGraphic FilterEffect (Closed)

Created:
4 years, 7 months ago by fs
Modified:
4 years, 7 months ago
Reviewers:
Stephen White
CC:
blink-reviews, blink-reviews-paint_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney, slimming-paint-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Drop SourceGraphic::createImageFilter override #

Patch Set 3 : Drop clearResult call in addAbsolutePaintRect #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -37 lines) Patch
M third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp View 1 2 1 chunk +0 lines, -2 lines 2 comments Download
M third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.h View 2 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp View 3 chunks +30 lines, -14 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.h View 1 2 chunks +0 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp View 1 2 chunks +0 lines, -15 lines 0 comments Download

Messages

Total messages: 15 (5 generated)
Stephen White
https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp File third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp (right): https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp#newcode59 third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp:59: ASSERT_NOT_REACHED(); I think this is problematic because there are ...
4 years, 7 months ago (2016-05-10 18:24:02 UTC) #3
fs
https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp File third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp (right): https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp#newcode59 third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp:59: ASSERT_NOT_REACHED(); On 2016/05/10 at 18:24:02, Stephen White wrote: > ...
4 years, 7 months ago (2016-05-11 10:54:23 UTC) #4
fs
https://codereview.chromium.org/1961083006/diff/40001/third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp File third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp (right): https://codereview.chromium.org/1961083006/diff/40001/third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp#newcode57 third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp:57: FloatRect FilterEffect::determineAbsolutePaintRect(const FloatRect& originalRequestedRect) (Tangent: Do we still need ...
4 years, 7 months ago (2016-05-11 17:08:12 UTC) #6
fs
On 2016/05/11 at 10:54:23, fs wrote: > https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp > File third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp (right): > > https://codereview.chromium.org/1961083006/diff/1/third_party/WebKit/Source/platform/graphics/filters/SourceGraphic.cpp#newcode59 ...
4 years, 7 months ago (2016-05-12 12:23:51 UTC) #7
Stephen White
On 2016/05/12 12:23:51, fs wrote: > On 2016/05/11 at 10:54:23, fs wrote: > > > ...
4 years, 7 months ago (2016-05-12 15:47:03 UTC) #8
commit-bot: I haz the power
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
4 years, 7 months ago (2016-05-12 15:49:30 UTC) #10
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 7 months ago (2016-05-12 17:27:50 UTC) #11
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/589ab53f384e1e8a0df92eb868acf3ea35839a2e Cr-Commit-Position: refs/heads/master@{#393280}
4 years, 7 months ago (2016-05-12 17:29:30 UTC) #13
Stephen White
https://codereview.chromium.org/1961083006/diff/40001/third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp File third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp (right): https://codereview.chromium.org/1961083006/diff/40001/third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp#newcode57 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: ...
4 years, 7 months ago (2016-05-12 18:42:30 UTC) #14
fs
4 years, 7 months ago (2016-05-13 08:05:31 UTC) #15
Message was sent while issue was closed.
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...

Powered by Google App Engine
This is Rietveld 408576698