Last Comment Bug 709732 - double color correction with X Color Management
: double color correction with X Color Management
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: GFX: Color Management (show other bugs)
: 8 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: ku.b@gmx.de
:
Mentors:
: 462398 (view as bug list)
Depends on: 817333
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-12 01:46 PST by ku.b@gmx.de
Modified: 2012-12-03 00:50 PST (History)
13 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch to fix the _ICC_PROFILE bug (647 bytes, patch)
2012-01-14 04:53 PST, ku.b@gmx.de
jmuizelaar: review-
Details | Diff | Splinter Review
revised patch to fix the _ICC_PROFILE bug (1.02 KB, patch)
2012-05-09 03:00 PDT, ku.b@gmx.de
jmuizelaar: review+
Details | Diff | Splinter Review

Description ku.b@gmx.de 2011-12-12 01:46:02 PST
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 ku.b@gmx.de 2011-12-12 01:50:26 PST
Might be a duplicate of bmo#462398
Comment 2 j.j. 2011-12-12 02:35:45 PST
("See Also" field is intended for links to external bug tracking systems)
Comment 3 ku.b@gmx.de 2011-12-12 03:25:41 PST
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 [:bz] 2011-12-12 06:35:27 PST
Jeff, who's the right person to look at this?
Comment 5 Jeff Muizelaar [:jrmuizel] 2011-12-12 08:13:32 PST
(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?
Comment 6 ku.b@gmx.de 2011-12-12 08:54:08 PST
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 ku.b@gmx.de 2012-01-14 04:53:59 PST
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.
Comment 8 Jan Horak 2012-04-18 08:59:53 PDT
(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.
Comment 9 ku.b@gmx.de 2012-04-18 23:56:47 PDT
(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.
Comment 10 Jan Horak 2012-05-03 02:40:00 PDT
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] 2012-05-03 13:12:48 PDT
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 ku.b@gmx.de 2012-05-09 03:00:22 PDT
Created attachment 622324 [details] [diff] [review]
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] 2012-05-09 07:01:01 PDT
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 ku.b@gmx.de 2012-07-17 00:24:37 PDT
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. 2012-07-17 01:02:24 PDT
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] 2012-07-18 18:09:48 PDT
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 ku.b@gmx.de 2012-07-18 22:42:47 PDT
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] 2012-07-19 07:32:09 PDT
https://hg.mozilla.org/mozilla-central/rev/ed8555e2f99e
Comment 19 j.j. 2012-07-19 07:54:04 PDT
For the record, this is fixed in Firefox 17.
ku.b, you can test in Nightly from tomorrow (afternoon):  http://nightly.mozilla.org/
Comment 20 Karl Tomlinson (:karlt) 2012-12-02 13:54:09 PST
*** Bug 462398 has been marked as a duplicate of this bug. ***
Comment 21 ku.b@gmx.de 2012-12-03 00:13:10 PST
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).

Note You need to log in before you can comment on or make changes to this bug.