1707590 - Initial implementation for JXL

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, task)

Comment 1

[Kagami [:saschanaz] (they/them)]

Attachment: Bug 1707590 - Part 1: Add vendored libjxl source r=aosmond

Comment 2

[Kagami [:saschanaz] (they/them)]

Attachment: Bug 1707590 - Part 2: Implement nsJXLDecoder r=tnikkel

Depends on D113358

Comment 3

[Kagami [:saschanaz] (they/them)]

Attachment: Bug 1707590 - Part 3: Add feature gate for JXL r=tnikkel

Depends on D113359

Comment 4

[Kagami [:saschanaz] (they/them)]

Attachment: Bug 1707590 - Part 4: Add image/jxl to Accept header and DownloadsViewableInternally r=tnikkel

Depends on D113360

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01bb0e75cb82
Part 1: Add vendored libjxl source r=aosmond
https://hg.mozilla.org/integration/autoland/rev/40a3f43a2306
Part 2: Implement nsJXLDecoder r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/3e76ff83ebe2
Part 3: Add feature gate for JXL r=tnikkel,fluent-reviewers,preferences-reviewers
https://hg.mozilla.org/integration/autoland/rev/f3f40c6fb8e2
Part 4: Add image/jxl to Accept header and DownloadsViewableInternally r=necko-reviewers,dragana

Comment 6

[Iulian Moraru]

Backed out for causing build bustages.

Push with failures

Failure log

Backout link

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f591c3da4311
Part 1: Add vendored libjxl source r=aosmond
https://hg.mozilla.org/integration/autoland/rev/a3f23a20b532
Part 2: Implement nsJXLDecoder r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/4b69f99caf69
Part 3: Add feature gate for JXL r=tnikkel,fluent-reviewers,preferences-reviewers
https://hg.mozilla.org/integration/autoland/rev/4ccdb31e1c5a
Part 4: Add image/jxl to Accept header and DownloadsViewableInternally r=necko-reviewers,dragana

Comment 8

[Dorel Luca [:dluca]]

Backed out 4 changesets (bug 1707590) for worker/checkouts/gecko/config/rules.mk. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer?job_id=338698659&repo=autoland&lineNumber=83749

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=4ccdb31e1c5a4944cbf02b2e613d676c2c69dfbf

Backout:
https://hg.mozilla.org/integration/autoland/rev/dca9a0ba9cbc552051c5264c7a0aed9c483915e1

Comment 9

[Kagami [:saschanaz] (they/them)]

Blocked by clang 5 compatibility: https://gitlab.com/wg1/jpeg-xl/-/issues/227

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ab607973f20
Part 1: Add vendored libjxl source r=aosmond
https://hg.mozilla.org/integration/autoland/rev/9dfd0d516062
Part 2: Implement nsJXLDecoder r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/3a7e5c98dd13
Part 3: Add feature gate for JXL r=tnikkel,fluent-reviewers,preferences-reviewers
https://hg.mozilla.org/integration/autoland/rev/fec7b65cc3d7
Part 4: Add image/jxl to Accept header and DownloadsViewableInternally r=necko-reviewers,dragana

Comment 11

[Natalia Csoregi [:nataliaCs]]

Backed out for causing gtest crashes.

backout: https://hg.mozilla.org/integration/autoland/rev/9f83edbeb83f7dbee267aed95452041fcdc73003

push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=fec7b65cc3d728fadbb3bfff2ebe164a03b90e68&group_state=expanded

failure logs:

• PROCESS-CRASH | gtest | application crashed [@ mozilla::Maybe<mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> >::operator*() const &]
• PROCESS-CRASH | gtest | application crashed [@ mozilla::image::SourceBuffer::ExpectLength(unsigned long)]
• REFTEST TEST-UNEXPECTED-FAIL | image/test/reftest/jxl/jxl-size-33x33.jxl == image/test/reftest/jxl/jxl-size-33x33.png | image comparison, max difference: 207, number of differing pixels: 6931

Comment 12

[Kagami [:saschanaz] (they/them)]

Oh no, https://treeherder.mozilla.org/jobs?repo=try&revision=35798c94cfa7a6f1141d61c561bb305514e5d282 was all green, has there been another change before my patches? 🤔

Comment 13

[Kagami [:saschanaz] (they/them)]

It's still green https://treeherder.mozilla.org/jobs?repo=try&revision=28f5078c8e7068e88fab3c73f7b24fba616f48a4 and also locally has no problem, so I'll give it another try.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25da391baa22
Part 1: Add vendored libjxl source r=aosmond
https://hg.mozilla.org/integration/autoland/rev/9c7204c3f03c
Part 2: Implement nsJXLDecoder r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/e5d2d6824408
Part 3: Add feature gate for JXL r=tnikkel,fluent-reviewers,preferences-reviewers
https://hg.mozilla.org/integration/autoland/rev/6edab66fe2c5
Part 4: Add image/jxl to Accept header and DownloadsViewableInternally r=necko-reviewers,dragana

Comment 15

[Kagami [:saschanaz] (they/them)]

It's failing again, not sure why it fails on autoland but not on try/local?

Comment 16

[Noemi Erli[:noemi_erli]]

Backed out 4 changesets (Bug 1707590) for causing gtest crashes CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=Rs6prC24RxGi57cYM1hpFA.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=6edab66fe2c58c317a0307cf724a4d7f8e51fcc3

Log: https://treeherder.mozilla.org/logviewer?job_id=338871044&repo=autoland&lineNumber=1739
https://treeherder.mozilla.org/logviewer?job_id=338870932&repo=autoland&lineNumber=5243

Backout: https://hg.mozilla.org/integration/autoland/rev/56d36c0ea666d8e31477f60d72986b87240c6da5

Comment 17

[Kagami [:saschanaz] (they/them)]

gtest is failing even before creating the decoder, I think LoadFile() fails to read my added jxl files and is just returning zero bytes?

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3201d860afef
Part 1: Add vendored libjxl source r=aosmond
https://hg.mozilla.org/integration/autoland/rev/21f41a9df048
Part 2: Implement nsJXLDecoder r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/8cc3eaf728b6
Part 3: Add feature gate for JXL r=tnikkel,fluent-reviewers,preferences-reviewers
https://hg.mozilla.org/integration/autoland/rev/3bd322893127
Part 4: Add image/jxl to Accept header and DownloadsViewableInternally r=necko-reviewers,dragana

Comment 19

[Kagami [:saschanaz] (they/them)]

Canceled the CI jobs, hoping it backs out early...

Comment 20

[Noemi Erli[:noemi_erli]]

Backed out 4 changesets (Bug 1707590) for causing gtest crashes CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=3bd32289312743cdae84f8a0741b158142d3e262&selectedTaskRun=cdEYRzHNRZ-dfSb3QGZqSQ.0

Log: https://treeherder.mozilla.org/logviewer?job_id=338889882&repo=autoland&lineNumber=5245
and there are also some TV failures which need attention: https://treeherder.mozilla.org/logviewer?job_id=338890911&repo=autoland&lineNumber=2234

Backout: https://hg.mozilla.org/integration/autoland/rev/c100827b4f4acf98cf34ab52de6911b2576c7f8a

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d806a39de22
Part 1: Add vendored libjxl source r=aosmond
https://hg.mozilla.org/integration/autoland/rev/c4a3f9962009
Part 2: Implement nsJXLDecoder r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/b9cbeba9c10b
Part 3: Add feature gate for JXL r=tnikkel,fluent-reviewers,preferences-reviewers
https://hg.mozilla.org/integration/autoland/rev/09d918e34a59
Part 4: Add image/jxl to Accept header and DownloadsViewableInternally r=necko-reviewers,dragana

Comment 22

[Kagami [:saschanaz] (they/them)]

I hope it finally land this time 🙏

Comment 23

[Kagami [:saschanaz] (they/them)]

Just to note, the empty file issue is bug 1709608.

Comment 24

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4d806a39de22
https://hg.mozilla.org/mozilla-central/rev/c4a3f9962009
https://hg.mozilla.org/mozilla-central/rev/b9cbeba9c10b
https://hg.mozilla.org/mozilla-central/rev/09d918e34a59

Comment 25

[Mike Hommey [:glandium]]

This broke the build on !tier 1 builds, such as ppc64 linux, s390x linux, mips64el linux, etc.
Hitting this: https://hg.mozilla.org/mozilla-central/file/tip/third_party/highway/hwy/base.h#l254

Comment 26

[Petr Sumbera]

For record. Solaris SPARC is also affected:

third_party/highway/hwy/targets.h:378:14: error: ?HWY_HIGHEST_TARGET_BIT? was not declared in this scope; did you mean ?HWY_HIGHEST_TARGET_BIT_RVV??

Comment 27

[Kagami [:saschanaz] (they/them)]

Oops. Reported it to https://github.com/google/highway/issues/183

Comment 28

[jan.wassenberg]

Thank you for reporting this. We've fixed it in Highway, JPEG XL will still need to update the submodule.

Comment 29

[Kagami [:saschanaz] (they/them)]

This is fixed in upstream. Is it urgent or can we wait for libjxl to update its dependency? If it's urgent I guess we can try updating highway only.

Comment 30

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Pulsebot from comment #21)

https://hg.mozilla.org/integration/autoland/rev/09d918e34a59
Part 4: Add image/jxl to Accept header and DownloadsViewableInternally
r=necko-reviewers,dragana

== Change summary for alert #29985 (as of Thu, 06 May 2021 15:09:52 GMT) ==

Regressions:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
0.43%	installer size		osx-shippable	instrumented	115,258,681.46 -> 115,750,972.67
0.34%	installer size		osx-shippable	nightly	85,778,752.88 -> 86,073,304.83
0.30%	installer size		osx-aarch64-shippable	aarch64 nightly	82,078,135.54 -> 82,322,820.75

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=29985

Comment 31

[Dan Horák]

(In reply to Kagami :saschanaz [away until 2021-05-17] from comment #29)

This is fixed in upstream. Is it urgent or can we wait for libjxl to update its dependency? If it's urgent I guess we can try updating highway only.

I would prefer an early fix by updating highway. Having builds broken for longer time makes our CI for ppc/s390x/... less useful as we can't catch possibly newly introduced issues.

Comment 32

[Petr Sumbera]

(In reply to Dan Horák from comment #31)

(In reply to Kagami :saschanaz [away until 2021-05-17] from comment #29)

This is fixed in upstream. Is it urgent or can we wait for libjxl to update its dependency? If it's urgent I guess we can try updating highway only.

I would prefer an early fix by updating highway. Having builds broken for longer time makes our CI for ppc/s390x/... less useful as we can't catch possibly newly introduced issues.

The same for me for Solaris SPARC. Thank you!

Comment 33

[tt_1]

also broken on armv7a-linux:

19:53.10 /usr/armv7a-unknown-linux-gnueabihf/tmp/portage/www-client/firefox-90.0/work/firefox-90.0/third_party/highway/hwy/ops/arm_neon-inl.h:876:326: error: no matching function for call to 'hwy::N_NEON::Mask128<float, 4>::Mask128(uint32x4_t)'

would be great if the upstream fix can be backported into the beta phase of firefox-90.0, so that beta testers can provide feedback if the fix actually works.

Comment 34

[Mike Hommey [:glandium]]

the jxl code is not listed in toolkit/content/license.html

Comment 35

[Kagami [:saschanaz] (they/them)]

Attachment: Bug 1707590 - Part 4: Add references to libjxl and highway in about:license r=glandium

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6cf0774534a5
Part 4: Add references to libjxl and highway in about:license r=mhoye

Comment 37

[Sandor Molnar]

https://hg.mozilla.org/mozilla-central/rev/6cf0774534a5

Comment 38

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

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)