Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1726823002:
Set SkColorSpace object for PNGs and parse ICC profiles (Closed)
DescriptionSet SkColorSpace object for PNGs and parse ICC profiles
Code for ICC profile parsing adapted from:
https://codereview.chromium.org/1707033002/
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1726823002
Committed: https://skia.googlesource.com/skia/+/6a738217eaeb9aed45a07efdfc1ae52372430d87
Patch Set 1 #Patch Set 2 : Add fColorProfile to SkCodec #
Total comments: 14
Patch Set 3 : Refactored a bit, added guesses for "default" gAMA/cHRM #
Total comments: 23
Patch Set 4 : Response to comments / adds code to parse icc profiles #
Total comments: 13
Patch Set 5 : Actually add the test #Patch Set 6 : Response to comments from Patch Set 4 #
Total comments: 4
Patch Set 7 : Remove constexpr #
Total comments: 16
Patch Set 8 : Response to comments from Patch Set 7 #
Total comments: 4
Patch Set 9 : Adding comments to the code #
Total comments: 2
Patch Set 10 : Change ref lifecycle of colorSpace as discussed in person #Patch Set 11 : Rebase #
Messages
Total messages: 39 (13 generated)
Description was changed from ========== Set SkColorSpace object for PNGs BUG=skia: ========== to ========== Set SkColorSpace object for PNGs BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
msarett@google.com changed reviewers: + reed@google.com
Patchset #2 (id:20001) has been deleted
msarett@google.com changed reviewers: + scroggo@google.com
Obvious obstacles to this landing include: (1) SkColorSpace.h is not ready to be public. (2) SkColorSpace::NewFromICC() is not yet implemented. https://codereview.chromium.org/1726823002/diff/40001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1726823002/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:358: gamma = 0; I need to figure out the gamma value that means "linear". https://codereview.chromium.org/1726823002/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:369: toXYZD50.fMat[i] = 0.0f; I need to figure out a default for cHRM.
https://codereview.chromium.org/1726823002/diff/40001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1726823002/diff/40001/include/codec/SkCodec.h... include/codec/SkCodec.h:103: * Returns the color space associated with the codec. Does not affect ownership https://codereview.chromium.org/1726823002/diff/40001/include/codec/SkCodec.h... include/codec/SkCodec.h:512: SkCodec(const SkImageInfo&, SkStream*, SkColorSpace* colorSpace = nullptr); Takes ownership (I guess it already does with SkStream, and there is no comment there...) FWIW, I think you can leave out "colorSpace" and just write: SkColorSpace* = nullptr https://codereview.chromium.org/1726823002/diff/40001/include/codec/SkCodec.h... include/codec/SkCodec.h:643: SkAutoTDelete<SkColorSpace> fColorSpace; It looks like SkColorSpace is ref-counted, so this should be SkAutoTUnref. https://codereview.chromium.org/1726823002/diff/40001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1726823002/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:191: SkColorSpace** colorSpace) { add a comment for colorSpace? https://codereview.chromium.org/1726823002/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:314: // iCCP variables Can you move these variables closer to where they're used? None of these need to declared outside "if (colorSpace)", and with some rejiggering of your else/if blocks we can move them even closer. Taking this a bit further, this method is already very long. Why not make creating the colorSpace its own method? You will only need to call it at creation time, not at rewind time. I think it will look something like this: SkColorSpace* read_color_space(png_ptr, info_ptr) { // iCCP variables if (PNG_INFO_iCCP == png_get_iCCP(...)) { return SkColorSpace::NewICC(...); } if (png_get_valid(png_ptr, info_ptr, PNG_INFO_sRGB)) { return SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named); } // cHRM variables if (png_get_cHRM_XYZ_fixed(...) { // calculations... return SkColorSpace::NewRGB(...); } png_fixed_point gamma; if (PNG_INFO_gAMA == png_get_gAMA_fixed(...)) { // calculations... return SkColorSpace::NewRGB(...); } return nullptr; https://codereview.chromium.org/1726823002/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:381: // But should we guess sRGB? Are most images intended to be sRGB? According to the W3C, "untagged" images should be treated as sRGB. I'm guessing we can leave that decision up to the client, though. Not sure what Android wants by default.
I noticed that reed@'s doc stated that color spaces would be opt-in. Should we make reading the color space optional? (I guess they would have to also opt-in for their surfaces, so we shouldn't accidentally switch them to the new path just because they were using SkCodec.)
"I noticed that reed@'s doc stated that color spaces would be opt-in. Should we make reading the color space optional? (I guess they would have to also opt-in for their surfaces, so we shouldn't accidentally switch them to the new path just because they were using SkCodec.)" I'm hoping that this is lightweight enough that it won't affect performance. So the client can choose to use or not use the information with no bad effects. libpng does the work for this stuff regardless of whether we ask for it or not. Worst case, I think we'll convert 10 ints to floats. I'll need to measure though once this compiles. https://codereview.chromium.org/1726823002/diff/40001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1726823002/diff/40001/include/codec/SkCodec.h... include/codec/SkCodec.h:103: * Returns the color space associated with the codec. On 2016/02/24 13:58:12, scroggo wrote: > Does not affect ownership Done. https://codereview.chromium.org/1726823002/diff/40001/include/codec/SkCodec.h... include/codec/SkCodec.h:512: SkCodec(const SkImageInfo&, SkStream*, SkColorSpace* colorSpace = nullptr); On 2016/02/24 13:58:12, scroggo wrote: > Takes ownership (I guess it already does with SkStream, and there is no comment > there...) > > FWIW, I think you can leave out "colorSpace" and just write: > > SkColorSpace* = nullptr I'm going with "Refs the SkColorSpace*". Done. https://codereview.chromium.org/1726823002/diff/40001/include/codec/SkCodec.h... include/codec/SkCodec.h:643: SkAutoTDelete<SkColorSpace> fColorSpace; On 2016/02/24 13:58:12, scroggo wrote: > It looks like SkColorSpace is ref-counted, so this should be SkAutoTUnref. Yes I think you're right. https://codereview.chromium.org/1726823002/diff/40001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1726823002/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:191: SkColorSpace** colorSpace) { On 2016/02/24 13:58:12, scroggo wrote: > add a comment for colorSpace? Done. https://codereview.chromium.org/1726823002/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:314: // iCCP variables On 2016/02/24 13:58:12, scroggo wrote: > Can you move these variables closer to where they're used? None of these need to > declared outside "if (colorSpace)", and with some rejiggering of your else/if > blocks we can move them even closer. > > Taking this a bit further, this method is already very long. Why not make > creating the colorSpace its own method? You will only need to call it at > creation time, not at rewind time. I think it will look something like this: > > SkColorSpace* read_color_space(png_ptr, info_ptr) { > // iCCP variables > if (PNG_INFO_iCCP == png_get_iCCP(...)) { > return SkColorSpace::NewICC(...); > } > > if (png_get_valid(png_ptr, info_ptr, PNG_INFO_sRGB)) { > return SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named); > } > > // cHRM variables > if (png_get_cHRM_XYZ_fixed(...) { > // calculations... > return SkColorSpace::NewRGB(...); > } > > png_fixed_point gamma; > if (PNG_INFO_gAMA == png_get_gAMA_fixed(...)) { > // calculations... > return SkColorSpace::NewRGB(...); > } > > return nullptr; Done. https://codereview.chromium.org/1726823002/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:381: // But should we guess sRGB? Are most images intended to be sRGB? On 2016/02/24 13:58:12, scroggo wrote: > According to the W3C, "untagged" images should be treated as sRGB. I'm guessing > we can leave that decision up to the client, though. Not sure what Android wants > by default. I'm leaving this as is, in case we want to treat the absence of a color profile different than explicitly sRGB. But we could easily default to sRGB.
> "I noticed that reed@'s doc stated that color spaces would be opt-in. Should we > make reading the color space optional? (I guess they would have to also opt-in > for their surfaces, so we shouldn't accidentally switch them to the new path > just because they were using SkCodec.)" > > I'm hoping that this is lightweight enough that it won't affect performance. So > the client can choose to use or not use the information with no bad effects. > > libpng does the work for this stuff regardless of whether we ask for it or not. > Worst case, I think we'll convert 10 ints to floats. > > I'll need to measure though once this compiles. My concern was not performance - I want to make sure we don't change the behavior if the client didn't change anything. I don't know whether the opt-in path is finalized, but my guess is that *both* the SkImage (created from the SkCodec) and its destination need to be tagged in order to take the new path. But in the case where I'm wrong, and tagging the SkImage alone changes the behavior, then we should only provide the tag if it's asked for. https://codereview.chromium.org/1726823002/diff/60001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1726823002/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:515: * Refs SkColorSpace* It looks like this does not call ref on the SkColorSpace (which is for the better, since the caller will never need their ref). https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:174: png_charp name; Will png_get_iCCP let you pass nullptr for name? It looks like we never use it. https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:175: int compression; Do we need this variable? https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:181: // Compression can only ever be "deflate", so this is uninteresting. I didn't understand this comment the first time around. It might be clearer as one of : "compression" |compression| compression // Note lowercase 'c', so it matches the variable name. Although if you can do without it, I would say drop the variable entirely. The real question here is, what does "deflate" mean to us? Is |profile| compressed? Do we need to decompress it to use it? Or has libpng already done that for us? https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:182: // Could knowing the name of the profile ever be interesting? Maybe for debugging? I could see it being useful in theory. No idea how it's used in practice. Might be more helpful if it's stored on the SkColorSpace? https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:189: // Here we are ignoring the sRGB "intent". Should we use the "intent"? FIXME/TODO? What is the "intent"? https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:201: // This could be wrong. I don't see any guarantee from the PNG specification that FIXME? Maybe specify how you came up with this computation? https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:203: // values if it sees an sRGB chunk. It seems like we'll only reach here if we did not see an sRGB chunk? https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:223: // Here we use the identity matrix as a default. Might want a method on SkFloat3x3 to set to the identity? https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:519: nullptr, nullptr, nullptr, nullptr)) { You can leave off this last nullptr if you call read_color_space directly inside NewFromStream. https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:823: &numberPasses, &colorSpace)) { I think read_color_space looks like a good standalone method. But I don't see any reason to call it from inside read_header, Instead, I think you should do the following: if (!read_header(...)) { return nullptr; } SkColorSpace* colorSpace = read_color_space(&png_ptr, &info_ptr);
I had presumed that the steps were: 1. extract the colorspace from the codec 2. decide to attach it to the image and that we could always inject heuristics etc. between the two, so that our codec code could always return the data...
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Description was changed from ========== Set SkColorSpace object for PNGs BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Set SkColorSpace object for PNGs and parse ICC profiles Code for ICC profile parsing adapted from: https://codereview.chromium.org/1707033002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
> Obvious obstacles to this landing include: > (1) SkColorSpace.h is not ready to be public. I put it back in src/core. > (2) SkColorSpace::NewFromICC() is not yet implemented. Added Mike's implementation to this CL. I also added a test that parses an ICC profile from a png. https://codereview.chromium.org/1726823002/diff/60001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1726823002/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:515: * Refs SkColorSpace* On 2016/02/24 18:31:36, scroggo wrote: > It looks like this does not call ref on the SkColorSpace (which is for the > better, since the caller will never need their ref). Gotcha of course. The SkColorSpace* will be deleted with the codec if nobody calls getColorSpace() and then refs the colorSpace. https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:174: png_charp name; On 2016/02/24 18:31:36, scroggo wrote: > Will png_get_iCCP let you pass nullptr for name? It looks like we never use it. We need to pass these in or png_get_iCCP does nothing. I've added a comment. https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:175: int compression; On 2016/02/24 18:31:36, scroggo wrote: > Do we need this variable? We need to pass these in or png_get_iCCP does nothing. I've added a comment. https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:181: // Compression can only ever be "deflate", so this is uninteresting. On 2016/02/24 18:31:36, scroggo wrote: > I didn't understand this comment the first time around. It might be clearer as > one of : > > "compression" > |compression| > compression // Note lowercase 'c', so it matches the variable name. > > Although if you can do without it, I would say drop the variable entirely. > > The real question here is, what does "deflate" mean to us? Is |profile| > compressed? Do we need to decompress it to use it? Or has libpng already done > that for us? libpng has already decompressed the profile for us. And libpng only supports inflate/deflate decompression. I'll improve the comment. This variable is completely irrelevant, but we need to pass something in. https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:182: // Could knowing the name of the profile ever be interesting? Maybe for debugging? On 2016/02/24 18:31:36, scroggo wrote: > I could see it being useful in theory. No idea how it's used in practice. Might > be more helpful if it's stored on the SkColorSpace? Yes I think that'd be the place to put it. https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:189: // Here we are ignoring the sRGB "intent". Should we use the "intent"? On 2016/02/24 18:31:36, scroggo wrote: > FIXME/TODO? What is the "intent"? Done. https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:201: // This could be wrong. I don't see any guarantee from the PNG specification that On 2016/02/24 18:31:36, scroggo wrote: > FIXME? Maybe specify how you came up with this computation? The computation isn't wrong. What might be wrong is that we are treating possibly non-D50 XYZ values as if they are D50. To my knowledge, this is the best we can do. libpng doesn't to guarantee anything about "color temperature" D50 vs D65 etc. https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:203: // values if it sees an sRGB chunk. On 2016/02/24 18:31:36, scroggo wrote: > It seems like we'll only reach here if we did not see an sRGB chunk? Yes you're right, the comment is unclear. I'll rewrite it. https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:223: // Here we use the identity matrix as a default. On 2016/02/24 18:31:36, scroggo wrote: > Might want a method on SkFloat3x3 to set to the identity? I'll add a FIXME. Don't want to do this yet since I'm not sure that identity is the right default. https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:519: nullptr, nullptr, nullptr, nullptr)) { On 2016/02/24 18:31:36, scroggo wrote: > You can leave off this last nullptr if you call read_color_space directly inside > NewFromStream. Done. https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:823: &numberPasses, &colorSpace)) { On 2016/02/24 18:31:36, scroggo wrote: > I think read_color_space looks like a good standalone method. But I don't see > any reason to call it from inside read_header, Instead, I think you should do > the following: > > if (!read_header(...)) { > return nullptr; > } > > SkColorSpace* colorSpace = read_color_space(&png_ptr, &info_ptr); Done.
https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1726823002/diff/60001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:223: // Here we use the identity matrix as a default. On 2016/02/29 21:47:55, msarett wrote: > On 2016/02/24 18:31:36, scroggo wrote: > > Might want a method on SkFloat3x3 to set to the identity? > > I'll add a FIXME. Don't want to do this yet since I'm not sure that identity is > the right default. sgtm. FWIW, even if it's not the default, we could add a method - not a constructor. SkMatrix's constructor does not initialize its fields. Clients are expected to initialize them, possibly by calling reset() which sets to the identity. https://codereview.chromium.org/1726823002/diff/140001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1726823002/diff/140001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:216: // This is identical libpng's fixed point -> double conversion (except that nit: identical to* ? https://codereview.chromium.org/1726823002/diff/140001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:243: gammas.fVec[0] = gammas.fVec[1] = gammas.fVec[2] = ((float) gamma) * 0.00001f; Should this be a function? (Perhaps a static function in this file, unless we decide it's needed elsewhere?) https://codereview.chromium.org/1726823002/diff/140001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1726823002/diff/140001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:191: static const size_t kICCHeaderSize = 132; Now that we are using C++11, should these be constexpr? https://codereview.chromium.org/1726823002/diff/140001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:222: bool init(const uint8_t* src, size_t len) { This method only ever returns true. Should it instead return void? https://codereview.chromium.org/1726823002/diff/140001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:237: return_if_false(fSize >= kICCHeaderSize, "Size is too small"); Hmm, this name doesn't tell me *what* it returns. I don't have a suggestion for a better name though.
FYI I forgot to upload the test in Patch Set 4, but it is uploaded now. https://codereview.chromium.org/1726823002/diff/140001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1726823002/diff/140001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:216: // This is identical libpng's fixed point -> double conversion (except that On 2016/02/29 22:18:49, scroggo wrote: > nit: identical to* ? Done. https://codereview.chromium.org/1726823002/diff/140001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:243: gammas.fVec[0] = gammas.fVec[1] = gammas.fVec[2] = ((float) gamma) * 0.00001f; On 2016/02/29 22:18:49, scroggo wrote: > Should this be a function? (Perhaps a static function in this file, unless we > decide it's needed elsewhere?) Yes that's a little clearer. https://codereview.chromium.org/1726823002/diff/140001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1726823002/diff/140001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:191: static const size_t kICCHeaderSize = 132; On 2016/02/29 22:18:49, scroggo wrote: > Now that we are using C++11, should these be constexpr? Sure sgtm https://codereview.chromium.org/1726823002/diff/140001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:222: bool init(const uint8_t* src, size_t len) { On 2016/02/29 22:18:49, scroggo wrote: > This method only ever returns true. Should it instead return void? Done. https://codereview.chromium.org/1726823002/diff/140001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:237: return_if_false(fSize >= kICCHeaderSize, "Size is too small"); On 2016/02/29 22:18:49, scroggo wrote: > Hmm, this name doesn't tell me *what* it returns. I don't have a suggestion for > a better name though. Me neither. I'm not opposed to typing out the actual code, if it's more clear.
to where did the colorspace header move?
On 2016/03/01 14:08:30, reed1 wrote: > to where did the colorspace header move? It's still there. Not sure why the "D" won't go away.
https://codereview.chromium.org/1726823002/diff/140001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1726823002/diff/140001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:243: gammas.fVec[0] = gammas.fVec[1] = gammas.fVec[2] = ((float) gamma) * 0.00001f; On 2016/03/01 00:39:34, msarett wrote: > On 2016/02/29 22:18:49, scroggo wrote: > > Should this be a function? (Perhaps a static function in this file, unless we > > decide it's needed elsewhere?) > > Yes that's a little clearer. Much clearer, thanks! https://codereview.chromium.org/1726823002/diff/140001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1726823002/diff/140001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:191: static const size_t kICCHeaderSize = 132; On 2016/03/01 00:39:35, msarett wrote: > On 2016/02/29 22:18:49, scroggo wrote: > > Now that we are using C++11, should these be constexpr? > > Sure sgtm Joke's on me - Visual Studio 2013 (which we're using) does not support constexpr :( https://codereview.chromium.org/1726823002/diff/180001/tests/ColorSpaceTest.cpp File tests/ColorSpaceTest.cpp (right): https://codereview.chromium.org/1726823002/diff/180001/tests/ColorSpaceTest.c... tests/ColorSpaceTest.cpp:2: * Copyright 2015 Google Inc. nit: 2016 https://codereview.chromium.org/1726823002/diff/180001/tests/ColorSpaceTest.c... tests/ColorSpaceTest.cpp:18: bool almost_equal(float a, float b) { I wanted to suggest you use SkScalarNearlyEqual, but that's for SkScalar, which theoretically may be a double at some point. ----------------------------------------------------------------- static?
https://codereview.chromium.org/1726823002/diff/140001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1726823002/diff/140001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:191: static const size_t kICCHeaderSize = 132; On 2016/03/01 14:42:29, scroggo wrote: > On 2016/03/01 00:39:35, msarett wrote: > > On 2016/02/29 22:18:49, scroggo wrote: > > > Now that we are using C++11, should these be constexpr? > > > > Sure sgtm > > Joke's on me - Visual Studio 2013 (which we're using) does not support constexpr > :( Back to const :) https://codereview.chromium.org/1726823002/diff/180001/tests/ColorSpaceTest.cpp File tests/ColorSpaceTest.cpp (right): https://codereview.chromium.org/1726823002/diff/180001/tests/ColorSpaceTest.c... tests/ColorSpaceTest.cpp:2: * Copyright 2015 Google Inc. On 2016/03/01 14:42:29, scroggo wrote: > nit: 2016 Done. https://codereview.chromium.org/1726823002/diff/180001/tests/ColorSpaceTest.c... tests/ColorSpaceTest.cpp:18: bool almost_equal(float a, float b) { On 2016/03/01 14:42:29, scroggo wrote: > I wanted to suggest you use SkScalarNearlyEqual, but that's for SkScalar, which > theoretically may be a double at some point. > > ----------------------------------------------------------------- > > static? Made this static.
https://codereview.chromium.org/1726823002/diff/200001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1726823002/diff/200001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:226: if (!(PNG_INFO_gAMA == png_get_gAMA_fixed(png_ptr, info_ptr, &gamma))) { nit: Why not if (PNG_INFO_gAMA != png_get_gAMA_fixed(...)) { https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:265: enum ICCIntents { Looks like this is never used? https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:335: static bool load_gamma(float* gamma, const uint8_t* src, size_t len) { This returns a bool, but the result is never used. Plan to use it in the future? https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:414: SkColorSpacePrintf("Not enough input data to read tag table entries.\n"); It seems like this should return null? (Otherwise we're going to read off the end in the next block?) https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:425: return nullptr; failure message? https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:443: !load_xyz(&toXYZ.fMat[6], b->addr((const uint8_t*) base), b->fLength)) { nit: hard to read this line as is. I find putting this { on the next line helps distinguish the condition from the block. https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.h File src/core/SkColorSpace.h (right): https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.... src/core/SkColorSpace.h:59: SkFloat3x3 xyz() const { return fToXYZD50; } Is this a useful method to have outside of debugging? Should it return a const reference? https://codereview.chromium.org/1726823002/diff/200001/tests/ColorSpaceTest.cpp File tests/ColorSpaceTest.cpp (right): https://codereview.chromium.org/1726823002/diff/200001/tests/ColorSpaceTest.c... tests/ColorSpaceTest.cpp:2: * Copyright 2015 Google Inc. nit: 2016
https://codereview.chromium.org/1726823002/diff/200001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1726823002/diff/200001/src/codec/SkPngCodec.c... src/codec/SkPngCodec.cpp:226: if (!(PNG_INFO_gAMA == png_get_gAMA_fixed(png_ptr, info_ptr, &gamma))) { On 2016/03/01 21:29:56, scroggo wrote: > nit: Why not > > if (PNG_INFO_gAMA != png_get_gAMA_fixed(...)) { Yes that's better :) https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:265: enum ICCIntents { On 2016/03/01 21:29:57, scroggo wrote: > Looks like this is never used? Yes we don't use this... I'll delete it and add a comment. https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:335: static bool load_gamma(float* gamma, const uint8_t* src, size_t len) { On 2016/03/01 21:29:56, scroggo wrote: > This returns a bool, but the result is never used. Plan to use it in the future? I think you could argue this should be void, since we are treating gamma as optional. But for now, let's use the return value to print an error if the gamma tag is missing or invalid. https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:414: SkColorSpacePrintf("Not enough input data to read tag table entries.\n"); On 2016/03/01 21:29:56, scroggo wrote: > It seems like this should return null? (Otherwise we're going to read off the > end in the next block?) Yes thanks. https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:425: return nullptr; On 2016/03/01 21:29:57, scroggo wrote: > failure message? Done. https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:443: !load_xyz(&toXYZ.fMat[6], b->addr((const uint8_t*) base), b->fLength)) { On 2016/03/01 21:29:57, scroggo wrote: > nit: hard to read this line as is. I find putting this { on the next line helps > distinguish the condition from the block. Done. https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.h File src/core/SkColorSpace.h (right): https://codereview.chromium.org/1726823002/diff/200001/src/core/SkColorSpace.... src/core/SkColorSpace.h:59: SkFloat3x3 xyz() const { return fToXYZD50; } On 2016/03/01 21:29:57, scroggo wrote: > Is this a useful method to have outside of debugging? Should it return a const > reference? Mike, can you weigh in this? const reference would be fine for my use case. I wrote it this way to match the getter for gamma(). https://codereview.chromium.org/1726823002/diff/200001/tests/ColorSpaceTest.cpp File tests/ColorSpaceTest.cpp (right): https://codereview.chromium.org/1726823002/diff/200001/tests/ColorSpaceTest.c... tests/ColorSpaceTest.cpp:2: * Copyright 2015 Google Inc. On 2016/03/01 21:29:57, scroggo wrote: > nit: 2016 Uggh sorry I actually fixed it this time.
is it reasonable to land the colorspace changes separately from the codec changes? https://codereview.chromium.org/1726823002/diff/220001/tests/ColorSpaceTest.cpp File tests/ColorSpaceTest.cpp (right): https://codereview.chromium.org/1726823002/diff/220001/tests/ColorSpaceTest.c... tests/ColorSpaceTest.cpp:33: REPORTER_ASSERT(r, 2.2f == gammas.fVec[0]); interesting that you don't need/use almost_equal here... 2.2 cannot be represented exactly in a float. https://codereview.chromium.org/1726823002/diff/220001/tests/ColorSpaceTest.c... tests/ColorSpaceTest.cpp:38: REPORTER_ASSERT(r, almost_equal(0.436066f, xyz.fMat[0])); // We got these 9 values from ... ?
> is it reasonable to land the colorspace changes separately from the codec > changes? Yeah I'm happy to. Is the motivation for this to get the color space changes checked in sooner? Or is this just too much for one CL? https://codereview.chromium.org/1726823002/diff/220001/tests/ColorSpaceTest.cpp File tests/ColorSpaceTest.cpp (right): https://codereview.chromium.org/1726823002/diff/220001/tests/ColorSpaceTest.c... tests/ColorSpaceTest.cpp:33: REPORTER_ASSERT(r, 2.2f == gammas.fVec[0]); On 2016/03/02 01:29:54, reed1 wrote: > interesting that you don't need/use almost_equal here... 2.2 cannot be > represented exactly in a float. I think we are lucky. Maybe we should use almost equal? This particular profile has a gamma table so we guess 2.2. I'm sure our "guess" is rounded to the closest float match for 2.2, which is likely the same as what happens to the 2.2 used in this test. https://codereview.chromium.org/1726823002/diff/220001/tests/ColorSpaceTest.c... tests/ColorSpaceTest.cpp:38: REPORTER_ASSERT(r, almost_equal(0.436066f, xyz.fMat[0])); On 2016/03/02 01:29:54, reed1 wrote: > // We got these 9 values from ... ? I printed out the profile when extracting it from a different PNG. These were the values. We couldn't actually check in the other PNG, so I stuffed the profile into color_wheel.png. I guess this tests that we are pulling out the "same values as before", but I feel confident these are the right values. If not, we can always change the test.
Alright, now I've responded to questions/comments with actual comments in the code :). I'll prepare a CL that splits off the color space stuff as a back-up, but I think this is pretty close and pretty safe as is.
lgtm https://codereview.chromium.org/1726823002/diff/240001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1726823002/diff/240001/include/codec/SkCodec.... include/codec/SkCodec.h:515: * Takes ownership of SkColorSpace* This is rarely our convention... to take ownership of a reference-counted object when its a parameter. We typically "ref" instead. Is that pattern difficult in this case?
https://codereview.chromium.org/1726823002/diff/240001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1726823002/diff/240001/include/codec/SkCodec.... include/codec/SkCodec.h:515: * Takes ownership of SkColorSpace* On 2016/03/04 20:22:08, reed1 wrote: > This is rarely our convention... to take ownership of a reference-counted object > when its a parameter. We typically "ref" instead. Is that pattern difficult in > this case? Changing the pattern :)
lgtm
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/1726823002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1726823002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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_...)
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/1726823002/#ps280001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1726823002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1726823002/280001
Message was sent while issue was closed.
Description was changed from ========== Set SkColorSpace object for PNGs and parse ICC profiles Code for ICC profile parsing adapted from: https://codereview.chromium.org/1707033002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Set SkColorSpace object for PNGs and parse ICC profiles Code for ICC profile parsing adapted from: https://codereview.chromium.org/1707033002/ 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/+/6a738217eaeb9aed45a07efdfc1ae52372430d87 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:280001) as https://skia.googlesource.com/skia/+/6a738217eaeb9aed45a07efdfc1ae52372430d87 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
