double color correction with X Color Management

RESOLVED FIXED in mozilla17

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: ku.b, Assigned: ku.b)

Tracking

(Depends on 1 bug)

8 Branch
mozilla17
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

8 years ago
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.
Assignee

Comment 1

8 years ago
Might be a duplicate of bmo#462398
See Also: → 462398

Comment 2

8 years ago
("See Also" field is intended for links to external bug tracking systems)
Component: General → General
Depends on: 462398
Product: Firefox → Core
QA Contact: general → general
See Also: 462398
Whiteboard: [dupme]
Assignee

Comment 3

8 years ago
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
Jeff, who's the right person to look at this?
Status: UNCONFIRMED → NEW
Component: General → Widget: Gtk
Ever confirmed: true
QA Contact: general → gtk
(In reply to ku.b@gmx.de 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?
Assignee: nobody → jmuizelaar
Assignee

Comment 6

8 years ago
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.
Assignee

Comment 7

8 years ago
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.
(In reply to ku.b@gmx.de 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 jrmuizel@mozilla.com) from module owner list page.
Assignee

Updated

7 years ago
Attachment #588625 - Flags: review?(jmuizelaar)
Assignee

Comment 9

7 years ago
(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 jrmuizel@mozilla.com) from module owner list page.

I was not aware of this procedure. Thanks for your hint.
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 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.
Attachment #588625 - Flags: review?(jmuizelaar) → review-
Assignee

Comment 12

7 years ago
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++.
Attachment #588625 - Attachment is obsolete: true
Attachment #622324 - Flags: review?(jmuizelaar)
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.
Attachment #622324 - Flags: review?(jmuizelaar) → review+
Assignee

Comment 14

7 years ago
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?
Assignee

Updated

7 years ago
Blocks: 462398
No longer depends on: 462398

Comment 15

7 years ago
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.
Keywords: checkin-needed
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
Assignee: jmuizelaar → ku.b
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: [dupme]
Assignee

Comment 17

7 years ago
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!
https://hg.mozilla.org/mozilla-central/rev/ed8555e2f99e
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Comment 19

7 years ago
For the record, this is fixed in Firefox 17.
ku.b, you can test in Nightly from tomorrow (afternoon):  http://nightly.mozilla.org/
No longer blocks: 462398
Duplicate of this bug: 462398
Component: Widget: Gtk → GFX: Color Management
Depends on: 817333
Assignee

Comment 21

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