Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1436663003:
Implement multi-color-stops in linear gradients using Sk4f (Closed)
DescriptionImplement multi-color-stops in linear gradients using Sk4f
#define SK_SUPPORT_LEGACY_LINEAR_GRADIENT_TABLE to restore the old behavior
BUG=skia:517
Committed: https://skia.googlesource.com/skia/+/f3182ebc72db2bf2e24119d5cea05f270473a491
Patch Set 1 #Patch Set 2 : #
Total comments: 1
Patch Set 3 : #Patch Set 4 : handles alpha, still operates in premul only" #Patch Set 5 : unroll inner loop, 18 -> 14 ms #Patch Set 6 : quad unroll helped. Still need unpremul and dx < 0 #Patch Set 7 : reorg to have struct per color-stop #Patch Set 8 : handle premul and unpremul #Patch Set 9 : add SK_SUPPORT_LEGACY_LINEAR_GRADIENT_TABLE flag #Patch Set 10 : #
Total comments: 30
Patch Set 11 : use explicit storage type since Sk4f is too tricky for that #Patch Set 12 : don't pass Sk4f as a parameter type #Patch Set 13 : combine neg and pos procs #Patch Set 14 : faster one-shot finders (forward, backward) #Patch Set 15 : #Patch Set 16 : clean up, add more comments #Patch Set 17 : fix win warnings #
Messages
Total messages: 37 (17 generated)
reed@google.com changed reviewers: + fmalita@chromium.org
reed@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1436663003/diff/20001/src/effects/gradients/S... File src/effects/gradients/SkLinearGradient.cpp (right): https://codereview.chromium.org/1436663003/diff/20001/src/effects/gradients/S... src/effects/gradients/SkLinearGradient.cpp:713: if (pos[prevIndex] > tiledX || tiledX > pos[prevIndex + 1]) { What if instead of projecting the span pixels onto the gradient vector (t space?), we map the gradient colors/stops onto the span vector? I think that would mean 1) we know exactly how many span pixels we can shade before we hit the next stop 2) for a given interval, the color differential is constant (and the division can be hoisted out of the inner loop) Basically my initial/uneducated float gradient idea looks like this: while (span not done) { interval = nextInterval(); // magic method returning interval a) start color, b) end color, c) len (mapped onto the current span vector, capped to count) // interval color gradient dC = (interval.endColor - interval.startColor) / interval.len; c = interval.startColor; foreach (span pixel in interval) { dstC[i++] = c; c += dC; } }
reed@google.com changed reviewers: + caryclark@google.com
Description was changed from ========== experiment w/ multi-color-stops and gradients using Sk4f BUG=skia: ========== to ========== experiment w/ multi-color-stops and gradients using Sk4f #define SK_SUPPORT_LEGACY_LINEAR_GRADIENT_TABLE to restore the old behavior BUG=skia: ==========
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436663003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436663003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
ptal
Description was changed from ========== experiment w/ multi-color-stops and gradients using Sk4f #define SK_SUPPORT_LEGACY_LINEAR_GRADIENT_TABLE to restore the old behavior BUG=skia: ========== to ========== Implement multi-color-stops in linear gradients using Sk4f #define SK_SUPPORT_LEGACY_LINEAR_GRADIENT_TABLE to restore the old behavior BUG=skia:517 ==========
lgtm My comments are mostly stream-of-consciousness nits -- not actionable, feel free to ignore if they irritate you https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... File src/effects/gradients/SkLinearGradient.cpp (right): https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:10: static const float kInv255Float = 1.0f / 255; This is a popular pattern: src/effects/SkColorMatrixFilter.cpp:449: src/effects/SkTableMaskFilter.cpp:88 include/gpu/GrColor.h:125: include/gpu/GrColor.h:134: src/gpu/GrFragmentProcessor.cpp:240: src/gpu/effects/GrConstColorProcessor.cpp:50 src/gpu/gl/GrGLGpu.cpp:1705: I wonder if it's worth having in one place (with one name)? https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:116: rec[0].fPosScale = 0; // should never get used SkDEBUGCODE() ? Assign SK_ScalarNaN instead? https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:118: rec[i].fPos = SkTMin(SkTMax(rec[i - 1].fPos, shader.fOrigPos[i]), 1.0f); SkASSERT(rec[i - 1].fPos <= rec[i].fPos); (see comment below) https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:121: rec[count - 1].fPos = 1; // overwrite the last value just to be sure we end at 1.0 This seems odd -- is it OK for the last iteration of the loop to derive fPosScale from the value that gets overridden here? https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:121: rec[count - 1].fPos = 1; // overwrite the last value just to be sure we end at 1.0 SkASSERT(rec[count - 2].fPos <= rec[count - 1].fPos); (see comment below) https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:135: } fApplyAlphaAfterInterp = !(shader.getGradFlags() & SkGradientShader::kInterpolateColorsInPremul_Flag) && !shader.colorsAreOpaque() ? https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:144: SkASSERT(rec[i - 1].fPos <= rec[i].fPos); this assert looks like it would be more at home above where fPos is computed (see prior comment) https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:154: SkASSERT(rec[i - 1].fPos <= rec[i].fPos); Ditto https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:659: } Is this array ever so large that it's worth a non-linear search? https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:710: *dstC++ = trunc_from_255<apply_alpha>(cd3); probably not worth it, but if (n >= 4) { do { *dstC++ ... x 4 cd0 = ... cd1 = ... if ((n -= 4) <= 0) { break; } cd2 = ... cd3 = ... } while (true); } might shave off a teeny bit https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:720: cd0 = cd0 + dc2; move this into the condition below? https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:756: const Rec* r = find(fx, rec, fRecs.count()); add const float p0 = r[0].fPos; to mirror code below? https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:760: const float t = (fx - r[0].fPos) * scale; const float t = (fx - p0) * scale; https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:766: int n = SkTMin((int)((p1-fx)*invDx) + 1, count); Is this the same as the SkFloatToIntFloor in the < 0 condition above? https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:771: SkASSERT(count >= 0); may be worth changing either the <0 condition or this to keep the order of the lines the same to make it slightly easier to read and maintain https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:808: fill<apply_alpha>(dstC, count, rec[0].fColor + dither0, rec[0].fColor + dither1); maybe Sk4f color = rec[0].fColor; fill<apply_alpha>(dstC, count, color + dither0, color + dither1); just to mirror the code above https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:856: }; Out of curiosity, could this be 1.0f/8, 5.0f/8, 7.0f/8, 3.0f/8, }; to remove the need for the comment? https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:867: fx = SkTMin(SkTMax(fx, 0.0f), 1.0f); SkTPin(fx, 0.0f, 1.0f)
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436663003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436663003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
inner loop add + store => lgtm :) https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... File src/effects/gradients/SkLinearGradient.cpp (right): https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:684: * - optimizations How does current perf compare to existing impl? https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:756: const Rec* r = find(fx, rec, fRecs.count()); Since we're peeking at r[1] below, I would SkASSERT(r < rec + fRecs.count() - 1) https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:783: template <bool apply_alpha>void SkLinearGradient::LinearGradientContext::shade4_neg_clamp( shade4_neg_clamp & shade4_pos_clamp look pretty similar - is (moar) templateing feasible?
https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... File src/effects/gradients/SkLinearGradient.cpp (right): https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:116: rec[0].fPosScale = 0; // should never get used On 2015/11/16 14:44:29, caryclark wrote: > SkDEBUGCODE() ? > Assign SK_ScalarNaN instead? Done. https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:659: } On 2015/11/16 14:44:29, caryclark wrote: > Is this array ever so large that it's worth a non-linear search? Could be (though my guess is it is usually between 2 and 6). My next step is to fold the searcher directly into the callers, since both callers (forward and backward) never need to search the entire list each time. https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:684: * - optimizations On 2015/11/16 15:16:20, f(malita) wrote: > How does current perf compare to existing impl? Slower, by 10-20% depending. https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:710: *dstC++ = trunc_from_255<apply_alpha>(cd3); On 2015/11/16 14:44:29, caryclark wrote: > probably not worth it, but > > if (n >= 4) { > do { > *dstC++ ... x 4 > cd0 = ... > cd1 = ... > if ((n -= 4) <= 0) { > break; > } > cd2 = ... > cd3 = ... > } while (true); > } > > might shave off a teeny bit Good observation. But it took me a full minute to understand how/why. I may time it, but only do it if it really pays off, as it is harder to parse (given that that pattern isn't used very much). https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:808: fill<apply_alpha>(dstC, count, rec[0].fColor + dither0, rec[0].fColor + dither1); On 2015/11/16 14:44:29, caryclark wrote: > maybe > > Sk4f color = rec[0].fColor; > fill<apply_alpha>(dstC, count, color + dither0, color + dither1); > > just to mirror the code above Done. https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:856: }; On 2015/11/16 14:44:29, caryclark wrote: > Out of curiosity, could this be > > 1.0f/8, 5.0f/8, > 7.0f/8, 3.0f/8, > }; > > to remove the need for the comment? Very good idea. Done. https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:867: fx = SkTMin(SkTMax(fx, 0.0f), 1.0f); On 2015/11/16 14:44:28, caryclark wrote: > SkTPin(fx, 0.0f, 1.0f) > Done.
https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... File src/effects/gradients/SkLinearGradient.cpp (right): https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:756: const Rec* r = find(fx, rec, fRecs.count()); On 2015/11/16 14:44:29, caryclark wrote: > add > const float p0 = r[0].fPos; > to mirror code below? Done. https://codereview.chromium.org/1436663003/diff/180001/src/effects/gradients/... src/effects/gradients/SkLinearGradient.cpp:756: const Rec* r = find(fx, rec, fRecs.count()); On 2015/11/16 15:16:20, f(malita) wrote: > Since we're peeking at r[1] below, I would SkASSERT(r < rec + fRecs.count() - 1) Done.
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436663003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436663003/260001
The CQ bit was unchecked by commit-bot@chromium.org
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...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436663003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436663003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436663003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436663003/320001
The CQ bit was unchecked by reed@google.com
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from caryclark@google.com, fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/1436663003/#ps320001 (title: "fix win warnings")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436663003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436663003/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://skia.googlesource.com/skia/+/f3182ebc72db2bf2e24119d5cea05f270473a491 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
