|
|
Descriptioncc: Add conversion between gfx::ColorSpace and SkColorSpace
Use the ICC profile to convert between the two for now. This is not
robust or efficient, but it is behind a development flag.
BUG=44872
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/c37fc95428d69f46a528627fe3564cb53f8ce474
Cr-Commit-Position: refs/heads/master@{#419653}
Patch Set 1 #Patch Set 2 : Move to CS #
Total comments: 5
Patch Set 3 : Restrict to just CUSTOM #Patch Set 4 : Add warnings #
Total comments: 2
Depends on Patchset: Messages
Total messages: 20 (5 generated)
Description was changed from ========== cc: Add conversion between gfx::ColorSpace and SkColorSpace Use the ICC profile to convert between the two for now. This is not robust or efficient, but it is behind a development flag. BUG=44872 ========== to ========== cc: Add conversion between gfx::ColorSpace and SkColorSpace Use the ICC profile to convert between the two for now. This is not robust or efficient, but it is behind a development flag. BUG=44872 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
ccameron@chromium.org changed reviewers: + hubbe@chromium.org, junov@chromium.org
This completes the skia part of the color correction stack, so that the canvases that we raster into when running with --enable-color-correct-rendering end up getting the SkColorSpace of the output device. This will need to be robust, eventually. For now the only callers do come from an ICC profile, and so the limited implementation is enough.
https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc#n... ui/gfx/color_space.cc:65: !memcmp(custom_primary_matrix_, other.custom_primary_matrix_, We should only do this if primaries_ == CUSTOM I think. https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc#n... ui/gfx/color_space.cc:104: gfx::ICCProfile::FromColorSpace(*this).GetData(); 1) Shouldn't we implement FromColorSpace() first? 2) This will probably never work properly for non-RGB color profiles (YUV in particular)
https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc#n... ui/gfx/color_space.cc:65: !memcmp(custom_primary_matrix_, other.custom_primary_matrix_, On 2016/09/19 19:21:27, hubbe wrote: > We should only do this if primaries_ == CUSTOM I think. Done. https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc#n... ui/gfx/color_space.cc:104: gfx::ICCProfile::FromColorSpace(*this).GetData(); On 2016/09/19 19:21:27, hubbe wrote: > 1) Shouldn't we implement FromColorSpace() first? This is to un-block work on rasterization, and rasterization targets always come from an ICC profile (from the monitor), so this covers the one case that is important right now. > 2) This will probably never work properly for non-RGB color profiles (YUV in > particular) If we have to return nullptr for some spaces then that is reasonable (IIUC software compositing of YUV also doesn't exist yet).
https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc#n... ui/gfx/color_space.cc:104: gfx::ICCProfile::FromColorSpace(*this).GetData(); On 2016/09/19 22:23:15, ccameron wrote: > On 2016/09/19 19:21:27, hubbe wrote: > > 1) Shouldn't we implement FromColorSpace() first? > > This is to un-block work on rasterization, and rasterization targets always come > from an ICC profile (from the monitor), so this covers the one case that is > important right now. > > > 2) This will probably never work properly for non-RGB color profiles (YUV in > > particular) > > If we have to return nullptr for some spaces then that is reasonable (IIUC > software compositing of YUV also doesn't exist yet). Then maybe remove the DCHECK and tell callers to expect null values? I agree that it will work for your use case, but there is no sensible way for a random caller to know what to expect right now.
On 2016/09/19 22:53:19, hubbe wrote: > https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc > File ui/gfx/color_space.cc (right): > > https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc#n... > ui/gfx/color_space.cc:104: gfx::ICCProfile::FromColorSpace(*this).GetData(); > On 2016/09/19 22:23:15, ccameron wrote: > > On 2016/09/19 19:21:27, hubbe wrote: > > > 1) Shouldn't we implement FromColorSpace() first? > > > > This is to un-block work on rasterization, and rasterization targets always > come > > from an ICC profile (from the monitor), so this covers the one case that is > > important right now. > > > > > 2) This will probably never work properly for non-RGB color profiles (YUV in > > > particular) > > > > If we have to return nullptr for some spaces then that is reasonable (IIUC > > software compositing of YUV also doesn't exist yet). > > Then maybe remove the DCHECK and tell callers to expect null values? I agree > that it will work for your use case, but there is no sensible way for a random > caller to know what to expect right now. Yeah, that's why I'd like to make it blow up when they hit something that isn't supported (which can then generate the question of "is the usage that is blowing up expected to work, or not").
On 2016/09/19 23:12:40, ccameron wrote: > On 2016/09/19 22:53:19, hubbe wrote: > > https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc > > File ui/gfx/color_space.cc (right): > > > > > https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc#n... > > ui/gfx/color_space.cc:104: gfx::ICCProfile::FromColorSpace(*this).GetData(); > > On 2016/09/19 22:23:15, ccameron wrote: > > > On 2016/09/19 19:21:27, hubbe wrote: > > > > 1) Shouldn't we implement FromColorSpace() first? > > > > > > This is to un-block work on rasterization, and rasterization targets always > > come > > > from an ICC profile (from the monitor), so this covers the one case that is > > > important right now. > > > > > > > 2) This will probably never work properly for non-RGB color profiles (YUV > in > > > > particular) > > > > > > If we have to return nullptr for some spaces then that is reasonable (IIUC > > > software compositing of YUV also doesn't exist yet). > > > > Then maybe remove the DCHECK and tell callers to expect null values? I agree > > that it will work for your use case, but there is no sensible way for a random > > caller to know what to expect right now. > > Yeah, that's why I'd like to make it blow up when they hit something that isn't > supported (which can then generate the question of "is the usage that is blowing > up expected to work, or not"). Since anybody can call ICCprofile::FromColorSpace, maybe we should put this somewhere else for now? (Since it's not really generally useful yet.)
On 2016/09/19 23:29:11, hubbe wrote: > On 2016/09/19 23:12:40, ccameron wrote: > > On 2016/09/19 22:53:19, hubbe wrote: > > > https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc > > > File ui/gfx/color_space.cc (right): > > > > > > > > > https://codereview.chromium.org/2351823002/diff/20001/ui/gfx/color_space.cc#n... > > > ui/gfx/color_space.cc:104: gfx::ICCProfile::FromColorSpace(*this).GetData(); > > > On 2016/09/19 22:23:15, ccameron wrote: > > > > On 2016/09/19 19:21:27, hubbe wrote: > > > > > 1) Shouldn't we implement FromColorSpace() first? > > > > > > > > This is to un-block work on rasterization, and rasterization targets > always > > > come > > > > from an ICC profile (from the monitor), so this covers the one case that > is > > > > important right now. > > > > > > > > > 2) This will probably never work properly for non-RGB color profiles > (YUV > > in > > > > > particular) > > > > > > > > If we have to return nullptr for some spaces then that is reasonable (IIUC > > > > software compositing of YUV also doesn't exist yet). > > > > > > Then maybe remove the DCHECK and tell callers to expect null values? I agree > > > that it will work for your use case, but there is no sensible way for a > random > > > caller to know what to expect right now. > > > > Yeah, that's why I'd like to make it blow up when they hit something that > isn't > > supported (which can then generate the question of "is the usage that is > blowing > > up expected to work, or not"). > > Since anybody can call ICCprofile::FromColorSpace, maybe we should put this > somewhere else for now? > (Since it's not really generally useful yet.) In PS1 I had it in ResourceProvider::, but I'm felt that it really belonged in gfx::ColorSpace. We're also going to need a gfx::ColorSpace::FromSkColorSpace soon too (it needs to be populated in the mailboxes at [1]). So I'd rather put the interfaces that we'll eventually want there, and leave landmines for not-yet-implemented functionality.
> In PS1 I had it in ResourceProvider::, but I'm felt that it really belonged in > gfx::ColorSpace. We're also going to need a gfx::ColorSpace::FromSkColorSpace > soon too (it needs to be populated in the mailboxes at [1]). > > So I'd rather put the interfaces that we'll eventually want there, and leave > landmines for not-yet-implemented functionality. And ... [1] was supposed to link to https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
On 2016/09/20 00:07:46, ccameron wrote: > > In PS1 I had it in ResourceProvider::, but I'm felt that it really belonged in > > gfx::ColorSpace. We're also going to need a gfx::ColorSpace::FromSkColorSpace > > soon too (it needs to be populated in the mailboxes at [1]). > > > > So I'd rather put the interfaces that we'll eventually want there, and leave > > landmines for not-yet-implemented functionality. > > And ... [1] was supposed to link to > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... The landmines don't make particularly happy, but I'm ok with doing it your way and dancing the I-told-you-so-dance later. :) If it was just us using gfx::ColorSpace, I'd be fine with it, but other people are starting to pick it up as well... LGTM
On 2016/09/20 00:28:11, hubbe wrote: > On 2016/09/20 00:07:46, ccameron wrote: > > > In PS1 I had it in ResourceProvider::, but I'm felt that it really belonged > in > > > gfx::ColorSpace. We're also going to need a > gfx::ColorSpace::FromSkColorSpace > > > soon too (it needs to be populated in the mailboxes at [1]). > > > > > > So I'd rather put the interfaces that we'll eventually want there, and leave > > > landmines for not-yet-implemented functionality. > > > > And ... [1] was supposed to link to > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... > > The landmines don't make particularly happy, but I'm ok with doing it your way > and dancing the I-told-you-so-dance later. :) Sounds like a plan! Actually, nullptr is a valid return value for an invalid gfx::ColorSpace, so I added that comment (but left the landmine for any other input value returning nullptr). > If it was just us using gfx::ColorSpace, I'd be fine with it, but other people > are starting to pick it up as well... Yeah, this is a bit scary ...
The CQ bit was checked by ccameron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hubbe@chromium.org Link to the patchset: https://codereview.chromium.org/2351823002/#ps60001 (title: "Add warnings")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== cc: Add conversion between gfx::ColorSpace and SkColorSpace Use the ICC profile to convert between the two for now. This is not robust or efficient, but it is behind a development flag. BUG=44872 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Add conversion between gfx::ColorSpace and SkColorSpace Use the ICC profile to convert between the two for now. This is not robust or efficient, but it is behind a development flag. BUG=44872 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/c37fc95428d69f46a528627fe3564cb53f8ce474 Cr-Commit-Position: refs/heads/master@{#419653} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c37fc95428d69f46a528627fe3564cb53f8ce474 Cr-Commit-Position: refs/heads/master@{#419653}
Message was sent while issue was closed.
Post review (was OOO) https://codereview.chromium.org/2351823002/diff/60001/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2351823002/diff/60001/ui/gfx/color_space.cc#n... ui/gfx/color_space.cc:108: if (*this == gfx::ColorSpace()) Just checking... in skia, unspecified colorspace means "do not color correct". Is this what you want here?
Message was sent while issue was closed.
https://codereview.chromium.org/2351823002/diff/60001/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2351823002/diff/60001/ui/gfx/color_space.cc#n... ui/gfx/color_space.cc:108: if (*this == gfx::ColorSpace()) On 2016/09/20 19:09:50, Justin Novosad wrote: > Just checking... in skia, unspecified colorspace means "do not color correct". > Is this what you want here? Yes. The default ctor for gfx::ColorSpace also means "do not color correct". This is the one time when we'll certainly want to return nullptr. In all other circumstances, nullptr means something went wrong. |