double color correction with X Color Management

RESOLVED FIXED in mozilla17

Status

()

Core
GFX: Color Management
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ku.b@gmx.de, Assigned: ku.b@gmx.de)

Tracking

(Depends on: 1 bug)

8 Branch
mozilla17
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 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

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

Comment 2

6 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: bug 462398
Whiteboard: [dupme]
(Assignee)

Comment 3

6 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

6 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

5 years ago
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

5 years ago
(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

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

Comment 9

5 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.

Comment 10

5 years ago
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.

Updated

5 years ago
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

5 years ago
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++.
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

5 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

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

Comment 15

5 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

5 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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Comment 19

5 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

5 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.