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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 221923007: Initial picture shader implementation (Closed)

Created:
6 years, 8 months ago by f(malita)
Modified:
6 years, 8 months ago
CC:
skia-review_googlegroups.com
Visibility:
Public.

Description

Initial picture shader implementation This CL adds an SkPictureShader class to support SkPicture-based patterns. The implementation renders the picture into an SkBitmap tile and then delegates to SkBitmapProcShader for the actual operation. R=reed@google.com,robertphillips@google.com,bsalomon@google.com Committed: http://code.google.com/p/skia/source/detail?r=14085 Committed: http://code.google.com/p/skia/source/detail?r=14092

Patch Set 1 #

Patch Set 2 : Win build fix + SkShader factory. #

Total comments: 34

Patch Set 3 : #

Patch Set 4 : Updated per comments, #

Patch Set 5 : Use SVD for tile scaling. #

Total comments: 4

Patch Set 6 : Updated per review. #

Patch Set 7 : Register flattenables. #

Patch Set 8 : Remove unneeded suppression. #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+434 lines, -4 lines) Patch
M expectations/gm/ignored-tests.txt View 1 2 3 4 5 6 7 1 chunk +0 lines, -4 lines 0 comments Download
A gm/pictureshader.cpp View 1 2 3 4 5 1 chunk +165 lines, -0 lines 0 comments Download
M gyp/core.gypi View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M gyp/gmslides.gypi View 1 chunk +1 line, -0 lines 0 comments Download
M include/core/SkShader.h View 1 2 3 4 5 2 chunks +11 lines, -0 lines 2 comments Download
A src/core/SkPictureShader.h View 1 2 1 chunk +60 lines, -0 lines 0 comments Download
A src/core/SkPictureShader.cpp View 1 2 3 4 1 chunk +185 lines, -0 lines 2 comments Download
M src/core/SkShader.cpp View 1 2 3 4 5 2 chunks +6 lines, -0 lines 0 comments Download
M src/ports/SkGlobalInitialization_chromium.cpp View 1 2 3 4 5 6 2 chunks +2 lines, -0 lines 0 comments Download
M src/ports/SkGlobalInitialization_default.cpp View 1 2 3 4 5 6 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
f(malita)
6 years, 8 months ago (2014-04-02 17:04:24 UTC) #1
robertphillips
lgtm + nits https://codereview.chromium.org/221923007/diff/20001/gm/pictureshader.cpp File gm/pictureshader.cpp (right): https://codereview.chromium.org/221923007/diff/20001/gm/pictureshader.cpp#newcode19 gm/pictureshader.cpp:19: Should we also test some of ...
6 years, 8 months ago (2014-04-02 18:15:35 UTC) #2
reed1
lets look at SVD in matrix, to see if it applies here as well. https://codereview.chromium.org/221923007/diff/20001/src/core/SkPictureShader.cpp ...
6 years, 8 months ago (2014-04-07 14:03:15 UTC) #3
f(malita)
https://codereview.chromium.org/221923007/diff/20001/gm/pictureshader.cpp File gm/pictureshader.cpp (right): https://codereview.chromium.org/221923007/diff/20001/gm/pictureshader.cpp#newcode19 gm/pictureshader.cpp:19: On 2014/04/02 18:15:35, robertphillips wrote: > Should we also ...
6 years, 8 months ago (2014-04-07 15:06:23 UTC) #4
f(malita)
On 2014/04/07 14:03:15, reed1 wrote: > lets look at SVD in matrix, to see if ...
6 years, 8 months ago (2014-04-07 15:27:30 UTC) #5
reed1
lgtm w/ nits https://codereview.chromium.org/221923007/diff/70001/include/core/SkShader.h File include/core/SkShader.h (right): https://codereview.chromium.org/221923007/diff/70001/include/core/SkShader.h#newcode351 include/core/SkShader.h:351: * @param src The picture to ...
6 years, 8 months ago (2014-04-07 15:30:09 UTC) #6
f(malita)
https://codereview.chromium.org/221923007/diff/70001/include/core/SkShader.h File include/core/SkShader.h (right): https://codereview.chromium.org/221923007/diff/70001/include/core/SkShader.h#newcode351 include/core/SkShader.h:351: * @param src The picture to use inside the ...
6 years, 8 months ago (2014-04-07 15:45:28 UTC) #7
f(malita)
The CQ bit was checked by fmalita@chromium.org
6 years, 8 months ago (2014-04-07 21:51:07 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/221923007/90001
6 years, 8 months ago (2014-04-07 21:51:14 UTC) #9
commit-bot: I haz the power
Change committed as 14085
6 years, 8 months ago (2014-04-07 23:11:50 UTC) #10
benchen
A revert of this CL has been created in https://codereview.chromium.org/227553010/ by bensong@google.com. The reason for ...
6 years, 8 months ago (2014-04-07 23:45:08 UTC) #11
f(malita)
On 2014/04/07 23:45:08, benchen wrote: > A revert of this CL has been created in ...
6 years, 8 months ago (2014-04-08 14:17:51 UTC) #12
f(malita)
The CQ bit was checked by fmalita@chromium.org
6 years, 8 months ago (2014-04-08 14:21:53 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/221923007/130001
6 years, 8 months ago (2014-04-08 14:22:01 UTC) #14
commit-bot: I haz the power
Change committed as 14092
6 years, 8 months ago (2014-04-08 15:20:07 UTC) #15
scroggo
Drive by comments, as I look at changes to SkPictureShader in https://codereview.chromium.org/207683004 https://codereview.chromium.org/221923007/diff/130001/include/core/SkShader.h File include/core/SkShader.h ...
6 years, 8 months ago (2014-04-11 15:23:15 UTC) #16
f(malita)
6 years, 8 months ago (2014-04-11 16:05:17 UTC) #17
Message was sent while issue was closed. 
https://codereview.chromium.org/221923007/diff/130001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/221923007/diff/130001/include/core/SkShader.h...
include/core/SkShader.h:351: *  @param src  The picture to use inside the shader
(if not NULL, its ref count
On 2014/04/11 15:23:15, scroggo wrote:
> Is it part of the intended use case to allow the caller to continue to modify
> the picture? I'm looking at this with an eye towards immutability of shaders.
If
> we don't want the client to be able to modify it, we could make this take
> ownership of the picture instead of reffing it. (And when we split the picture
> API into picture record and picture playback, this could take a playback.)

The intent is to treat the picture (playback) as immutable. If we can adjust the
API to better reflect this, SGTM.

https://codereview.chromium.org/221923007/diff/130001/src/core/SkPictureShade...
File src/core/SkPictureShader.cpp (right):

https://codereview.chromium.org/221923007/diff/130001/src/core/SkPictureShade...
src/core/SkPictureShader.cpp:59: if (!fPicture || (0 == fPicture->width() && 0
== fPicture->height())) {
On 2014/04/11 15:23:15, scroggo wrote:
> It appears that if the picture is NULL or it has zero width/height, this
shader
> never does anything. What if the factory just returned NULL in that case? Then
> we can skip these checks (possibly replacing them with asserts).

Good point. After making the constructor private, there's no need to keep these
checks around.

Powered by Google App Engine
This is Rietveld 408576698