Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2691213007: color: Don't use QCMS for transforms unless necessary (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months, 1 week ago by ccameron
Modified:
8 months ago
CC:
chromium-reviews, jam, darin-cc_chromium.org, piman+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

color: Don't use QCMS for transforms unless necessary Don't use QCMS for color space transformations unless the ColorSpace is not a faithful representation of the ICCProfile. With this in place, we can make a few housekeeping changes. Actually create ICCProfiles for ColorSpaces that weren't created from an ICCProfile to begin with. Move ICCProfile::FromColorSpace to ColorSpace::GetICCProfile, which can fail for unrepresentable results. Add an IsValid method to ICCProfile to indicate if SkICC was able to parse the profile. BUG=667966 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2691213007 Cr-Commit-Position: refs/heads/master@{#451244} Committed: https://chromium.googlesource.com/chromium/src/+/3b6fe6da979b8d9e084160ce6d8b7bd83f3aa878

Patch Set 1 #

Patch Set 2 : Fix cache hit computation of parse result #

Total comments: 10

Patch Set 3 : Rebase #

Patch Set 4 : Rebase #

Patch Set 5 : color: Don't use QCMS for transforms unless necessary #

Total comments: 2

Patch Set 6 : Incorporate review feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+171 lines, -95 lines) Patch
M content/browser/web_contents/web_contents_view_aura.cc View 1 chunk +3 lines, -4 lines 0 comments Download
M gpu/ipc/client/gpu_memory_buffer_impl_io_surface.cc View 1 chunk +13 lines, -8 lines 0 comments Download
M ui/gfx/color_space.h View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
M ui/gfx/color_space.cc View 1 2 3 4 5 3 chunks +52 lines, -0 lines 0 comments Download
M ui/gfx/color_transform.cc View 1 2 3 4 5 3 chunks +11 lines, -5 lines 0 comments Download
M ui/gfx/color_transform_unittest.cc View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M ui/gfx/icc_profile.h View 3 chunks +17 lines, -0 lines 0 comments Download
M ui/gfx/icc_profile.cc View 1 5 chunks +50 lines, -70 lines 0 comments Download
M ui/gfx/icc_profile_unittest.cc View 2 chunks +16 lines, -6 lines 0 comments Download
Commit queue not available (can’t edit this change).

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 36 (24 generated)
ccameron
ptal -- I pulled this off from the other patch.
8 months ago (2017-02-16 01:09:32 UTC) #11
hubbe
https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.cc#newcode103 ui/gfx/color_space.cc:103: ColorSpace result(ColorSpace::PrimaryID::CUSTOM, It would be really cool if we ...
8 months ago (2017-02-16 01:28:29 UTC) #12
ccameron
updated https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.cc#newcode103 ui/gfx/color_space.cc:103: ColorSpace result(ColorSpace::PrimaryID::CUSTOM, On 2017/02/16 01:28:29, hubbe wrote: > ...
8 months ago (2017-02-16 01:58:48 UTC) #13
hubbe
On 2017/02/16 01:58:48, ccameron wrote: > updated > > https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.cc > File ui/gfx/color_space.cc (right): > ...
8 months ago (2017-02-16 05:35:56 UTC) #18
ccameron
On 2017/02/16 05:35:56, hubbe wrote: > Well, clearly defaulting to BT709 was not the right ...
8 months ago (2017-02-16 07:14:29 UTC) #20
ccameron
On 2017/02/16 07:14:29, ccameron wrote: > On 2017/02/16 05:35:56, hubbe wrote: > > > Well, ...
8 months ago (2017-02-16 07:15:31 UTC) #21
hubbe
lgtm https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h File ui/gfx/color_space.h (right): https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h#newcode183 ui/gfx/color_space.h:183: // will be constructed ignoring the range adjust ...
8 months ago (2017-02-16 08:33:03 UTC) #22
ccameron
Thanks! https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h File ui/gfx/color_space.h (right): https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h#newcode183 ui/gfx/color_space.h:183: // will be constructed ignoring the range adjust ...
8 months ago (2017-02-16 22:33:10 UTC) #23
ccameron
Adding avi@ for content/ stamp
8 months ago (2017-02-17 01:34:36 UTC) #25
Avi (ping after 24h)
content lgtm
8 months ago (2017-02-17 01:44:31 UTC) #28
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/2691213007/90001
8 months ago (2017-02-17 04:36:17 UTC) #33
commit-bot: I haz the power
8 months ago (2017-02-17 06:22:44 UTC) #36
Message was sent while issue was closed.
Committed patchset #6 (id:90001) as
https://chromium.googlesource.com/chromium/src/+/3b6fe6da979b8d9e084160ce6d8b...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa