Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1925753002:
Parse A2B0 tag on ICC profiles (Closed)
DescriptionParse A2B0 tag on ICC profiles
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1925753002
Committed: https://skia.googlesource.com/skia/+/85def2e0673f3b75c4500440b95ab3dac7435702
Patch Set 1 : #
Total comments: 35
Patch Set 2 : Response to comments #
Total comments: 2
Patch Set 3 : Fix 100 chars #Patch Set 4 : MSVS fixes #Patch Set 5 : Try again #Patch Set 6 : Explicit move constructor for MSVS 2013 #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 29 (14 generated)
Description was changed from ========== Parse A2B0 tag on ICC profiles BUG=skia: ========== to ========== Parse A2B0 tag on ICC profiles BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #1 (id:1) has been deleted
msarett@google.com changed reviewers: + scroggo@google.com
Sorry for the lack of the tests. I don't actually have a "public" image with one of these profiles. My long term plan is to add a set of "colorspace" images to the cloud and decode them in DM with some sort of CodecColorSpaceSrc. There's a lot that may change in follow-ups: Ex: New struct for gamma (value, or table, or parametric) Ex: The order of values in the Color LUT Ex: Maybe we combine toXYZ and toXYZOffset into one matrix https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:178: #define return_if_false(pred, msg) \ I think maybe in a follow-up change I'll create a SkColorSpacePriv.h and move a bunch of this stuff there? This file is getting really long. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:374: // We need to handle the possibility that the gamma curve does not correspond to 2.2f. Next up, I will probably create a gamma struct, where we can represent gamma as a value, curve, or parametric curve. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:375: static bool load_gammas(float* gammas, uint32_t numGammas, const uint8_t* src, size_t len) { This function is mostly the same minus a few changes mentioned below... https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:376: for (uint32_t i = 0; i < numGammas; i++) { Added a loop so we can handle multiple gamma curves stored consecutively in a profile. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:383: size_t tagBytes; Now we need to count the number of bytes in the tag, so we are able to move to the next tag on the next loop iteration. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:388: if (0 == count) { count == 0 now means unit gamma. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:422: switch(read_big_endian_short(src + 8)) { Added switch statement to determine how many bytes are in parametric gamma tags. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:448: // Adjust src and len if there is another gamma curve to load. The rest of this function is also new. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:666: if (r && g && b) { Moved a bunch of code into this block, but didn't actually change it. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h File src/core/SkColorSpace.h (right): https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h... src/core/SkColorSpace.h:90: SkColorSpace(const SkFloat3& gamma, const SkFloat3x3& toXYZ, Named); Reversed the order here to match the new constructor. I think it makes sense to list the parameters in the order in which they should be applied.
https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:178: #define return_if_false(pred, msg) \ On 2016/04/27 18:56:44, msarett wrote: > I think maybe in a follow-up change I'll create a SkColorSpacePriv.h and move a > bunch of this stuff there? This file is getting really long. Do you think this stuff will be used in other files? I would lean towards leaving it where it's used if not. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:379: return false; If the first gamma is valid, but the second is not (e.g. missing some bytes), we'll return false, correct? Should we instead return the first one? (Maybe this is a rare error case we need not support?) https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:383: size_t tagBytes; On 2016/04/27 18:56:45, msarett wrote: > Now we need to count the number of bytes in the tag, so we are able to move to > the next tag on the next loop iteration. Maybe these comments would be useful to have in the code? https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:450: // Each curve will be padded to 4-byte alignment. I found this sentence confusing - I thought you meant *you* will be padding them, but you're saying that they're already padded? Maybe this should say "Each curve is ..."? https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:482: colorLUT->fOutputChannels = outputChannels; Should we SkASSERT(inputChannels <= kMaxChannels && outputChannels <= kMaxChannels)? https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:570: // We read these in the order that the elements should be applied, rather than the It might be clearer to move these to where they're used? uint32_t offsetToACurves = read_big_endian_int(src + 28); uint32_t offsetToBCurves = read_big_endian_int(src + 12); if (0 != offsetToACurves || 0 != offsetToBCurves) { ... Wait, does this comment imply that BCurves will be applied last? https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:592: hasColorLUT = false; It looks like this is never used again. Do you need this local variable? (Same for the below has___ booleans.) https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:682: !load_gammas(&gamma.fVec[0], 1, r->addr((const uint8_t*) base), r->fLength)) Previously we required the length to be at least 14, but now we require only 12 bytes, but the caller did not get updated. Is that okay? https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h File src/core/SkColorSpace.h (right): https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h... src/core/SkColorSpace.h:38: struct SkColorLUT { LookUpTable? https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h... src/core/SkColorSpace.h:44: std::unique_ptr<float[]> fTable; Are we already switching to unique_ptr? https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h... src/core/SkColorSpace.h:46: SkColorLUT() {} Can you rely on the generated version? On the other hand, should we disable copy constructor, which won't do what we want? (Or does having the non-copyable member fTable do that for us?) https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h... src/core/SkColorSpace.h:90: SkColorSpace(const SkFloat3& gamma, const SkFloat3x3& toXYZ, Named); On 2016/04/27 18:56:45, msarett wrote: > Reversed the order here to match the new constructor. I think it makes sense to > list the parameters in the order in which they should be applied. sgtm
https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:178: #define return_if_false(pred, msg) \ On 2016/04/27 20:18:46, scroggo wrote: > On 2016/04/27 18:56:44, msarett wrote: > > I think maybe in a follow-up change I'll create a SkColorSpacePriv.h and move > a > > bunch of this stuff there? This file is getting really long. > > Do you think this stuff will be used in other files? I would lean towards > leaving it where it's used if not. sgtm https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:379: return false; On 2016/04/27 20:18:46, scroggo wrote: > If the first gamma is valid, but the second is not (e.g. missing some bytes), > we'll return false, correct? Should we instead return the first one? (Maybe this > is a rare error case we need not support?) I have no idea if we'd come across this error. I'll add a FIXME. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:383: size_t tagBytes; On 2016/04/27 20:18:46, scroggo wrote: > On 2016/04/27 18:56:45, msarett wrote: > > Now we need to count the number of bytes in the tag, so we are able to move to > > the next tag on the next loop iteration. > > Maybe these comments would be useful to have in the code? Done. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:450: // Each curve will be padded to 4-byte alignment. On 2016/04/27 20:18:46, scroggo wrote: > I found this sentence confusing - I thought you meant *you* will be padding > them, but you're saying that they're already padded? Maybe this should say "Each > curve is ..."? Done. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:482: colorLUT->fOutputChannels = outputChannels; On 2016/04/27 20:18:46, scroggo wrote: > Should we SkASSERT(inputChannels <= kMaxChannels && outputChannels <= > kMaxChannels)? Done. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:570: // We read these in the order that the elements should be applied, rather than the On 2016/04/27 20:18:46, scroggo wrote: > It might be clearer to move these to where they're used? > > uint32_t offsetToACurves = read_big_endian_int(src + 28); > uint32_t offsetToBCurves = read_big_endian_int(src + 12); > > if (0 != offsetToACurves || 0 != offsetToBCurves) { > ... > > Wait, does this comment imply that BCurves will be applied last? sgtm. Yes, the B curve will be applied last, if we ever find an image where there is a B curve to apply. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:592: hasColorLUT = false; On 2016/04/27 20:18:46, scroggo wrote: > It looks like this is never used again. Do you need this local variable? (Same > for the below has___ booleans.) Deleting them... At some point, we'll need to know if these tags exist, so we can apply them if necessary. But I haven't gotten that far yet. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:682: !load_gammas(&gamma.fVec[0], 1, r->addr((const uint8_t*) base), r->fLength)) On 2016/04/27 20:18:46, scroggo wrote: > Previously we required the length to be at least 14, but now we require only 12 > bytes, but the caller did not get updated. Is that okay? Yes, it appears that a gamma chunk with count == 0 is a way to specify that gamma = 1.0f. So now we allow gamma chunks to be only 12 bytes long (no table entries when count is 0). https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h File src/core/SkColorSpace.h (right): https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h... src/core/SkColorSpace.h:38: struct SkColorLUT { On 2016/04/27 20:18:46, scroggo wrote: > LookUpTable? sgtm https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h... src/core/SkColorSpace.h:44: std::unique_ptr<float[]> fTable; On 2016/04/27 20:18:46, scroggo wrote: > Are we already switching to unique_ptr? Is there a skia pointer that would work equally well here? I used this on a suggestion from Ben...was having trouble figuring out the right type to allow me to transfer ownership properly. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h... src/core/SkColorSpace.h:46: SkColorLUT() {} On 2016/04/27 20:18:46, scroggo wrote: > Can you rely on the generated version? I added this constructor in order to make this field const. Some ICC profiles don't use this - and then the compiler complains about leaving a const member uninitialized. I'm going to have to create some flags/bool to indicate which fields are actually being used - we'll need this when we start applying these profiles. Maybe zero is a good indicator of "not in use". I'll change this constructor to zero the struct? > On the other hand, should we disable copy > constructor, which won't do what we want? (Or does having the non-copyable > member fTable do that for us?) Having a noncopyable member makes this noncopyable. I tried to explicitly delete the copy constructor because I thought it would be good documentation. Ex: SkColorLookUpTable(const SkColorLookUpTable& that) = delete; But somehow that causes the use of std::move not to compile...
lgtm https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h File src/core/SkColorSpace.h (right): https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h... src/core/SkColorSpace.h:44: std::unique_ptr<float[]> fTable; On 2016/04/27 21:17:58, msarett wrote: > On 2016/04/27 20:18:46, scroggo wrote: > > Are we already switching to unique_ptr? > > Is there a skia pointer that would work equally well here? I used this on a > suggestion from Ben...was having trouble figuring out the right type to allow me > to transfer ownership properly. I know Ben was trying to convince us to switch from SkAutoTDelete to unique_ptr, and mtklein/Hal have been doing some conversions/steps towards switching. We also have SkAutoTDeleteArray, and both versions now inherit publicly from unique_ptr, so I think you could use our wrapper classes, but given that we are in the process of switching over I guess it's fine to switch early. https://codereview.chromium.org/1925753002/diff/20001/src/core/SkColorSpace.h... src/core/SkColorSpace.h:46: SkColorLUT() {} On 2016/04/27 21:17:58, msarett wrote: > On 2016/04/27 20:18:46, scroggo wrote: > > Can you rely on the generated version? > > I added this constructor in order to make this field const. Some ICC profiles > don't use this - and then the compiler complains about leaving a const member > uninitialized. > > I'm going to have to create some flags/bool to indicate which fields are > actually being used - we'll need this when we start applying these profiles. > Maybe zero is a good indicator of "not in use". I'll change this constructor to > zero the struct? sgtm > > > On the other hand, should we disable copy > > constructor, which won't do what we want? (Or does having the non-copyable > > member fTable do that for us?) > > Having a noncopyable member makes this noncopyable. > > I tried to explicitly delete the copy constructor because I thought it would be > good documentation. Ex: > SkColorLookUpTable(const SkColorLookUpTable& that) = delete; > > But somehow that causes the use of std::move not to compile... https://codereview.chromium.org/1925753002/diff/40001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1925753002/diff/40001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:106: SkColorSpace::SkColorSpace(SkColorLookUpTable colorLUT, const SkFloat3& gamma, const SkFloat3x3& toXYZD50, nit: over 100 chars
https://codereview.chromium.org/1925753002/diff/40001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1925753002/diff/40001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:106: SkColorSpace::SkColorSpace(SkColorLookUpTable colorLUT, const SkFloat3& gamma, const SkFloat3x3& toXYZD50, On 2016/04/28 12:16:02, scroggo wrote: > nit: over 100 chars Done.
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/1925753002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925753002/60001
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/1925753002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925753002/80001
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/1925753002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925753002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) 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/1925753002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925753002/120001
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/1925753002/#ps120001 (title: "Explicit move constructor for MSVS 2013")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1925753002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925753002/120001
Message was sent while issue was closed.
Description was changed from ========== Parse A2B0 tag on ICC profiles BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Parse A2B0 tag on ICC profiles 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/+/85def2e0673f3b75c4500440b95ab3dac7435702 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://skia.googlesource.com/skia/+/85def2e0673f3b75c4500440b95ab3dac7435702 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
