Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2000713003:
Add SkColorSpace to SkImageInfo (Closed)
DescriptionAdd SkColorSpace to SkImageInfo
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=2000713003
Committed: https://skia.googlesource.com/skia/+/23c5110ba291cfc303e3fdc73af99cd3d595df67
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Rebase #Patch Set 4 : Check for 2.2 or linear gamma #
Total comments: 4
Patch Set 5 : Response to comments #Patch Set 6 : Add SkColorSpace as alternate option #Patch Set 7 : #Patch Set 8 : Ref counting fixes and other fixes #
Total comments: 2
Patch Set 9 : Fix uses of SkImageInfo #Patch Set 10 : #
Total comments: 7
Messages
Total messages: 41 (16 generated)
Description was changed from ========== Add SkColorSpace to SkImageInfo BUG=skia: ========== to ========== Add SkColorSpace to SkImageInfo BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
msarett@google.com changed reviewers: + brianosman@google.com, herb@google.com, reed@google.com
https://codereview.chromium.org/2000713003/diff/1/include/codec/SkEncodedInfo.h File include/codec/SkEncodedInfo.h (left): https://codereview.chromium.org/2000713003/diff/1/include/codec/SkEncodedInfo... include/codec/SkEncodedInfo.h:120: SkColorProfileType profileType = SkDefaultColorProfile(); I don't feel good about this living in the codec and dictating what color space information we report. I want to work toward removing it. Let's discuss what it'll take. https://codereview.chromium.org/2000713003/diff/1/include/core/SkImageInfo.h File include/core/SkImageInfo.h (right): https://codereview.chromium.org/2000713003/diff/1/include/core/SkImageInfo.h#... include/core/SkImageInfo.h:256: bool isSRGBGamma() const { I don't like these... I think they are too limiting. Client might want to know unmarked (null colorSpace), linear (on the colorSpace), or srgb (on the colorspace). Let's just expose the colorSpace.
https://codereview.chromium.org/2000713003/diff/20001/include/core/SkImageInfo.h File include/core/SkImageInfo.h (right): https://codereview.chromium.org/2000713003/diff/20001/include/core/SkImageInf... include/core/SkImageInfo.h:246: sk_sp<SkColorSpace> colorSpace() const { return fColorSpace; } Can we avoid including this call until we have to. I suggest adding queries to SkImageInfo so it does not have to give out the reference. For example, to simulate profileType() == kSRGB_SkProfileType, we could have imageInfo.IsGamma20Ok()
This looks much simpler after a rebase. https://codereview.chromium.org/2000713003/diff/20001/include/core/SkImageInfo.h File include/core/SkImageInfo.h (right): https://codereview.chromium.org/2000713003/diff/20001/include/core/SkImageInf... include/core/SkImageInfo.h:246: sk_sp<SkColorSpace> colorSpace() const { return fColorSpace; } On 2016/05/20 21:19:13, herb_g wrote: > Can we avoid including this call until we have to. I suggest adding queries to > SkImageInfo so it does not have to give out the reference. For example, to > simulate profileType() == kSRGB_SkProfileType, we could have > imageInfo.IsGamma20Ok() Yes I think that's a good idea! I've added checks for linear and srgb gamma. I don't think we need to handle this now - but what might we want to know about gamut?
https://codereview.chromium.org/2000713003/diff/60001/include/core/SkImageInfo.h File include/core/SkImageInfo.h (right): https://codereview.chromium.org/2000713003/diff/60001/include/core/SkImageInf... include/core/SkImageInfo.h:245: SkColorProfileType profileType() const { return fProfileType; } would this guy be deprecated? https://codereview.chromium.org/2000713003/diff/60001/include/core/SkImageInf... include/core/SkImageInfo.h:261: return SkColorSpace::k2Dot2Curve_GammaNamed == fColorSpace->gammaNamed(); Must there be a colorspace object, or could it be null?
https://codereview.chromium.org/2000713003/diff/60001/include/core/SkImageInfo.h File include/core/SkImageInfo.h (right): https://codereview.chromium.org/2000713003/diff/60001/include/core/SkImageInf... include/core/SkImageInfo.h:245: SkColorProfileType profileType() const { return fProfileType; } On 2016/05/24 16:06:33, reed1 wrote: > would this guy be deprecated? Yes, though I think there is plenty of work to do in order to transition. Should we go ahead and mark it as deprecated? https://codereview.chromium.org/2000713003/diff/60001/include/core/SkImageInf... include/core/SkImageInfo.h:261: return SkColorSpace::k2Dot2Curve_GammaNamed == fColorSpace->gammaNamed(); On 2016/05/24 16:06:33, reed1 wrote: > Must there be a colorspace object, or could it be null? Ahhh yes this is a bug. It could definitely be null. In terms of images, I think we should say null (meaning unmarked) == 2Dot2. I've made this change. Will this break our assumptions anywhere else? In that case, I can pass a pointer to the sRGB color space when the image doesn't have one, so that way we can say null == Linear (and images will always be 2Dot2).
I'm still foggy here. My first thought (not always to be trusted) would be that null means legacy means linear...
Patchset #6 (id:100001) has been deleted
Leaving profile type alone, adding SkColorSpace for us to see if it works to start switching over.
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/2000713003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000713003/140001
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...)
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/2000713003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000713003/160001
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-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
Patchset #8 (id:160001) has been deleted
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/2000713003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000713003/180001
Please take another look... I've needed to make numerous fixes. I think I've found all of the issues. https://codereview.chromium.org/2000713003/diff/180001/src/core/SkBitmap.cpp File src/core/SkBitmap.cpp (right): https://codereview.chromium.org/2000713003/diff/180001/src/core/SkBitmap.cpp#... src/core/SkBitmap.cpp:32: this->fInfo = SkImageInfo::MakeUnknown(); I could also add a reset() function to SkImageInfo... https://codereview.chromium.org/2000713003/diff/180001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/2000713003/diff/180001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:409: memset(mipmap->writable_data(), 0, mipmap->size()); Not sure if this ok... I can also make it more fine-grained. If we interpret garbage memory as an SkImageInfo, and then try to unref its sk_sp, bad things happen...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
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/2000713003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000713003/200001
Cleaned up some of the fixes a little bit. I'll probably try to land this and see what breaks in blink.
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 reed@google.com Link to the patchset: https://codereview.chromium.org/2000713003/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2000713003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000713003/220001
Message was sent while issue was closed.
Description was changed from ========== Add SkColorSpace to SkImageInfo BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add SkColorSpace to SkImageInfo 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/+/23c5110ba291cfc303e3fdc73af99cd3d595df67 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:220001) as https://skia.googlesource.com/skia/+/23c5110ba291cfc303e3fdc73af99cd3d595df67
Message was sent while issue was closed.
scroggo@google.com changed reviewers: + scroggo@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/2000713003/diff/220001/include/core/SkImageIn... File include/core/SkImageInfo.h (right): https://codereview.chromium.org/2000713003/diff/220001/include/core/SkImageIn... include/core/SkImageInfo.h:244: SkColorSpace* colorSpace() const { return fColorSpace.get(); } Now that SkColorSpace is in include/, can this return a const ref to sk_sp? From Herb's comment [1], it looks like the goal was to not include SkColorSpace.h, but we still include it? [1] https://codereview.chromium.org/2000713003/diff/20001/include/core/SkImageInf... https://codereview.chromium.org/2000713003/diff/220001/include/core/SkImageIn... include/core/SkImageInfo.h:299: fProfileType == other.fProfileType && fColorSpace == other.fColorSpace; Doesn't this check whether fColorSpace is the same object as other.fColorSpace? Is that deliberate? It at least seems misleading. (I had to upload patch set 18 of crrev.com/1997703003 because two SkCodecs created from the same file had different color space objects, though I would expect them to be the same.)
Message was sent while issue was closed.
https://codereview.chromium.org/2000713003/diff/220001/include/core/SkImageIn... File include/core/SkImageInfo.h (right): https://codereview.chromium.org/2000713003/diff/220001/include/core/SkImageIn... include/core/SkImageInfo.h:244: SkColorSpace* colorSpace() const { return fColorSpace.get(); } On 2016/05/31 13:48:25, scroggo wrote: > Now that SkColorSpace is in include/, can this return a const ref to sk_sp? > Yeah I think it could. The reason I went with a bare ptr was so the client doesn't necessarily have to ref the color space. I think the const ref to sk_sp would accomplish the same thing. I'm not sure which is better (or what is "Skia style"). Whatever we decide here, we should do the same thing in SkCodec.h. > From Herb's comment [1], it looks like the goal was to not include > SkColorSpace.h, but we still include it? > > [1] > https://codereview.chromium.org/2000713003/diff/20001/include/core/SkImageInf... I'll look if we might be able to get away with a forward declare... My read of Herb's comment was a suggestion to expose information about the gamma rather than the entire SkColorSpace object. I ultimately decided against this approach for a few reasons: (1) I was wrapping APIs that are already on SkColorSpace. (2) What should we return when the SkColorSpace is nullptr? (3) The SkColorSpace API is already limited. Is there a reason to limit it further? (4) We can expose this object without forcing the client to ref it. https://codereview.chromium.org/2000713003/diff/220001/include/core/SkImageIn... include/core/SkImageInfo.h:299: fProfileType == other.fProfileType && fColorSpace == other.fColorSpace; On 2016/05/31 13:48:25, scroggo wrote: > Doesn't this check whether fColorSpace is the same object as other.fColorSpace? > Is that deliberate? It at least seems misleading. (I had to upload patch set 18 > of crrev.com/1997703003 because two SkCodecs created from the same file had > different color space objects, though I would expect them to be the same.) This is deliberate, though maybe it's the wrong decision. We have once ptrs for sRGB and adobe RGB (and probably will add another once ptr when we have a commonly used linear color space). So this will work most of the time with some false negatives. False negatives should be rare, since they will only occur when we are comparing two rare profiles that happen to be identical. Another option is to avoid false negatives by doing a deep compare here. FWIW, I think that the problem in crrev.com/1997703003 is probably because I've temporarily regressed our ability to recognize parametric gammas as sRGB. I plan to fix this. At the very least, I think this needs a comment.
Message was sent while issue was closed.
https://codereview.chromium.org/2000713003/diff/220001/include/core/SkImageIn... File include/core/SkImageInfo.h (right): https://codereview.chromium.org/2000713003/diff/220001/include/core/SkImageIn... include/core/SkImageInfo.h:244: SkColorSpace* colorSpace() const { return fColorSpace.get(); } On 2016/05/31 14:30:08, msarett wrote: > On 2016/05/31 13:48:25, scroggo wrote: > > Now that SkColorSpace is in include/, can this return a const ref to sk_sp? > > > > Yeah I think it could. The reason I went with a bare ptr was so the client > doesn't necessarily have to ref the color space. I think the const ref to sk_sp > would accomplish the same thing. I'm not sure which is better (or what is "Skia > style"). I don't think Skia style has been updated, but my read of https://docs.google.com/document/d/1PZRnDBwhnB_Ug6DtlkphAvJ9Ga5U3ONbSn1wf6jXX-8 tells me that a getter should return a const ref. > Whatever we decide here, we should do the same thing in SkCodec.h. +1 > > > From Herb's comment [1], it looks like the goal was to not include > > SkColorSpace.h, but we still include it? > > > > [1] > > > https://codereview.chromium.org/2000713003/diff/20001/include/core/SkImageInf... > > I'll look if we might be able to get away with a forward declare... > > My read of Herb's comment was a suggestion to expose information about the gamma > rather than the entire SkColorSpace object. I ultimately decided against this > approach for a few reasons: > (1) I was wrapping APIs that are already on SkColorSpace. > (2) What should we return when the SkColorSpace is nullptr? > (3) The SkColorSpace API is already limited. Is there a reason to limit it > further? > (4) We can expose this object without forcing the client to ref it. sgtm https://codereview.chromium.org/2000713003/diff/220001/include/core/SkImageIn... include/core/SkImageInfo.h:299: fProfileType == other.fProfileType && fColorSpace == other.fColorSpace; On 2016/05/31 14:30:08, msarett wrote: > On 2016/05/31 13:48:25, scroggo wrote: > > Doesn't this check whether fColorSpace is the same object as > other.fColorSpace? > > Is that deliberate? It at least seems misleading. (I had to upload patch set > 18 > > of crrev.com/1997703003 because two SkCodecs created from the same file had > > different color space objects, though I would expect them to be the same.) > > This is deliberate, though maybe it's the wrong decision. We have once ptrs for > sRGB and adobe RGB (and probably will add another once ptr when we have a > commonly used linear color space). > > So this will work most of the time with some false negatives. False negatives > should be rare, since they will only occur when we are comparing two rare > profiles that happen to be identical. FWIW, I don't know that my test case is a normal use case. > > Another option is to avoid false negatives by doing a deep compare here. > > FWIW, I think that the problem in crrev.com/1997703003 is probably because I've > temporarily regressed our ability to recognize parametric gammas as sRGB. I > plan to fix this. > > At the very least, I think this needs a comment. +1
Message was sent while issue was closed.
https://codereview.chromium.org/2000713003/diff/220001/include/core/SkImageIn... File include/core/SkImageInfo.h (right): https://codereview.chromium.org/2000713003/diff/220001/include/core/SkImageIn... include/core/SkImageInfo.h:244: SkColorSpace* colorSpace() const { return fColorSpace.get(); } On 2016/05/31 14:52:06, scroggo wrote: > On 2016/05/31 14:30:08, msarett wrote: > > On 2016/05/31 13:48:25, scroggo wrote: > > > Now that SkColorSpace is in include/, can this return a const ref to sk_sp? > > > > > > > Yeah I think it could. The reason I went with a bare ptr was so the client > > doesn't necessarily have to ref the color space. I think the const ref to > sk_sp > > would accomplish the same thing. I'm not sure which is better (or what is > "Skia > > style"). > > I don't think Skia style has been updated, but my read of > https://docs.google.com/document/d/1PZRnDBwhnB_Ug6DtlkphAvJ9Ga5U3ONbSn1wf6jXX-8 > tells me that a getter should return a const ref. > > > Whatever we decide here, we should do the same thing in SkCodec.h. > > +1 > > > > > > From Herb's comment [1], it looks like the goal was to not include > > > SkColorSpace.h, but we still include it? > > > > > > [1] > > > > > > https://codereview.chromium.org/2000713003/diff/20001/include/core/SkImageInf... > > > > I'll look if we might be able to get away with a forward declare... > > > > My read of Herb's comment was a suggestion to expose information about the > gamma > > rather than the entire SkColorSpace object. I ultimately decided against this > > approach for a few reasons: > > (1) I was wrapping APIs that are already on SkColorSpace. > > (2) What should we return when the SkColorSpace is nullptr? > > (3) The SkColorSpace API is already limited. Is there a reason to limit it > > further? > > (4) We can expose this object without forcing the client to ref it. > > sgtm > That doc needs updating. We have (so far at least) decided to not change what our getters return, so I think bare-pointer is correct. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
