709732 - double color correction with X Color Management

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[[e-mail]]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 2011110500

Steps to reproduce:

Use a desktop colour server (CompICC) to do whole screen colour correction.
Set the monitor ICC profile using the _ICC_PROFILE in X spec.
Start Firefox with images containing embedded ICC profiles and compare the same images in external viewers (eog, calligra-krita).


Actual results:

The images differ. The more the monitor needs colour correction compared to sRGB, the more the difference becomes obvious. In a virtual environment all colours look proper as sRGB is assumed. On a wide gamut monitor under a colour server the image looks most desaturated.


Expected results:

Images look always the same at least inside the sRGB space.

Comment 1

[[e-mail]]

Might be a duplicate of bmo#462398

Comment 2

[j.j.]

("See Also" field is intended for links to external bug tracking systems)

Comment 3

[[e-mail]]

I had to fix the _ICC_PROFILE bug and patch Firefox to run CompICC without double colour correction in the Firefox window:
https://build.opensuse.org/package/rdiff?opackage=MozillaFirefox&oproject=openSUSE%3A11.4%3AUpdate&package=MozillaFirefox&project=home%3Abekun%3Abranches%3AopenSUSE%3A11.4%3AUpdate&rev=3

The essential part is in mozilla-gfx-icc-profile-fix.patch the substitution of
-                               False, AnyPropertyType,
+                               False, (Atom) 6/*XA_CARDINAL*/,
in the calls to XGetWindowProperty().

That requirement is specified in all versions of the ICC Profiles in X Specification:
http://www.freedesktop.org/wiki/OpenIcc/ICC_Profiles_in_X_Specification_0.4

btw. more linux colour management specs are here, like paths and X Color Management:
http://www.freedesktop.org/wiki/OpenIcc#Specifications

thanks

Comment 4

[Boris Zbarsky [:bzbarsky]]

Jeff, who's the right person to look at this?

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(In reply to [e-mail] from comment #3)
> I had to fix the _ICC_PROFILE bug and patch Firefox to run CompICC without
> double colour correction in the Firefox window:
> https://build.opensuse.org/package/
> rdiff?opackage=MozillaFirefox&oproject=openSUSE%3A11.
> 4%3AUpdate&package=MozillaFirefox&project=home%3Abekun%3Abranches%3AopenSUSE%
> 3A11.4%3AUpdate&rev=3
> 
> The essential part is in mozilla-gfx-icc-profile-fix.patch the substitution
> of
> -                               False, AnyPropertyType,
> +                               False, (Atom) 6/*XA_CARDINAL*/,
> in the calls to XGetWindowProperty().

Can you explain why this would have any impact? Shouldn't AnyPropertyType work just as well as XA_CARDINAL?

Comment 6

[[e-mail]]

No, I have not much an idea about the Xorg servers internals. That behaviour appears to be undocumented.

However I had to explicitly use XA_CARDINAL in Oyranos CMS in order to get the _ICC_PROFILE(_xxx) spec atoms working. The same way that change fixed FireFox.

Comment 7

[[e-mail]]

Attachment: patch to fix the _ICC_PROFILE bug

The osb patch contained actually more changes than just using XA_CARDINAL.
I found that the first XGetWindowProperty call returns the length of the atom in the retAfter variable, which should be used in the second call. Patch is attached.

Comment 8

[Jan Horak [:jhorak]]

(In reply to [e-mail] from comment #7)
> Created attachment 588625 [details] [diff] [review]
> patch to fix the _ICC_PROFILE bug
> 
> The osb patch contained actually more changes than just using XA_CARDINAL.
> I found that the first XGetWindowProperty call returns the length of the
> atom in the retAfter variable, which should be used in the second call.
> Patch is attached.

You need to ask for review to your patch, then it can be commited. According to 
module owners in: https://wiki.mozilla.org/Modules/All#Graphics (it seems to be Graphics section). See patch details, set 'review' to ? and pick an owner or peer (like [e-mail]) from module owner list page.

Comment 9

[[e-mail]]

(In reply to jhorak from comment #8)
> You need to ask for review to your patch, then it can be commited. According
> to 
> module owners in: https://wiki.mozilla.org/Modules/All#Graphics (it seems to
> be Graphics section). See patch details, set 'review' to ? and pick an owner
> or peer (like [e-mail]) from module owner list page.

I was not aware of this procedure. Thanks for your hint.

Comment 10

[Jan Horak [:jhorak]]

Could we have a review to this patch please? This issue is hitting some color correction awarded users of Fedora:
https://bugzilla.redhat.com/show_bug.cgi?id=813851
Thanks.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 588625 [details] [diff] [review]
patch to fix the _ICC_PROFILE bug

After looking at this some more, I think we can replace the double call with a single call that just passes in a large length like MAX_LONG. The length property is only used to limit the amount returned and we have no need for that. Doing a single call will avoid the round trip and you can see a similar convention in gdk.

Comment 12

[[e-mail]]

Attachment: revised patch to fix the _ICC_PROFILE bug

Sorry for the delay due to a conference. 
As Jeff suggested, I removed the second call to XGetWindowProperty.

The maximum length is specified through INT_MAX, which should be supported on recent compilers through C99 and in VC++.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 622324 [details] [diff] [review]
revised patch to fix the _ICC_PROFILE bug

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

Looks fine to me.

Comment 14

[[e-mail]]

Inbetween FF-13 came out and still shows this bug. Looking into the sources the bug is still in the code. Anything else I can help?

Comment 15

[j.j.]

Nobody checked this in. There is the "checkin-needed" keyword for that purpose. I have no clue whether it's appropriate here, setting nevertheless just to trigger action.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8555e2f99e

Thanks for the patch! One request - please follow the directions at the link below for future patches you submit. It makes life much easier for those checking in patches on your behalf. Thanks again!
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

Comment 17

[[e-mail]]

Ok, will follow the Howto next time. Below for reference,
https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch

Thanks to all who helped getting the fix in!

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ed8555e2f99e

Comment 19

[j.j.]

For the record, this is fixed in Firefox 17.
ku.b, you can test in Nightly from tomorrow (afternoon):  http://nightly.mozilla.org/

Comment 21

[[e-mail]]

Confirming that distribution packages of FF-17.0 in openSUSE and Ubuntu work correctly like the mozilla.org tar ball for 17.0.1(32bit).