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

Issue 2195893002: Always return ImageShader, even from SkShader::MakeBitmapShader (Closed)

Created:
4 years, 4 months ago by reed1
Modified:
4 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

Always return ImageShader, even from SkShader::MakeBitmapShader Lessons learned 1. ImageShader (correctly) always compresses (typically via PNG) during serialization. This has the surprise results of - if the image was marked opaque, but has some non-opaque pixels (i.e. bug in blitter or caller), then compressing may "fix" those pixels, making the deserialized version draw differently. bug filed. - 565 compressess/decompresses to 8888 (at least on Mac), which draws differently (esp. under some filters). bug filed. 2. BitmapShader did not enforce a copy for mutable bitmaps, but ImageShader does (since it creates an Image). Thus the former would see subsequent changes to the pixels after shader creation, while the latter does not, hence the change to the BlitRow test to avoid this modify-after-create pattern. I sure hope this prev. behavior was a bug/undefined-behavior, since this CL changes that. BUG=skia:5595 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2195893002 Committed: https://skia.googlesource.com/skia/+/320a40d7733979703bdf675c31108255e011e34e

Patch Set 1 #

Patch Set 2 : don't use bitmapprocshader #

Patch Set 3 : rm bitmapprocshader #

Patch Set 4 : update #

Patch Set 5 : adsf #

Patch Set 6 : rebase #

Patch Set 7 : update shadertest #

Patch Set 8 : make mesh/shader after we colorize the bitmap #

Patch Set 9 : update serialize-8888 ignore list: skbug.com/5595 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+137 lines, -301 lines) Patch
M include/core/SkShader.h View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M samplecode/SampleLighting.cpp View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M samplecode/SampleLitAtlas.cpp View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M src/core/SkBitmapProcShader.h View 1 2 3 4 2 chunks +1 line, -31 lines 1 comment Download
M src/core/SkBitmapProcShader.cpp View 1 2 3 4 5 3 chunks +4 lines, -235 lines 1 comment Download
M src/core/SkGlobalInitialization_core.cpp View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/image/SkImageShader.h View 1 2 3 4 5 3 chunks +5 lines, -2 lines 1 comment Download
M src/image/SkImageShader.cpp View 1 2 3 4 5 4 chunks +103 lines, -6 lines 0 comments Download
M tests/BlitRowTest.cpp View 1 2 3 4 5 6 7 2 chunks +5 lines, -10 lines 0 comments Download
M tests/ShaderTest.cpp View 1 2 3 4 5 6 3 chunks +9 lines, -12 lines 0 comments Download
M tools/dm_flags.py View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 36 (27 generated)
reed1
I *think* I need to keep some of bitmapprocshader around, so we have a CreateProc ...
4 years, 4 months ago (2016-07-29 22:39:25 UTC) #4
reed1
4 years, 4 months ago (2016-07-29 22:39:47 UTC) #6
reed1
motivated by earlier CLs (and future ones) where we unify around always recording/serializing images, not ...
4 years, 4 months ago (2016-07-29 22:40:28 UTC) #7
reed1
ptal
4 years, 4 months ago (2016-08-01 23:57:25 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2195893002/150001
4 years, 4 months ago (2016-08-02 02:02:15 UTC) #30
f(malita)
Cool! LGTM https://codereview.chromium.org/2195893002/diff/150001/src/core/SkBitmapProcShader.h File src/core/SkBitmapProcShader.h (right): https://codereview.chromium.org/2195893002/diff/150001/src/core/SkBitmapProcShader.h#newcode16 src/core/SkBitmapProcShader.h:16: class SkBitmapProcLegacyShader : public SkShader { This ...
4 years, 4 months ago (2016-08-02 13:10:28 UTC) #31
commit-bot: I haz the power
Committed patchset #9 (id:150001) as https://skia.googlesource.com/skia/+/320a40d7733979703bdf675c31108255e011e34e
4 years, 4 months ago (2016-08-02 13:12:11 UTC) #33
f(malita)
On 2016/08/02 13:12:11, commit-bot: I haz the power wrote: > Committed patchset #9 (id:150001) as ...
4 years, 4 months ago (2016-08-02 13:14:35 UTC) #34
scroggo
4 years, 3 months ago (2016-09-08 13:42:24 UTC) #36
Message was sent while issue was closed.
> 2. BitmapShader did not enforce a copy for mutable bitmaps, but ImageShader
does (since it creates an Image). Thus the former would see subsequent changes
to the pixels after shader creation, while the latter does not, hence the change
to the BlitRow test to avoid this modify-after-create pattern. I sure hope this
prev. behavior was a bug/undefined-behavior, since this CL changes that.


"this prev. behavior" = "see subsequent changes to the pixels after shader
creation"?

We had a bug tracking that : about.com/3157. There was some concern at the time
about fixing it cheaply.

https://codereview.chromium.org/2195893002/diff/150001/src/core/SkBitmapProcS...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/2195893002/diff/150001/src/core/SkBitmapProcS...
src/core/SkBitmapProcShader.cpp:207: return s;
This is effectively the same as the old code, right? Either way, we don't need
both.

Powered by Google App Engine
This is Rietveld 408576698