Color: Don't duplicate ICC profile data

Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133
Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Committed: https://crrev.com/0c3ad3287b3fae9b6329f8e7c34c0117b5c8da65
Committed: https://crrev.com/f1689c8b1c5d6411b8a14912227590345c3c3a94
Cr-Original-Original-Commit-Position: refs/heads/master@{#405211}
Cr-Original-Commit-Position: refs/heads/master@{#405424}
Cr-Commit-Position: refs/heads/master@{#405552}

[ccameron, 2016-07-11 22:01:59]

ccameron@chromium.org changed reviewers:
+ hubbe@chromium.org

[ccameron, 2016-07-11 22:01:59]

This removes the "make a bazillion copies of the ICC profile" issue.

I consider this to be better than SkColorSpace's approach, because this will
always guarantee that we are pointing to the same underlying "global data" --
with sk_sp<SkColorSpace>, we have no such guarantee (so, for instance, we may
allocate a bazillion copies of the same color space).

This can be used to cache ICC calculations (and potentially other things needed
for transform computation). This also allows construction of std::maps based on
gfx::ColorSpace objects.

sg?

[hubbe, 2016-07-11 22:51:59]

Unfortunately, it's still possible to have multiple representations of the same
color space, but with different ICC byte streams, not sure how to solve that
though, or if it's even a real problem.

https://codereview.chromium.org/2140803002/diff/20001/ui/gfx/color_space.cc
File ui/gfx/color_space.cc (right):

https://codereview.chromium.org/2140803002/diff/20001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:55: auto found = map_.find(key);
use insert here

https://codereview.chromium.org/2140803002/diff/20001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:81: Key key_;
I think this is really confusing, because the key is also the data.

https://codereview.chromium.org/2140803002/diff/20001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:83: static std::map<Key, GlobalData*> map_;
I think this should be hash_set<scoped_refptr<GlobalData>, CustomHash,
CustomCompare> ?

[ccameron, 2016-07-12 02:49:01]

True, this will not keep the same object for two different-but-the-same ICC
profiles, but that seems reasonable to me. This was aimed at ensuring the named
spaces (and identical-ICC spaces) have a single place where data is calculated
and stored.

https://codereview.chromium.org/2140803002/diff/20001/ui/gfx/color_space.cc
File ui/gfx/color_space.cc (right):

https://codereview.chromium.org/2140803002/diff/20001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:55: auto found = map_.find(key);
On 2016/07/11 22:51:59, hubbe wrote:
> use insert here

Done.

https://codereview.chromium.org/2140803002/diff/20001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:81: Key key_;
On 2016/07/11 22:51:59, hubbe wrote:
> I think this is really confusing, because the key is also the data.

Good point. We need to be able to erase |this| from |map_| when its last
reference goes away, but that can be done by keeping around an iterator into
|map_|, instead.

https://codereview.chromium.org/2140803002/diff/20001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:83: static std::map<Key, GlobalData*> map_;
On 2016/07/11 22:51:59, hubbe wrote:
> I think this should be hash_set<scoped_refptr<GlobalData>, CustomHash,
> CustomCompare> ?

The hash_set would then keep a reference to the GlobalData, which would keep it
around forever -- my goal with this complication is to keep the GlobalData
around for as long as there exists instances of the color space (and then get
rid of it).

[hubbe, 2016-07-12 05:12:40]

lgtm

https://codereview.chromium.org/2140803002/diff/40001/ui/gfx/color_space.cc
File ui/gfx/color_space.cc (right):

https://codereview.chromium.org/2140803002/diff/40001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:81: std::map<Key, GlobalData*>::iterator iterator_;
Seems like map_.erase(Key(icc_profile_)) would work just fine.

[ccameron, 2016-07-12 19:11:03]

Thanks!

https://codereview.chromium.org/2140803002/diff/40001/ui/gfx/color_space.cc
File ui/gfx/color_space.cc (right):

https://codereview.chromium.org/2140803002/diff/40001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:81: std::map<Key, GlobalData*>::iterator iterator_;
On 2016/07/12 05:12:40, hubbe wrote:
> Seems like map_.erase(Key(icc_profile_)) would work just fine.

True, ror now it's pretty small (we'd also need the type), but the next step
would be to throw in named color spaces, so the key would start replicating data
in the gfx::ColorSpace. But, I'm totally up for iterating on this.

[ccameron, 2016-07-12 19:11:06]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-07-12 19:14:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 21:39:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-12 21:39:15]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ccameron, 2016-07-12 22:02:46]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-07-12 22:07:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 01:12:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-13 01:12:23]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ccameron, 2016-07-13 16:43:57]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-07-13 16:44:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 17:36:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-13 17:39:36]

Description was changed from

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133
==========

to

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133

Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Cr-Commit-Position: refs/heads/master@{#405211}
==========

[commit-bot: I haz the power, 2016-07-13 17:39:37]

Patchset 3 (id:??) landed as
https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Cr-Commit-Position: refs/heads/master@{#405211}

[Dan Beam, 2016-07-13 19:15:09]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-07-13 19:15:10]

https://codereview.chromium.org/2140803002/diff/40001/ui/gfx/color_space.cc
File ui/gfx/color_space.cc (right):

https://codereview.chromium.org/2140803002/diff/40001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:87: base::Lock ColorSpace::GlobalData::map_lock_;
i think these are adding static initializers:
https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/22278

[Dan Beam, 2016-07-13 19:19:20]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2148013002/ by dbeam@chromium.org.

The reason for reverting is: Broke static initializers step of sizes on Linux:
https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/22278

Maybe making map_ a base::LazyInstance would solve your problem?.

[ccameron, 2016-07-13 19:37:46]

On 2016/07/13 19:19:20, Dan Beam wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/2148013002/ by mailto:dbeam@chromium.org.
> 
> The reason for reverting is: Broke static initializers step of sizes on Linux:
> https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/22278
> 
> Maybe making map_ a base::LazyInstance would solve your problem?.

Thanks for the revert. I was very surprised that I didn't need to do a
LazyInstance -- that nothing in the CQ complained about it -- I figured that
something might have been updated in our c++ support.

[ccameron, 2016-07-13 20:30:21]

Description was changed from

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133

Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Cr-Commit-Position: refs/heads/master@{#405211}
==========

to

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133

Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Cr-Commit-Position: refs/heads/master@{#405211}
==========

[ccameron, 2016-07-13 20:30:33]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 20:31:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 22:39:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-13 22:39:58]

Dry run: This issue passed the CQ dry run.

[ccameron, 2016-07-14 02:59:07]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-07-14 02:59:07]

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/2140803002/#ps60001
(title: "Use LazyInstance")

[commit-bot: I haz the power, 2016-07-14 02:59:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 03:03:54]

Description was changed from

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133

Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Cr-Commit-Position: refs/heads/master@{#405211}
==========

to

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133

Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Cr-Commit-Position: refs/heads/master@{#405211}
==========

[commit-bot: I haz the power, 2016-07-14 03:03:55]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-14 03:04:01]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 03:05:01]

Description was changed from

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133

Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Cr-Commit-Position: refs/heads/master@{#405211}
==========

to

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133

Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Committed: https://crrev.com/0c3ad3287b3fae9b6329f8e7c34c0117b5c8da65
Cr-Original-Commit-Position: refs/heads/master@{#405211}
Cr-Commit-Position: refs/heads/master@{#405424}
==========

[commit-bot: I haz the power, 2016-07-14 03:05:02]

Patchset 4 (id:??) landed as
https://crrev.com/0c3ad3287b3fae9b6329f8e7c34c0117b5c8da65
Cr-Commit-Position: refs/heads/master@{#405424}

[magjed_chromium, 2016-07-14 10:17:44]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2150883002/ by magjed@chromium.org.

The reason for reverting is: Breaks bot
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win7%20Tester/build...

It's not allowed to add new callbacks to AtExitManager while the callbacks are
already being processed. So that lazy global data is causing trouble.

[3972:940:0713/225822:FATAL:at_exit.cc(60)] Check failed:
!g_top_manager->processing_callbacks_. 
Backtrace:
	base::debug::StackTrace::StackTrace [0x02F60BB7+23]
	logging::LogMessage::~LogMessage [0x02F1D2E1+49]
	base::AtExitManager::RegisterTask [0x02F61AAA+186]
	base::AtExitManager::RegisterCallback [0x02F619D9+137]
	base::internal::CompleteLazyInstance [0x02F3F2EB+27]
	base::LazyInstance<base::Lock,base::DefaultLazyInstanceTraits<base::Lock>
>::Pointer [0x01483E7A+47]
	base::RefCountedThreadSafe<gfx::ColorSpace::GlobalData,base::DefaultRefCountedThreadSafeTraits<gfx::ColorSpace::GlobalData>
>::DeleteInternal [0x038E9685+17]
	gfx::ColorSpace::~ColorSpace [0x038E9505+14]
	base::internal::Invoker<base::internal::BindState<void (__cdecl*)(void const
*),void const *>,void __cdecl(void)>::Run [0x02F61AFE+14]
	base::AtExitManager::ProcessCallbacksNow [0x02F61894+388]
	base::AtExitManager::~AtExitManager [0x02F616D3+163]
	std::default_delete<base::AtExitManager>::operator() [0x0169C6FD+17]
	base::TestSuite::~TestSuite [0x02FB1D0D+44]
	content::LaunchTests [0x049A02D7+592]
	LaunchChromeTests [0x02F13585+49]
	main [0x00D0B49E+94]
	__scrt_common_main_seh [0x04A0EC12+255]
(f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:255)
	BaseThreadInitThunk [0x766E338A+18]
	RtlInitializeExceptionChain [0x77D49902+99]
	RtlInitializeExceptionChain [0x77D498D5+54]
.

[ccameron, 2016-07-14 17:51:51]

Description was changed from

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133

Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Committed: https://crrev.com/0c3ad3287b3fae9b6329f8e7c34c0117b5c8da65
Cr-Original-Commit-Position: refs/heads/master@{#405211}
Cr-Commit-Position: refs/heads/master@{#405424}
==========

to

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133

Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Committed: https://crrev.com/0c3ad3287b3fae9b6329f8e7c34c0117b5c8da65
Cr-Original-Commit-Position: refs/heads/master@{#405211}
Cr-Commit-Position: refs/heads/master@{#405424}
==========

[ccameron, 2016-07-14 18:36:53]

ccameron@chromium.org changed reviewers:
+ magjed@chromium.org

[ccameron, 2016-07-14 18:42:11]

ptal -- IIUC this fixes things.

Are there any trybots for the Windows failure?

https://codereview.chromium.org/2140803002/diff/120001/ui/gfx/color_space.cc
File ui/gfx/color_space.cc (right):

https://codereview.chromium.org/2140803002/diff/120001/ui/gfx/color_space.cc#...
ui/gfx/color_space.cc:84: // back into it at unpredictable times during
tear-down.
The previous patch had 2 problems, one of which the bot caught:

Problem 1: Accessing map_ during AtExit will cause problems because it will try
to register an AtExit handler to tear it down.
Problem 2: We can't predict when the scoped_ptrs that point back to map_ will be
destroyed, and we may end up re-accessing map_ after it is destroyed.

IIUC making this Leaky will fix both issues.
Fix 1: Leaky doesn't register an AtExit dtor callback
Fix 2: map_ will never be destroyed, so the scoped_ptrs that point back into it
can continue to access it during tear-down.

[hubbe, 2016-07-14 18:54:07]

lgtm

https://codereview.chromium.org/2140803002/diff/120001/ui/gfx/color_space.cc
File ui/gfx/color_space.cc (right):

https://codereview.chromium.org/2140803002/diff/120001/ui/gfx/color_space.cc#...
ui/gfx/color_space.cc:84: // back into it at unpredictable times during
tear-down.
On 2016/07/14 18:42:10, ccameron wrote:
> The previous patch had 2 problems, one of which the bot caught:
> 
> Problem 1: Accessing map_ during AtExit will cause problems because it will
try
> to register an AtExit handler to tear it down.
> Problem 2: We can't predict when the scoped_ptrs that point back to map_ will
be
> destroyed, and we may end up re-accessing map_ after it is destroyed.
> 
> IIUC making this Leaky will fix both issues.
> Fix 1: Leaky doesn't register an AtExit dtor callback
> Fix 2: map_ will never be destroyed, so the scoped_ptrs that point back into
it
> can continue to access it during tear-down.

Might also want to explain that since the map doesn't actually *own* anything,
only the map is leaky, not the GlobalData* that it tracks.

[ccameron, 2016-07-14 19:03:31]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 19:04:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ccameron, 2016-07-14 19:06:25]

On 2016/07/14 18:54:07, hubbe wrote:
> lgtm
> 
> https://codereview.chromium.org/2140803002/diff/120001/ui/gfx/color_space.cc
> File ui/gfx/color_space.cc (right):
> 
>
https://codereview.chromium.org/2140803002/diff/120001/ui/gfx/color_space.cc#...
> ui/gfx/color_space.cc:84: // back into it at unpredictable times during
> tear-down.
> On 2016/07/14 18:42:10, ccameron wrote:
> > The previous patch had 2 problems, one of which the bot caught:
> > 
> > Problem 1: Accessing map_ during AtExit will cause problems because it will
> try
> > to register an AtExit handler to tear it down.
> > Problem 2: We can't predict when the scoped_ptrs that point back to map_
will
> be
> > destroyed, and we may end up re-accessing map_ after it is destroyed.
> > 
> > IIUC making this Leaky will fix both issues.
> > Fix 1: Leaky doesn't register an AtExit dtor callback
> > Fix 2: map_ will never be destroyed, so the scoped_ptrs that point back into
> it
> > can continue to access it during tear-down.
> 
> Might also want to explain that since the map doesn't actually *own* anything,
> only the map is leaky, not the GlobalData* that it tracks.

Good point -- updated.

[ccameron, 2016-07-14 19:38:00]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2016-07-14 19:38:01]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-07-14 19:38:01]

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/2140803002/#ps140001
(title: "Update comment")

[commit-bot: I haz the power, 2016-07-14 19:38:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 20:12:16]

Description was changed from

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133

Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Committed: https://crrev.com/0c3ad3287b3fae9b6329f8e7c34c0117b5c8da65
Cr-Original-Commit-Position: refs/heads/master@{#405211}
Cr-Commit-Position: refs/heads/master@{#405424}
==========

to

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133

Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Committed: https://crrev.com/0c3ad3287b3fae9b6329f8e7c34c0117b5c8da65
Cr-Original-Commit-Position: refs/heads/master@{#405211}
Cr-Commit-Position: refs/heads/master@{#405424}
==========

[commit-bot: I haz the power, 2016-07-14 20:12:17]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-14 20:12:38]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 20:14:29]

Description was changed from

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133

Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Committed: https://crrev.com/0c3ad3287b3fae9b6329f8e7c34c0117b5c8da65
Cr-Original-Commit-Position: refs/heads/master@{#405211}
Cr-Commit-Position: refs/heads/master@{#405424}
==========

to

==========
Color: Don't duplicate ICC profile data

Make gfx::ColorSpace have an internal pointer to a globally-unique
structure for the color space, where its ICC profile can be stored.

BUG=622133

Committed: https://crrev.com/92294e78c762e993e8dcf538860abf65629f3320
Committed: https://crrev.com/0c3ad3287b3fae9b6329f8e7c34c0117b5c8da65
Committed: https://crrev.com/f1689c8b1c5d6411b8a14912227590345c3c3a94
Cr-Original-Original-Commit-Position: refs/heads/master@{#405211}
Cr-Original-Commit-Position: refs/heads/master@{#405424}
Cr-Commit-Position: refs/heads/master@{#405552}
==========

[commit-bot: I haz the power, 2016-07-14 20:14:30]

Patchset 8 (id:??) landed as
https://crrev.com/f1689c8b1c5d6411b8a14912227590345c3c3a94
Cr-Commit-Position: refs/heads/master@{#405552}