1464257 - qcms: lut8Type implemented incorrectly

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[u473386]

Attachment: lut8Type

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.139 Safari/537.36

Steps to reproduce:

I found this when I wondered why a perfectly valid profile in the qcms oss-fuzz corpus didn't reach the code it should and is considered invalid.

There are two very similar types (which are handled by the same code): lut8type and lut16type.

Please refer to 10.8 (page 49) and 10.9 (page 52) of the ICC specification.

http://www.color.org/specification/ICC1v43_2010-12.pdf

You'll notice that the tables for lut16Type begin at offset 52, but at offset 48 for lut8Type.

I've attached a patch.

Comment 1

[u473386]

There is still a minor bug in that incorrect values are read from the table. I'll attach a new patch shortly.

Comment 2

[u473386]

Attachment: lut8Type

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 8980451 [details] [diff] [review]
lut8Type

(Adding Bas as potential reviewer in case Jeff is unavailable for a while)

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8980451 [details] [diff] [review]
lut8Type

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

Seems good to me.

Comment 5

[Dorel Luca [:dluca]]

Hello,

I am unable to apply your patch in order to land it.

Please check it out,

This is the error given when trying to apply:

patching file gfx/qcms/iccread.c
Hunk #1 FAILED at 697
Hunk #2 FAILED at 765
2 out of 2 hunks FAILED -- saving rejects to file gfx/qcms/iccread.c.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh lut8Type.diff

Comment 6

[u473386]

Attachment: lut8Type

Comment 7

[u473386]

Attachment: lut8Type-new

I've reworked the patch a bit for more clarity.

Comment 8

[Bas Schouten (:bas.schouten)]

Comment on attachment 8984457 [details] [diff] [review]
lut8Type-new

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

::: gfx/qcms/iccread.c
@@ -695,5 @@
>     size_t entry_size;
>     struct lutType *lut;
>     uint32_t i;
>  
> -   /* I'm not sure why the spec specifies a fixed number of entries for LUT8 tables even though

Is this comment somehow no longer valid?

Comment 9

[u473386]

Yes, because the author apparently misread the LUT8 specs. LUT8 (unlike LUT16) doesn't have fields for the number of tables entries, because the number is fixed to 256. That's also why the offset to the tables differs by 4 bytes between LUT8 and LUT16. (You can verify this on the pages I mentioned in my first post.)

Comment 10

[u473386]

Comment on attachment 8984457 [details] [diff] [review]
lut8Type-new

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

::: gfx/qcms/iccread.c
@@ -695,5 @@
>     size_t entry_size;
>     struct lutType *lut;
>     uint32_t i;
>  
> -   /* I'm not sure why the spec specifies a fixed number of entries for LUT8 tables even though

Yes, because the author apparently misread the LUT8 specs. LUT8 (unlike LUT16) doesn't have fields for the number of tables entries, because the number is fixed to 256. That's also why the offset to the tables differs by 4 bytes between LUT8 and LUT16. (You can verify this on the pages I mentioned in my first post.)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please also make sure that your updated patch includes proper commit information in it (name/email/descriptive commit message). You can then use the checkin-needed bug keyword when the patch is ready to land (works better with the automated bug marking tools).

Comment 12

[u473386]

I have a question. Can I still switch this patch to MozReview? (I have not tried it yet.) I have 1-2 additional patches, so might as well try it.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

You can, but it'll need to get reviewed again in MozReview before it can land (the Autoland system hooked into it has review requirements before it'll land the patch).

Comment 14

[u473386]

Attachment: fix lut8Type tag implementation

This should work now, I think.

Comment 15

[u473386]

Comment on attachment 8987713 [details] [diff] [review]
fix lut8Type tag implementation

I can't add checkin-needed as suggested, so checkin must do. (The patch is unchanged from the reviewed patch, other than using bzexport now.)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8987713 [details] [diff] [review]
fix lut8Type tag implementation

That should do it ;)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa6f82070a0
fix lut8Type tag implementation. r=bas

Comment 18

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/cfa6f82070a0