1729071 - AVIF with multiple ColourInformationBox (colr) entries per item are rejected

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[Ksenia Berezina [:ksenia]]

To reproduce visit https://www.theatlantic.com/ in Firefox Nightly 93.0a1 (2021-09-03) desktop and observe the page.

Expected:
All images load

Actual:
Some images don't load

The link to one of the images:
https://cdn.theatlantic.com/thumbor/x-01kU3VRY0zEK432q1-SL-V6C8=/53x0:1820x994/543x305/media/img/mt/2021/09/Atlantic_immun_v1/original.jpg
(the image actually ends up being jpg, not avif format, when I save it to my file system).

Also setting image.avif.compliance_strictness to 0 doesn't make it work, so it seems to be a different issue from those already reported.

Comment 1

[Ksenia Berezina [:ksenia]]

Hi Jon, could you please take a look?

Comment 2

[Jon Bauman [:jbauman:]]

This is indeed an issue we've never seen before. I'm currently discussing with the author of code that wrote this file to get clarity on whether it's invalid or a bug in our parser.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

We can't ship AVIF in 92 with this bug. Escalating accordingly.

Comment 4

[Jon Bauman [:jbauman:]]

Per the published standard, this file is invalid because it provides multiple ColourInformationBox (colr) entries (one ICC profile, and one NCLX) where the HEIF specification ISO/IEC 23008-12:2017 § 6.5.5.1 requires

Quantity (per item): At most one

After inquiring with the editor, I was able to obtain an unpublished revision (registered as ISO/IEC DIS 23008-12, but not publicly available, even for purchase) which changes that to

Quantity (per item): At most one for a given value of colour_type

This is what libavif (the writer responsible for these images) is based on, and despite regular testing against libavif's output, I must not have done a comprehensive retest subsequent to implementing the colr box parsing in mp4parse rust. I find it very odd that we haven't run into this previously since the change landed over a month ago in bug 1723247, but here we are.

I'm currently discussing options with the release manager for 92, but I think it's likely we will disable AVIF by default for now and not ship in 92. See 1682995 for details.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

AVIF support has been disabled for 92.0rc3.
https://hg.mozilla.org/releases/mozilla-release/rev/74a9748f90655e731a34c298948869e67ceaa102

Comment 6

[Abbe]

Following avif URL fails to render in Firefox 92.0b9 (GNU/Linux):

https://d1g3mdmxf8zbo9.cloudfront.net/images/i3/[e-mail]

It's linked from https://vincent.bernat.ch/en/blog/2021-i3-window-manager

Following is the cURL to fetch the AVIf version:

curl 'https://d1g3mdmxf8zbo9.cloudfront.net/images/i3/[e-mail]' -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:92.0) Gecko/20100101 Firefox/92.0' -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8' -H 'Accept-Language: en-US,en;q=0.5' --compressed -H 'DNT: 1' -H 'Connection: keep-alive' -H 'Referer: https://vincent.bernat.ch/' -H 'Upgrade-Insecure-Requests: 1' -H 'Sec-Fetch-Dest: document' -H 'Sec-Fetch-Mode: navigate' -H 'Sec-Fetch-Site: cross-site' -H 'Sec-Fetch-User: ?1' -H 'Sec-GPC: 1' -H 'Pragma: no-cache' -H 'Cache-Control: no-cache'

Thanks!

Comment 7

[Mira Lobontiu (Android QA)]

Verified on the latest Focus RC 92.1.1 with Lenovo Tab M10 (Android 10).
The images load without issues.

Comment 8

[Daniel Veditz [:dveditz]]

After inquiring with the editor, I was able to obtain an unpublished draft amendment (registered as DAmd 2, but not publicly available,

That spec is marked "Deleted" -- does that mean the change was rejected?

Comment 9

[biobunny.exe]

Hi there, I ran into this issue during some web dev, and I'd like to offer my two cents.

For a long time, it seemed like avifenc from libavif, somehow adds two colr entries if the source image has an existing color profile. Running it on a file without a color profile seemed to produce files that Firefox Nightly (back then, being 73), rendered fine.

I can't seem to reproduce this issue anymore with newly converted images today on avifenc with a JPEG source file + ICC profile. So they may have resolved that particular issue since then. Of course, this doesn't do much to help existing images in the wild. I can send some image samples, if it would help out.

Here are the relevant system details for reference:

Libavif
Version: 0.9.2 (aom [enc/dec]:3.1.2, rav1e [enc]:0.4.1 (v0.4.1))
libyuv : unavailable

macOS 11.5.2, Firefox Nightly 94.0a1

But a more important point worth mentioning is that these invalid images with multiple colr entries seem to pass the AVIF image validator at https://gpac.github.io/ComplianceWarden-wasm/avif.html just fine, making debugging this issue a bit of a pain for web developers on Firefox.

Comment 10

[Jon Bauman [:jbauman:]]

(In reply to Daniel Veditz [:dveditz] from comment #8)

After inquiring with the editor, I was able to obtain an unpublished draft amendment (registered as DAmd 2, but not publicly available,

That spec is marked "Deleted" -- does that mean the change was rejected?

No, apparently the change to the colr box will be in https://www.iso.org/standard/83650.html, so I was mistaken with my previous link

Comment 11

[Vincent Bernat]

This single pixel AVIF does not work either:

echo AAAAIGZ0eXBhdmlmAAAAAGF2aWZtaWYxbWlhZk1BMUIAAAEcbWV0YQAAAAAAAABIaGRscgAAAAAAAAAAcGljdAAAAAAAAAAAAAAAAGNhdmlmIC0gaHR0cHM6Ly9naXRodWIuY29tL2xpbmstdS9jYXZpZgAAAAAeaWxvYwAAAAAEQAABAAEAAAAAAUQAAQAAABcAAAAqaWluZgEAAAAAAAABAAAAGmluZmUCAAAAAAEAAGF2MDFJbWFnZQAAAAAOcGl0bQAAAAAAAQAAAHJpcHJwAAAAUmlwY28AAAAQcGFzcAAAAAEAAAABAAAAFGlzcGUAAAAAAAAAAQAAAAEAAAAQcGl4aQAAAAADCAgIAAAAFmF2MUOBAAwACggYAAYICGgIIAAAABhpcG1hAAAAAAAAAAEAAQUBAoMDhAAAAB9tZGF0CggYAAYICGgIIBoFHiAAAEQiBACwDoA= | base64 -d > test1.avif

Sep 10 08:51:03 neo i3[828073]: [2021-09-10T06:51:03Z ERROR mp4parse] Multiple values for pixi: [Channels(PixelInformation { bits_per_channel: [8, 8, 8] }), Channels(PixelInformation { bits_per_channel: [8, 8, 8] })]

Comment 12

[Pascal Chevrel:pascalc]

This is P1/S1 and blocking all our upcoming releases, this needs an assignee, Andrew can you help?

Comment 13

[Jon Bauman [:jbauman:]]

This should be fixed shortly. I'm just waiting for review on https://github.com/mozilla/mp4parse-rust/pull/338

I'll be requesting uplift as soon as this is done

Comment 14

[Jon Bauman [:jbauman:]]

Attachment: data_review-bug-1696045.md

This just adds a "both" value to the existing AVIF_COLR histogram reviewed in bug 1696045 comment 4

Comment 15

[Chris H-C :chutten|PTO (back Oct 23)]

Comment on attachment 9241417 [details]
data_review-bug-1696045.md

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, Chun-Min Chang ([e-mail]) and Jon Bauman ([e-mail]) are responsible.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

Is the data collection request for default-on or default-off?

Default on for all channels.
Default on for pre-release channels only.

Does the instrumentation include the addition of any new identifiers?

No.

Is the data collection covered by the existing Firefox privacy notice?

Yes.

Does the data collection use a third-party collection tool?

No.

---

Result: datareview+

Comment 16

[Jon Bauman [:jbauman:]]

Attachment: Bug 1729071 - AVIF with multiple ColourInformationBox (colr) entries per item are rejected. r=aosmond

The logic to allow parsing of the multiple colr boxes is in the parent
update of mp4parse-rust to 72eb355. This change adds a confirmatory test in
gecko and updates telemetry to add new new "both" case for colr tracking.

Depends on D125627

Comment 17

[Lars Sonchocky-Helldorf]

Attachment: IMG_IPHONE12PRO_0_6761.avif

Here is a file which is affected by this bug. Uploading it here for verifying and testing purposes.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ceb97a8e80f6
AVIF with multiple ColourInformationBox (colr) entries per item are rejected. r=aosmond

Comment 20

[Cristina Cozmuta (:CrissCozmuta)]

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

Comment 21

[Pascal Chevrel:pascalc]

This bug was the blocker to ship AVIF in 92, its fix should be uplifted to 93 if we want to ship in that version, can you request the uplift please? Thanks

Comment 22

[Jon Bauman [:jbauman:]]

Comment on attachment 9241427 [details]
Bug 1729071 - AVIF with multiple ColourInformationBox (colr) entries per item are rejected. r=aosmond

Beta/Release Uplift Approval Request

• User impact if declined: A significant number of technically invalid (but generated by the reference implementation) AVIF will be rejected by Firefox
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1730784
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a fairly minor change, mostly just removing the check that rejects multiple colr box instances (which are invalid in the current published specification)
• String changes made/needed:

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9241427 [details]
Bug 1729071 - AVIF with multiple ColourInformationBox (colr) entries per item are rejected. r=aosmond

Approved for 93.0b7.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9a0f5ec33f5d

Comment 25

[Ciprian Georgiu, Desktop QA]

Reproduced the issue with STR from comment 0, on an affected Nightly build.

The issue is verified as fixed on latest Beta 93.0b7 and Nightly 94.0a1 under macOS 11, Ubuntu 18.04 x64 and Win 10 x64.