Closed Bug 1494222 Opened Last year Closed 11 months ago

minor qcms bugs


(Core :: GFX: Color Management, enhancement, minor)

Not set



Tracking Status
firefox64 --- fixed


(Reporter: pdknsk, Assigned: pdknsk)



(5 files, 1 obsolete file)

751 bytes, patch
: review+
Details | Diff | Splinter Review
899 bytes, patch
: review+
Details | Diff | Splinter Review
826 bytes, patch
: review+
Details | Diff | Splinter Review
733 bytes, patch
: review+
Details | Diff | Splinter Review
845 bytes, patch
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch qcms-1Splinter Review
qcms_profile_is_bogus doesn't check bogosity of profiles with a LUT.

It does miss two other types of LUT-based profiles, which have the same A2B0 or B2A0 tags, but with a different LUT type.

So any such profile is considered bogus, unless it also contains redundant {b,g,r}XYZ tags, because otherwise the checks further down in qcms_profile_is_bogus always fail. The fuzzer can produce those profiles, but it also affects Firefox when mBA-only profiles are set as the monitor profile, which should be very rare.
Assignee: nobody → pdknsk
Attached patch qcms-2Splinter Review
One of the profiles used in the target is the built-in sRGB profile, which incorrectly doesn't set mandatory field PCS, so the following code can never be reached.

Note this doesn't only affect the fuzzer, but also Firefox itself in rare situations: when the input profile has uncommon PCS LAB, and no monitor profile is set (which makes qcms fallback to the built-in sRGB profile).
Attached patch fuzzer-1 (obsolete) — Splinter Review
I used conditions size & 1 and size % 0x10, which excludes half the combinations.
I did a review of the oss-fuzz coverage for target Qcms and noticed some code wasn't reached that should be. That's caused by a minor bug in the target itself, and two minor bugs in qcms.

Those 3 patches are very simple. You you can probably review them yourself as the owner of qcms on the oss-fuzz side now. Otherwise please suggest a reviewer. Thanks.
Flags: needinfo?(agaynor)
I believe :nical has been handling qcms bugs.
Flags: needinfo?(agaynor) → needinfo?(nical.bugzilla)
Component: General → GFX: Color Management
Product: Firefox → Core
Flags: needinfo?(nical.bugzilla)
Attachment #9012086 - Flags: review?(jmuizelaar)
Attachment #9012089 - Flags: review?(jmuizelaar)
Attachment #9012090 - Flags: review?(jmuizelaar)
Comment on attachment 9012090 [details] [diff] [review]

I changed the reviewer of the patch that changes the target (as opposed to qcms) to you. It's so trivial it might not even need a review.
Attachment #9012090 - Flags: review?(jmuizelaar) → review?(agaynor)
Is there anything more to this patch than "take this branch 14/15 times instead of 15/16" so that it gets covered better?
Yes. It's because I use size & 1 elsewhere, which makes that combination static.

>>> 16 & 1
>>> 16 % 16
>>> 32 & 1
>>> 32 % 16

With an odd number, it alternates between odd and even.

>>> 15 & 1
>>> 15 % 15
>>> 30 & 1
>>> 30 % 15
Ah great, can you expand the commit message to include this detail?
Is that sufficient or do you want a more detailed explanation in the commit comment?

Bug 1494222: increase qcms_fuzzer coverage by enabling more path combinations
Or actually, this is more accurate.

Bug 1494222: fix qcms_fuzzer coverage by enabling all path combinations
Attached patch fuzzer-1+Splinter Review
Attachment #9012771 - Flags: review?(agaynor)
Attachment #9012090 - Attachment is obsolete: true
Attachment #9012090 - Flags: review?(agaynor)
Attached patch profile-1Splinter Review
Adds another profile to cover a specific path the fuzzer hasn't reached yet.
Attachment #9012772 - Flags: review?(agaynor)
With the pending changes, qcms has full coverage. The only blocks not covered are unused utility functions, plus non-SSE and SSE1 paths, which are chosen at runtime based on CPU capabilities. I don't think Firefox runs on any CPU now that supports SSE1 but not SSE2. Even if, those paths are almost identical. Non-SSE runs on ARM, but qcms is disabled on Mobile Firefox.
Attachment #9012771 - Flags: review?(agaynor) → review+
Attachment #9012772 - Flags: review?(agaynor) → review+
It appears the reviewer is busy. I can also explain those simple patches in more detail to you or someone else.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 9012086 [details] [diff] [review]

Review of attachment 9012086 [details] [diff] [review]:

Apologies for the review lag.
Attachment #9012086 - Flags: review?(jmuizelaar) → review+
Attachment #9012089 - Flags: review?(jmuizelaar) → review+
QA Contact: dbolter
Keywords: checkin-needed
Pushed by
don't check bogosity of profiles with LUT-types mAB/mBA. r=nical
set XYZ PCS on internal RGB profiles. r=nical
fix qcms_fuzzer coverage by enabling all path combinations. r=agaynor
add combined mAB/mBA profile to qcms_fuzzer samples. r=agaynor
Keywords: checkin-needed
I forgot a patch.
Resolution: FIXED → ---
Attached patch qcms-3Splinter Review
Two functions have blocks which cannot be reached right now, because they depend on source pixel values. Like R>G>B and other combinations. Adds more bytes to cover those.
Attachment #9015164 - Flags: review?(agaynor)
Attachment #9015164 - Flags: review?(agaynor) → review+
qcms-3 only. Thanks.
Keywords: checkin-needed
Pushed by
Add source bytes to fix qcms_fuzzer coverage. r=agaynor
Keywords: checkin-needed
Closed: 11 months ago11 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.