Firefox color management does not honor _ICC_PROFILE X11 atoms.

RESOLVED DUPLICATE of bug 709732

Status

()

Core
GFX: Color Management
RESOLVED DUPLICATE of bug 709732
9 years ago
5 years ago

People

(Reporter: Hal Engel, Assigned: Roland Kuck)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

9 years ago
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.
Component: General → GFX
Product: Firefox → Core
QA Contact: general → general
Product: Core → Core Graveyard
(Reporter)

Updated

9 years ago
Component: GFX → GFX: Xlib
Hardware: x86 → All
(Assignee)

Comment 1

6 years ago
Created attachment 577163 [details] [diff] [review]
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.
Component: GFX: Xlib → GFX: Color Management
Product: Core Graveyard → Core
QA Contact: general → color-management
Attachment #577163 - Flags: review?(jmuizelaar)
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.
Attachment #577163 - Flags: review?(jmuizelaar) → review-
Created attachment 577169 [details] [diff] [review]
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
Attachment #577163 - Attachment is obsolete: true
Attachment #577169 - Flags: review?(jmuizelaar)

Updated

6 years ago
Assignee: nobody → roland.kuck
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 577169 [details] [diff] [review]
fix patch

># HG changeset patch
># User Roland Kuck <roland.kuck@gmx.de>
># 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);
Attachment #577169 - Flags: review?(jmuizelaar) → review-
Do you want to make this change Roland?
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.

Updated

6 years ago
Blocks: 709732

Comment 7

6 years ago
fix, description and link to a patch is in 709732

Updated

5 years ago
No longer blocks: 709732
Depends on: 709732
Looks like this should be fixed by the change in bug 709732
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
No longer depends on: 709732
Resolution: --- → DUPLICATE
Duplicate of bug: 709732
You need to log in before you can comment on or make changes to this bug.