462398 - Firefox color management does not honor _ICC_PROFILE X11 atoms.

Status: RESOLVED DUPLICATE of bug 709732

(Core :: Graphics: Color Management, defect)

Description

[Hal Engel]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.3) Gecko/2008093018 Gentoo Firefox/3.0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.3) Gecko/2008093018 Gentoo Firefox/3.0.3

After testing it has been found that firefox is not honoring the the _ICC_PROFILE atoms on X11 systems.  As a result it is using the default sRGB profile even when the _ICC_PROFILE has been correctly set.

Reproducible: Always

Steps to Reproduce:
1. Use a utility like Oyranos oyranos_monitor, ArgyllCMS dispwin or the KDE Kolor Manager systems settings applet to install http://hoech.net/files/BRG.icc as the system default profile for one of the monitors on the test system.  All of these utilities set the ICC_PROFILE X11 atoms for single and multi-monitor systems.  This profile will result in some very strange colors if it is used and it is intended for testing purposes.

2. Open firefox and go to http://www.color.org/version4html.xalter.  This web page is from the ICC and it has 4 images with ICC profile tags. Firefox will display all of the images in the sRGB color space rather than the profile specified by the X11 _ICC_PROFILE atom.  In other words the images will look normal.

3.  To see what the above should have looked like if firefox honored the X11 _ICC_PROFILE atom change the gfx.color_management_display_profile setting to point to the above profile.  Then restart firefox.  Going to the ICC test web page will result in the images being displayed in some rather garish colors which confirms that the profile is now being used. 
Actual Results:  
Firefox does not use the X11 _ICC_PROFILE atoms when users have set gfx.color_management_display_profile is set to default.

Expected Results:  
Firefox should use the X11 _ICC_PROFILE atoms when users have set gfx.color_management_display_profile is set to default.

Comment 1

[Roland Kuck]

Attachment: Use correct length of property to retrieve whole content

As far as I can see the bug still exists in the current source tree. This can be reproduced following the steps in the bug description. I actually only tested it with the Firefox 7 and 8.

The attached patch fixes the issue. The code to retrieve the _ICC_PROFILE property is using the XGetWindowProperty function twice, once to get the actual size of the data and then to retrieve it using that size. The second invocation uses the length of the _returned_ data of the first invocation, which has to be zero. The patch corrects this and uses the number of remaining bytes in the property, which is the size of the complete data, as nothing has been read before. The multiplication and division is required to convert the given number of bytes into the number of 32-bit values as required by the function.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 577163 [details] [diff] [review]
Use correct length of property to retrieve whole content

Actually, I'm pretty sure what you've done is wrong since it doesn't match your description.  I think you meant to include parentheses like this:

 (retAfter + 3) / 4

If you revise it accordingly, could you then attach a revised patch and request review on that patch from ":jrmuizel".  (He might not be the best person, but if he's not he should know who is.)


Also, it would help to know how you've tested that the patch works correctly.

Comment 3

[Benoit Girard (:BenWa)]

Attachment: fix patch

Fixed up the patch since it was a trivial typo. I attributed to the patch to "Roland Kuck" with your bugzilla email.

Here's the relevant documentation: 
http://tronche.com/gui/x/xlib/window-information/XGetWindowProperty.html

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 577169 [details] [diff] [review]
fix patch

># HG changeset patch
># User Roland Kuck <[e-mail]>
># Date 1322436238 18000
># Node ID 6e4db956c27145b91969f79bdba457b60919a7eb
># Parent  f3fd9999da3e8e40a6e9a1243295a82bae807351
>Bug 462398 - Firefox color management does not honor _ICC_PROFILE X11 atoms. r=jrmuizel
>
>diff --git a/gfx/thebes/gfxPlatformGtk.cpp b/gfx/thebes/gfxPlatformGtk.cpp
>--- a/gfx/thebes/gfxPlatformGtk.cpp
>+++ b/gfx/thebes/gfxPlatformGtk.cpp
>@@ -544,17 +544,17 @@ gfxPlatformGtk::GetPlatformCMSOutputProf
>     if (iccAtom) {
>         // read once to get size, once for the data
>         if (Success == XGetWindowProperty(dpy, root, iccAtom,
>                                           0, 0 /* length */,
>                                           False, AnyPropertyType,
>                                           &retAtom, &retFormat, &retLength,
>                                           &retAfter, &retProperty)) {
>             XGetWindowProperty(dpy, root, iccAtom,
>-                               0, retLength,
>+                               0, (retAfter+3)/4,

It would be better if we gave the value (retAfter+3)/4 a symbolic name like: totalLongLength.

>                                False, AnyPropertyType,
>                                &retAtom, &retFormat, &retLength,
>                                &retAfter, &retProperty);
> 
>             qcms_profile* profile = NULL;
> 
>             if (retLength > 0)
>                 profile = qcms_profile_from_memory(retProperty, retLength);

Comment 5

[Benoit Girard (:BenWa)]

Do you want to make this change Roland?

Comment 6

[Karl Tomlinson (:karlt)]

Is is possible to use only one XGetWindowProperty round trip by passing a large number to for the |long_length| parameter.  GDK for example uses G_MAXLONG, which Xlib truncates to unsigned int for the wire protocol, so something like PR_INT32_MAX would be safe.

Comment 7

[[e-mail]]

fix, description and link to a patch is in 709732

Comment 8

[Karl Tomlinson (:karlt)]

Looks like this should be fixed by the change in bug 709732