1626738 - CMS code loads color profile in main thread prior to first paint

Status: NEW

(Core :: Graphics: Color Management, defect, P3)

Description

[Chris Martin [:cmartin]]

Currently the browser_startup_mainthreadio.js incorrectly passes because the test is run with the gfx.color_management.force_srgb pref set to true, which causes a hard-coded RGB profile to be used instead of the default system color profile (here).

But in real Firefox usage, that flag won't be set and the main thread will call GetCMSOutputProfileData(), which by default on Windows will access the hard drive to load the contents of Windows\System32\spool\drivers\color\sRGB Color Space Profile.icm into memory.

Comment 1

[Florian Quèze [:florian]]

(In reply to Chris Martin [:cmartin] from comment #0)

Currently the browser_startup_mainthreadio.js incorrectly passes because the test is run with the gfx.color_management.force_srgb pref set to true, which causes a hard-coded RGB profile to be used instead of the default system color profile (here).

The comments I saw show that this pref is set to true for reftests. Why is it also true when running mochitests?

Comment 2

[Chris Martin [:cmartin]]

(In reply to Florian Quèze [:florian] from comment #1)

(In reply to Chris Martin [:cmartin] from comment #0)

Currently the browser_startup_mainthreadio.js incorrectly passes because the test is run with the gfx.color_management.force_srgb pref set to true, which causes a hard-coded RGB profile to be used instead of the default system color profile (here).

The comments I saw show that this pref is set to true for reftests. Why is it also true when running mochitests?

I'm not sure why this pref is set for this test, and my intuition is that it shouldn't be. But I'm probably not the right person to ask - I can only confirm from my own testing that the pref is true when the test is run.

I'm not really sure who is the best person to talk to about this.

I have re-added you to that other review I have for remoteing the color profile loading. It adds the color profile loading to the exception list for main thread I/O. Keep in mind that there really is no change - We've always been loading the color profile on the main thread in real Firefox. It's that we are testing a fake usecase.

Comment 3

[Florian Quèze [:florian]]

(In reply to Chris Martin [:cmartin] from comment #2)

I have re-added you to that other review I have for remoteing the color profile loading. It adds the color profile loading to the exception list for main thread I/O.

Thanks for putting us (front-end performance) in the loop! :-)

Keep in mind that there really is no change - We've always been loading the color profile on the main thread in real Firefox. It's that we are testing a fake usecase.

Here's what we see in a startup profile on a real Windows machine: https://perfht.ml/2ykHHAn
The file is first opened when calling GetICMProfileW and then it seems we open it ourselves a second time in qcms_data_from_unicode_path to read it. Is one of these 2 things avoidable?

And it seems we do the same thing on content process startup: https://perfht.ml/2UZlkYU

So I guess if the preference wasn't messed up, your patch would remove a whitelist entry in https://searchfox.org/mozilla-central/rev/7fba7adfcd695343236de0c12e8d384c9b7cd237/browser/base/content/test/performance/browser_startup_content_mainthreadio.js#65, is this correct?

Comment 4

[Chris Martin [:cmartin]]

So I guess if the preference wasn't messed up, your patch would remove a whitelist entry

Unfortunately, no. My new change still requires the main thread I/O. The difference is that my new change (D66126) reads the color profile regardless of the gfx.color_management.force_srgb pref, because it has to send it to the child processes. So now the test won't pass because the force_srgb pref no longer masks the I/O. So I actually have to add the color profile path to the whitelist in order for my change to work.

The file is first opened when calling GetICMProfileW and then it seems we open it ourselves a second time in qcms_data_from_unicode_path to read it. Is one of these 2 things avoidable?

Unfortunately, I don't think so. The first read is done by the Win32 API to validate that the profile exists and is valid before it returns the path to it. There doesn't seem to be a way for us to tell Win32 not to do this validation step. The second read is done by Firefox when we actually read the file at the returned path.

It also doesn't appear that Win32 has an API that allows us to do both in one step :(

And it seems we do the same thing on content process startup

We did, but this is a win for both of us! Now, thanks to this change, the child processes won't need to do any I/O for color profile anymore. They will all be passed a copy from the parent through the SetXPCOMProcessAttributes message.

Comment 5

[Florian Quèze [:florian]]

(In reply to Chris Martin [:cmartin] from comment #4)

So I guess if the preference wasn't messed up, your patch would remove a whitelist entry

Unfortunately, no. My new change still requires the main thread I/O.

If I understand correctly, currently we do the main thread I/O on both the parent process and every content process. After your changes we'll only do it in the parent.
browser_startup_content_mainthreadio.js which I linked to in my previous comment is the same test as the one you modified, but for content processes.

Comment 6

[Chris Martin [:cmartin]]

browser_startup_content_mainthreadio.js which I linked to in my previous comment is the same test as the one you modified, but for content processes.

Ah, I see. Sorry - Easy to miss the word "content" in there.

You're correct: After my change, there will no longer be main thread I/O for color profile in the content processes.

Comment 7

[Firefox Bug Husbandry Bot]

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)