Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2038823002:
Add SkDefaultXform as a catch-all to handle color conversions (Closed)
DescriptionAdd SkDefaultXform as a catch-all to handle color conversions
I'd like to start optimizing the common case for color xforms,
but before doing that, I think it makes sense to have correct
code to support all xforms.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2038823002
Committed: https://skia.googlesource.com/skia/+/dc27a648d2ff23b2e96232c00c15976c46e1d48d
Patch Set 1 #Patch Set 2 : Comments and FIXMEs #Patch Set 3 : Add tests and fix a few things #
Total comments: 27
Patch Set 4 : Response to comments #
Total comments: 2
Patch Set 5 : #Patch Set 6 : Include SkColorSpace_Base #
Messages
Total messages: 29 (15 generated)
Description was changed from ========== Add SkDefaultXform as a catch-all to handle color space versions I'd like to start optimizing the common case for color xforms, but before doing that, I think it makes sense to have correct code to support all xforms. BUG=skia: ========== to ========== Add SkDefaultXform as a catch-all to handle color space versions I'd like to start optimizing the common case for color xforms, but before doing that, I think it makes sense to have correct code to support all xforms. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2038823002 ==========
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
msarett@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:679: const float scale = 65535.0 / 32768.0; We now support testing on Upper_Left.jpg (because we have table lookups). Which reminds me that I never bothered to scale this type of matrix correctly (also used in Upper_Left.jpg).
https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:679: const float scale = 65535.0 / 32768.0; On 2016/06/03 19:12:12, msarett wrote: > We now support testing on Upper_Left.jpg (because we have table lookups). Which > reminds me that I never bothered to scale this type of matrix correctly (also > used in Upper_Left.jpg). nit: I think this can be constexpr? https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:24: if (!srcSpace || !dstSpace || as_CSB(srcSpace)->colorLUT() || as_CSB(dstSpace)->colorLUT()) { Just making sure I understand - colorLUT is not yet supported? https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:53: static float byte_to_float(uint8_t v) { Should this be inline? https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:121: float index = byte * (1.0f / 255.0f) * (tableSize - 1); nit: byte * (1.0f / 255.0f) Won't "byte" get promoted to float, and then this is the the same as saying byte_to_float(byte) * (tableSize - 1) ? https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:123: return table[(int) floor(index)] * (1.0f - diff) + table[(int) ceil(index)] * diff; Note: We have macros for sk_float_floor2int and sk_ceil_floor2int. Maybe you should use them here? https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:127: // have strange results when the table is non-increasing. But any sane gamma Have you seen any insane gamma functions? In that case, do we just return a strange result, or do we avoid calling this method? https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:136: for (uint32_t i = 1; i < tableSize; i++) { Would it be worth it to use something faster than a linear search? https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:223: // Is this math safe? Are we at risk of dividing by zero? The risk is dividing by E or G. Those were read from the input. Did you correct them if they were zero? (Is zero wrong?) If not, the stream can be arbitrary - including garbage - so we need to catch it somehow. https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:234: *dst = SkPackARGB32NoCheck(((*src >> 24) & 0xFF), Why is it safe to use the NoCheck version? Is this unpremultiplied? (I don't see that documented anywhere.) https://codereview.chromium.org/2038823002/diff/80001/tests/ColorSpaceXformTe... File tests/ColorSpaceXformTest.cpp (right): https://codereview.chromium.org/2038823002/diff/80001/tests/ColorSpaceXformTe... tests/ColorSpaceXformTest.cpp:27: const uint32_t srcPixels[width] = { can this be constexpr too? https://codereview.chromium.org/2038823002/diff/80001/tests/ColorSpaceXformTe... tests/ColorSpaceXformTest.cpp:73: // Parametric gamma curves Since this appears to be a whole new test that doesn't reuse anything, it might be clearer to make a new DEF_TEST here. https://codereview.chromium.org/2038823002/diff/80001/tests/ColorSpaceXformTe... tests/ColorSpaceXformTest.cpp:93: // Exponential gamma curves Again, this could probably be a new DEF_TEST.
Description was changed from ========== Add SkDefaultXform as a catch-all to handle color space versions I'd like to start optimizing the common case for color xforms, but before doing that, I think it makes sense to have correct code to support all xforms. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2038823002 ========== to ========== Add SkDefaultXform as a catch-all to handle color conversions I'd like to start optimizing the common case for color xforms, but before doing that, I think it makes sense to have correct code to support all xforms. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2038823002 ==========
Patchset #4 (id:100001) has been deleted
https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:679: const float scale = 65535.0 / 32768.0; On 2016/06/06 14:06:44, scroggo wrote: > On 2016/06/03 19:12:12, msarett wrote: > > We now support testing on Upper_Left.jpg (because we have table lookups). > Which > > reminds me that I never bothered to scale this type of matrix correctly (also > > used in Upper_Left.jpg). > > nit: I think this can be constexpr? Ahh yes! https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:24: if (!srcSpace || !dstSpace || as_CSB(srcSpace)->colorLUT() || as_CSB(dstSpace)->colorLUT()) { On 2016/06/06 14:06:44, scroggo wrote: > Just making sure I understand - colorLUT is not yet supported? Yes, this is a mix of unimplemented and invalid. I think it's more clear if I separate into two statements. FWIW, I don't know that we'll ever support colorLUTs on dstSpaces. But it may make sense to just ignore them... https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:53: static float byte_to_float(uint8_t v) { On 2016/06/06 14:06:44, scroggo wrote: > Should this be inline? Yeah I think so. "inline" is just a suggestion to the compiler (and I'm 99% sure this will get inlined regardless of whether I suggest it or not), but at the very least, this adds a bit of documentation. https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:121: float index = byte * (1.0f / 255.0f) * (tableSize - 1); On 2016/06/06 14:06:44, scroggo wrote: > nit: > > byte * (1.0f / 255.0f) > > Won't "byte" get promoted to float, and then this is the the same as saying > > byte_to_float(byte) * (tableSize - 1) > > ? Yes it is, thanks! https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:123: return table[(int) floor(index)] * (1.0f - diff) + table[(int) ceil(index)] * diff; On 2016/06/06 14:06:44, scroggo wrote: > Note: We have macros for sk_float_floor2int and sk_ceil_floor2int. Maybe you > should use them here? Agreed, done. https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:127: // have strange results when the table is non-increasing. But any sane gamma On 2016/06/06 14:06:44, scroggo wrote: > Have you seen any insane gamma functions? In that case, do we just return a > strange result, or do we avoid calling this method? I have not seen any insane gammas. If we did encounter some, I think we would return a strange result. In general, I have a lot of work left to do on destination profiles with strange gammas. I'm not even if sure if or how often these profiles exist. This code fills a gap in the implementation (I think we'll need this because qcms supports it), but is quite a bit less efficient/robust than qcms I think. Maybe it would be better for me to not include this at all (until I can do a better job)? And just do strange src gammas in this CL? But I think it's alright to leave it here, and add more verbose FIXMEs. Having it does help with testing. https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:136: for (uint32_t i = 1; i < tableSize; i++) { On 2016/06/06 14:06:44, scroggo wrote: > Would it be worth it to use something faster than a linear search? If we verify that this function is increasing beforehand, we could/should use a better search. I think this entire strategy is flawed though - I think it's more likely that I'll change this implementation than try to optimize it. https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:223: // Is this math safe? Are we at risk of dividing by zero? On 2016/06/06 14:06:44, scroggo wrote: > The risk is dividing by E or G. Those were read from the input. Did you correct > them if they were zero? (Is zero wrong?) If not, the stream can be arbitrary - > including garbage - so we need to catch it somehow. You're right. I added some checks and removed the FIXME. Also fixed a bug where I forgot to divide by A. This passed the test because A = 1 in the test. https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:234: *dst = SkPackARGB32NoCheck(((*src >> 24) & 0xFF), On 2016/06/06 14:06:44, scroggo wrote: > Why is it safe to use the NoCheck version? Is this unpremultiplied? (I don't see > that documented anywhere.) This function is (currently) opaque->opaque or unpremul->unpremul. There is a comment on the base class declaration of xform_RGBA_8888() that states that the output is not premultiplied. When we do add a premultiplication option to this function, it is probably most correct to premultiply the linear floats before applying the dst gamma. https://codereview.chromium.org/2038823002/diff/80001/tests/ColorSpaceXformTe... File tests/ColorSpaceXformTest.cpp (right): https://codereview.chromium.org/2038823002/diff/80001/tests/ColorSpaceXformTe... tests/ColorSpaceXformTest.cpp:27: const uint32_t srcPixels[width] = { On 2016/06/06 14:06:45, scroggo wrote: > can this be constexpr too? Yes I think so. https://codereview.chromium.org/2038823002/diff/80001/tests/ColorSpaceXformTe... tests/ColorSpaceXformTest.cpp:73: // Parametric gamma curves On 2016/06/06 14:06:45, scroggo wrote: > Since this appears to be a whole new test that doesn't reuse anything, it might > be clearer to make a new DEF_TEST here. SGTM https://codereview.chromium.org/2038823002/diff/80001/tests/ColorSpaceXformTe... tests/ColorSpaceXformTest.cpp:93: // Exponential gamma curves On 2016/06/06 14:06:45, scroggo wrote: > Again, this could probably be a new DEF_TEST. SGTM
https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:127: // have strange results when the table is non-increasing. But any sane gamma On 2016/06/06 17:33:44, msarett wrote: > On 2016/06/06 14:06:44, scroggo wrote: > > Have you seen any insane gamma functions? In that case, do we just return a > > strange result, or do we avoid calling this method? > > I have not seen any insane gammas. If we did encounter some, I think we would > return a strange result. > > In general, I have a lot of work left to do on destination profiles with strange > gammas. I'm not even if sure if or how often these profiles exist. This code > fills a gap in the implementation (I think we'll need this because qcms supports > it), but is quite a bit less efficient/robust than qcms I think. > > Maybe it would be better for me to not include this at all (until I can do a > better job)? And just do strange src gammas in this CL? But I think it's > alright to leave it here, and add more verbose FIXMEs. Having it does help with > testing. FIXME sounds good to me. https://codereview.chromium.org/2038823002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:136: for (uint32_t i = 1; i < tableSize; i++) { On 2016/06/06 17:33:44, msarett wrote: > On 2016/06/06 14:06:44, scroggo wrote: > > Would it be worth it to use something faster than a linear search? > > If we verify that this function is increasing beforehand, we could/should use a > better search. I think this entire strategy is flawed though - I think it's > more likely that I'll change this implementation than try to optimize it. sgtm https://codereview.chromium.org/2038823002/diff/120001/src/core/SkColorSpaceX... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2038823002/diff/120001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:246: } I think this should be an else statement, or else we'll still divide by 0?
https://codereview.chromium.org/2038823002/diff/120001/src/core/SkColorSpaceX... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2038823002/diff/120001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:246: } On 2016/06/06 17:40:50, scroggo wrote: > I think this should be an else statement, or else we'll still divide by 0? Wow yeah, thanks!
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/2038823002/140001
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-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
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/2038823002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
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/2038823002/160001
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
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/2038823002/#ps160001 (title: "Include SkColorSpace_Base")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038823002/160001
Message was sent while issue was closed.
Description was changed from ========== Add SkDefaultXform as a catch-all to handle color conversions I'd like to start optimizing the common case for color xforms, but before doing that, I think it makes sense to have correct code to support all xforms. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2038823002 ========== to ========== Add SkDefaultXform as a catch-all to handle color conversions I'd like to start optimizing the common case for color xforms, but before doing that, I think it makes sense to have correct code to support all xforms. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2038823002 Committed: https://skia.googlesource.com/skia/+/dc27a648d2ff23b2e96232c00c15976c46e1d48d ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://skia.googlesource.com/skia/+/dc27a648d2ff23b2e96232c00c15976c46e1d48d |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
