1916038 - [GCC] Firefox-130.0 has issues when showing AVIF images

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[x3x7]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:129.0) Gecko/20100101 Firefox/129.0

Steps to reproduce:

Using firefox-130.0-2.fc40 load the page from https://yle.fi/news

Actual results:

Images are shown in total black.

Expected results:

Proper images to be shown.

Comment 1

[x3x7]

Firefox 129.0.2 and older do work properly.

Comment 2

[x3x7]

Attachment: Sample image

Comment 3

[x3x7]

Firefox developer edition version 130.0b9 does work properly.

Comment 4

[Francesco Lodolo [:flod]]

Using firefox-130.0-2.fc40 load the page from https://yle.fi/news

Is firefox-130.0-2.fc40 a package released by Fedora? Not sure why the naming, given 130 hasn't been released yet (it will be on Sep 3).

Firefox developer edition version 130.0b9 does work properly.

Did you get beta 9 from the same source, or directly from mozilla.org?

It seems unlikely that such a breakage would be introduced between the last beta and release candidate, so it feels like a package problem.

Comment 5

[x3x7]

I got the beta 9 directly from mozilla.org. I will test also the 130 version when available. Fedora's version is not 100% same as the bug might be triggered by some of the applied patches.

Comment 7

[x3x7]

(In reply to Francesco Lodolo [:flod] from comment #4)

Is firefox-130.0-2.fc40 a package released by Fedora? Not sure why the naming, given 130 hasn't been released yet (it will be on Sep 3).

Yes, links below.

https://koji.fedoraproject.org/koji/buildinfo?buildID=2538547

https://kojipkgs.fedoraproject.org//packages/firefox/130.0/2.fc40/x86_64/firefox-130.0-2.fc40.x86_64.rpm
https://kojipkgs.fedoraproject.org//packages/firefox/130.0/2.fc40/src/firefox-130.0-2.fc40.src.rpm

Comment 8

[x3x7]

Tried the mozilla.org 130.0 release (https://archive.mozilla.org/pub/firefox/releases/130.0/linux-x86_64/en-US/firefox-130.0.tar.bz2) and it works properly.

Comment 9

[x3x7]

I do not know how it was possible for Fedora to release the 130.0 version so early. I checked the source package and it uses the same source as Mozilla's (https://archive.mozilla.org/pub/firefox/releases/130.0/source/firefox-130.0.source.tar.xz).

Some patched are applied so maybe that can cause the problem or testing environment issue.

Comment 10

[Francesco Lodolo [:flod]]

They probably build it off the RC version.

Can you file a bug with Fedora to investigate the issue on their side?

Comment 11

[x3x7]

Can you file a bug with Fedora to investigate the issue on their side?

I tried to file this issue with Fedora but found it simpler with Mozilla bug tracker... Do you have an URL where to proceed for Fedora?

Comment 12

[x3x7]

This seems to be the proper site, https://bugzilla.redhat.com.

Comment 13

[x3x7]

https://bugzilla.redhat.com/show_bug.cgi?id=2309203

Comment 14

[Francesco Lodolo [:flod]]

I'm going to close this one as MOVED, hopefully someone from Fedora will be able to help debug. If not, please report back.

Comment 15

[Martin Stránský [:stransky] (ni? me)]

It's caused by GCC. Tested latest trunk and when it's built with GCC it paints AVIFs as black.

Comment 16

[Martin Stránský [:stransky] (ni? me)]

Looks like pretty serious breakage...I see lot of empty images on web pages when browsing by firefox-130.0 built with GCC. I wonder if we should temporary disable AVIF support as a workaround.

Comment 17

[Timothy Nikkel (:tnikkel)]

My guess for what could have caused this is the changes to libyuv and the yuv to RGB conversion code. That's the only change I can think of related to avif decoding lately.

Comment 18

[Timothy Nikkel (:tnikkel)]

To be specific, bug 1906720, bug 1907121, bug 1905842 is where yuv related changes happened that could be the cause here.

Comment 19

[Martin Stránský [:stransky] (ni? me)]

Thanks. GCC debug build works so looks like optimization issue.

Comment 20

[x3x7]

Tested with gcc 13.3.1 and the problem is still there. I use a custom -O2 level and noticed -O3 is still used in some instances. Would be nice to verify if lowering the -O3 to -O2 avoids the bug but I do not know how to configure the build for this.

I thought the Dark Ages of computing are over and such compiler bugs do not happen these days. I do not use -O3 in Linux kernel builds but if the bug is there even at -O2 then trouble is pending.

Comment 21

[Mike Hommey [:glandium]]

FWIW, I'm having problems too with a build with GCC 11.

Comment 22

[Martin Stránský [:stransky] (ni? me)]

Mike, how can I disable optimization for particular file/directory? I tried to set CXXFLAGS in moz.build but it doesn't seem to work.
Thanks.

Comment 23

[Timothy Nikkel (:tnikkel)]

Maybe CFLAGS too?

https://searchfox.org/mozilla-central/rev/cc01f11adfacca9cd44a75fd140d2fdd8f9a48d4/gfx/cairo/cairo/src/moz.build#278

Comment 24

[Martin Stránský [:stransky] (ni? me)]

build hook (https://bugzilla.mozilla.org/show_bug.cgi?id=1870059#c7) or COMPILE_FLAGS["OPTIMIZE"] = ["-O0"] in moz.build file does the trick.
I bisected it to the libyuv update, if the lib is build without optimization (-O0) the avif images are okay.

Comment 25

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

Set release status flags based on info from the regressing bug 1905842

:ng, since you are the author of the regressor, bug 1905842, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 26

[xgreenlandforwyy]

Also observed same issue on Debian build 130.0-1 https://packages.debian.org/sid/firefox. There's already a bug ticket on https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1080518

I tried install Mozilla's .deb on https://ftp.mozilla.org/pub/firefox/releases/130.0/linux-x86_64/en-US/ and cannot reproduce it

Comment 27

[Martin Stránský [:stransky] (ni? me)]

Reverted Bug 1905842 seems to fix it.

Comment 28

[Martin Stránský [:stransky] (ni? me)]

Attachment: libyuv-downgrade.patch

Complete downgrade patch if anyone needs it.

Comment 29

[Martin Stránský [:stransky] (ni? me)]

Fedora will ship with downgraded libyuv as we can't ship such broken AVIF setup. Would be great to get a way how to debug/build particular libyuv files without optimization to find the broken one.

Comment 30

[x3x7]

Would be good if a gcc developer examines this downgrade to find the actual reason the compiler fails. What are the compiler optimization options for libyuv used in the firefox build?

Could firefox use the existing system libyuv library and focus our efforts in dealing with libyuv alone? (we could also use a clang system libyuv build as temporary fix)

I tested the firefox 130.0 build also with gcc 11.5.0 and gcc 12.4.1 and they both fail.

Comment 31

[x3x7]

The libyuv setup in the firefox built uses the default set of compiler options. for the build. Unfortunately even -O2 triggers the problem.

Comment 32

[x3x7]

The build fails also with gcc 10.5.0

There are 4 instances of libyuv in the tree:

./media/libyuv/libyuv
./media/libvpx/libvpx/third_party/libyuv
./third_party/libwebrtc/third_party/libyuv
./third_party/aom/third_party/libyuv

Two of these look to be active in the build:

./objdir/media/libyuv/libyuv
./objdir/third_party/libwebrtc/third_party/libyuv

Comment 33

[Martin Stránský [:stransky] (ni? me)]

It's the one at media/libyuv. We need to find out which file is affected here, then it may be fairly easy to find affected code part. Then it can be passed to GCC folks for fix.

Could firefox use the existing system libyuv library and focus our efforts in dealing with libyuv alone? (we could also use a clang system libyuv build as temporary fix)

Fedora doesn't ship system libyuv for instance, I think it's too specific.

Comment 34

[Martin Stránský [:stransky] (ni? me)]

You can directly edit objdir/media/libyuv/libyuv/libyuv_libyuv/backend.mk file, then delete *.o files from the dir and run make from objdir. that eliminates the build file update and build libyuv with modified build config. But I don't know how to bisect it to particular files.

Comment 35

[x3x7]

I tested with hardening disabled and gcc 14.2.1 but the problem is still there.

Comment 36

[Mike Hommey [:glandium]]

SOURCES["file.c"].flags += ["-O0"] in moz.build should help

Comment 37

[Timothy Nikkel (:tnikkel)]

There's also potentially the option of using pragmas to turn off optimization on specific code blocks that could narrow it down even further. libyuv already does it for msvs

https://searchfox.org/mozilla-central/rev/f09e3f9603a08b5b51bf504846091579bc2ff531/media/libyuv/libyuv/source/cpu_id.cc#127

gcc seems to have pragmas or there is attribute

https://stackoverflow.com/questions/2219829/how-can-i-prevent-gcc-optimizing-some-statements-in-c

Comment 38

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #36)

SOURCES["file.c"].flags += ["-O0"] in moz.build should help

Of course, that doesn't help because libyuv is using gyp...

Comment 39

[Mike Hommey [:glandium]]

Oh, the irony. Of all the files, row_gcc.cc is the affected one. Even at -O1.

Comment 40

[Mike Hommey [:glandium]]

Regression started with this change: https://chromium.googlesource.com/libyuv/libyuv/+/b0dfa70114d607fba655d5af6f481cc5d0a559b9%5E%21/#F2

Comment 41

[Mike Hommey [:glandium]]

And it turns out https://chromium.googlesource.com/libyuv/libyuv/+/616bee5420b62a7be09fda0252034e8be85f91b0%5E%21/#F10 fixed it... partially. The fix for this bug is to... add more volatiles.

Comment 42

[Mike Hommey [:glandium]]

Attachment: Bug 1916038 - Add volatile for gcc inline to avoid being removed.

This extends the fix upstream did in 616bee5420b62a7be09fda0252034e8be85f91b0,
which was not enough.

Comment 43

[Mike Hommey [:glandium]]

scale_gcc.cc /might/ have the same problem too, and possibly macros_msa.h.

Comment 44

[Jeff Muizelaar [:jrmuizel]]

Has the issue been reported to gcc upstream?

Comment 45

[x3x7]

Tested with gcc 10, 11, 12, 13 and 14 and all are affected. Looking at the fix, however, it might be that lacking the volatile specification it is behaving as intended so no compiler fix for it.

I wonder how does clang deal with the inline assembly, if using that part of the code at all. Perhaps it defaults to volatile behavior...

Comment 47

[tt_1]

Mike, can you please try to upstream your patch to libyuv upstream?

I will test if it fixes the linking errors I ran into and report back here later.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32a7a66074d1
Add volatile for gcc inline to avoid being removed. r=gfx-reviewers,nical

Comment 49

[Mike Hommey [:glandium]]

Attachment: Bug 1916038 - Add volatile for gcc inline to avoid being removed.

This extends the fix upstream did in 616bee5420b62a7be09fda0252034e8be85f91b0,
which was not enough.

Original Revision: https://phabricator.services.mozilla.com/D221275

Comment 50

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Black rendering of AVIF images when Firefox is built with GCC
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: This restores the volatile keyword that was removed in bug 1905842
• String changes made/needed: N/A
• Is Android affected?: no

Comment 51

[Mike Hommey [:glandium]]

Attachment: Bug 1916038 - Add volatile for gcc inline to avoid being removed.

This extends the fix upstream did in 616bee5420b62a7be09fda0252034e8be85f91b0,
which was not enough.

Original Revision: https://phabricator.services.mozilla.com/D221275

Comment 52

[Phabricator Automation]

release Uplift Approval Request

• User impact if declined: Black rendering of AVIF images when Firefox is built with GCC
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: This restores the volatile keyword that was removed in bug 1905842
• String changes made/needed: N/A
• Is Android affected?: no

Comment 53

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/32a7a66074d1

Comment 54

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/1c176b54cf3f

Comment 55

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/5a91ed7840f5

Comment 56

[Pascal Chevrel:pascalc]

This fix will be included in our planned 130 dot release, thanks.

Comment 58

[Timothy Nikkel (:tnikkel)]

Filed https://libyuv.issues.chromium.org/issues/366309840 to upstream this fix to libyuv.