Closed Bug 1464257 Opened Last year Closed Last year

qcms: lut8Type implemented incorrectly

Categories

(Core :: GFX: Color Management, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: pdknsk, Assigned: pdknsk)

Details

(Whiteboard: gfx-noted)

Attachments

(1 file, 4 obsolete files)

Attached patch lut8Type (obsolete) — Splinter Review
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.
There is still a minor bug in that incorrect values are read from the table. I'll attach a new patch shortly.
Attached patch lut8Type (obsolete) — Splinter Review
Attachment #8980443 - Attachment is obsolete: true
Component: Untriaged → GFX: Color Management
Product: Firefox → Core
Attachment #8980451 - Flags: review?(jmuizelaar)
Whiteboard: gfx-noted
Comment on attachment 8980451 [details] [diff] [review]
lut8Type

(Adding Bas as potential reviewer in case Jeff is unavailable for a while)
Attachment #8980451 - Flags: review?(bas)
Comment on attachment 8980451 [details] [diff] [review]
lut8Type

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

Seems good to me.
Attachment #8980451 - Flags: review?(bas) → review+
Attachment #8980451 - Flags: review?(jmuizelaar)
Assignee: nobody → pdknsk
Keywords: checkin-needed
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
Flags: needinfo?(pdknsk)
Attached patch lut8Type (obsolete) — Splinter Review
Attachment #8980451 - Attachment is obsolete: true
Flags: needinfo?(pdknsk)
Attached patch lut8Type-new (obsolete) — Splinter Review
I've reworked the patch a bit for more clarity.
Attachment #8982108 - Attachment is obsolete: true
Attachment #8984457 - Flags: review?(bas)
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?
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 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.)
Attachment #8984457 - Flags: review?(bas) → review+
Attachment #8984457 - Flags: checkin?
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).
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(pdknsk)
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.
Flags: needinfo?(pdknsk) → needinfo?(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).
Flags: needinfo?(ryanvm)
This should work now, I think.
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.)
Attachment #8987713 - Flags: checkin?(ryanvm)
Attachment #8984457 - Attachment is obsolete: true
Comment on attachment 8987713 [details] [diff] [review]
fix lut8Type tag implementation

That should do it ;)
Attachment #8987713 - Flags: checkin?(ryanvm)
https://hg.mozilla.org/mozilla-central/rev/cfa6f82070a0
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.