Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(146)

Issue 2351823002: cc: Add conversion between gfx::ColorSpace and SkColorSpace (Closed)

Created:
4 years, 3 months ago by ccameron
Modified:
4 years, 3 months ago
Reviewers:
hubbe, Justin Novosad
CC:
chromium-reviews, cc-bugs_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

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
Unified diffs Side-by-side diffs Delta from patch set Stats (+44 lines, -10 lines) Patch
M cc/resources/resource_provider.cc View 1 1 chunk +3 lines, -3 lines 0 comments Download
M ui/gfx/color_space.h View 1 2 3 2 chunks +7 lines, -0 lines 0 comments Download
M ui/gfx/color_space.cc View 1 2 3 3 chunks +34 lines, -7 lines 2 comments Download

Depends on Patchset:

Messages

Total messages: 20 (5 generated)
ccameron
This completes the skia part of the color correction stack, so that the canvases that ...
4 years, 3 months ago (2016-09-19 19:10:30 UTC) #3
hubbe
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#newcode65 ui/gfx/color_space.cc:65: !memcmp(custom_primary_matrix_, other.custom_primary_matrix_, We should only do this if primaries_ ...
4 years, 3 months ago (2016-09-19 19:21:27 UTC) #4
ccameron
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#newcode65 ui/gfx/color_space.cc:65: !memcmp(custom_primary_matrix_, other.custom_primary_matrix_, On 2016/09/19 19:21:27, hubbe wrote: > We ...
4 years, 3 months ago (2016-09-19 22:23:15 UTC) #5
hubbe
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#newcode104 ui/gfx/color_space.cc:104: gfx::ICCProfile::FromColorSpace(*this).GetData(); On 2016/09/19 22:23:15, ccameron wrote: > On 2016/09/19 ...
4 years, 3 months ago (2016-09-19 22:53:19 UTC) #6
ccameron
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#newcode104 > ...
4 years, 3 months ago (2016-09-19 23:12:40 UTC) #7
hubbe
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 > ...
4 years, 3 months ago (2016-09-19 23:29:11 UTC) #8
ccameron
On 2016/09/19 23:29:11, hubbe wrote: > On 2016/09/19 23:12:40, ccameron wrote: > > On 2016/09/19 ...
4 years, 3 months ago (2016-09-19 23:52:28 UTC) #9
ccameron
> In PS1 I had it in ResourceProvider::, but I'm felt that it really belonged ...
4 years, 3 months ago (2016-09-20 00:07:46 UTC) #10
hubbe
On 2016/09/20 00:07:46, ccameron wrote: > > In PS1 I had it in ResourceProvider::, but ...
4 years, 3 months ago (2016-09-20 00:28:11 UTC) #11
ccameron
On 2016/09/20 00:28:11, hubbe wrote: > On 2016/09/20 00:07:46, ccameron wrote: > > > In ...
4 years, 3 months ago (2016-09-20 01:24:27 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2351823002/60001
4 years, 3 months ago (2016-09-20 01:24:58 UTC) #15
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 3 months ago (2016-09-20 02:29:20 UTC) #16
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/c37fc95428d69f46a528627fe3564cb53f8ce474 Cr-Commit-Position: refs/heads/master@{#419653}
4 years, 3 months ago (2016-09-20 02:31:36 UTC) #18
Justin Novosad
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#newcode108 ui/gfx/color_space.cc:108: if (*this == gfx::ColorSpace()) Just ...
4 years, 3 months ago (2016-09-20 19:09:50 UTC) #19
ccameron
4 years, 3 months ago (2016-09-20 19:12:07 UTC) #20
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.

Powered by Google App Engine
This is Rietveld 408576698