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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1342113002: add ImageShader, sharing code with its Bitmap cousin (Closed)

Created:
5 years, 3 months ago by reed1
Modified:
5 years, 3 months ago
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

add ImageShader, sharing code with its Bitmap cousin This is done by having abstracted the BitmapShaderContext to take a BitmapProvider, instead of just a bitmap. This allows us to share all of that code between SkBitmap and SkImage, since both are valid providers. It also means that we can simplify SkImage_Base to not need a virtual for onNewShader, since ALL images can uniformly be turned into a shader now. BUG=skia: Committed: https://skia.googlesource.com/skia/+/0b93e3149d2cb30860c51f9f3204ae811d9a97ca

Patch Set 1 #

Patch Set 2 : now with actual added files #

Total comments: 1

Patch Set 3 : #

Patch Set 4 : rebase w/ provider-based bitmapstate #

Patch Set 5 : rebase #

Patch Set 6 : #

Patch Set 7 : rebase #

Patch Set 8 : #

Total comments: 2

Patch Set 9 : fix copyright date #

Patch Set 10 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+193 lines, -40 lines) Patch
M gyp/core.gypi View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M src/image/SkImage.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download
A src/image/SkImageShader.h View 1 1 chunk +44 lines, -0 lines 0 comments Download
A src/image/SkImageShader.cpp View 1 2 3 4 5 6 7 8 1 chunk +145 lines, -0 lines 0 comments Download
M src/image/SkImage_Base.h View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -4 lines 0 comments Download
M src/image/SkImage_Generator.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +0 lines, -16 lines 0 comments Download
M src/image/SkImage_Gpu.h View 1 1 chunk +0 lines, -3 lines 0 comments Download
M src/image/SkImage_Gpu.cpp View 1 1 chunk +0 lines, -7 lines 0 comments Download
M src/image/SkImage_Raster.cpp View 1 2 3 4 5 6 2 chunks +0 lines, -9 lines 0 comments Download

Messages

Total messages: 33 (15 generated)
reed1
just want to review the raster portion at the moment
5 years, 3 months ago (2015-09-15 14:37:50 UTC) #2
f(malita)
Raster looks great! https://codereview.chromium.org/1342113002/diff/20001/include/core/SkShader.h File include/core/SkShader.h (right): https://codereview.chromium.org/1342113002/diff/20001/include/core/SkShader.h#newcode417 include/core/SkShader.h:417: bool computeTotalInverse(const ContextRec&, SkMatrix* totalInverse) const; ...
5 years, 3 months ago (2015-09-15 15:01:41 UTC) #3
reed1
ok, runs, but I need to solve the raster-image -> asTextureRef to make the shader ...
5 years, 3 months ago (2015-09-15 15:26:04 UTC) #5
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1342113002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1342113002/20001
5 years, 3 months ago (2015-09-15 15:26:13 UTC) #7
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot/builds/3171) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on ...
5 years, 3 months ago (2015-09-15 15:27:14 UTC) #9
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1342113002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1342113002/40001
5 years, 3 months ago (2015-09-15 17:21:38 UTC) #11
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 3 months ago (2015-09-15 17:29:29 UTC) #13
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1342113002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1342113002/60001
5 years, 3 months ago (2015-09-15 20:51:18 UTC) #15
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 3 months ago (2015-09-15 20:58:03 UTC) #17
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1342113002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1342113002/120001
5 years, 3 months ago (2015-09-16 20:16:44 UTC) #19
reed1
PTAL -- no GM diffs
5 years, 3 months ago (2015-09-16 20:19:03 UTC) #22
f(malita)
lgtm
5 years, 3 months ago (2015-09-17 14:41:14 UTC) #23
robertphillips
lgtm + nits https://codereview.chromium.org/1342113002/diff/140001/src/image/SkImageShader.cpp File src/image/SkImageShader.cpp (right): https://codereview.chromium.org/1342113002/diff/140001/src/image/SkImageShader.cpp#newcode1 src/image/SkImageShader.cpp:1: /* 2015 ? https://codereview.chromium.org/1342113002/diff/140001/src/image/SkImageShader.cpp#newcode114 src/image/SkImageShader.cpp:114: GrTextureParams::FilterMode ...
5 years, 3 months ago (2015-09-17 15:04:07 UTC) #24
reed1
Will attempt to re-factor (to a common base-class) to try to share gpu code, in ...
5 years, 3 months ago (2015-09-17 16:19:48 UTC) #25
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1342113002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1342113002/180001
5 years, 3 months ago (2015-09-18 15:20:08 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1342113002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1342113002/180001
5 years, 3 months ago (2015-09-18 15:25:38 UTC) #31
commit-bot: I haz the power
Committed patchset #10 (id:180001) as https://skia.googlesource.com/skia/+/0b93e3149d2cb30860c51f9f3204ae811d9a97ca
5 years, 3 months ago (2015-09-18 15:26:29 UTC) #32
tomhudson
5 years, 3 months ago (2015-09-18 16:36:02 UTC) #33
Message was sent while issue was closed. 
A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/1355863002/ by tomhudson@google.com.

The reason for reverting is: Failing ImageNewShaderTest on both Android (Tegra3
GPU) and iOS bots.

e.g.

/Users/chrome-bot/buildbot/skiabot-ipad4-000/build/slave/workdir/build/skia/tests/
	ImageNewShaderTest.cpp:24	0 == memcmp(bm1.getPixels(), bm2.getPixels(),
bm1.getSize())
	ImageNewShaderTest.cpp:95	0xFFFF0000 == bmt.getColor(0, y)
	ImageNewShaderTest.cpp:98	0xFFDEDEDE == bmt.getColor(x, y)
	ImageNewShaderTest.cpp:98	0xFFDEDEDE == bmt.getColor(x, y)
	ImageNewShaderTest.cpp:98	0xFFDEDEDE == bmt.getColor(x, y)
	ImageNewShaderTest.cpp:98	0xFFDEDEDE == bmt.getColor(x, y)
	ImageNewShaderTest.cpp:95	0xFFFF0000 == bmt.getColor(0, y)
...

.

Powered by Google App Engine
This is Rietveld 408576698