Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1928123002:
Introduce SkGammas type to represent ICC gamma curves (Closed)
DescriptionIntroduce SkGammas type to represent ICC gamma curves
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1928123002
Committed: https://skia.googlesource.com/skia/+/7b2c6dd8c918209cb92e1338905d511c68da3eb2
Committed: https://skia.googlesource.com/skia/+/ffc2aea3cb6981a5cc26f6c0f2ebf889ca5eb73f
Patch Set 1 : Difficulty passing ownership of SkGamma[] #Patch Set 2 : SkRGBGamma #Patch Set 3 : Rename and fix test #
Total comments: 4
Patch Set 4 : Move structs into SkColorSpace, hide implementation details #
Total comments: 10
Patch Set 5 : Response to comments #
Total comments: 4
Patch Set 6 : Fix bug :) #Patch Set 7 : Fix Mac clang #Patch Set 8 : Fix TSAN #Patch Set 9 : Rebase #Patch Set 10 : Fix Mac clang again #
Messages
Total messages: 59 (31 generated)
Description was changed from ========== Introduce SkGamma type to represent ICC gamma curves BUG=skia: ========== to ========== Introduce SkGamma type to represent ICC gamma curves BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Description was changed from ========== Introduce SkGamma type to represent ICC gamma curves BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Introduce SkGammas type to represent ICC gamma curves BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
msarett@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/1928123002/diff/80001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (left): https://codereview.chromium.org/1928123002/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:361: // If all of the gammas we encounter are 1.0f, indicate that we failed to load gammas. I don't think this is the right place for this optimization. https://codereview.chromium.org/1928123002/diff/80001/src/core/SkColorSpace.h File src/core/SkColorSpace.h (left): https://codereview.chromium.org/1928123002/diff/80001/src/core/SkColorSpace.h... src/core/SkColorSpace.h:50: SkColorLookUpTable(SkColorLookUpTable&& that) Now that we are on VS 2015, I think that this is no longer necessary. Should be implicit.
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/1928123002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928123002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by 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/1928123002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928123002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reed@google.com changed reviewers: + reed@google.com
Why do we need to make the specifics of the gamma curve public like this? We should assume that SkColorSpace.h will be public soon, and I wonder if clients like chrome or android need to see these (sub)types.
brianosman@google.com changed reviewers: + brianosman@google.com
https://codereview.chromium.org/1928123002/diff/80001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1928123002/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:311: // We should recognize commonly occurring tables and just set gamma to 2.2f. Best approach for this might just be to compute the error vs. pow(x, 2.2) as you're copying the table, and record the largest absolute difference, then we can just have a threshold for deciding if the curve is close enough.
Patchset #4 (id:100001) has been deleted
"Why do we need to make the specifics of the gamma curve public like this? We should assume that SkColorSpace.h will be public soon, and I wonder if clients like chrome or android need to see these (sub)types." You're right, I'm exposing way too much. I've moved my structs into SkColorSpace, and made most details private. SkGammas is exposed as a container for three floats (though it may be more complicated underneath). https://codereview.chromium.org/1928123002/diff/80001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1928123002/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:311: // We should recognize commonly occurring tables and just set gamma to 2.2f. On 2016/04/29 13:15:50, Brian Osman wrote: > Best approach for this might just be to compute the error vs. pow(x, 2.2) as > you're copying the table, and record the largest absolute difference, then we > can just have a threshold for deciding if the curve is close enough. I think that's a really good idea! I've also considered sampling a few arbitrary points and checking if they are a close match to some canonical curves. This may be a bit less computation, but also way less robust. I'm planning to handle this in a follow up. https://codereview.chromium.org/1928123002/diff/120001/src/core/SkColorSpace.h File src/core/SkColorSpace.h (right): https://codereview.chromium.org/1928123002/diff/120001/src/core/SkColorSpace.... src/core/SkColorSpace.h:40: struct SkGammaCurve { Strange to have the private structs first, but I need to define SkGammaCurve before SkGammas. https://codereview.chromium.org/1928123002/diff/120001/src/core/SkColorSpace.... src/core/SkColorSpace.h:96: float red() const { return fRed.fValue; } May not even need to expose these, but they are useful for testing.
https://codereview.chromium.org/1928123002/diff/120001/src/core/SkColorSpace.h File src/core/SkColorSpace.h (right): https://codereview.chromium.org/1928123002/diff/120001/src/core/SkColorSpace.... src/core/SkColorSpace.h:40: struct SkGammaCurve { On 2016/04/29 14:07:08, msarett wrote: > Strange to have the private structs first, but I need to define SkGammaCurve > before SkGammas. Would forward declaring them fix this? https://codereview.chromium.org/1928123002/diff/120001/src/core/SkColorSpace.... src/core/SkColorSpace.h:42: bool isTable() const { return 0 != fTableSize; } It seems this does not do anything to protect from isValue and isTable returning true? https://codereview.chromium.org/1928123002/diff/120001/src/core/SkColorSpace.... src/core/SkColorSpace.h:96: float red() const { return fRed.fValue; } On 2016/04/29 14:07:08, msarett wrote: > May not even need to expose these, but they are useful for testing. SkDEBUGCODE? https://codereview.chromium.org/1928123002/diff/120001/src/core/SkColorSpace.... src/core/SkColorSpace.h:97: float green() const { return fGreen.fValue; } Is it okay to return fValue if !isValue()?
https://codereview.chromium.org/1928123002/diff/120001/src/core/SkColorSpace.h File src/core/SkColorSpace.h (right): https://codereview.chromium.org/1928123002/diff/120001/src/core/SkColorSpace.... src/core/SkColorSpace.h:40: struct SkGammaCurve { On 2016/04/29 14:17:23, scroggo wrote: > On 2016/04/29 14:07:08, msarett wrote: > > Strange to have the private structs first, but I need to define SkGammaCurve > > before SkGammas. > > Would forward declaring them fix this? Unfortunately no, that doesn't work. https://codereview.chromium.org/1928123002/diff/120001/src/core/SkColorSpace.... src/core/SkColorSpace.h:42: bool isTable() const { return 0 != fTableSize; } On 2016/04/29 14:17:23, scroggo wrote: > It seems this does not do anything to protect from isValue and isTable returning > true? Good point. Now that this is private, that could only be a coding error. I'll add some asserts. https://codereview.chromium.org/1928123002/diff/120001/src/core/SkColorSpace.... src/core/SkColorSpace.h:96: float red() const { return fRed.fValue; } On 2016/04/29 14:17:23, scroggo wrote: > On 2016/04/29 14:07:08, msarett wrote: > > May not even need to expose these, but they are useful for testing. > > SkDEBUGCODE? Good idea! https://codereview.chromium.org/1928123002/diff/120001/src/core/SkColorSpace.... src/core/SkColorSpace.h:97: float green() const { return fGreen.fValue; } On 2016/04/29 14:17:23, scroggo wrote: > Is it okay to return fValue if !isValue()? In that case, we return 0.0f. This is a bit weird, though I'm not sure what else to return. It's probably better to just not have these APIs at all. So I think this is another +1 to making them SkDEBUGCODE.
https://codereview.chromium.org/1928123002/diff/140001/src/core/SkColorSpace.h File src/core/SkColorSpace.h (right): https://codereview.chromium.org/1928123002/diff/140001/src/core/SkColorSpace.... src/core/SkColorSpace.h:43: SkASSERT(!result || (!this->isTable() && !this->isParametric())); It looks like no one calls these yet. Are you sure these don't infinitely recurse? https://codereview.chromium.org/1928123002/diff/140001/tests/ColorSpaceTest.cpp File tests/ColorSpaceTest.cpp (right): https://codereview.chromium.org/1928123002/diff/140001/tests/ColorSpaceTest.c... tests/ColorSpaceTest.cpp:40: const SkColorSpace::SkGammas& gammas = colorSpace->gammas(); put gammas inside the SK_DEBUG code?
https://codereview.chromium.org/1928123002/diff/140001/src/core/SkColorSpace.h File src/core/SkColorSpace.h (right): https://codereview.chromium.org/1928123002/diff/140001/src/core/SkColorSpace.... src/core/SkColorSpace.h:43: SkASSERT(!result || (!this->isTable() && !this->isParametric())); On 2016/04/29 15:29:26, scroggo wrote: > It looks like no one calls these yet. Are you sure these don't infinitely > recurse? Ahh nice catch, thanks! https://codereview.chromium.org/1928123002/diff/140001/tests/ColorSpaceTest.cpp File tests/ColorSpaceTest.cpp (right): https://codereview.chromium.org/1928123002/diff/140001/tests/ColorSpaceTest.c... tests/ColorSpaceTest.cpp:40: const SkColorSpace::SkGammas& gammas = colorSpace->gammas(); On 2016/04/29 15:29:26, scroggo wrote: > put gammas inside the SK_DEBUG code? Done.
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/1928123002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928123002/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/1928123002/40002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928123002/40002
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/1928123002/#ps40002 (title: "Fix Mac clang")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928123002/40002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928123002/40002
Message was sent while issue was closed.
Description was changed from ========== Introduce SkGammas type to represent ICC gamma curves BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Introduce SkGammas type to represent ICC gamma curves BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/7b2c6dd8c918209cb92e1338905d511c68da3eb2 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:40002) as https://skia.googlesource.com/skia/+/7b2c6dd8c918209cb92e1338905d511c68da3eb2
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:40002) has been created in https://codereview.chromium.org/1933863002/ by msarett@google.com. The reason for reverting is: Breaks TSAN https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX....
Message was sent while issue was closed.
Description was changed from ========== Introduce SkGammas type to represent ICC gamma curves BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/7b2c6dd8c918209cb92e1338905d511c68da3eb2 ========== to ========== Introduce SkGammas type to represent ICC gamma curves BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/7b2c6dd8c918209cb92e1338905d511c68da3eb2 ==========
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/1928123002/#ps190001 (title: "Fix TSAN")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928123002/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928123002/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...) 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_...) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
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/1928123002/#ps210001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928123002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928123002/210001
The CQ bit was unchecked by commit-bot@chromium.org
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/1928123002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928123002/230001
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/1928123002/#ps230001 (title: "Fix Mac clang again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928123002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928123002/230001
Message was sent while issue was closed.
Description was changed from ========== Introduce SkGammas type to represent ICC gamma curves BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/7b2c6dd8c918209cb92e1338905d511c68da3eb2 ========== to ========== Introduce SkGammas type to represent ICC gamma curves BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/7b2c6dd8c918209cb92e1338905d511c68da3eb2 Committed: https://skia.googlesource.com/skia/+/ffc2aea3cb6981a5cc26f6c0f2ebf889ca5eb73f ==========
Message was sent while issue was closed.
Committed patchset #10 (id:230001) as https://skia.googlesource.com/skia/+/ffc2aea3cb6981a5cc26f6c0f2ebf889ca5eb73f |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
