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

NEW
Assigned to

Status

()

Core
Graphics
4 years ago
4 years ago

People

(Reporter: milan, Assigned: milan)

Tracking

(Depends on: 1 bug)

28 Branch
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
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".
(Assignee)

Updated

4 years ago
Assignee: nobody → milan
Depends on: 912794
(Assignee)

Updated

4 years ago
OS: Mac OS X → All
(Assignee)

Comment 1

4 years ago
Hmm, this exactly contradicts bug 452125 work.
See Also: → bug 452125
(Assignee)

Comment 2

4 years ago
(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.
(Assignee)

Comment 3

4 years ago
Created attachment 8374479 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Depends on: 972099
(Assignee)

Updated

4 years ago
Depends on: 971817
(Assignee)

Comment 4

4 years ago
Created attachment 8375828 [details] [diff] [review]
Make the CMS prefs "once per session" and delete the code we don't need after that change.
Attachment #8374479 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Depends on: 910860
No longer depends on: 971817
(Assignee)

Comment 5

4 years ago
Created attachment 8376299 [details] [diff] [review]
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.
Attachment #8376299 - Flags: review?(bgirard)
(Assignee)

Updated

4 years ago
Attachment #8375828 - Attachment is obsolete: true
(Assignee)

Comment 6

4 years ago
Created attachment 8376317 [details] [diff] [review]
Make the CMS prefs "once per session" and delete the code we don't need after that change.
Attachment #8376299 - Attachment is obsolete: true
Attachment #8376299 - Flags: review?(bgirard)
Attachment #8376317 - Flags: review?(bgirard)
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 :)
Attachment #8376317 - Flags: review?(bgirard) → review+
(Assignee)

Comment 8

4 years ago
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!
(Assignee)

Comment 9

4 years ago
Created attachment 8376491 [details] [diff] [review]
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.
Attachment #8376317 - Attachment is obsolete: true
Attachment #8376491 - Flags: review+
(Assignee)

Comment 10

4 years ago
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".
You need to log in before you can comment on or make changes to this bug.