685206 - Read new tags of ICC profiles and improve performance of current reading

Status: NEW

(Core :: Graphics: Color Management, enhancement)

Description

[JordiM]

To use the rendering intents for ICC profiles it's necessary to read new tags that are not read in the current stage.

These tags are A2B1 A2B2 B2A1 B2A2, and for absolute-colorimetry intent, mediawhitepoint.

Also, the way that the tags are read seems that does some extra checks that would not be necessary, so I think it could be possible to improve performance.

Comment 1

[JordiM]

Attachment: Reads new tags and tries to improve performance

Patch to review.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 558860 [details] [diff] [review]
Reads new tags and tries to improve performance

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

Overall this patch looks good. With a few changes I think we should take it.

I know the spacing situation in qcms isn't perfect but I'd like to prevent it from getting worse. We should be using tabs in these files instead of spaces even if it's not the proper mozilla style. See 'noexpandtab' in the header.

::: gfx/qcms/iccread.c
@@ +332,3 @@
>  	struct matrix matrix;
> +    uint8_t i;
> +    uint32_t offset = read_u32(src, 128 + 4 + 4*index*3 + 4);

We have this pattern |128 + 4 + 4*index*3 + 4| repeated in a lot of places. I think we should refactor this to either a helper function or macro. |index| should be named to something more descriptive such as |tag_index|.

@@ -1067,5 @@
> -				profile->greenTRC = read_tag_curveType(src, index, TAG_gTRC);
> -				profile->blueTRC = read_tag_curveType(src, index, TAG_bTRC);
> -
> -				if (!profile->redTRC || !profile->blueTRC || !profile->greenTRC)
> -					goto invalid_tag_table;

Your patching is missing some of the validation that was performed here. For example a RGB color space profile should have a rgb TRC when not running in iccv4 mode.

@@ -1073,5 @@
> -		} else if (profile->color_space == GRAY_SIGNATURE) {
> -
> -			profile->grayTRC = read_tag_curveType(src, index, TAG_kTRC);
> -			if (!profile->grayTRC)
> -				goto invalid_tag_table;

Like wise here. Can we confirm that these are safely checked elsewhere?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

What's the performance difference this makes?

Comment 4

[JordiM]

Attachment: Modifications previous patch

Changes done.

Comment 5

[JordiM]

Sorry Jeff, I didn't saw your comment yesterday.
The performance improvement it's not expected to be great but there is.
In the original code we have this:

1027 	index = read_tag_table(profile, src);

This returns the tag table that is in scr, so we are duplicating some part of the memory.

After the tag table duplicate we have this kind of code:
1040 	if (find_tag(index, TAG_A2B0)) {
1041 		if (read_u32(src, find_tag(index, TAG_A2B0)->offset) == LUT8_TYPE ||
1042 		    read_u32(src, find_tag(index, TAG_A2B0)->offset) == LUT16_TYPE) {
1043 			profile->A2B0 = read_tag_lutType(src, index, TAG_A2B0);

The find_tag function is looking for tag signatures in the previous array "index", from start, until it reaches the tag if it exist. In this code we see that makes the search of the same tag 3 times in the if statements and one more in read_tag_lutType() which is very redundant, if we enter the function it's because the tag is present. The worst case scenario is the tag exist but is in the last position. This is reading the entire array 4 times. If we have in mind that we want to read all the profile makes no much sense a search function, it fits more a code that reads and process tags one by one.

Thinking in this, I eliminated the memory duplicate and replaced the find function to a switch/case statement. It allows to only access only one time in the existing tags.

So, the improvement in run time would be, use less memory, and less memory accesses.
I know that this would not be great improvement in this case. The best improvement that I see is that the code it's clearer and easier to add new tags to be read (less if statements to check if the tag is present).

Comment 6

[Benoit Girard (:BenWa)]

(In reply to JordiM from comment #5)
> So, the improvement in run time would be, use less memory, and less memory
> accesses.
> I know that this would not be great improvement in this case. The best
> improvement that I see is that the code it's clearer and easier to add new
> tags to be read (less if statements to check if the tag is present).

First I want to apologize for taking so long on the review. I was away for travel.
 
This patch does reduce the searching we do for tags which is describable, but we may read unnecessary tags (rgb tags for gray signature) although this isn't strictly an objection. I don't see any memory space reduction?

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 558969 [details] [diff] [review]
Modifications previous patch

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

We still need to convert the spaces to tabs. Best way I know of is saving the patch, unapply it, back it up, run a regex for a line starting in '<Start of line>+<space>' and replacing by '+<tab>', reapplying the patch. This wont add additional lines to your patch. Let me know if you want a hand with this.

Once we get a final version of the patch, and I ran some test, we will put it into the qcms repo and then merge that with the mozilla code base pending Jeff's approval.

::: gfx/qcms/iccread.c
@@ +963,5 @@
> +                switch(read_u32(mem, 128 + 4 + 12*i)){
> +                
> +                    case(TAG_bXYZ):
> +                        profile->blueColorant = read_tag_XYZType(mem, i);
> +						colorantIsPresent[0]=true;

Style nit:
colorantIsPresent[0..2] = true;

@@ -986,0 +943,141 @@
> > +bool read_tag_table(qcms_profile *profile, struct mem_source *mem)
> > +{
> > +    uint32_t tagCount,offset;
> > +    unsigned int i;
NaN more ...

What's colorantIsPresent for? Isn't the TRC null check sufficient?

Need to check for RGB_SIGNATURE.

The format here should be '<tab>if (...)'

Comment 8

[Benoit Girard (:BenWa)]

(In reply to Benoit Girard (:BenWa) from comment #6)
> describable
s/describable/desirable

Comment 9

[JordiM]

The possibility to read RGB tags in a gray signature it's only possible in malformed ICC profiles. The standard describes the gray profiles to only contain common tags and the grayTRCtag. In the case of RGB signatures, they don't have to have grayTRCtag. See clause 8 in ICC.1:2004-10.
The memory reduction is minimal and only inside the function qcms_profile_from_memory.
It's the result of not copying the tag table from src to the index variable in 1027 in the original code.
http://mxr.mozilla.org/mozilla-central/source/gfx/qcms/iccread.c#1027

About the colorant variable I think you are right, but I'm going to check it out again tomorrow to be sure it makes no sense. I'm sleepy and it's possible I miss something.
The RGB_Signature it's checked, but not in an explicit way.
::: gfx/qcms/iccread.c
@@ +960 @@
>if (profile->color_space == RGB_SIGNATURE || profile->color_space == GRAY_SIGNATURE) {
@@ +1085 @@
if (profile->color_space == GRAY_SIGNATURE && !profile->grayTRC)

In the first if, we have that the only accepted signatures are gray and RGB. This last if, checks the signature. If it's gray, we know the signature is gray, if not, it can only be RGB. Another one will not pass the first if. So, we have indirectly checked RGB.
I don't know if you want another type of check.

Comment 10

[JordiM]

Rechecked the colorant variable.
I put it to make sure that all the colorants are present. In the original code when a tag is found it reads the three tags, in the new code it's possible that a malformed profile contains only one or two colorants.

Comment 11

[JordiM]

Attachment: No spaces version

Changed spaces to tabs. I tried to follow the stile nit for colorant, but it doesn't compile so I didn't do it. In this patch the colorant initialization is equal as the previous patch.

Comment 12

[Benoit Girard (:BenWa)]

Hi JordiM. I sent you an email last week asking if you were available for a short conference call with Jeff and I to discuss how we should handling parsing of the new tags.

Comment 13

[JordiM]

Sorry. Email answered.

Comment 14

[JordiM]

Hi guys.
I'm sorry but I think that the best I could do is leave the firefox developing. I thought that I would have enough time to spend but as you can see by the long time without any patch I was wrong.
I hope that someone will get this patch and that in the future I could contribute more than I had done this time.

Regards,
Jordi.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:aosmond, could you have a look please?
For more information, please visit auto_nag documentation.