Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2084673002:
Use a table-based implementation of SkDefaultXform (Closed)
DescriptionUse a table-based implementation of SkDefaultXform
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084673002
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
Committed: https://skia.googlesource.com/skia/+/b39067696ad08a26bbe49b71a71f0546dc42a075
Patch Set 1 #
Total comments: 17
Patch Set 2 : Response to comments #
Total comments: 1
Patch Set 3 : Don't loop over floats on the outTable #Patch Set 4 : Accidental rebase - sorry #
Total comments: 10
Patch Set 5 : Response to comments #
Total comments: 2
Patch Set 6 : quickEquals() #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 26 (11 generated)
Description was changed from ========== Improve implementation of SkDefaultXform BUG=skia: ========== to ========== Improve implementation of SkDefaultXform BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084673002 CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ==========
Description was changed from ========== Improve implementation of SkDefaultXform BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084673002 CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ========== to ========== Use a table-based implementation of SkDefaultXform BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084673002 CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
msarett@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:577: void SkDefaultXform::xform_RGB1_8888(uint32_t* dst, const uint32_t* src, uint32_t len) const { This is a functions that we can actually optimize. https://codereview.chromium.org/2084673002/diff/40001/tests/ColorSpaceXformTe... File tests/ColorSpaceXformTest.cpp (right): https://codereview.chromium.org/2084673002/diff/40001/tests/ColorSpaceXformTe... tests/ColorSpaceXformTest.cpp:39: 0xFF32AB52, 0xFF0383BC, 0xFF000102, 0xFFFFFFFF, 0xFFDDEEFF, }; A little more test coverage of the lower end of the curve... https://codereview.chromium.org/2084673002/diff/40001/tests/ColorSpaceXformTe... tests/ColorSpaceXformTest.cpp:98: // Note that the function is continuous (it's actually sRGB). The original test that I created doesn't round-trip because Y(1) = 1.125 (which is illegal). The sRGB curve makes for a good test though. https://codereview.chromium.org/2084673002/diff/40001/tests/ColorSpaceXformTe... tests/ColorSpaceXformTest.cpp:111: red.fValue = green.fValue = blue.fValue = 1.4f; The old value was very steep, leading to some large pixel diffs - this allows us to keep tolerance values low.
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:170: outTable[i] = powf(byte_to_float(i), exponent); Might be clearer and/or faster to just walk the float? for (float f = 0.0f; f <= 1.0f; f += (1/255.0f) { *outTable++ = powf(f, exponent); } Same sort of thing below too. https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:175: static float interp_lut(float input, float* table, uint32_t tableSize) { const float* table? https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:182: static void build_table_linear_from_gamma(float* outTable, float* inTable, uint32_t inTableSize) { // outTable is always 256 entries. inTable can be larger or smaller. That right? const float* inTable? https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:419: (float*) SK_OPTS_NS::linear_from_srgb; This is weird to refer to symbols in SK_OPTS_NS. Let's put these tables in here and just declare them in SkColorXform_opts.h? (This will also reduce duplication... there are probably identical sk_default::linear_from_srgb and an sk_sse41::linear_from_srgb tables right now.) https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.h (right): https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.h:66: float* fSrcGammaTables[3]; const float* for these pointers? Presumably we're not going to be changing those precomputed tables... https://codereview.chromium.org/2084673002/diff/40001/src/opts/SkColorXform_o... File src/opts/SkColorXform_opts.h (right): https://codereview.chromium.org/2084673002/diff/40001/src/opts/SkColorXform_o... src/opts/SkColorXform_opts.h:263: inline void color_xform_RGB1_srgb_to_2dot2(uint32_t* dst, const uint32_t* src, int len, ? These should probably be best as static inline.
brianosman@google.com changed reviewers: + brianosman@google.com
https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:170: outTable[i] = powf(byte_to_float(i), exponent); On 2016/06/21 20:32:52, mtklein_C wrote: > Might be clearer and/or faster to just walk the float? > > for (float f = 0.0f; f <= 1.0f; f += (1/255.0f) { > *outTable++ = powf(f, exponent); > } > > Same sort of thing below too. But 1/255 isn't exact in float, so you could be setting yourself up for some (slight) additional error.
https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:170: outTable[i] = powf(byte_to_float(i), exponent); On 2016/06/21 20:38:17, Brian Osman wrote: > On 2016/06/21 20:32:52, mtklein_C wrote: > > Might be clearer and/or faster to just walk the float? > > > > for (float f = 0.0f; f <= 1.0f; f += (1/255.0f) { > > *outTable++ = powf(f, exponent); > > } > > > > Same sort of thing below too. > > But 1/255 isn't exact in float, so you could be setting yourself up for some > (slight) additional error. Interesting thoughts. I would guess not "faster" enough to matter since powf() is so slow... I would also guess not "less accurate" enough to matter given that the backward transform with LUT is so inaccurate... I lean towards walking the floats (if it works), just seems like nicer code... Ahhh but I'm indifferent. https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:175: static float interp_lut(float input, float* table, uint32_t tableSize) { On 2016/06/21 20:32:52, mtklein_C wrote: > const float* table? Done. https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:182: static void build_table_linear_from_gamma(float* outTable, float* inTable, uint32_t inTableSize) { On 2016/06/21 20:32:52, mtklein_C wrote: > // outTable is always 256 entries. inTable can be larger or smaller. > > That right? > > const float* inTable? Done. https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:419: (float*) SK_OPTS_NS::linear_from_srgb; On 2016/06/21 20:32:52, mtklein_C wrote: > This is weird to refer to symbols in SK_OPTS_NS. Let's put these tables in here > and just declare them in SkColorXform_opts.h? > > (This will also reduce duplication... there are probably identical > sk_default::linear_from_srgb and an sk_sse41::linear_from_srgb tables right > now.) SGTM. Done. https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.h (right): https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.h:66: float* fSrcGammaTables[3]; On 2016/06/21 20:32:52, mtklein_C wrote: > const float* for these pointers? Presumably we're not going to be changing > those precomputed tables... Done. https://codereview.chromium.org/2084673002/diff/40001/src/opts/SkColorXform_o... File src/opts/SkColorXform_opts.h (right): https://codereview.chromium.org/2084673002/diff/40001/src/opts/SkColorXform_o... src/opts/SkColorXform_opts.h:263: inline void color_xform_RGB1_srgb_to_2dot2(uint32_t* dst, const uint32_t* src, int len, On 2016/06/21 20:32:52, mtklein_C wrote: > ? > > These should probably be best as static inline. Think we can just go back to static now that I'm not including this file anymore? https://codereview.chromium.org/2084673002/diff/60001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2084673002/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:444: static uint8_t clamp_normalized_float_to_byte(float v) { Just moving this closer to where it's used.
On 2016/06/21 20:38:17, Brian Osman wrote: > https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... > File src/core/SkColorSpaceXform.cpp (right): > > https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... > src/core/SkColorSpaceXform.cpp:170: outTable[i] = powf(byte_to_float(i), > exponent); > On 2016/06/21 20:32:52, mtklein_C wrote: > > Might be clearer and/or faster to just walk the float? > > > > for (float f = 0.0f; f <= 1.0f; f += (1/255.0f) { > > *outTable++ = powf(f, exponent); > > } > > > > Same sort of thing below too. > > But 1/255 isn't exact in float, so you could be setting yourself up for some > (slight) additional error. Right there with you in principle. Luckily, I checked... we're good in this case. #include <stdio.h> #include <string.h> int main(void) { int i = 0; for (float f = 0.0f; f <= 1.0f; f += (1/255.0f)) { unsigned hex; memcpy(&hex, &f, 4); printf("%d\t%15f\t%08x\n", i++, f, hex); } return 0; } ~~> 0 0.000000000000000 00000000 1 0.003921568859369 3b808081 2 0.007843137718737 3c008081 ... 253 0.992156863212585 3f7dfdfe 254 0.996078431606293 3f7efeff 255 1.000000000000000 3f800000
On 2016/06/21 21:35:10, mtklein wrote: > On 2016/06/21 20:38:17, Brian Osman wrote: > > > https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... > > File src/core/SkColorSpaceXform.cpp (right): > > > > > https://codereview.chromium.org/2084673002/diff/40001/src/core/SkColorSpaceXf... > > src/core/SkColorSpaceXform.cpp:170: outTable[i] = powf(byte_to_float(i), > > exponent); > > On 2016/06/21 20:32:52, mtklein_C wrote: > > > Might be clearer and/or faster to just walk the float? > > > > > > for (float f = 0.0f; f <= 1.0f; f += (1/255.0f) { > > > *outTable++ = powf(f, exponent); > > > } > > > > > > Same sort of thing below too. > > > > But 1/255 isn't exact in float, so you could be setting yourself up for some > > (slight) additional error. > > Right there with you in principle. Luckily, I checked... we're good in this > case. > > #include <stdio.h> > #include <string.h> > > int main(void) { > int i = 0; > for (float f = 0.0f; f <= 1.0f; f += (1/255.0f)) { > unsigned hex; > memcpy(&hex, &f, 4); > printf("%d\t%15f\t%08x\n", i++, f, hex); > } > return 0; > } > > > ~~> > > 0 0.000000000000000 00000000 > 1 0.003921568859369 3b808081 > 2 0.007843137718737 3c008081 > ... > 253 0.992156863212585 3f7dfdfe > 254 0.996078431606293 3f7efeff > 255 1.000000000000000 3f800000 Turns out we don't end up at 1 when trying a similar loop from 0 to 1 with increments of 1/1023 (thanks Mike!). Keeping my changes to forward loops, but undoing my changes to the backward loops.
https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpaceX... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:149: extern const float linear_from_srgb[256] = { Anything extern is going into the global namespace, so should probably get an sk_ prefix at least. https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:469: static float inverse_interp_lut(float input, float* table, uint32_t tableSize) { what's the deal with size_t -> uint32_t? Counts in Skia are generally int, and occassionally size_t for bytes. https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:549: (float*) linear_from_srgb; what's the point of the (float*) cast here? Doesn't it do that automatically? https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpace_... File src/core/SkColorSpace_Base.h (right): https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_Base.h:87: (this->fTableSize == that.fTableSize) && (this->fTable == that.fTable) && Don't we need to compare the table entries? https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_Base.h:163: SkGammas* gammas() const { return fGammas.get(); } It's not usually a good idea for a const method to return an non-const pointer to the object's internals. Can this be const SkGammas* ?
https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpaceX... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:149: extern const float linear_from_srgb[256] = { On 2016/06/22 14:59:54, mtklein_C wrote: > Anything extern is going into the global namespace, so should probably get an > sk_ prefix at least. Done. https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:469: static float inverse_interp_lut(float input, float* table, uint32_t tableSize) { On 2016/06/22 14:59:53, mtklein_C wrote: > what's the deal with size_t -> uint32_t? > > Counts in Skia are generally int, and occassionally size_t for bytes. Sounds like this should be "int". Changed in a bunch of places. https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:549: (float*) linear_from_srgb; On 2016/06/22 14:59:54, mtklein_C wrote: > what's the point of the (float*) cast here? Doesn't it do that automatically? Done, yeah it's fine without it. I used to need it, but now I can't remember why... I think maybe I was missing a const somewhere. https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpace_... File src/core/SkColorSpace_Base.h (right): https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_Base.h:87: (this->fTableSize == that.fTableSize) && (this->fTable == that.fTable) && On 2016/06/22 14:59:54, mtklein_C wrote: > Don't we need to compare the table entries? Yes, this doesn't work for tables. I'm punting on this, see comments from SkColorSpaceXform.cpp: // Check if this curve matches the first curve. In this case, we can // share the same table pointer. Logically, this should almost always // be true. I've never seen a profile where all three gamma curves // didn't match. But it is possible that they won't. // TODO (msarett): // This comparison won't catch the case where each gamma curve has a // pointer to its own look-up table (but the tables actually match). // Should we perform a deep compare of gamma tables here? Or should // we catch this when parsing the profile? Or should we not worry // about a bit of redundant work? I think it's a bit misleading to type "==" and then have the implementation do a mem compare of arbitrarily large tables... though maybe that's what I'll end up deciding to do. https://codereview.chromium.org/2084673002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_Base.h:163: SkGammas* gammas() const { return fGammas.get(); } On 2016/06/22 14:59:54, mtklein_C wrote: > It's not usually a good idea for a const method to return an non-const pointer > to the object's internals. Can this be const SkGammas* ? Done.
https://codereview.chromium.org/2084673002/diff/120001/src/core/SkColorSpace_... File src/core/SkColorSpace_Base.h (right): https://codereview.chromium.org/2084673002/diff/120001/src/core/SkColorSpace_... src/core/SkColorSpace_Base.h:85: inline bool operator==(const SkGammaCurve& that) const { I think it's misleading for operator==() to be anything other than completely correct; this function returns true if *this and that are observationally equivalent, and false if they are not. The reason for my strong stance on this is that == is very easy to overlook. It tends to be skimmed over, and there's no way for the reader to see context or caveats about when it might not exactly mean equals. Let's drop !=, name this something like quickEquals(), and note that it can have false negatives but no false positives?
https://codereview.chromium.org/2084673002/diff/120001/src/core/SkColorSpace_... File src/core/SkColorSpace_Base.h (right): https://codereview.chromium.org/2084673002/diff/120001/src/core/SkColorSpace_... src/core/SkColorSpace_Base.h:85: inline bool operator==(const SkGammaCurve& that) const { On 2016/06/22 20:23:00, mtklein wrote: > I think it's misleading for operator==() to be anything other than completely > correct; this function returns true if *this and that are observationally > equivalent, and false if they are not. > > The reason for my strong stance on this is that == is very easy to overlook. It > tends to be skimmed over, and there's no way for the reader to see context or > caveats about when it might not exactly mean equals. > > Let's drop !=, name this something like quickEquals(), and note that it can have > false negatives but no false positives? sgtm, I think you're right, this would be a nasty state to leave things in. Not being able to check equality of color spaces is a problem that has popped up elsewhere as well, I will be coming back to this.
lgtm
The CQ bit was checked by msarett@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/2084673002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2084673002/140001
Message was sent while issue was closed.
Description was changed from ========== Use a table-based implementation of SkDefaultXform BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084673002 CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ========== to ========== Use a table-based implementation of SkDefaultXform BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084673002 CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot Committed: https://skia.googlesource.com/skia/+/b39067696ad08a26bbe49b71a71f0546dc42a075 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://skia.googlesource.com/skia/+/b39067696ad08a26bbe49b71a71f0546dc42a075 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
