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

Issue 22854005: GPU Gradients (Closed)

Created:
7 years, 4 months ago by dierk
Modified:
7 years, 3 months ago
Reviewers:
scroggo, bsalomon
CC:
skia-review_googlegroups.com
Visibility:
Public.

Description

Two and three color GPU gradients without textures. R=bsalomon@google.com Committed: https://code.google.com/p/skia/source/detail?r=11158

Patch Set 1 #

Patch Set 2 : Created Separate pathways for 2 and 3 color gradients. #

Patch Set 3 : Stuck at the moment -- Issues with Uni asserts #

Patch Set 4 : Moved mode toggling back to base class, fixed EffectKey masking. #

Total comments: 26

Patch Set 5 : Fixes from code reviews #

Total comments: 7

Patch Set 6 : 2 Color gpu gradients working, 3 color working with middle color bug. #

Total comments: 4

Patch Set 7 : Fixed 3 color bug (except for Sweep) and onIsEquals fn #

Patch Set 8 : Using texture method for 3 color sweeps. #

Total comments: 4

Patch Set 9 : Fixed onIsEqual enum bug #

Patch Set 10 : Fixed 3 Color sweep bug #

Patch Set 11 : reverted defer flag to true #

Total comments: 11

Patch Set 12 : added .f to some floats #

Patch Set 13 : Added Mike's Suggestions #

Total comments: 2

Patch Set 14 : Added Brian's Suggestions #

Patch Set 15 : trying to fix git issues #

Patch Set 16 : added new gm file #

Patch Set 17 : fixed gm file by forcing it to make another #

Patch Set 18 : Fix gr->sk assert #

Patch Set 19 : Bring to TOT, handle premul/unpremul #

Patch Set 20 : Fix alpha premul, add test cases, rename GM #

Patch Set 21 : Fix subclass key gen #

Patch Set 22 : remove dead line of code #

Unified diffs Side-by-side diffs Delta from patch set Stats (+464 lines, -101 lines) Patch
A gm/gradients_no_texture.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +130 lines, -0 lines 0 comments Download
M gyp/gmslides.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M include/core/SkShader.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +0 lines, -1 line 0 comments Download
M samplecode/SampleApp.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -1 line 0 comments Download
M src/core/SkShader.cpp View 1 chunk +0 lines, -1 line 0 comments Download
M src/effects/gradients/SkGradientShader.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 4 chunks +236 lines, -64 lines 0 comments Download
M src/effects/gradients/SkGradientShaderPriv.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 4 chunks +74 lines, -12 lines 0 comments Download
M src/effects/gradients/SkLinearGradient.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +3 lines, -4 lines 0 comments Download
M src/effects/gradients/SkRadialGradient.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +3 lines, -3 lines 0 comments Download
M src/effects/gradients/SkSweepGradient.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +4 lines, -3 lines 0 comments Download
M src/effects/gradients/SkTwoPointConicalGradient.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 6 chunks +6 lines, -7 lines 0 comments Download
M src/effects/gradients/SkTwoPointRadialGradient.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +5 lines, -4 lines 0 comments Download
M src/gpu/GrContext.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 32 (0 generated)
dierk
Here's the latest update on GPU gradients. Intended to be tested with GM:gradients_gpu, and currently ...
7 years, 4 months ago (2013-08-13 00:29:20 UTC) #1
reed1
just nits -- leaving the meat of the review to brian https://codereview.chromium.org/22854005/diff/11001/samplecode/SampleApp.cpp File samplecode/SampleApp.cpp (right): ...
7 years, 4 months ago (2013-08-13 12:33:41 UTC) #2
bsalomon
Idea: The YCoord and NumColors are never both valid, they're mutually exclusive. Would it be ...
7 years, 4 months ago (2013-08-13 14:28:36 UTC) #3
bsalomon
One other thought... we already have a lot of gradient GMs. Can we adapt one ...
7 years, 4 months ago (2013-08-13 15:23:31 UTC) #4
dierk
https://codereview.chromium.org/22854005/diff/11001/gm/gradients_gpu.cpp File gm/gradients_gpu.cpp (right): https://codereview.chromium.org/22854005/diff/11001/gm/gradients_gpu.cpp#newcode110 gm/gradients_gpu.cpp:110: for (size_t j = 0; j < 1; ++j){ ...
7 years, 4 months ago (2013-08-13 19:50:17 UTC) #5
bsalomon
https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkGradientShader.cpp File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkGradientShader.cpp#newcode928 src/effects/gradients/SkGradientShader.cpp:928: //same as 2 color for now... causes bug from ...
7 years, 4 months ago (2013-08-13 20:40:03 UTC) #6
bsalomon
https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkGradientShader.cpp File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/22854005/diff/11001/src/effects/gradients/SkGradientShader.cpp#newcode1024 src/effects/gradients/SkGradientShader.cpp:1024: this->useAtlas() == s.useAtlas() && On 2013/08/13 19:50:17, dierk wrote: ...
7 years, 4 months ago (2013-08-13 21:07:52 UTC) #7
dierk
https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkGradientShaderPriv.h File src/effects/gradients/SkGradientShaderPriv.h (right): https://codereview.chromium.org/22854005/diff/21001/src/effects/gradients/SkGradientShaderPriv.h#newcode319 src/effects/gradients/SkGradientShaderPriv.h:319: kColorKeyMask = kThreeColorKey On 2013/08/13 21:07:52, bsalomon wrote: > ...
7 years, 4 months ago (2013-08-14 21:33:15 UTC) #8
bsalomon
This is coming along well. Other than the imcomplete isEqual() part it looks right to ...
7 years, 4 months ago (2013-08-15 13:16:42 UTC) #9
dierk
Fixed color bug for 3 color gradients, but it's still drawing wrong for Sweep gradients. ...
7 years, 4 months ago (2013-08-15 18:18:47 UTC) #10
dierk
Reverted 3 color sweeps back to texture-lookup method (so I can land this)
7 years, 4 months ago (2013-08-15 18:50:44 UTC) #11
bsalomon
I'm a little nervous about just disabling sweep in case this is masking some other ...
7 years, 4 months ago (2013-08-15 19:21:18 UTC) #12
bsalomon
https://codereview.chromium.org/22854005/diff/51001/src/effects/gradients/SkGradientShader.cpp File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/22854005/diff/51001/src/effects/gradients/SkGradientShader.cpp#newcode1058 src/effects/gradients/SkGradientShader.cpp:1058: if (2 == this->fColorType) { Is this the bug? ...
7 years, 4 months ago (2013-08-15 19:25:29 UTC) #13
dierk
https://codereview.chromium.org/22854005/diff/51001/src/effects/gradients/SkGradientShader.cpp File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/22854005/diff/51001/src/effects/gradients/SkGradientShader.cpp#newcode957 src/effects/gradients/SkGradientShader.cpp:957: builder->fsCodeAppendf("\tvec4 colorTemp = clamp(oneMinus2t, 0, 1) * %s +(1 ...
7 years, 4 months ago (2013-08-15 19:56:51 UTC) #14
dierk
7 years, 4 months ago (2013-08-15 21:20:05 UTC) #15
reed1
https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkGradientShader.cpp File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkGradientShader.cpp#newcode1056 src/effects/gradients/SkGradientShader.cpp:1056: if (this->fColorType == s.getColorType()){ nit: Skia doesn't use this-> ...
7 years, 4 months ago (2013-08-15 21:32:15 UTC) #16
bsalomon
A couple minor comments, otherwise looks good. https://codereview.chromium.org/22854005/diff/77001/src/effects/gradients/SkGradientShaderPriv.h File src/effects/gradients/SkGradientShaderPriv.h (right): https://codereview.chromium.org/22854005/diff/77001/src/effects/gradients/SkGradientShaderPriv.h#newcode261 src/effects/gradients/SkGradientShaderPriv.h:261: kMaxAnalyticColors = ...
7 years, 4 months ago (2013-08-16 13:06:04 UTC) #17
bsalomon
One other thing: Let's not use "_gpu" in the name of the GM since the ...
7 years, 4 months ago (2013-08-16 14:39:00 UTC) #18
dierk
https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkGradientShader.cpp File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/22854005/diff/72001/src/effects/gradients/SkGradientShader.cpp#newcode1056 src/effects/gradients/SkGradientShader.cpp:1056: if (this->fColorType == s.getColorType()){ On 2013/08/15 21:32:15, reed1 wrote: ...
7 years, 4 months ago (2013-08-16 16:59:56 UTC) #19
bsalomon
lgtm
7 years, 4 months ago (2013-08-16 17:32:59 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/dierk@google.com/22854005/10001
7 years, 4 months ago (2013-08-16 17:53:04 UTC) #21
commit-bot: I haz the power
Retried try job too often on Build-Mac10.7-Clang-x86-Release-Trybot for step(s) BuildGm http://108.170.217.252:10117/buildstatus?builder=Build-Mac10.7-Clang-x86-Release-Trybot&number=416
7 years, 4 months ago (2013-08-16 18:07:18 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/dierk@google.com/22854005/10001
7 years, 4 months ago (2013-08-16 18:48:37 UTC) #23
commit-bot: I haz the power
Retried try job too often on Build-Mac10.7-Clang-x86-Release-Trybot for step(s) BuildGm http://108.170.217.252:10117/buildstatus?builder=Build-Mac10.7-Clang-x86-Release-Trybot&number=420
7 years, 4 months ago (2013-08-16 18:58:15 UTC) #24
dierk
7 years, 4 months ago (2013-08-16 19:44:58 UTC) #25
dierk
7 years, 4 months ago (2013-08-16 19:55:49 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/dierk@google.com/22854005/100001
7 years, 4 months ago (2013-08-16 20:16:54 UTC) #27
commit-bot: I haz the power
Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for step(s) BuildGm http://108.170.217.252:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Release-Trybot&number=387
7 years, 4 months ago (2013-08-16 20:26:30 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/dierk@google.com/22854005/104001
7 years, 4 months ago (2013-08-19 19:36:34 UTC) #29
commit-bot: I haz the power
Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for step(s) BuildGm http://173.255.115.253:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Release-Trybot&number=406
7 years, 4 months ago (2013-08-19 22:08:10 UTC) #30
scroggo
On 2013/08/19 22:08:10, I haz the power (commit-bot) wrote: > Retried try job too often ...
7 years, 4 months ago (2013-08-19 22:11:44 UTC) #31
bsalomon
7 years, 3 months ago (2013-09-09 15:36:39 UTC) #32
Message was sent while issue was closed.
Committed patchset #22 manually as r11158 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698