Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2223203003:
Exact Ganesh Gradients for Special Cases (Closed)
DescriptionAdd specialized code for hard stop gradients on GPU
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003
Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d
Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b
Committed: https://skia.googlesource.com/skia/+/2a4959181fc98d5d7ee862e7cd1c7993b3343be6
Committed: https://skia.googlesource.com/skia/+/cd9a1d0ec3995ffa0be37fd7f6ec1e194b416818
Patch Set 1 #Patch Set 2 : Move hard stop code into proper #if block #
Total comments: 6
Patch Set 3 : Brian's suggestions #Patch Set 4 : Remove calls to setReserve() #
Total comments: 2
Patch Set 5 : Trim columns, remove unused functions, re-align enum fields #
Total comments: 2
Patch Set 6 : Simplify emitUniforms(), whitespace nit #Patch Set 7 : Rename to determineColorType(), add #if SK_SUPPORT_GPU #Patch Set 8 : Builds when SK_SUPPORT_GPU is off #Patch Set 9 : Fix unused function error #Patch Set 10 : Remove unused field fSubGradients, fix valgrind 'uninitialized value' bug #
Messages
Total messages: 48 (24 generated)
Description was changed from ========== Remove generalized gradient code BUG=skia: ========== to ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 ==========
fmenozzi@google.com changed reviewers: + bsalomon@google.com
This is the rest of the previous CL, without the generalized subgradient code. ptal
Description was changed from ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 ========== to ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 ==========
https://codereview.chromium.org/2223203003/diff/20001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/2223203003/diff/20001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:231: static inline bool close_to_one_half(const SkFixed& val) { I think we have something like SkScalarNearlyEqual https://codereview.chromium.org/2223203003/diff/20001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:946: case GrGradientEffect::kHardStopCentered_ColorType: Any reason to now always use a uniform array for colors (expect in the texture case)? Could have a function that converts the ColorType enum to a color count and much of the code could be unified. https://codereview.chromium.org/2223203003/diff/20001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:1337: fAllColors.setReserve(shader.fColorCount); Maybe just call this fColors? https://codereview.chromium.org/2223203003/diff/20001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:1338: fAllColors = SkTDArray<SkColor>(shader.fOrigColors, shader.fColorCount); Does this do a move?
Added suggestions, ptal https://codereview.chromium.org/2223203003/diff/20001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/2223203003/diff/20001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:1338: fAllColors = SkTDArray<SkColor>(shader.fOrigColors, shader.fColorCount); On 2016/08/09 19:46:23, bsalomon wrote: > Does this do a move? I don't think so; the ctor for SkTDArray that takes an array and a count just does a memcpy on the underlying data.
https://codereview.chromium.org/2223203003/diff/20001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/2223203003/diff/20001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:1338: fAllColors = SkTDArray<SkColor>(shader.fOrigColors, shader.fColorCount); On 2016/08/09 20:45:49, fmenozzi wrote: > On 2016/08/09 19:46:23, bsalomon wrote: > > Does this do a move? > > I don't think so; the ctor for SkTDArray that takes an array and a count just > does a memcpy on the underlying data. It looks like this will call SkTDArray<T>& operator=(SkTDArray<T>&& src) which will move the right side into fAllColors by swapping the two arrays. So that is good. However, that means that the allocation used by setReserved will be freed when the temporary goes out of scope. So I think this would be more efficient if the the setReserved calls are removed (save a call to malloc and free).
Removed calls to setReserve(), ptal
There are a bunch of lines that exceed 100 cols https://codereview.chromium.org/2223203003/diff/60001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/2223203003/diff/60001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:989: static inline void set_after_interp_color_uni(const GrGLSLProgramDataManager& pdman, Are these two functions used anymore? https://codereview.chromium.org/2223203003/diff/60001/src/effects/gradients/S... File src/effects/gradients/SkGradientShaderPriv.h (right): https://codereview.chromium.org/2223203003/diff/60001/src/effects/gradients/S... src/effects/gradients/SkGradientShaderPriv.h:465: kClampTileMode = 16, Can you preserve the alignment of the values?
ptal
lgtm w/ one whitespace nit and optional suggestion. https://codereview.chromium.org/2223203003/diff/80001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/2223203003/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:966: switch (ge.getColorType()) { Could this whole thing become: if (int colorCount = color_type_to_color_count(ge.getColorType())) { fColorsUni = uniformHandler->addUniformArray(kFragment_GrShaderFlag, ... } else { fFSYUni = uniformHandler->addUniform(kFragment_GrShaderFlag, ... } https://codereview.chromium.org/2223203003/diff/80001/src/effects/gradients/S... File src/effects/gradients/SkGradientShaderPriv.h (right): https://codereview.chromium.org/2223203003/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShaderPriv.h:454: kPremulBeforeInterpKey = 1, missing space before 1?
The CQ bit was checked by fmenozzi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/2223203003/#ps100001 (title: "Simplify emitUniforms(), whitespace nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 ========== to ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2227373004/ by fmenozzi@google.com. The reason for reverting is: Certain sweep gradients are breaking with certain t-values. Currently investigating..
Message was sent while issue was closed.
Description was changed from ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d ========== to ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d ==========
Fixed paren bug, made one other minor change (add #if SK_SUPPORT_GPU). ptal
lgtm but can you make sure it compiles when gyp var skia_gpu is set to 0 if you haven't already?
The CQ bit was checked by fmenozzi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/2223203003/#ps140001 (title: "Builds when SK_SUPPORT_GPU is off")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d ========== to ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2241483003/ by halcanary@google.com. The reason for reverting is: Chrome iOS build break https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/build... error: unused function 'close_to_one_half'.
Message was sent while issue was closed.
Description was changed from ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b ========== to ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b ==========
The CQ bit was checked by fmenozzi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/2223203003/#ps160001 (title: "Fix unused function error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b ========== to ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b Committed: https://skia.googlesource.com/skia/+/2a4959181fc98d5d7ee862e7cd1c7993b3343be6 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://skia.googlesource.com/skia/+/2a4959181fc98d5d7ee862e7cd1c7993b3343be6
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2245533005/ by halcanary@google.com. The reason for reverting is: specualtive revert to fix valgrind bot Perf-Ubuntu-GCC-ShuttleA-GPU-GTX550Ti-x86_64-Release-Valgrind.
Message was sent while issue was closed.
Description was changed from ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b Committed: https://skia.googlesource.com/skia/+/2a4959181fc98d5d7ee862e7cd1c7993b3343be6 ========== to ========== Remove generalized gradient code BUG=skia: CQ_INCLUDE_TRYBOTS=tryserver.skia:Build-Mac10.9-Clang-x86_64-Debug GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b Committed: https://skia.googlesource.com/skia/+/2a4959181fc98d5d7ee862e7cd1c7993b3343be6 ==========
Message was sent while issue was closed.
Description was changed from ========== Remove generalized gradient code BUG=skia: CQ_INCLUDE_TRYBOTS=tryserver.skia:Build-Mac10.9-Clang-x86_64-Debug GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b Committed: https://skia.googlesource.com/skia/+/2a4959181fc98d5d7ee862e7cd1c7993b3343be6 ========== to ========== Remove generalized gradient code BUG=skia: CQ_INCLUDE_TRYBOTS=tryserver.skia:Build-Mac10.9-Clang-x86_64-Debug,tryserver.blink:linux_precise_blink_rel GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b Committed: https://skia.googlesource.com/skia/+/2a4959181fc98d5d7ee862e7cd1c7993b3343be6 ==========
Message was sent while issue was closed.
fmenozzi@google.com changed reviewers: + tomhudson@google.com
Message was sent while issue was closed.
lgtm
The CQ bit was checked by fmenozzi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/2223203003/#ps180001 (title: "Remove unused field fSubGradients, fix valgrind 'uninitialized value' bug")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Your CL can not be processed by CQ because of: * Failed to parse additional trybots
Description was changed from ========== Remove generalized gradient code BUG=skia: CQ_INCLUDE_TRYBOTS=tryserver.skia:Build-Mac10.9-Clang-x86_64-Debug,tryserver.blink:linux_precise_blink_rel GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b Committed: https://skia.googlesource.com/skia/+/2a4959181fc98d5d7ee862e7cd1c7993b3343be6 ========== to ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b Committed: https://skia.googlesource.com/skia/+/2a4959181fc98d5d7ee862e7cd1c7993b3343be6 ==========
Description was changed from ========== Remove generalized gradient code BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b Committed: https://skia.googlesource.com/skia/+/2a4959181fc98d5d7ee862e7cd1c7993b3343be6 ========== to ========== Add specialized code for hard stop gradients on GPU BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b Committed: https://skia.googlesource.com/skia/+/2a4959181fc98d5d7ee862e7cd1c7993b3343be6 ==========
The CQ bit was checked by fmenozzi@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add specialized code for hard stop gradients on GPU BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b Committed: https://skia.googlesource.com/skia/+/2a4959181fc98d5d7ee862e7cd1c7993b3343be6 ========== to ========== Add specialized code for hard stop gradients on GPU BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2223203003 Committed: https://skia.googlesource.com/skia/+/0c63006b88a16e3418d92852a62771615799839d Committed: https://skia.googlesource.com/skia/+/99818d69372d29a139935cfe5c379e491432931b Committed: https://skia.googlesource.com/skia/+/2a4959181fc98d5d7ee862e7cd1c7993b3343be6 Committed: https://skia.googlesource.com/skia/+/cd9a1d0ec3995ffa0be37fd7f6ec1e194b416818 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://skia.googlesource.com/skia/+/cd9a1d0ec3995ffa0be37fd7f6ec1e194b416818 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
