Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2343253002:
Support for color-spaces with multi-stop (texture) gradients (Closed)
|
Created:
4 years, 3 months ago by Brian Osman Modified:
4 years, 3 months ago CC:
reviews_skia.org Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSupport for color-spaces with multi-stop (texture) gradients
Texture is F16 linear, unless that's not supported. In that
case, we pack down to sRGB.
Added more test patches to the gamut GM with many stops,
to test this case. Now they render correctly.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343253002
Committed: https://skia.googlesource.com/skia/+/d454609f621471998edc382cdbd25bd2f05f0bde
Patch Set 1 #Patch Set 2 : New test code, fix buffer overrun #Patch Set 3 : Formatting cleanup #Patch Set 4 : Fix bugs: Always init color xform. Include xform in shader key. #Patch Set 5 : Bug fixes #
Total comments: 20
Patch Set 6 : Review feedback #
Total comments: 1
Patch Set 7 : SkASSERT -> SkDEBUGFAIL #
Messages
Total messages: 19 (10 generated)
Description was changed from ========== Support for color-spaces with multi-stop (texture) gradients BUG=skia: ========== to ========== Support for color-spaces with multi-stop (texture) gradients BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343253002 ==========
Description was changed from ========== Support for color-spaces with multi-stop (texture) gradients BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343253002 ========== to ========== Support for color-spaces with multi-stop (texture) gradients Texture is F16 linear, unless that's not supported. In that case, we pack down to sRGB. Added more test patches to the gamut GM with many stops, to test this case. Now they render correctly. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343253002 ==========
brianosman@google.com changed reviewers: + bsalomon@google.com, fmalita@chromium.org, reed@google.com
The CQ bit was checked by brianosman@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Added some review comments to call out decision making process and explain what's happening here a little better. Thoughts? https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:548: void SkGradientShaderBase::initLinearBitmap(SkBitmap* bitmap) const { This is essentially a condensed version of the logic from Build32bitCache (above), that can construct either an F16 or sRGB format bitmap containing the gradient strip, interpolated in premul or not. https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:674: // For these cases we use the bitmap cache, but not the GradientShaderCache. So just We talked about not using the bitmap cache (or bitmaps at all) in the new code path, but that would mean adding some other caching mechanism, to avoid re-building this data every frame. Long term, we could make a larger change (texture strip atlas takes an explicit key, along with callback function to construct the strip?), but for now it made sense to leverage the code we already had. https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:1421: GrGLSLColorSpaceXformHelper colorSpaceHelper(uniformHandler, ge.fColorSpaceXform.get(), I *could* have baked the xform into the texture, and that might make more sense. This method lets us reuse the cached texture for other destination spaces, but that's probably not a real common situation. The downside here is that we're applying the matrix on the GPU every time we draw. So yeah, maybe moving to destination gamut up front is better. Of course, in pathological cases like the many_stops GM (200 stops), then we're actually spending a lot of time on the CPU to prepare everything. Sigh.
https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:548: void SkGradientShaderBase::initLinearBitmap(SkBitmap* bitmap) const { On 2016/09/20 17:29:53, Brian Osman wrote: > This is essentially a condensed version of the logic from Build32bitCache > (above), that can construct either an F16 or sRGB format bitmap containing the > gradient strip, interpolated in premul or not. Kinda feels like we're reinventing a specialized version of Sk4fLinearGradient here... I wonder what it would take to just set up a context and invoke that code instead? (not necessarily in this iteration, just a thought) Also, is the legacy Build32bitCache going to be used going forward? I think we're going to switch SW gradients to the analytical path sooner or later, but maybe the GPU multi-stop impl will still use it for L32? If so, do you think we could merge these two at some point? https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:571: writeSizedPixel(Sk4f(c[0] * c[3], c[1] * c[3], c[2] * c[3], c[3]), index); nit: c * Sk4f(c[3], c[3], c[3], 1)? https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:578: int nextIndex = (fColorCount == 2) ? (kCache32Count - 1) wouldn't SkFixedToFFFF(fRecs[i].fPos) >> kCache32Shift work for fColorCount == 2 also? https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:586: c0 = Sk4f(c0[0] * c0[3], c0[1] * c0[3], c0[2] * c0[3], c0[3]); nit: c0 * Sk4f(c0[3], c0[3], c0[3], 1)? https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:587: c1 = Sk4f(c1[0] * c1[3], c1[1] * c1[3], c1[2] * c1[3], c1[3]); nit: ditto https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:599: } SkASSERT(prevIndex == kCache32Count - 1)? https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:695: initLinearBitmap(bitmap); nit: this-> https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... File src/effects/gradients/SkGradientShaderPriv.h (right): https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShaderPriv.h:181: kHalfFloat = 2, nit: do we actually care about these values being explicit?
Addressed all the little stuff - going to experiment with just using the raster context. https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:548: void SkGradientShaderBase::initLinearBitmap(SkBitmap* bitmap) const { On 2016/09/21 02:51:35, f(malita) wrote: > On 2016/09/20 17:29:53, Brian Osman wrote: > > This is essentially a condensed version of the logic from Build32bitCache > > (above), that can construct either an F16 or sRGB format bitmap containing the > > gradient strip, interpolated in premul or not. > > Kinda feels like we're reinventing a specialized version of Sk4fLinearGradient > here... I wonder what it would take to just set up a context and invoke that > code instead? That sounds like a good idea. Almost looks like I could just make a LinearGradient4fContext on the stack and invoke it? I'm going to try that experiment before landing this. > Also, is the legacy Build32bitCache going to be used going forward? I think > we're going to switch SW gradients to the analytical path sooner or later, but > maybe the GPU multi-stop impl will still use it for L32? If so, do you think we > could merge these two at some point? The GPU is going to need *some* way to build textures. Yes, if I can create a non-gamma-correct context to shade the texture span, I think we could make all the handling much more consistent. (It's trickier for me to do the L32 case right now, because this code is using the 4f colors, which are already linear). https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:571: writeSizedPixel(Sk4f(c[0] * c[3], c[1] * c[3], c[2] * c[3], c[3]), index); On 2016/09/21 02:51:35, f(malita) wrote: > nit: c * Sk4f(c[3], c[3], c[3], 1)? Duh! I was sure there was a clearer way to write this, but my brain was not working that day. https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:578: int nextIndex = (fColorCount == 2) ? (kCache32Count - 1) On 2016/09/21 02:51:35, f(malita) wrote: > wouldn't SkFixedToFFFF(fRecs[i].fPos) >> kCache32Shift work for fColorCount == 2 > also? I don't think so. The constructor never initializes the contents of fRecs unless fColorCount > 2. (All the other code that uses fRecs is similarly guarded). https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:586: c0 = Sk4f(c0[0] * c0[3], c0[1] * c0[3], c0[2] * c0[3], c0[3]); On 2016/09/21 02:51:35, f(malita) wrote: > nit: c0 * Sk4f(c0[3], c0[3], c0[3], 1)? Done. https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:587: c1 = Sk4f(c1[0] * c1[3], c1[1] * c1[3], c1[2] * c1[3], c1[3]); On 2016/09/21 02:51:35, f(malita) wrote: > nit: ditto Done. https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:599: } On 2016/09/21 02:51:35, f(malita) wrote: > SkASSERT(prevIndex == kCache32Count - 1)? Done. https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:695: initLinearBitmap(bitmap); On 2016/09/21 02:51:35, f(malita) wrote: > nit: this-> Done. https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... File src/effects/gradients/SkGradientShaderPriv.h (right): https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShaderPriv.h:181: kHalfFloat = 2, On 2016/09/21 02:51:35, f(malita) wrote: > nit: do we actually care about these values being explicit? Nope. Fixed.
non-GL bits LGTM https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/2343253002/diff/80001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:578: int nextIndex = (fColorCount == 2) ? (kCache32Count - 1) On 2016/09/21 13:24:18, Brian Osman wrote: > On 2016/09/21 02:51:35, f(malita) wrote: > > wouldn't SkFixedToFFFF(fRecs[i].fPos) >> kCache32Shift work for fColorCount == > 2 > > also? > > I don't think so. The constructor never initializes the contents of fRecs unless > fColorCount > 2. (All the other code that uses fRecs is similarly guarded). Acknowledged.
Do we just wind up creating a second instance of the texture strip atlas for f16? https://codereview.chromium.org/2343253002/diff/100001/src/effects/gradients/... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/2343253002/diff/100001/src/effects/gradients/... src/effects/gradients/SkGradientShader.cpp:1502: SkASSERT("Requesting a gamma-correct gradient FP without F16 or sRGB"); Won't this string always be true? I believe we use SkDEBUGFAIL for this purpose.
On 2016/09/22 14:16:03, bsalomon wrote: > Do we just wind up creating a second instance of the texture strip atlas for > f16? Yes. Do we want to limit the number of atlases? > https://codereview.chromium.org/2343253002/diff/100001/src/effects/gradients/... > File src/effects/gradients/SkGradientShader.cpp (right): > > https://codereview.chromium.org/2343253002/diff/100001/src/effects/gradients/... > src/effects/gradients/SkGradientShader.cpp:1502: SkASSERT("Requesting a > gamma-correct gradient FP without F16 or sRGB"); > Won't this string always be true? > > I believe we use SkDEBUGFAIL for this purpose. Oops. Yes, that's what I meant to type.
On 2016/09/22 14:21:46, Brian Osman wrote: > On 2016/09/22 14:16:03, bsalomon wrote: > > Do we just wind up creating a second instance of the texture strip atlas for > > f16? > > Yes. Do we want to limit the number of atlases? > I was just curious about how it worked. > > > https://codereview.chromium.org/2343253002/diff/100001/src/effects/gradients/... > > File src/effects/gradients/SkGradientShader.cpp (right): > > > > > https://codereview.chromium.org/2343253002/diff/100001/src/effects/gradients/... > > src/effects/gradients/SkGradientShader.cpp:1502: SkASSERT("Requesting a > > gamma-correct gradient FP without F16 or sRGB"); > > Won't this string always be true? > > > > I believe we use SkDEBUGFAIL for this purpose. > > Oops. Yes, that's what I meant to type. lgtm
The CQ bit was checked by brianosman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/2343253002/#ps120001 (title: "SkASSERT -> SkDEBUGFAIL")
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 ========== Support for color-spaces with multi-stop (texture) gradients Texture is F16 linear, unless that's not supported. In that case, we pack down to sRGB. Added more test patches to the gamut GM with many stops, to test this case. Now they render correctly. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343253002 ========== to ========== Support for color-spaces with multi-stop (texture) gradients Texture is F16 linear, unless that's not supported. In that case, we pack down to sRGB. Added more test patches to the gamut GM with many stops, to test this case. Now they render correctly. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343253002 Committed: https://skia.googlesource.com/skia/+/d454609f621471998edc382cdbd25bd2f05f0bde ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/d454609f621471998edc382cdbd25bd2f05f0bde |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
