971126 - Switch CMS preferences to once per session, rather than updated as they change

Status: NEW

(Core :: Graphics, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

From BenWa:
To properly handle a output display profile change we need to load the new output profile and reconvert all our images. We don't have the code to that properly ATM.

That's being said what we do is very wrong and should be changed. I think our options here are:
1) Never touch the output display profile after startup.
2) Set the new display profile and any images decoded from now on will have the new profile. This can lead to a page having some images with one output profile and some with the other.
3) Implement the code path to invalidate and recovert the images. I'm not familiar with imagelib enough to know how difficult this would be to do.

Since CMS is a low priority, and runtime switching should be an even lower priority I vote for 1).
---------------------

This bug is about switching the CMS preferences to "once per session".

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Hmm, this exactly contradicts bug 452125 work.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #1)
> Hmm, this exactly contradicts bug 452125 work.

Or maybe not - I can't tell if bug 452125 is just saying we need a force-srgb preference, or if it's also saying that we need it updated during the session execution.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Make the CMS prefs "once per session" and deletet the code we don't need after that change.

This needs bug 912794 to land, will ask for a review when that happens.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Make the CMS prefs "once per session" and delete the code we don't need after that change.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Make the CMS prefs "once per session" and delete the code we don't need after that change.

This requires a patch from bug 910860.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Make the CMS prefs "once per session" and delete the code we don't need after that change.

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 8376317 [details] [diff] [review]
Make the CMS prefs "once per session" and delete the code we don't need after that change.

Review of attachment 8376317 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPlatform.cpp
@@ +1827,5 @@
>         color_management.mode.  These calls should be made before gfxPrefs
>         is initialized, otherwise we may not pick up the correct values
>         with the gfxPrefs functions.
>      */
> +    MOZ_ASSERT(!gfxPrefs::Exists());

I don't understand this assert and even this hunk. I wonder if we can just nuke this migrate now. I checked with jeff and he agrees. I'd guess this dates back from early FF3.x. I don't think we should carry it in the future forever and now is likely a good time to remove it. We can get a few nanosecond of startup time back :)

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

The assert was saying "we better convert the prefs before we read them once in the session".  However, if we can get rid of the whole thing, even better!

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Make the CMS prefs "once per session" and delete the code we don't need after that change. Carry r=bgirard.

Also removed MigratePrefs.  See comment 7.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Just a note - This needs 972099 in order to move gfx.color_management.force_srgb to "once per session".  Right now, reftest set that preference to true (the default is false), but that set (coming from reftest-cmdline.js) happens after the first opportunity we had to read the pref.  After that, any changes are ignored, if the preference itself is "once per session".