Rendering intent is ignored completely during color management

NEW
Unassigned

Status

()

defect
8 years ago
7 years ago

People

(Reporter: Smorpg, Unassigned)

Tracking

(Depends on 1 bug, Blocks 1 bug)

8 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:8.0a1) Gecko/20110811 Firefox/8.0a1
Build ID: 20110811030759

Steps to reproduce:

Load the attached ICC profile in Firefox v8.0a1, and enable color management for *all* graphics (not just tagged images). Then switch the gfx.color_management.rendering_intent flag between its possible values (-1 through 3), restarting the browser after every change.

Test image is attached as well, to identify what profile type is being used.


Actual results:

gfx.color_management.enablev4: false
gfx.color_management.rendering_intent: *any value*
Result: Always uses matrix+TRC (tone response curve), result is that the front ball appears brownish.

gfx.color_management.enablev4: true
gfx.color_management.rendering_intent: *any value*
Result: Always uses the A2B0 table (perceptual) mode, regardless of setting.


Expected results:

Expected output is to use the appropriate rendering intent based on the flag's setting:

If set to 1 or 3 (colorimetric), the image should be dominated by a green color with pink highlights and bits of yellow. In the mode 1 the white point should be adjusted as well but this isn't as easily noticeable.

If set to 0 (perceptual), the image should be dominated by a red color with cyan highlights and bits of green.

I don't know how it should behave for 2.
(Reporter)

Comment 1

8 years ago
Posted image Test case
(Reporter)

Comment 2

8 years ago
Duplicate of this bug: 675303
I don't have time to look at this at the moment but we would take a patch. Willing to help out if someone wants to look at this.

Relevant source files:
gfx/qcms/transform.c <-- A2B table selected
gfx/qcms/chain.c     <-- A2B table selected
gfx/qcms/iccread.c   <-- A2B tables parsed
gfx/qcms/qcms.h      <-- Rendering intent selected

See the ICC v4 specification for more information on rendering intents.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug][mentor=benwa]
OS: Windows 7 → All
Hardware: x86 → All

Comment 5

8 years ago
I'm going to get this. It's my first bug, I don't know if it will take long :/
Let me know if you have any questions, this patch is important for improving our color management support. The idea is that currently the intent parameters is mostly ignored and we will want to use it to select the right A2B table.
Blocks: 679875

Comment 7

8 years ago
I have various questions. Some I think they are a little bit evident, but I prefer to ask.
1)	I need to use the variable that contains the rendering intent that is currently using firefox.
In the about:config appears as (gfx.color_management.rendering_intent) but I can't use it in ICCread.c ¿There is a way to use it in there or I'm totally wrong and the variable has another name?

2)	If the tag for the intent currently selected in firefox is not present in the file, what intent it's assumed to be used? The profile intent?

3)	The code in ICCread seems to try to get all info of the tables for the profile, also when it's possible that it's no needed. When I start coding what is best that I do, read all possible data or get only the tables that the intent is going to use?

4)	Should I add in the qcms_profile struct  A2B1, A2B2, B2A1, B2A2 variables or use the A2B0 B2A0 to store the tables although they don't correspond to that tables? If I'm correct it's only necessary a pair. (for example, store in the A2B0 the A2B2 table).

5)	In transform.c I don't know what I have to do exactly, if something is to be done. The only reference to tables that I found is for not use precache.
(Reporter)

Comment 8

8 years ago
2) I would assume so, yes

3,4) You should be fine just loading the correct table since firefox doesn't reload color management settings until you restart the browser either way. Probably best to rename it A2B and B2A instead of A2B0 to prevent confusion.
You can find the ICCv4 specification here:

(In reply to JordiM from comment #7)
> I have various questions. Some I think they are a little bit evident, but I
> prefer to ask.
> 1)	I need to use the variable that contains the rendering intent that is
> currently using firefox.
> In the about:config appears as (gfx.color_management.rendering_intent) but I
> can't use it in ICCread.c ¿There is a way to use it in there or I'm totally
> wrong and the variable has another name?
> 
The variable is passed through to 'qcms_transform_create' in qcms already but it's ignored.  You can use the following to trace exactly how it is read and passed to qcms by searching string and identifier:
http://mxr.mozilla.org/mozilla-central/

> 2)	If the tag for the intent currently selected in firefox is not present in
> the file, what intent it's assumed to be used? The profile intent?
> 

I think it should default to perceptual. You might section 6.2 and table 21 useful in the ICC spec.

> 3)	The code in ICCread seems to try to get all info of the tables for the
> profile, also when it's possible that it's no needed. When I start coding
> what is best that I do, read all possible data or get only the tables that
> the intent is going to use?
> 
I think for now we want iccread to read the entire profile and not make any assumption about how it will be used. You are correct that we can end up reading some tags that will not be used.

> 4)	Should I add in the qcms_profile struct  A2B1, A2B2, B2A1, B2A2 variables
> or use the A2B0 B2A0 to store the tables although they don't correspond to
> that tables? If I'm correct it's only necessary a pair. (for example, store
> in the A2B0 the A2B2 table).
> 

I would store them all.

> 5)	In transform.c I don't know what I have to do exactly, if something is to
> be done. The only reference to tables that I found is for not use precache.

If we read all the tables then we will need to select the correct table in transform.c.

Comment 10

8 years ago
I have done a lot of modifications in the ICCread.c, basically change how to access the tags, because I found it very inefficient if all the tags of the standard must be checked. 
Before, it created a tag list, and with a search function was checking the signatures and when the tag was found the data was added in the profile. Now it reads a tag and puts the data in the profile.

I don't have any problem with the code, but I paste it so you can take a look if you want. (It's not completed yet)
qcmsint.h: http://pastebin.com/peSEGYWh (only new variables in the profile)
ICCread.c: http://pastebin.com/4gvpzV6r (I have to quit the find_tag function. It's not used anymore)

My problem is when it’s time to test what I have done.
I'm pretty sure that what I have works well, but I have to check it manually with a debugger. There is a test suite or something that performs an automatic test for this part of code?

I found this web but I don't know if any of this test check the ICC parsing:
https://developer.mozilla.org/en/Mozilla_automated_testing

If no one of these covers it, what I should read to make my own tests?
Can you post a patch so that I can easily see what changes you've done. If you've made you changes under mozilla-central/gfx you should be able to use |hg diff . > file.patch| and attach that to the bug.

If you're working on changes unrelated to these issues please file a separate bug explaining what you think the issue is.

We don't have a regression test suite icc profile but we have considered adding one. I have some test that I run offline but I don't have a package to provide for that at this time.

Comment 13

8 years ago
Uploaded patch. I only modified files in gfx/qcms.

The changes are related to the bug. I needed to read more tags for the ICC-absolute intent, and I don't know if more are needed. Because of these I though I needed a better way to add new tags in the parsing, instead the one that was present (and also will come in handy to make the standard full compliant to read all tags in another bug).

Comment 14

8 years ago
Posted patch rendering intent for V4 (obsolete) — Splinter Review
New patch and new problems. This patch allows to use the rendering intents, but only works if gfx.color_management.enablev4 is true. Also absolute and media-color are the same.I have to figure out how to apply the whitepointTag in the transforms to achieve the absolute rendering. Any Idea on this?

The renderings are "working" in images but not in the browser.
Example: 
Use the profile attached to the bug and put mode 1 in color management, 0 to rendering intent and enable v4.
Visit this website: http://www.tomasjonsson.eu/colortests/colortest-xhtml.html
(The first 8 colors should be the same).
Try any other rendering, the colors will not be the same for all.
I don't have any idea why this happens. I thought that the system was made for all, images and browser. Again, any idea?
(Reporter)

Comment 15

8 years ago
(In reply to JordiM from comment #14)
> Created attachment 556691 [details] [diff] [review]
> rendering intent for V4
> 
> New patch and new problems. This patch allows to use the rendering intents,
> but only works if gfx.color_management.enablev4 is true. Also absolute and
> media-color are the same.I have to figure out how to apply the whitepointTag
> in the transforms to achieve the absolute rendering. Any Idea on this?

The white point transformation should only be done for relative colorimetric - absolute should leave the white point unchanged.

> The renderings are "working" in images but not in the browser.
> Example: 
> Use the profile attached to the bug and put mode 1 in color management, 0 to
> rendering intent and enable v4.
> Visit this website:
> http://www.tomasjonsson.eu/colortests/colortest-xhtml.html
> (The first 8 colors should be the same).
> Try any other rendering, the colors will not be the same for all.
> I don't have any idea why this happens. I thought that the system was made
> for all, images and browser. Again, any idea?

They should look different in any rendering intent other than “absolute colorimetric”, because perceptual / relative modes apply transformations that are dependent on the source gamut as well. Only in absolute colorimetric mode should they truly be the same.
(In reply to JordiM from comment #14)
> Created attachment 556691 [details] [diff] [review]
> rendering intent for V4
> 
> New patch and new problems. This patch allows to use the rendering intents,
> but only works if gfx.color_management.enablev4 is true. 
We're keeping the new changes behind this preference because further testing and security review is required before enabling this feature.

> Also absolute and
> media-color are the same.I have to figure out how to apply the whitepointTag
> in the transforms to achieve the absolute rendering. Any Idea on this?
> 
If I remember correctly the whitepoint is adjusted using a chromatic adaption matrix. I wrote the code but it didn't appear to improve the results. The transformation happens here:
http://mxr.mozilla.org/mozilla-central/source/gfx/qcms/chain.c#916

> The renderings are "working" in images but not in the browser.

That's expected, we only apply color management for image decoding. Certain OS like OS X will apply color management to other areas automatically using their color management system.

Comment 17

8 years ago
I think that I'm right about the use of white point to get the absolute intent.
In section 9.2.25 says that the mediaWhiteTag is used to generate the ICC-absolute.
In 6.3.2 there are formulas to obtain ICC-absolute with media-relative and media white point.

BenWa, I think your code it has nothing to do with the intents. The chromatic adapt tag it's only necessary to adjust illumination to D50 if the illumination is another one and used always (if the tag is present, obviously) regardless of the intent.

I'm working to get V2 and absolute intent working.
Thanks for the answers.
(In reply to JordiM from comment #17)
> I think that I'm right about the use of white point to get the absolute
> intent.
> In section 9.2.25 says that the mediaWhiteTag is used to generate the
> ICC-absolute.
> In 6.3.2 there are formulas to obtain ICC-absolute with media-relative and
> media white point.
> 
> BenWa, I think your code it has nothing to do with the intents. The
> chromatic adapt tag it's only necessary to adjust illumination to D50 if the
> illumination is another one and used always (if the tag is present,
> obviously) regardless of the intent.
> 

That's right, it is for the illumination.

JordiM, Jeff and I are happy with the patches you're working on. Your patch is already 100 KB and does refactoring that is outside the description of this bug. qcms changes also have the burden of requiring throughout review because of security concerns. It would make the review process significantly faster if you can split up the changes into smaller patch such as the refactoring to the tag parsing and changes to support intent and any other logical changes. I had made the same mistake when adding my changes to support CLUTs and the review took very long. Changes that don't relate directly to color intents can be handled in a separate bug that can be flagged as |blocking| this bug if the change is required for color intent (or the patch is based upon it). 

Let me know if you need any help with splitting up the patches. Mercurial queue is a great tool for this but how you do it is up to personal preference:
http://mercurial.selenic.com/wiki/MqExtension
http://mercurial.selenic.com/wiki/MqTutorial

Updated

8 years ago
Depends on: 685206

Comment 19

8 years ago
Sorry for so long time without any response.
I tried to do the part of the absolute intent and I had some troubles making the patch.
Finally I couldn't get the absolute intent working as it's expected so I decided to make the patch without it (media-relative and absolute intent return the same image in this patch).
I made a new bug for the tag parsing (blocking this) because it reads tags necessary for the intents and the media white point for future improvement.
The patch added in this bug works only if V4 is active. I also tried to do for V2 but I can't :/
Attachment #558866 - Flags: review?(bgirard)
Comment on attachment 558866 [details] [diff] [review]
Rendering intent for V4 whiteout ICCread modifications

Review of attachment 558866 [details] [diff] [review]:
-----------------------------------------------------------------

Took a quick look. This patch looks great. We will need to fix the spacing (see my other review comment for a suggestion on how to fix this).

::: gfx/qcms/chain.c
@@ +747,5 @@
> +            }
> +            break;
> +    }
> +    
> +    if (in->A2B0) {

Since this isn't in its own case we could end up chaining the transform of the above A2B1/2 and A2B0. Is this intended? If not this should be moved up into its own case.

We should also define the default case as well as QCMS_INTENT_DEFAULT/QCMS_INTENT_PERCEPTUAL to be explicit about what should happen in these cases.

@@ +848,5 @@
> +            }
> +            break;
> +    }
> +    
> +    if (out->B2A0) {

Likewise
Attachment #558866 - Flags: review?(bgirard)

Comment 21

8 years ago
I know it's a little bit confusing but the code it's intended to be like is.
First, it's not possible to chain A2B1/2 with A2B0 because all transforms have their own return. 
The only way to reach A2B0 is by having a perceptual rendering intent or because the selected rendering intent is not available in the ICC profile.
About the perceptual intent outside the switch/case statement we can discuss.
I put a comment about this “//Perceptual intent is after the switch in order to preserve tag priority usage described in ICC.1:2004-10 clause 8.10”
If I putted the perceptual intent inside the switch, then I ought to duplicate the perceptual code after to comply with the standard. 
I decided that I didn't want repeated code, so I put it outside and didn't use the default case in order that the perceptual intent in the switch does nothing. The result is that if the rendering intent is 0 (perceptual), the switch does nothing and goes to A2B0 (perceptual intent). If the rendering intent is not correct (default case) also goes to A2B0, which I think it should be the thing to do if the rendering is incorrect.
If you want I can put the cases explicit, but we will have the A2B0 code three times. I think this could be a little bit confusing.
(In reply to JordiM from comment #21)

You're absolutely right. I skipped over the return statements. I would suggest adding:
case (QCMS_INTENT_DEFAULT):
  break;

just so that it's clear that the intent is meant to fall through. Your reference to 8.10 is correct. Style nit for comment, add a space after '//'.

This patch looks great!

Comment 23

8 years ago
Added the default case and changed the spaces to tabs.
Attachment #555059 - Attachment is obsolete: true
Attachment #556691 - Attachment is obsolete: true
Attachment #558866 - Attachment is obsolete: true
Attachment #562775 - Flags: review?(bgirard)
Whiteboard: [good first bug][mentor=benwa]
Attachment #562775 - Flags: review?(bgirard)
You need to log in before you can comment on or make changes to this bug.