Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2449243003:
Initial implementation of a SkColorSpace_A2B xform (Closed)
DescriptionInitial implementation of a SkColorSpace_A2B xform
There is support for all features of SkColorSpace_A2B.
Tests for these functionality were adapted from
the XYZ xform, plus a CLUT-specific test was added.
Shared functions used by both SkColorSpaceXform_XYZ and SkColorSpaceXform_A2B
have been moved into a shared header.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2449243003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
Committed: https://skia.googlesource.com/skia/+/2563601fc2b0505619f905f86bd249ae630197cc
Patch Set 1 #
Total comments: 1
Patch Set 2 : build error+forgot to premuliplty last 0-3 pixels of a line+whitespace warnings #
Total comments: 20
Patch Set 3 : expressed all non-table gammas as parametric for simplicity #Patch Set 4 : switched from .gypi to .gni to fix build errors, also whitespace fixes #
Total comments: 33
Patch Set 5 : updated implementation to use SkRasterPipeline #
Total comments: 75
Patch Set 6 : responding to comments #
Total comments: 2
Patch Set 7 : moved all the (now) unused stuff back from SkColorSpaceXformPriv.h to SkColorSpaceXform.cpp #
Total comments: 12
Patch Set 8 : responding to comments #
Total comments: 2
Patch Set 9 : responding to comments #Patch Set 10 : fixed compile error on certain trybots related to RVO move-elision #
Depends on Patchset: Messages
Total messages: 65 (43 generated)
Description was changed from ========== Initial implementation of a SkColorSpace_A2B xform There is support for all features of SkColorSpace_A2B except for mixed-gammas (ie red is SRGB, green is linear, blue is table). Value (x^n) and Parametric gammas will most likely be optimized later as they are simply evaluated right now instead of being pre-computed into a table at construction. Tests for these functionality (sans mixed gammas) were adapted from the XYZ xform, plus a CLUT-specific test was added. Shared functions used by both SkColorSpaceXform_XYZ and SkColorSpaceXform_A2B have been moved into a shared header. BUG=skia: ========== to ========== Initial implementation of a SkColorSpace_A2B xform There is support for all features of SkColorSpace_A2B except for mixed-gammas (ie red is SRGB, green is linear, blue is table). Value (x^n) and Parametric gammas will most likely be optimized later as they are simply evaluated right now instead of being pre-computed into a table at construction. Tests for these functionality (sans mixed gammas) were adapted from the XYZ xform, plus a CLUT-specific test was added. Shared functions used by both SkColorSpaceXform_XYZ and SkColorSpaceXform_A2B have been moved into a shared header. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2449243003 ==========
The CQ bit was checked by raftias@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: Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by raftias@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...
Description was changed from ========== Initial implementation of a SkColorSpace_A2B xform There is support for all features of SkColorSpace_A2B except for mixed-gammas (ie red is SRGB, green is linear, blue is table). Value (x^n) and Parametric gammas will most likely be optimized later as they are simply evaluated right now instead of being pre-computed into a table at construction. Tests for these functionality (sans mixed gammas) were adapted from the XYZ xform, plus a CLUT-specific test was added. Shared functions used by both SkColorSpaceXform_XYZ and SkColorSpaceXform_A2B have been moved into a shared header. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2449243003 ========== to ========== Initial implementation of a SkColorSpace_A2B xform There is support for all features of SkColorSpace_A2B except for mixed-gammas (ie red is SRGB, green is linear, blue is table). Value (x^n) and Parametric gammas will most likely be optimized later as they are simply evaluated right now instead of being pre-computed into a table at construction. Tests for these functionality (sans mixed gammas) were adapted from the XYZ xform, plus a CLUT-specific test was added. Shared functions used by both SkColorSpaceXform_XYZ and SkColorSpaceXform_A2B have been moved into a shared header. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2449243003 ==========
raftias@google.com changed reviewers: + msarett@google.com, mtklein@google.com
https://codereview.chromium.org/2449243003/diff/1/src/core/SkColorSpaceXform_... File src/core/SkColorSpaceXform_A2B.cpp (right): https://codereview.chromium.org/2449243003/diff/1/src/core/SkColorSpaceXform_... src/core/SkColorSpaceXform_A2B.cpp:81: /*const Sk4f l = *rSrc * 100.f; This is the above code but in 2 separate steps. It should be slightly faster above, but less readable - although colour-space conversions aren't something that changes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Android-Clang-Nexus5-GPU-Adreno330-arm-Release-GN_Android-Trybot on master.client.skia.android (JOB_FAILED, http://build.chromium.org/p/client.skia.android/builders/Test-Android-Clang-N...)
General design looks good! I'm excited about this change. Very intrigued about this potentially being an SkRasterPipeline - Mike what do you think? https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXformPriv.h (right): https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXformPriv.h:17: #define SkCSXformPrintfDefined 0 What's this for? https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXformPriv.h:20: static AI void interp_3d_clut(float dst[3], float src[3], const SkColorLookUpTable* colorLUT) { Can this be moved to SkColorSpaceXform_A2B.cpp? https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXformPriv.h:122: const Sk4f& rXgXbX, const Sk4f& rYgYbY, const Sk4f& rZgZbZ, nit: spacing https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXformPriv.h:137: Sk4f& rXgXbX, Sk4f& rYgYbY, Sk4f& rZgZbZ, Sk4f& rTgTbT) { nit: spacing https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform_A2B.cpp (right): https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:42: void apply(Sk4f* rSrc, Sk4f* gSrc, Sk4f* bSrc, int len) override { This feels like a really good place to use SkRasterPipeline. The interface of all of these functions could be made to match the StageFn interface. And then these could chain together and we could handle 1 (or 4) pixels at a time. Rather than repeating this loop in each ProcessingElement. Mike, what do you think? Maybe I'm jumping ahead again... https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:44: // unpack into float[3] = {r, g, b} arrays like interp_3d_clut() needs If it's convenient, you are welcome to change the interface of interp_3d_clut() - I think you are the only user. Maybe it's more convenient to leave as is for this CL though. https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:159: static inline SkNx<N,float> sk_linear_from_float_srgb_math(const SkNx<N,float>& x) { Let's move this to SkSRGB.h. I think the int->float version can call this. https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:197: // x^(35/16) = x^(2.1875) is an okay one as well and would be quicker: Please remove this comment. It's not a bad point, but it can be a bit confusing on code reviews when code is moved and subtle changes are added. https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:222: // TODO(raftias): See if there's some actually-vectorized implementation somewhere? As far I know, there's not. I don't even think there is a scalar hardware instruction that does powf. This is fine though. I find the SkNx_ prefix on the function name is confusing though, given that this is not a part of the SkNx library. https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:624: //concatTransforms(); Let's drop this for now if it's not being used yet. https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpace_A... File src/core/SkColorSpace_A2B.h (right): https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpace_A... src/core/SkColorSpace_A2B.h:63: explicit Element(SkGammaNamed gammaNamed) I think this constructor should convert gammaNamed to an SkGammas - then Element doesn't even need a kGammaNamed Type. Or maybe it's easier to convert earlier and eliminate this constructor altogether?
FYI, ignore the failed Nexus 5 bot, it is flaky.
The CQ bit was checked by raftias@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: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...) Test-Android-Clang-Nexus5-GPU-Adreno330-arm-Release-GN_Android-Trybot on master.client.skia.android (JOB_FAILED, http://build.chromium.org/p/client.skia.android/builders/Test-Android-Clang-N...) Build-Mac-Clang-x86_64-Release-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...) Build-Ubuntu-Clang-arm-Debug-GN_Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...) Build-Ubuntu-Clang-arm64-Debug-GN_Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...) Build-Ubuntu-Clang-arm64-Debug-GN_Android_FrameworkDefs-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...) Build-Ubuntu-Clang-mips64el-Debug-GN_Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-m...) Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...) Build-Win-MSVC-x86-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) Build-Win-MSVC-x86_64-Release-Vulkan-Trybot on master.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 raftias@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...
https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXformPriv.h (right): https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXformPriv.h:17: #define SkCSXformPrintfDefined 0 On 2016/10/26 20:43:29, msarett wrote: > What's this for? To block out chunks of code (variable declarations, etc) that exist just for printing. https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXformPriv.h:20: static AI void interp_3d_clut(float dst[3], float src[3], const SkColorLookUpTable* colorLUT) { On 2016/10/26 20:43:29, msarett wrote: > Can this be moved to SkColorSpaceXform_A2B.cpp? Done. https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXformPriv.h:122: const Sk4f& rXgXbX, const Sk4f& rYgYbY, const Sk4f& rZgZbZ, On 2016/10/26 20:43:29, msarett wrote: > nit: spacing Will fix in next patchset https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform_A2B.cpp (right): https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:42: void apply(Sk4f* rSrc, Sk4f* gSrc, Sk4f* bSrc, int len) override { On 2016/10/26 20:43:29, msarett wrote: > This feels like a really good place to use SkRasterPipeline. The interface of > all of these functions could be made to match the StageFn interface. > > And then these could chain together and we could handle 1 (or 4) pixels at a > time. Rather than repeating this loop in each ProcessingElement. > > Mike, what do you think? Maybe I'm jumping ahead again... Acknowledged. https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:44: // unpack into float[3] = {r, g, b} arrays like interp_3d_clut() needs On 2016/10/26 20:43:29, msarett wrote: > If it's convenient, you are welcome to change the interface of interp_3d_clut() > - I think you are the only user. > > Maybe it's more convenient to leave as is for this CL though. It involved look-up tables so without constructing a gridpoints^12 element look-up table, I don't see how you'd vectorize it, so the loop would just end up in the CLUT code anyway. The input tables could be removed, but the output tables would need to be there, since as far as I know the sk4f needs to be built all at once with all 4 values provided, rather than editing single ones. Since both the input and output tables here are the same array, I don't think it would make it that much less gross to move the vector stuff into the interp_3d_clut code. I could be wrong though. https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:159: static inline SkNx<N,float> sk_linear_from_float_srgb_math(const SkNx<N,float>& x) { On 2016/10/26 20:43:29, msarett wrote: > Let's move this to SkSRGB.h. I think the int->float version can call this. Removed function entirely. Will consider this if/when we add back in SRGB as non-parametric. https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:197: // x^(35/16) = x^(2.1875) is an okay one as well and would be quicker: On 2016/10/26 20:43:29, msarett wrote: > Please remove this comment. It's not a bad point, but it can be a bit confusing > on code reviews when code is moved and subtle changes are added. Removed function entirely. Will consider this if/when we add back in 2.2 as non-parametric https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:624: //concatTransforms(); On 2016/10/26 20:43:29, msarett wrote: > Let's drop this for now if it's not being used yet. Done. https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpace_A... File src/core/SkColorSpace_A2B.h (right): https://codereview.chromium.org/2449243003/diff/20001/src/core/SkColorSpace_A... src/core/SkColorSpace_A2B.h:63: explicit Element(SkGammaNamed gammaNamed) On 2016/10/26 20:43:30, msarett wrote: > I think this constructor should convert gammaNamed to an SkGammas - then Element > doesn't even need a kGammaNamed Type. > > Or maybe it's easier to convert earlier and eliminate this constructor > altogether? I think with what I just changed in the xofrm it isn't that much extra code to have it processed in the xform.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXformPriv.h (right): https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXformPriv.h:30: const Sk4f& rXgXbX, const Sk4f& rYgYbY, const Sk4f& rZgZbZ, nit: spacing https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXformPriv.h:51: nit: Remove line https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform_A2B.cpp (right): https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:197: fMatrix.asColMajorf(colMajor); Can we do this in the constructor and have fMatrix be a float[16]? https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:220: // TODO(raftias): remove and store in tables? I think this is fine as is. Tables will hurt accuracy - let's not optimize too soon. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:229: const SkColorSpaceTransferFn& bParams) nit: spacing https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:235: // TODO(raftias): profile against a non-vectorized implementation, since if the (short) Let's start with the non-vectorized implementation. We are already in scalar code to apply powf() - using the vector if-then-else seems unnecessary to me. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:357: StoreFn store = store_linear<kRGBA_Order>; This code seems familiar, can we share it? https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:412: const int allocLen = (count + 3) / 4; I find the "driver code" (from here to the end of the function) a little messy/confusing. Can we see what this looks like as a raster pipeline? https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:430: if (loadLen > 0) { Can we load vectors for the tail just filling with zeros (without memcpy)? https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:487: static inline SkColorSpaceTransferFn skgammanamed_to_parametric(SkGammaNamed gammaNamed) { remove "sk" https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:498: SkASSERT(false); Move this and the return statement into the default case. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:502: static inline SkColorSpaceTransferFn skgamma_to_parametric(const SkGammas& gammas, int channel) { remove "sk" https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:504: { move to previous line https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:555: SkCSXformPrintf("(yet). Using only 1st table gamma channel.\n"); No need for two statements here just pick up the string... Ex: SkCSXformPrintf("Long string that " "doesn't fit on one line"); https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform_A2B.h (right): https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.h:28: void concatTransforms(); nit: Remove.
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform_A2B.cpp (right): https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:159: Sk4f Y = (l + 16.f) * (1.f/116.f); It's usually enough to write one .f, e.g (1/116.f). That at least lets you read the fraction without any .f in the way. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:413: SkAutoTMalloc<Sk4f> rSrc(allocLen); I don't actually know if malloc() and SkAutoTMalloc guarantee enough alignment for Sk4f on all platforms. I suspect they do not on 32-bit targets... I think the typical guarantee is 2*sizeof(void*), which is 8 there. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:487: static inline SkColorSpaceTransferFn skgammanamed_to_parametric(SkGammaNamed gammaNamed) { On 2016/10/27 at 19:36:44, msarett wrote: > remove "sk" Or maybe, SkGammaNamed_to_parametric is what you were after here? https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:502: static inline SkColorSpaceTransferFn skgamma_to_parametric(const SkGammas& gammas, int channel) { On 2016/10/27 at 19:36:44, msarett wrote: > remove "sk" Same deal, SkGammas_to_parametric? https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform_A2B.h (right): https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.h:18: enum SkGammaNamed : uint8_t; This is neat, but seems fragile. I think we should probably include SkColorSpace_Base.h? https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.h:30: class ProcessingElement; Do these classes need access to SkColorSpaceXform_A2B's private data or methods? If not, they can be made even more private, by moving them to be standalone types in an anonymous namespace ( namespace { ... } ) completely inside SkColorSpaceXForm_A2B.cpp. No mention of them ever need leak out of that file.
Description was changed from ========== Initial implementation of a SkColorSpace_A2B xform There is support for all features of SkColorSpace_A2B except for mixed-gammas (ie red is SRGB, green is linear, blue is table). Value (x^n) and Parametric gammas will most likely be optimized later as they are simply evaluated right now instead of being pre-computed into a table at construction. Tests for these functionality (sans mixed gammas) were adapted from the XYZ xform, plus a CLUT-specific test was added. Shared functions used by both SkColorSpaceXform_XYZ and SkColorSpaceXform_A2B have been moved into a shared header. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2449243003 ========== to ========== Initial implementation of a SkColorSpace_A2B xform There is support for all features of SkColorSpace_A2B except for mixed-gammas (ie red is SRGB, green is linear, blue is table). Value (x^n) and Parametric gammas will most likely be optimized later as they are simply evaluated right now instead of being pre-computed into a table at construction. Tests for these functionality (sans mixed gammas) were adapted from the XYZ xform, plus a CLUT-specific test was added. Shared functions used by both SkColorSpaceXform_XYZ and SkColorSpaceXform_A2B have been moved into a shared header. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2449243003 CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ==========
The CQ bit was checked by raftias@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...
https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXformPriv.h (right): https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXformPriv.h:30: const Sk4f& rXgXbX, const Sk4f& rYgYbY, const Sk4f& rZgZbZ, On 2016/10/27 19:36:43, msarett wrote: > nit: spacing Done. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXformPriv.h:51: On 2016/10/27 19:36:43, msarett wrote: > nit: Remove line Done. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform_A2B.cpp (right): https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:197: fMatrix.asColMajorf(colMajor); On 2016/10/27 19:36:44, msarett wrote: > Can we do this in the constructor and have fMatrix be a float[16]? Yeah. It was just like this for merging, but I guess even if we do that again it might make more sense to just reconstruct the matrix in the merge since it's only executed once per xform, not per row. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:357: StoreFn store = store_linear<kRGBA_Order>; On 2016/10/27 19:36:43, msarett wrote: > This code seems familiar, can we share it? If we convert this entire xform to the enum that merges gamma-type / dst format together, then it could be shared. The XYZ xform sets store and store_1 here, and merges dst gamma / dst format together as one enum (I assume to allow inlining/conditional elimination). https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:487: static inline SkColorSpaceTransferFn skgammanamed_to_parametric(SkGammaNamed gammaNamed) { On 2016/10/27 19:36:44, msarett wrote: > remove "sk" Done. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:498: SkASSERT(false); On 2016/10/27 19:36:43, msarett wrote: > Move this and the return statement into the default case. Done. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:502: static inline SkColorSpaceTransferFn skgamma_to_parametric(const SkGammas& gammas, int channel) { On 2016/10/27 19:36:44, msarett wrote: > remove "sk" Done. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:504: { On 2016/10/27 19:36:43, msarett wrote: > move to previous line Done. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:555: SkCSXformPrintf("(yet). Using only 1st table gamma channel.\n"); On 2016/10/27 19:36:43, msarett wrote: > No need for two statements here just pick up the string... Ex: > SkCSXformPrintf("Long string that " > "doesn't fit on one line"); Done. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform_A2B.h (right): https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.h:18: enum SkGammaNamed : uint8_t; On 2016/10/31 18:43:00, mtklein_C wrote: > This is neat, but seems fragile. I think we should probably include > SkColorSpace_Base.h? Not needed in new implementation. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.h:28: void concatTransforms(); On 2016/10/27 19:36:44, msarett wrote: > nit: Remove. Not needed in new implementation. https://codereview.chromium.org/2449243003/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.h:30: class ProcessingElement; On 2016/10/31 18:43:00, mtklein_C wrote: > Do these classes need access to SkColorSpaceXform_A2B's private data or methods? > > If not, they can be made even more private, by moving them to be standalone > types in an anonymous namespace ( namespace { ... } ) completely inside > SkColorSpaceXForm_A2B.cpp. No mention of them ever need leak out of that file. No, but they're not needed in the SkRasterPipeline implementation anyway. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... File src/opts/SkRasterPipeline_opts.h (right): https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:511: // Clamp colors into [0,1] premul (e.g. just before storing back to memory). I noticed when I pulled before uploading that this was recently removed from the other store functions - however without it I get really psychedelic results from over(or under?)flow. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:766: STAGE(labtoxyz, true) { I think this can be expressed as a matrix_4x4 followed by a param_gamma, however this would be slower (powf(x, 3), other stuff) and possibly less clear, but would reduce the total number of raster pipeline stock stages. What would you recommend?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
msarett@chromium.org changed reviewers: + msarett@chromium.org
I very much like the overall design. https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (left): https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:495: static AI void load_matrix(const float matrix[16], Please make sure that we've only moved the functions that we needed to. https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform_A2B.cpp (right): https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:27: SkRasterPipeline entirePipeline; I don't love this variable name. https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:43: //entirePipeline.append(SkRasterPipeline::premul); Why comment this out? https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:164: #if (SkCSXformPrintfDefined) How extremely helpful is this for debugging? https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:173: for (size_t i = 0; i < srcSpace->count(); ++i) { nit: Count with ints https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:182: fElementsPipeline.append(param_gamma_stages[channel], &fParametricGammas.front()); nit: 100 chars https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:186: case SkColorSpace_A2B::Element::Type::kGammas: { I think it might be possible to make the table case much simpler. Assuming the Element has reffed the SkGammas struct: float* table = gammas.table(i); int size = gammas.data(i).fSize; // Store the |table| and the |size| in the ctx pointer. https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:188: SkCSXformPrintf("Adding gamme stage:"); nit: spelling https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:257: if (kNonStandard_SkGammaNamed != dstSpace->gammaNamed()) { Is there some refactoring you can do to make this case simpler? https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:259: invert_parametric(gammanamed_to_parametric(dstSpace->gammaNamed()))); I have confidence that this is much, much better than whatever I was doing to invert parametric curves. But let's just reuse the old code and consider this a separate change? https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:279: fElementsPipeline.append(param_gamma_stages[channel], &fParametricGammas.front()); nit: 100 chars https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpace_A... File src/core/SkColorSpace_A2B.h (right): https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpace_A... src/core/SkColorSpace_A2B.h:135: size_t count() const { return fElements.size(); } Sorry, unrelated to this CL. Can this return an int? Skia typically uses ints for counts. (And size_t for bytes) https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... File src/opts/SkRasterPipeline_opts.h (right): https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:140: #define GAMMA_STAGE(name) \ Instead of this, I think I would prefer 6 normal stages. Each of the stages can call a helper function to apply the gamma. So the code that actually applies the gamma is shared. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:178: SI void SK_VECTORCALL name##_a(BodyStage* st, size_t x, \ All we need to do with "a" is load it and store. The transformation happens on r, g, and b. Perhaps this is for when we abuse "a" for CMYK? In that case, let's not make this change until we are adding CMYK support. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:489: STAGE(load_s_linear_rgba, true) { nit: Follow style conventions from above Use whitespace to line things up 0xff instead of 0xFF https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:511: // Clamp colors into [0,1] premul (e.g. just before storing back to memory). On 2016/11/08 21:19:58, raftias wrote: > I noticed when I pulled before uploading that this was recently removed from the > other store functions - however without it I get really psychedelic results from > over(or under?)flow. I believe the idea is to not waste time clamping when we are sure that 0-1 input values guarantee 0-1 output values. When we are not sure, we should clamp. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:527: store<kIsTail>(tail, ( SkNx_cast<int>(255.0f * r + 0.5f) << 0 I don't think you need the "+ 0.5f" terms. I think Mike is just adding those above to avoid weird rounding issues where alpha ends up greater than r, g, or b? In that case, definitely remove from r, g, and b - and I think from a as well. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:604: dr = fma(mat.get(0, 0),r, fma(mat.get(0, 1),g, fma(mat.get(0, 2),b, mat.get(0, 3)*a))); No need for "*a". Actually I think we don't want it for sure. We're probably just lucky that it's always 1 here. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:606: db = fma(mat.get(2, 0),r, fma(mat.get(2, 1),g, fma(mat.get(2, 2),b, mat.get(2, 3)*a))); Mike, is it ok that we're destructive to dr, dg, db, da here? https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:639: return (s <= gamma.fD).thenElse(gamma.fE * s + gamma.fF, nit: < instead of <= https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:656: static inline void interp_3d_clut(float dst[3], float src[3], const SkColorLookUpTable* colorLUT) { This maybe does not need to belong in this file. Possibly should be SkColorSpaceXformPriv.h. Mike?
I wrote a lot because this is fun. I like where this is going. https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform_A2B.cpp (right): https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:26: const void* src, int count, SkAlphaType alphaType) const { Is this the alphaType for src or dst? I'm guessing dst, and src is always unpremul? https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:36: SkCSXformPrintf("F16/F32 source color format not supported\n"); load_s_f16 will probably work just fine. And we can make load_s_f32 if you like... we already have a store_f32. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... File src/opts/SkRasterPipeline_opts.h (right): https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:140: #define GAMMA_STAGE(name) \ On 2016/11/09 at 00:01:05, msarett1 wrote: > Instead of this, I think I would prefer 6 normal stages. Each of the stages can call a helper function to apply the gamma. So the code that actually applies the gamma is shared. I think you mean 3 normal stages? Each STAGE invocation handles Body/Tail pair. Yeah, none of the GAMMA_STAGEs look like they're going to perform significantly differently from serial code, so we're probably not going to mind the gamma being a function call. Might be kind of fun to see just how general-purpose we can make these: STAGE(fn_r) { auto fn = (const std::function<float(float)>*)ctx; float result[N]; for (int i = 0; i < N; i++) { result[i] = (*fn)(r[i]); } r = SkNf::Load(result); } STAGE(fn_g) { ... } STAGE(fn_b) { ... } STAGE(fn_a) { ... } https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:489: STAGE(load_s_linear_rgba, true) { Let's call these _8888. That's our common shorthand for gamma=1 8-bit component colors. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:500: STAGE(load_s_linear_bgra, true) { How about we write everything in terms of rgba, and use an extra swap_rb stage if we need to handle bgra. Something like this? STAGE(swap_rb) { SkTSwap(r,b); } STAGE(load_s_8888, true) { ... } STAGE(store_8888, false) { ... } https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:511: // Clamp colors into [0,1] premul (e.g. just before storing back to memory). On 2016/11/09 at 00:01:05, msarett1 wrote: > On 2016/11/08 21:19:58, raftias wrote: > > I noticed when I pulled before uploading that this was recently removed from the > > other store functions - however without it I get really psychedelic results from > > over(or under?)flow. > > I believe the idea is to not waste time clamping when we are sure that 0-1 input values guarantee 0-1 output values. When we are not sure, we should clamp. This has now been split into two stages, clamp_0 and clamp_a. We noticed that in most pipelines clamping was not required, and often when it was, only in one direction. As stages, we can add one, both, or neither as needed. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:527: store<kIsTail>(tail, ( SkNx_cast<int>(255.0f * r + 0.5f) << 0 On 2016/11/09 at 00:01:05, msarett1 wrote: > I don't think you need the "+ 0.5f" terms. I think Mike is just adding those above to avoid weird rounding issues where alpha ends up greater than r, g, or b? > > In that case, definitely remove from r, g, and b - and I think from a as well. No, we're doing that to round to the nearest byte rather than truncate. I think it's right to leave them all in. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:602: const SkMatrix44& mat = *(const SkMatrix44*)ctx; I'd like matrix_4x4 and matrix_4x5 to look and behave as similarly as possible, so that the differences they do have stand out. Can just take a column-major float[16] here, swap the order of coefficients in the fma()s in one of the two functions, etc? I wouldn't mind splitting matrix_4x5 into matrix_4x4 and translate_4x1. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:604: dr = fma(mat.get(0, 0),r, fma(mat.get(0, 1),g, fma(mat.get(0, 2),b, mat.get(0, 3)*a))); On 2016/11/09 at 00:01:05, msarett1 wrote: > No need for "*a". Actually I think we don't want it for sure. We're probably just lucky that it's always 1 here. If we don't *a here, we can't really call this stage matrix_4x4. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:606: db = fma(mat.get(2, 0),r, fma(mat.get(2, 1),g, fma(mat.get(2, 2),b, mat.get(2, 3)*a))); On 2016/11/09 at 00:01:05, msarett1 wrote: > Mike, is it ok that we're destructive to dr, dg, db, da here? The pedantic answer is that that depends what you're doing with them later... it may be okay for this particular use case, but it sure limits other use of matrix_4x4 if it always destroys those values. Generally there's no need to do this. It's almost always better to make new names like we do in matrix_4x5. There are usually plenty of temporary registers available. Sometimes calling convention limitations mean {dr,dg,db,da} live on the stack while there are several registers available as temporaries within a function. But the most important reason not to do this is it reads weird. We should only change dr if the code's purpose is to change dr. If we write code like this, it looks like we're intentionally leaving these values in both {r,g,b,a} and {dr,dg,db,da}. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:628: static inline Sk4f powNf(const Sk4f& x, float exp) { Generally this file writes static inline as SI. It's not important. (It _is_ important that we mark these functions as both static and inline; SI is there to make it easy.) https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:647: const float* gammaTables = (const float*)ctx; This name makes it seem like we're going to be using different tables. There's only one table here right? auto table = (const float*)ctx; https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:648: s = SkNf::Min(SkNf::Max(maxIndex * s, 0.f), maxIndex); If we're not going to source the 1024 constant from somewhere, I'd rather just go fully nameless: s = SkNx::Max(0, SkNf::Min(s, 1)) * 1023; https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:656: static inline void interp_3d_clut(float dst[3], float src[3], const SkColorLookUpTable* colorLUT) { On 2016/11/09 at 00:01:05, msarett1 wrote: > This maybe does not need to belong in this file. Possibly should be SkColorSpaceXformPriv.h. Mike? Why don't we make this a normal, separately-compiled method of SkColorLookupTable? It's giant, serial, and we're going to call it a loop serially around that. Inlining it can't be important... https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:748: STAGE(clut, true) { how about color_lookup_table? https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:749: const SkColorLookUpTable* colorLUT = (const SkColorLookUpTable*)ctx; Side note: it's going to drive me nuts that we capitalized SkColorLookUpTable this way. It's a Lookup Table, not a Look Up Table... https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:751: alignas(alignof(SkNf)) float result[3][N]; Let's drop the alignment business. SkNf::Load() should always work. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:766: STAGE(labtoxyz, true) { On 2016/11/08 at 21:19:58, raftias wrote: > I think this can be expressed as a matrix_4x4 followed by a param_gamma, however this would be slower (powf(x, 3), other stuff) and possibly less clear, but would reduce the total number of raster pipeline stock stages. What would you recommend? I think this is clearer as its own stage. It's probably best to put some underscores in the name there so people don't have to guess where the word divisions are. lab_to_xyz? https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:771: auto X = lab_a * (1.f/500.f) + Y; One .f is plenty to get these solidly as float constants. I like it to be the denominator (1/500.0f) so that the fraction is readable. I don't have much of an opinion on whether or not to write .0f or .f, but I would like this file to be consistent. Please either change all this new code to use .0f, or change all the existing float constants to .f. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:776: cubed = Y*Y*Y; At a glance it looks like cubed must be accumulating some value that affects X, then Y, then Z in turn, where really they're all independent. This sort of code is usually clearer if you don't change values. Let's write auto X3 = X*X*X, Y3 = Y*Y*Y, Z3 = Z*Z*Z; X = (X3 > ...) ...; Y = (Y3 > ...) ...; Z = (Z3 > ...) ...; This way all the parallel operations are visibly parallel. If you write code in this style, you will rarely find yourself marking things as const. Everything's const and it doesn't need to be mentioned.
The CQ bit was checked by raftias@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: Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
The CQ bit was checked by raftias@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...
https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (left): https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:495: static AI void load_matrix(const float matrix[16], On 2016/11/09 00:01:04, msarett1 wrote: > Please make sure that we've only moved the functions that we needed to. Was used in a previous patchset I think. I'll move it back here in the next patchset. https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform_A2B.cpp (right): https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:27: SkRasterPipeline entirePipeline; On 2016/11/09 00:01:04, msarett1 wrote: > I don't love this variable name. Acknowledged. https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:43: //entirePipeline.append(SkRasterPipeline::premul); On 2016/11/09 00:01:04, msarett1 wrote: > Why comment this out? Mistake, I forgot to uncomment it after I tested something. https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:173: for (size_t i = 0; i < srcSpace->count(); ++i) { On 2016/11/09 00:01:05, msarett1 wrote: > nit: Count with ints Done. (forgot to upload the file though - I'll do that in the next patchset) https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:186: case SkColorSpace_A2B::Element::Type::kGammas: { On 2016/11/09 00:01:04, msarett1 wrote: > I think it might be possible to make the table case much simpler. Assuming the > Element has reffed the SkGammas struct: > > float* table = gammas.table(i); > int size = gammas.data(i).fSize; > // Store the |table| and the |size| in the ctx pointer. Done. https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:188: SkCSXformPrintf("Adding gamme stage:"); On 2016/11/09 00:01:04, msarett1 wrote: > nit: spelling Acknowledged. https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:257: if (kNonStandard_SkGammaNamed != dstSpace->gammaNamed()) { On 2016/11/09 00:01:04, msarett1 wrote: > Is there some refactoring you can do to make this case simpler? I had an addGamma() method that would add the storage and append the stage, but then removed it since some of the places would share storage and some wouldn't, and it didn't save much code. Or do you mean the fLinearDstGamma thing? https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:259: invert_parametric(gammanamed_to_parametric(dstSpace->gammaNamed()))); On 2016/11/09 00:01:04, msarett1 wrote: > I have confidence that this is much, much better than whatever I was doing to > invert parametric curves. But let's just reuse the old code and consider this a > separate change? And use tables instead? The code before only evaluated a single point. This code creates a new parametric so it can evaluate any point. https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpace_A... File src/core/SkColorSpace_A2B.h (right): https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpace_A... src/core/SkColorSpace_A2B.h:135: size_t count() const { return fElements.size(); } On 2016/11/09 00:01:05, msarett1 wrote: > Sorry, unrelated to this CL. Can this return an int? Skia typically uses ints > for counts. (And size_t for bytes) Done. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... File src/opts/SkRasterPipeline_opts.h (right): https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:140: #define GAMMA_STAGE(name) \ On 2016/11/09 11:04:36, mtklein_C wrote: > On 2016/11/09 at 00:01:05, msarett1 wrote: > > Instead of this, I think I would prefer 6 normal stages. Each of the stages > can call a helper function to apply the gamma. So the code that actually > applies the gamma is shared. > > I think you mean 3 normal stages? Each STAGE invocation handles Body/Tail pair. > > Yeah, none of the GAMMA_STAGEs look like they're going to perform significantly > differently from serial code, so we're probably not going to mind the gamma > being a function call. Might be kind of fun to see just how general-purpose we > can make these: > > STAGE(fn_r) { > auto fn = (const std::function<float(float)>*)ctx; > > float result[N]; > for (int i = 0; i < N; i++) { > result[i] = (*fn)(r[i]); > } > r = SkNf::Load(result); > } > STAGE(fn_g) { ... } > STAGE(fn_b) { ... } > STAGE(fn_a) { ... } I did this with fn_1_r/g/b. If we add in specific functions for other gamma types (sRGB/2.2) that have a vectorized implementation we could do an efficient fn_n_r/g/b that uses those I guess. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:178: SI void SK_VECTORCALL name##_a(BodyStage* st, size_t x, \ On 2016/11/09 00:01:05, msarett1 wrote: > All we need to do with "a" is load it and store. The transformation happens on > r, g, and b. > > Perhaps this is for when we abuse "a" for CMYK? In that case, let's not make > this change until we are adding CMYK support. It was indeed for CMYK/etc support. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:500: STAGE(load_s_linear_bgra, true) { On 2016/11/09 11:04:36, mtklein_C wrote: > How about we write everything in terms of rgba, and use an extra swap_rb stage > if we need to handle bgra. Something like this? > > STAGE(swap_rb) { > SkTSwap(r,b); > } > > STAGE(load_s_8888, true) { ... } > STAGE(store_8888, false) { ... } I that before (with that exact name, even), then talked to Matt and removed it. I could add it back, though. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:602: const SkMatrix44& mat = *(const SkMatrix44*)ctx; On 2016/11/09 11:04:36, mtklein_C wrote: > I'd like matrix_4x4 and matrix_4x5 to look and behave as similarly as possible, > so that the differences they do have stand out. Can just take a column-major > float[16] here, swap the order of coefficients in the fma()s in one of the two > functions, etc? > > I wouldn't mind splitting matrix_4x5 into matrix_4x4 and translate_4x1. They were just different since I had written it and didn't see 4x5 until I merged right before submitting the CL. I could do that though. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:604: dr = fma(mat.get(0, 0),r, fma(mat.get(0, 1),g, fma(mat.get(0, 2),b, mat.get(0, 3)*a))); On 2016/11/09 11:04:36, mtklein_C wrote: > On 2016/11/09 at 00:01:05, msarett1 wrote: > > No need for "*a". Actually I think we don't want it for sure. We're probably > just lucky that it's always 1 here. > > If we don't *a here, we can't really call this stage matrix_4x4. It's 3x4 now. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:606: db = fma(mat.get(2, 0),r, fma(mat.get(2, 1),g, fma(mat.get(2, 2),b, mat.get(2, 3)*a))); On 2016/11/09 11:04:36, mtklein_C wrote: > On 2016/11/09 at 00:01:05, msarett1 wrote: > > Mike, is it ok that we're destructive to dr, dg, db, da here? > > The pedantic answer is that that depends what you're doing with them later... it > may be okay for this particular use case, but it sure limits other use of > matrix_4x4 if it always destroys those values. > > Generally there's no need to do this. It's almost always better to make new > names like we do in matrix_4x5. There are usually plenty of temporary registers > available. Sometimes calling convention limitations mean {dr,dg,db,da} live on > the stack while there are several registers available as temporaries within a > function. > > But the most important reason not to do this is it reads weird. We should only > change dr if the code's purpose is to change dr. If we write code like this, it > looks like we're intentionally leaving these values in both {r,g,b,a} and > {dr,dg,db,da}. I'll remove these and put them in temporaries. I just thought we were low on registers due to a comment near the top of the file about Windows and that these would already be in registers. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:628: static inline Sk4f powNf(const Sk4f& x, float exp) { On 2016/11/09 11:04:36, mtklein_C wrote: > Generally this file writes static inline as SI. It's not important. (It _is_ > important that we mark these functions as both static and inline; SI is there to > make it easy.) Acknowledged. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:639: return (s <= gamma.fD).thenElse(gamma.fE * s + gamma.fF, On 2016/11/09 00:01:05, msarett1 wrote: > nit: < instead of <= Done. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:647: const float* gammaTables = (const float*)ctx; On 2016/11/09 11:04:36, mtklein_C wrote: > This name makes it seem like we're going to be using different tables. There's > only one table here right? > > auto table = (const float*)ctx; Acknowledged. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:648: s = SkNf::Min(SkNf::Max(maxIndex * s, 0.f), maxIndex); On 2016/11/09 11:04:36, mtklein_C wrote: > If we're not going to source the 1024 constant from somewhere, I'd rather just > go fully nameless: > > s = SkNx::Max(0, SkNf::Min(s, 1)) * 1023; ApplyTable stores it now. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:656: static inline void interp_3d_clut(float dst[3], float src[3], const SkColorLookUpTable* colorLUT) { On 2016/11/09 11:04:36, mtklein_C wrote: > On 2016/11/09 at 00:01:05, msarett1 wrote: > > This maybe does not need to belong in this file. Possibly should be > SkColorSpaceXformPriv.h. Mike? > > Why don't we make this a normal, separately-compiled method of > SkColorLookupTable? > > It's giant, serial, and we're going to call it a loop serially around that. > Inlining it can't be important... Done. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:748: STAGE(clut, true) { On 2016/11/09 11:04:36, mtklein_C wrote: > how about color_lookup_table? Done. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:749: const SkColorLookUpTable* colorLUT = (const SkColorLookUpTable*)ctx; On 2016/11/09 11:04:36, mtklein_C wrote: > Side note: it's going to drive me nuts that we capitalized SkColorLookUpTable > this way. It's a Lookup Table, not a Look Up Table... I didn't name it, but my guess is that it's because everywhere in the ICC it's referred to shorthand as a CLUT. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:751: alignas(alignof(SkNf)) float result[3][N]; On 2016/11/09 11:04:36, mtklein_C wrote: > Let's drop the alignment business. SkNf::Load() should always work. Done. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:766: STAGE(labtoxyz, true) { On 2016/11/09 11:04:36, mtklein_C wrote: > On 2016/11/08 at 21:19:58, raftias wrote: > > I think this can be expressed as a matrix_4x4 followed by a param_gamma, > however this would be slower (powf(x, 3), other stuff) and possibly less clear, > but would reduce the total number of raster pipeline stock stages. What would > you recommend? > > I think this is clearer as its own stage. > > It's probably best to put some underscores in the name there so people don't > have to guess where the word divisions are. lab_to_xyz? Done. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:771: auto X = lab_a * (1.f/500.f) + Y; On 2016/11/09 11:04:36, mtklein_C wrote: > One .f is plenty to get these solidly as float constants. I like it to be the > denominator (1/500.0f) so that the fraction is readable. > > I don't have much of an opinion on whether or not to write .0f or .f, but I > would like this file to be consistent. Please either change all this new code > to use .0f, or change all the existing float constants to .f. Acknowledged. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:776: cubed = Y*Y*Y; On 2016/11/09 11:04:36, mtklein_C wrote: > At a glance it looks like cubed must be accumulating some value that affects X, > then Y, then Z in turn, where really they're all independent. > > This sort of code is usually clearer if you don't change values. Let's write > > auto X3 = X*X*X, > Y3 = Y*Y*Y, > Z3 = Z*Z*Z; > X = (X3 > ...) ...; > Y = (Y3 > ...) ...; > Z = (Z3 > ...) ...; > > This way all the parallel operations are visibly parallel. > > If you write code in this style, you will rarely find yourself marking things as > const. Everything's const and it doesn't need to be mentioned. Done. https://codereview.chromium.org/2449243003/diff/100001/gm/labpcsdemo.cpp File gm/labpcsdemo.cpp (right): https://codereview.chromium.org/2449243003/diff/100001/gm/labpcsdemo.cpp#newc... gm/labpcsdemo.cpp:1: /* Do we want to get rid of this file right now, or keep it a little longer?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
All my comments are nits, so lgtm. Particularly, SkColorSpaceXform lgtm. Let's wait on Mike's approval for raster pipeline though. https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform_A2B.cpp (right): https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:26: const void* src, int count, SkAlphaType alphaType) const { On 2016/11/09 11:04:36, mtklein_C wrote: > Is this the alphaType for src or dst? I'm guessing dst, and src is always > unpremul? It refers to the dst, and the src is always unpremul. Opaque provides a hint that the src is opaque (so, if we want, we can just write 1.0 to the dst alpha channel rather than preserving the src alpha value). https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:164: #if (SkCSXformPrintfDefined) On 2016/11/09 00:01:05, msarett1 wrote: > How extremely helpful is this for debugging? The reason I ask is that we like to avoid #if code if at all possible. Perhaps these could be moved to SkColorSpaceXformPriv.h? That way, we don't see the #if here. https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:257: if (kNonStandard_SkGammaNamed != dstSpace->gammaNamed()) { On 2016/11/10 21:36:05, raftias wrote: > On 2016/11/09 00:01:04, msarett1 wrote: > > Is there some refactoring you can do to make this case simpler? > > I had an addGamma() method that would add the storage and append the stage, but > then removed it since some of the places would share storage and some wouldn't, > and it didn't save much code. Or do you mean the fLinearDstGamma thing? Sorry my comment was unclear - please ignore https://codereview.chromium.org/2449243003/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform_A2B.cpp:259: invert_parametric(gammanamed_to_parametric(dstSpace->gammaNamed()))); On 2016/11/10 21:36:06, raftias wrote: > On 2016/11/09 00:01:04, msarett1 wrote: > > I have confidence that this is much, much better than whatever I was doing to > > invert parametric curves. But let's just reuse the old code and consider this > a > > separate change? > > And use tables instead? The code before only evaluated a single point. This code > creates a new parametric so it can evaluate any point. That is what I meant, but I think you're right, better to land as is. Inverse parametric code lgtm. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... File src/opts/SkRasterPipeline_opts.h (right): https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:500: STAGE(load_s_linear_bgra, true) { On 2016/11/10 21:36:07, raftias wrote: > On 2016/11/09 11:04:36, mtklein_C wrote: > > How about we write everything in terms of rgba, and use an extra swap_rb stage > > if we need to handle bgra. Something like this? > > > > STAGE(swap_rb) { > > SkTSwap(r,b); > > } > > > > STAGE(load_s_8888, true) { ... } > > STAGE(store_8888, false) { ... } > > I that before (with that exact name, even), then talked to Matt and removed it. > I could add it back, though. Let's defer to Mike on this one. lgtm, as is. https://codereview.chromium.org/2449243003/diff/80001/src/opts/SkRasterPipeli... src/opts/SkRasterPipeline_opts.h:749: const SkColorLookUpTable* colorLUT = (const SkColorLookUpTable*)ctx; On 2016/11/10 21:36:06, raftias wrote: > On 2016/11/09 11:04:36, mtklein_C wrote: > > Side note: it's going to drive me nuts that we capitalized SkColorLookUpTable > > this way. It's a Lookup Table, not a Look Up Table... > > I didn't name it, but my guess is that it's because everywhere in the ICC it's > referred to shorthand as a CLUT. I don't feel strongly about the name. Feel free to rename it (or not) in this CL. https://codereview.chromium.org/2449243003/diff/100001/gm/labpcsdemo.cpp File gm/labpcsdemo.cpp (right): https://codereview.chromium.org/2449243003/diff/100001/gm/labpcsdemo.cpp#newc... gm/labpcsdemo.cpp:1: /* On 2016/11/10 21:36:07, raftias wrote: > Do we want to get rid of this file right now, or keep it a little longer? If it doesn't test anything interesting that is not covered by the image tests, let's delete. https://codereview.chromium.org/2449243003/diff/120001/src/core/SkColorSpaceX... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2449243003/diff/120001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:483: static AI void load_rgb_linear(const uint32_t* src, Sk4f& r, Sk4f& g, Sk4f& b, Sk4f& a, nit: move these down, back where they were https://codereview.chromium.org/2449243003/diff/120001/src/core/SkColorSpaceX... File src/core/SkColorSpaceXform_A2B.cpp (right): https://codereview.chromium.org/2449243003/diff/120001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform_A2B.cpp:210: // add in all device -> PCS xforms nit: Maybe "device" isn't the right word here? Could say "encoded" or just "xforms to PCS" https://codereview.chromium.org/2449243003/diff/120001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform_A2B.cpp:275: addGamma(ApplyParametric( nit: Can this somehow fit on two lines? https://codereview.chromium.org/2449243003/diff/120001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform_A2B.cpp:282: if (SkGammas::Type::kTable_Type == gammas.type(channel)) { nit: Can you make the code in this if statement a call to a static helper function (that builds the inverse table)? https://codereview.chromium.org/2449243003/diff/120001/src/opts/SkRasterPipel... File src/opts/SkRasterPipeline_opts.h (right): https://codereview.chromium.org/2449243003/diff/120001/src/opts/SkRasterPipel... src/opts/SkRasterPipeline_opts.h:614: //SkTSwap(r, b); What's going on here? https://codereview.chromium.org/2449243003/diff/120001/tests/ColorSpaceXformT... File tests/ColorSpaceXformTest.cpp (right): https://codereview.chromium.org/2449243003/diff/120001/tests/ColorSpaceXformT... tests/ColorSpaceXformTest.cpp:64: const auto diff = SkTAbs(x - y); nit: Can we have two versions of almost_equal()? One for XYZ and one for A2B?
Sorry forgot one thing. From the CL description: > There is support for all features of SkColorSpace_A2B except for mixed-gammas > (ie red is SRGB, green is linear, blue is table). Is this still true? Seems like you do support this?
Description was changed from ========== Initial implementation of a SkColorSpace_A2B xform There is support for all features of SkColorSpace_A2B except for mixed-gammas (ie red is SRGB, green is linear, blue is table). Value (x^n) and Parametric gammas will most likely be optimized later as they are simply evaluated right now instead of being pre-computed into a table at construction. Tests for these functionality (sans mixed gammas) were adapted from the XYZ xform, plus a CLUT-specific test was added. Shared functions used by both SkColorSpaceXform_XYZ and SkColorSpaceXform_A2B have been moved into a shared header. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2449243003 CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ========== to ========== Initial implementation of a SkColorSpace_A2B xform There is support for all features of SkColorSpace_A2B. Tests for these functionality were adapted from the XYZ xform, plus a CLUT-specific test was added. Shared functions used by both SkColorSpaceXform_XYZ and SkColorSpaceXform_A2B have been moved into a shared header. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2449243003 CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ==========
https://codereview.chromium.org/2449243003/diff/120001/src/core/SkColorSpaceX... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2449243003/diff/120001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:483: static AI void load_rgb_linear(const uint32_t* src, Sk4f& r, Sk4f& g, Sk4f& b, Sk4f& a, On 2016/11/11 14:36:51, msarett1 wrote: > nit: move these down, back where they were Done. https://codereview.chromium.org/2449243003/diff/120001/src/core/SkColorSpaceX... File src/core/SkColorSpaceXform_A2B.cpp (right): https://codereview.chromium.org/2449243003/diff/120001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform_A2B.cpp:210: // add in all device -> PCS xforms On 2016/11/11 14:36:51, msarett1 wrote: > nit: Maybe "device" isn't the right word here? Could say "encoded" or just > "xforms to PCS" Acknowledged. https://codereview.chromium.org/2449243003/diff/120001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform_A2B.cpp:275: addGamma(ApplyParametric( On 2016/11/11 14:36:51, msarett1 wrote: > nit: Can this somehow fit on two lines? Not without making kRGB_Channels look like an argument to ApplyParametric() instead of addGamma() https://codereview.chromium.org/2449243003/diff/120001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform_A2B.cpp:282: if (SkGammas::Type::kTable_Type == gammas.type(channel)) { On 2016/11/11 14:36:51, msarett1 wrote: > nit: Can you make the code in this if statement a call to a static helper > function (that builds the inverse table)? Done. https://codereview.chromium.org/2449243003/diff/120001/src/opts/SkRasterPipel... File src/opts/SkRasterPipeline_opts.h (right): https://codereview.chromium.org/2449243003/diff/120001/src/opts/SkRasterPipel... src/opts/SkRasterPipeline_opts.h:614: //SkTSwap(r, b); On 2016/11/11 14:36:51, msarett1 wrote: > What's going on here? Debugging artifact. Removed. https://codereview.chromium.org/2449243003/diff/120001/tests/ColorSpaceXformT... File tests/ColorSpaceXformTest.cpp (right): https://codereview.chromium.org/2449243003/diff/120001/tests/ColorSpaceXformT... tests/ColorSpaceXformTest.cpp:64: const auto diff = SkTAbs(x - y); On 2016/11/11 14:36:51, msarett1 wrote: > nit: Can we have two versions of almost_equal()? One for XYZ and one for A2B? I just changed it back to the old one - I guess with the new code the greater lenience for small values wasn't needed. It's probably because we now LERP tables.
With the caveat that I read only files with "SkRasterPipeline" in their names, this looks great. lgtm! go go go!
https://codereview.chromium.org/2449243003/diff/140001/src/opts/SkRasterPipel... File src/opts/SkRasterPipeline_opts.h (right): https://codereview.chromium.org/2449243003/diff/140001/src/opts/SkRasterPipel... src/opts/SkRasterPipeline_opts.h:14: #include "SkMatrix44.h" This is probably no longer needed.
https://codereview.chromium.org/2449243003/diff/140001/src/opts/SkRasterPipel... File src/opts/SkRasterPipeline_opts.h (right): https://codereview.chromium.org/2449243003/diff/140001/src/opts/SkRasterPipel... src/opts/SkRasterPipeline_opts.h:14: #include "SkMatrix44.h" On 2016/11/11 21:20:57, mtklein_C wrote: > This is probably no longer needed. Removed. SkColorSpace_Base isn't needed either after moving SkColorLookUpTable to its own file as well.
The CQ bit was checked by raftias@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@chromium.org, mtklein@chromium.org Link to the patchset: https://codereview.chromium.org/2449243003/#ps160001 (title: "responding to comments")
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
Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
The CQ bit was checked by raftias@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@chromium.org, mtklein@chromium.org Link to the patchset: https://codereview.chromium.org/2449243003/#ps180001 (title: "fixed compile error on certain trybots related to RVO move-elision")
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
Try jobs failed on following builders: Test-Android-Clang-Nexus5-GPU-Adreno330-arm-Release-GN_Android-Trybot on master.client.skia.android (JOB_FAILED, http://build.chromium.org/p/client.skia.android/builders/Test-Android-Clang-N...)
The CQ bit was checked by raftias@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 ========== Initial implementation of a SkColorSpace_A2B xform There is support for all features of SkColorSpace_A2B. Tests for these functionality were adapted from the XYZ xform, plus a CLUT-specific test was added. Shared functions used by both SkColorSpaceXform_XYZ and SkColorSpaceXform_A2B have been moved into a shared header. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2449243003 CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ========== to ========== Initial implementation of a SkColorSpace_A2B xform There is support for all features of SkColorSpace_A2B. Tests for these functionality were adapted from the XYZ xform, plus a CLUT-specific test was added. Shared functions used by both SkColorSpaceXform_XYZ and SkColorSpaceXform_A2B have been moved into a shared header. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2449243003 CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot Committed: https://skia.googlesource.com/skia/+/2563601fc2b0505619f905f86bd249ae630197cc ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://skia.googlesource.com/skia/+/2563601fc2b0505619f905f86bd249ae630197cc |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
