color: Clean up gfx::ICCProfile and gfx::ColorSpace relationship |
||
Issue descriptionWith the cleanup in issue 759215 , many of the constraints that led to the complicated relationship between gfx::ICCProfile and gfx::ColorSpace will go away. With these issues gone, we will be free to clean up this relationship. In particular, we should aim to: * get rid of the gfx::ICCProfile MRU cache * add the ability to create a gfx::ColorSpace directly from ICC profile data * change gfx::ICCProfile to be "profile metadata" that can be attached to a gfx::ColorSpace * have gfx::ColorSpace hold a scoped refptr to this "profile metadata", instead of looking this up by ICC profile ID
,
Oct 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ac3c37d52e71853c54921c488cc52b7a9ebfb2fb commit ac3c37d52e71853c54921c488cc52b7a9ebfb2fb Author: Christopher Cameron <ccameron@chromium.org> Date: Tue Oct 31 21:33:10 2017 Make ICCProfile no longer store a ColorSpace directly Make ICCProfile store the primaries and transfer function that it read or computed directly, rather than storing a ColorSpace object. Construct the ColorSpace object when requested, rather than storing it. Store an approximate (or guessed) primary matrix and transfer function for ICC_BASED ColorSpace objects. When computing the parametric approximation of an ICC_BASED ColorSpace object, use these values directly, rather than having to look them up from original ICCProfile. Bug: 766736 Change-Id: I21d6dfaa38706f814f6c9dd54517806e99366113 Reviewed-on: https://chromium-review.googlesource.com/745311 Reviewed-by: Fredrik Hubinette <hubbe@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#512958} [modify] https://crrev.com/ac3c37d52e71853c54921c488cc52b7a9ebfb2fb/ui/gfx/color_space.cc [modify] https://crrev.com/ac3c37d52e71853c54921c488cc52b7a9ebfb2fb/ui/gfx/color_space.h [modify] https://crrev.com/ac3c37d52e71853c54921c488cc52b7a9ebfb2fb/ui/gfx/color_transform_unittest.cc [modify] https://crrev.com/ac3c37d52e71853c54921c488cc52b7a9ebfb2fb/ui/gfx/icc_profile.cc [modify] https://crrev.com/ac3c37d52e71853c54921c488cc52b7a9ebfb2fb/ui/gfx/icc_profile.h [modify] https://crrev.com/ac3c37d52e71853c54921c488cc52b7a9ebfb2fb/ui/gfx/icc_profile_unittest.cc
,
Nov 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e35e232819aa1bfaef408e7f1e2d6c080da8ab9b commit e35e232819aa1bfaef408e7f1e2d6c080da8ab9b Author: ccameron <ccameron@chromium.org> Date: Thu Nov 02 23:33:46 2017 Revert "Make ICCProfile no longer store a ColorSpace directly" This reverts commit ac3c37d52e71853c54921c488cc52b7a9ebfb2fb. Reason for revert: crbug.com/780848 , crbug.com/780415 Original change's description: > Make ICCProfile no longer store a ColorSpace directly > > Make ICCProfile store the primaries and transfer function that it > read or computed directly, rather than storing a ColorSpace object. > Construct the ColorSpace object when requested, rather than storing it. > > Store an approximate (or guessed) primary matrix and transfer function > for ICC_BASED ColorSpace objects. When computing the parametric > approximation of an ICC_BASED ColorSpace object, use these values > directly, rather than having to look them up from original ICCProfile. > > Bug: 766736 > Change-Id: I21d6dfaa38706f814f6c9dd54517806e99366113 > Reviewed-on: https://chromium-review.googlesource.com/745311 > Reviewed-by: Fredrik Hubinette <hubbe@chromium.org> > Commit-Queue: ccameron <ccameron@chromium.org> > Cr-Commit-Position: refs/heads/master@{#512958} TBR=ccameron@chromium.org,hubbe@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 766736 Change-Id: Ie36929800ab8f748cfc331b1a6ccda328c6559f8 Reviewed-on: https://chromium-review.googlesource.com/752061 Reviewed-by: ccameron <ccameron@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#513624} [modify] https://crrev.com/e35e232819aa1bfaef408e7f1e2d6c080da8ab9b/ui/gfx/color_space.cc [modify] https://crrev.com/e35e232819aa1bfaef408e7f1e2d6c080da8ab9b/ui/gfx/color_space.h [modify] https://crrev.com/e35e232819aa1bfaef408e7f1e2d6c080da8ab9b/ui/gfx/color_transform_unittest.cc [modify] https://crrev.com/e35e232819aa1bfaef408e7f1e2d6c080da8ab9b/ui/gfx/icc_profile.cc [modify] https://crrev.com/e35e232819aa1bfaef408e7f1e2d6c080da8ab9b/ui/gfx/icc_profile.h [modify] https://crrev.com/e35e232819aa1bfaef408e7f1e2d6c080da8ab9b/ui/gfx/icc_profile_unittest.cc
,
Nov 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fdc0f878dd3ad482663a16414fc015e46a71aa9e commit fdc0f878dd3ad482663a16414fc015e46a71aa9e Author: Christopher Cameron <ccameron@chromium.org> Date: Thu Nov 09 03:22:16 2017 Make ICCProfile no longer store a ColorSpace directly Make ICCProfile store the primaries and transfer function that it read or computed directly, rather than storing a ColorSpace object. Construct the ColorSpace object when requested, rather than storing it. Bug: 766736 Change-Id: I3b0958545a1bfd889a9b88fb8689c6382d26c781 Reviewed-on: https://chromium-review.googlesource.com/752746 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Fredrik Hubinette <hubbe@chromium.org> Cr-Commit-Position: refs/heads/master@{#515084} [modify] https://crrev.com/fdc0f878dd3ad482663a16414fc015e46a71aa9e/ui/gfx/icc_profile.cc [modify] https://crrev.com/fdc0f878dd3ad482663a16414fc015e46a71aa9e/ui/gfx/icc_profile.h
,
Nov 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f02091998c3ca16eb2f98d460b264c507ecabb7e commit f02091998c3ca16eb2f98d460b264c507ecabb7e Author: Christopher Cameron <ccameron@chromium.org> Date: Thu Nov 09 23:12:22 2017 Remove ICC_BASED primaries UMA indicates that every color space that we need to fall back to using a LUT to support * has a primary matrix * has table based transfer functions There exist other profiles, but we are unable to handle them. Because we always have a primary matrix, remove PrimaryID::ICC_BASED, and always set the retrieved CUSTOM matrix. Make ColorSpace::GetParametricApproximation no longer look up the approximation from the ICC profile, because it now already has the custom primaries (the only information it would have gotten). Bug: 766736 Change-Id: I8cf59cf67a7bfa2c713354a5a8b25c24b55668f1 Reviewed-on: https://chromium-review.googlesource.com/752331 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Fredrik Hubinette <hubbe@chromium.org> Cr-Commit-Position: refs/heads/master@{#515337} [modify] https://crrev.com/f02091998c3ca16eb2f98d460b264c507ecabb7e/ui/gfx/color_space.cc [modify] https://crrev.com/f02091998c3ca16eb2f98d460b264c507ecabb7e/ui/gfx/color_space.h [modify] https://crrev.com/f02091998c3ca16eb2f98d460b264c507ecabb7e/ui/gfx/color_space_win.cc [modify] https://crrev.com/f02091998c3ca16eb2f98d460b264c507ecabb7e/ui/gfx/color_transform.cc [modify] https://crrev.com/f02091998c3ca16eb2f98d460b264c507ecabb7e/ui/gfx/icc_profile.cc
,
Nov 10 2017
One of those CLs seems to have broken things again. :(
,
Nov 10 2017
In particular?
,
Nov 10 2017
I'm seeing the exact same issue that reverting the CL earlier fixed - no icons to the sides of the omnibox, overdraw on tabs (With a dark theme), etc.
,
Nov 10 2017
i.e. issue 780495 , which was merged into issue 780415.
,
Nov 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/43f17057cc88b7c1ee868cb288d6681b2eb708b2 commit 43f17057cc88b7c1ee868cb288d6681b2eb708b2 Author: Christopher Cameron <ccameron@chromium.org> Date: Sat Nov 11 01:52:01 2017 Use base::RefCounted for ICC profile internals One of ICCProfileCache's jobs was to mainting a list of displays that had been histogrammed, for each ICCProfile. The simpler solution to this problem is to have ICCProfile's internals be a scoped_refptr that are shared by all profiles with the same data (as much as is possible), and put the set of histogrammed displays in that structure. Replace ICCProfileCache with a simple MRUCache of ICCProfiles. There are some strange acrobatics performed in this cache to ensure that behavior exactly matches previous behavior (including all idiosyncracies). The next patch will change some of these behaviors to simplify logic. Bug: 766736 Change-Id: I1fe2d52fc8b3745d73f87115965914aeec367f61 Reviewed-on: https://chromium-review.googlesource.com/753424 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Fredrik Hubinette <hubbe@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#515787} [modify] https://crrev.com/43f17057cc88b7c1ee868cb288d6681b2eb708b2/ui/gfx/color_space.cc [modify] https://crrev.com/43f17057cc88b7c1ee868cb288d6681b2eb708b2/ui/gfx/icc_profile.cc [modify] https://crrev.com/43f17057cc88b7c1ee868cb288d6681b2eb708b2/ui/gfx/icc_profile.h [modify] https://crrev.com/43f17057cc88b7c1ee868cb288d6681b2eb708b2/ui/gfx/ipc/color/gfx_param_traits.cc
,
Nov 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/deea25256b5be6a8fff78b74e5adc03e2829ba31 commit deea25256b5be6a8fff78b74e5adc03e2829ba31 Author: Christopher Cameron <ccameron@chromium.org> Date: Thu Nov 16 07:10:52 2017 Remove ICC_BASED transfer id In gfx::ColorSpace, make the presence of an ICC profile id indicate that a given color space is not parametric. Change the cache of gfx::ICCProfiles to use a gfx::ColorSpace as the key. Update gfx::ICCProfile and gfx::ColorSpace to consider the profile id to be relevant in testing equality and order. Bug: 766736 Change-Id: I85ba1240a62b4b99da97578aba2eaeac21f64d08 Reviewed-on: https://chromium-review.googlesource.com/768611 Reviewed-by: Fredrik Hubinette <hubbe@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#517008} [modify] https://crrev.com/deea25256b5be6a8fff78b74e5adc03e2829ba31/ui/display/mac/screen_mac.mm [modify] https://crrev.com/deea25256b5be6a8fff78b74e5adc03e2829ba31/ui/gfx/BUILD.gn [modify] https://crrev.com/deea25256b5be6a8fff78b74e5adc03e2829ba31/ui/gfx/color_space.cc [modify] https://crrev.com/deea25256b5be6a8fff78b74e5adc03e2829ba31/ui/gfx/color_space.h [modify] https://crrev.com/deea25256b5be6a8fff78b74e5adc03e2829ba31/ui/gfx/color_space_win.cc [modify] https://crrev.com/deea25256b5be6a8fff78b74e5adc03e2829ba31/ui/gfx/color_transform.cc [modify] https://crrev.com/deea25256b5be6a8fff78b74e5adc03e2829ba31/ui/gfx/icc_profile.cc [modify] https://crrev.com/deea25256b5be6a8fff78b74e5adc03e2829ba31/ui/gfx/icc_profile.h [delete] https://crrev.com/787046bf5046e0b21840f4a924d2972569c17882/ui/gfx/icc_profile_mac.mm [modify] https://crrev.com/deea25256b5be6a8fff78b74e5adc03e2829ba31/ui/gfx/icc_profile_unittest.cc [modify] https://crrev.com/deea25256b5be6a8fff78b74e5adc03e2829ba31/ui/gfx/test/icc_profiles.cc
,
Nov 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce commit 3bc3d5ec4badb7429c10f26a4b5977a76eb026ce Author: Christopher Cameron <ccameron@chromium.org> Date: Thu Nov 23 03:48:11 2017 Separate out platform specific ICCProfile uses Wrap the ScreenInfo::icc_profile in a OS_MACOSX guard, since it is not used (nor will be used) on other platforms. Split ColorSpace::GetICCProfile into three separate functions for the three different contexts in which it is needed - ICCProfile::FromCacheMac for the Mac-specific power issues - ICCProfile::FromParametricColorSpace for generating profiles - ICCProfile::GetSkColorSpaceFromId for building LUTS Give all three instances separate caches, since they don't need to interact with each other (and trying to unify them creates unpredictable behavior). Enable histogramming of empty profiles (since they were not included in UMAs before). Bug: 787418 , 766736 Change-Id: I6794284fd3ca44d1a4667a8ce0f104948bdd596a Reviewed-on: https://chromium-review.googlesource.com/783659 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#518842} [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/content/browser/web_contents/web_contents_view_aura.cc [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/content/browser/web_contents/web_contents_view_mac.mm [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/content/common/frame_messages.h [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/content/public/common/screen_info.h [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/tools/metrics/histograms/enums.xml [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/ui/display/display.cc [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/ui/display/win/color_profile_reader.cc [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/ui/display/win/color_profile_reader.h [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/ui/gfx/color_space.cc [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/ui/gfx/color_space.h [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/ui/gfx/color_transform.cc [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/ui/gfx/icc_profile.cc [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/ui/gfx/icc_profile.h [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/ui/gfx/icc_profile_unittest.cc [modify] https://crrev.com/3bc3d5ec4badb7429c10f26a4b5977a76eb026ce/ui/gfx/mac/io_surface.cc
,
Nov 27 2017
This is still messy but at least it's less messy than it was before. I need to stop working on this and go to more useful things. |
||
►
Sign in to add a comment |
||
Comment 1 by ccameron@chromium.org
, Oct 31 2017