Since --with-system-dav1d doesn't exist yet disabled assembly makes media.av1.use-dav1d slower than default (libaom). Let's break the platform whitelists by making Linux case generic to any ELF system. Unlike Linux some 32-bit systems (e.g., FreeBSD, OpenBSD) kept 4-byte alignment by default, so explicit -mstack-alignment=16 is required.

$ cc --version
FreeBSD clang version 8.0.0 (branches/release_80 355677) (based on LLVM 8.0.0)
Target: x86_64-unknown-freebsd13.0
Thread model: posix
InstalledDir: /usr/bin

$ cat config.asm
; Autogenerated by the Meson build system.
; Do not edit, your changes will be lost.

%define ARCH_X86_32 0

%define ARCH_X86_64 1

%define STACK_ALIGNMENT 32

vs.

$ cc --version
FreeBSD clang version 8.0.0 (branches/release_80 355677) (based on LLVM 8.0.0)
Target: i386-unknown-freebsd13.0
Thread model: posix
InstalledDir: /usr/bin

$ cat config.asm
; Autogenerated by the Meson build system.
; Do not edit, your changes will be lost.

%define ARCH_X86_32 1

%define ARCH_X86_64 0

%define PIC 1

%define STACK_ALIGNMENT 16

Thanks for the report, we are about to enable dav1d in Linux.

In Linux we keep stack alignment to 16 due to crashes (Bug 1515933) even on x64. We do rely on the defaults for that. I guess we need to change it now and set the stack alignment flags explicitly.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc0dccfb72c7a6f4436e0594f6235035e0670cb5

Something is weird. I don't notice perf impact unlike with standalone dav1d.

Have you set "media.av1.use-dav1d" pref to true?

Obviously. Looks like media.rdd-process.enabled (per bug 1534814) is the culprit.

Is somewhere documented that default stack alignment is 4 bytes on x86 ? The only doc I can find is the gcc docs that default is -mpreferred-stack-boundary=4 (16-byte alignment), which must set a default -mincoming-stack-boundary=4.

(In reply to Alex Chronopoulos [:achronop] from comment #9)
See https://lists.freebsd.org/pipermail/freebsd-current/2012-November/037563.html
which points to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38496

Clang has a concise list which OSes changed from 4-byte to 16-byte stack alignment by default:
https://github.com/llvm/llvm-project/blob/llvmorg-8.0.0/llvm/lib/Target/X86/X86Subtarget.cpp#L282-L288
https://github.com/llvm/llvm-project/blob/llvmorg-8.0.0/llvm/lib/Target/X86/X86Subtarget.h#L410-L412

(In reply to Jan Beich from comment #13)

Not sure what are you trying to prove. -mincoming-stack-boundary=2 is neither in standalone build nor is needed here.

it is needed when we set the flags for entrypoint_source_files. I said that we need to keep it limited on x86 as you have done in your patches.

How is it related to this bug? I don't see crashes on either i386 (aka x86) or amd64 (aka x86_64).

We don't have crashes because we are on 16-byte stack alignment already in Linux. In comment 1 mentioned that this is due to crashes reported in Bug 1515933.

This is related to this bug because, on Linux, we need to allow the default stack alignment for amd64 but to set the stack alignment explicitly in i386 (previous comment).

Also, in asm/moz.build we need to use the asm/x86_64/linux/config.as for Linux and not the asm/x86_64/config.as.

Looks like I need to revert to v0 i.e., pass -mstack-alignment=16 / -mpreferred-stack-boundary=4 on i386 (aka x86) again. On Linux those are nop but elsewhere using 4-byte stack alignment may to lead to crashes as SSE2 assumes 16-byte alignment. And, if not obvious, BSDs don't force SSE2 on i386 (aka x86) by default unlike Linux.

nice, please note that the change in config.h is already landed.

(In reply to Jan Beich from comment #14)

so whatever broke build for you (Try links in bug 1536538 don't work for me) was probably obfuscation/optimization for Linux special/broken case.

The try link is the following, it's something broken on our end and reports the wrong links in the bugs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fff724e2b985694dc3724876ceb5aff954ecde27

My build broke because I used the -mpreferred-stack-boundary=4/-mincoming-stack-boundary=2 for the entry source files on amd64.

(BMO disabled r?, so assume f? is actually r?.)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=608521f0330964f4d2097e0faf236e0755f30678

A regression from review by a non-build peer.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87a977b737ba6811bc0644575379d183e73ec520

Do you mind if I do small modifications in your patches and put them up for review? If you create an account in [1] you would be able to post your comments there.

[1] https://phabricator.services.mozilla.com/

(In reply to Alex Chronopoulos [:achronop] from comment #19)

Do you mind if I do small modifications in your patches and put them up for
review?

Sure but I may need to check the code still works on FreeBSD amd64 + i386 and doesn't exclude at least DragonFly, NetBSD, OpenBSD, Solaris.

If you create an account in [1] you would be able to post your comments there.

[1] https://phabricator.services.mozilla.com/

I can't login due to "Bugzilla MFA Not Enabled", see bug 1536716. Posting comments on Phabricator won't be possible.

Sorry for confusing wording. My patches here are in the public domain. Feel free to do whatever.

(In reply to Jan Beich from comment #21)

Sorry for confusing wording. My patches here are in the public domain. Feel free to do whatever.

Sure thanks a lot, I believe we can work together to get them landed properly.

I have just posted the patch including your changes with some small modification. I believe we need them in order to avoid crashes. I wont land them until you give me the green light, which means to agree with the code and check that it works in every os/platform you think it should work.

Since phabricator does not work post here any comment or test result.

The correct try link is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=147dd528ed76c60a12f13c006d31d6cd776a4a47

Comment on attachment 9052582 [details]
Bug 1536070 - Enable ASM in dav1d for Tier3 platforms.

Works fine but I don't like the style.

• Bug 1515933 doesn't affect FreeBSD amd64 (aka x86_64 or x64), so forcing 16-byte (re)alignment with AVX2 may affect performance
• BSDs and Solaris are not "Linux flavors": kernel and libc are different; see bug 1401093 how dangerous such assumptions can be
• media/libdav1d/asm/moz.build and toolkit/moz.configure are out of sync, breaking build on GNU/kFreeBSD

Landry, are you fine with OpenBSD being treated as a "Linux flavor"? If not apply my patches and confirm bundled dav1d doesn't crash on amd64 when playing AV1 content. Note, (on Firefox 67) apply patches in bug 1533559 and bug 1536538 as well.

OTOH, even Linux will be fixed soon or you don't care enough testing AV1 support in advance would be appreciated to avoid surprises like bug 952048.

I will eventually look at dav1d when its enabled by default for us or starts breaking the build. Those days my time for that is limited, i never really looked at phabricator, i login less and less to bugzilla (because MFA and i dont always have my cell around), and i stopped looking at trunk status on my buildbot (mozbuild is unreliable), i only build released betas. Quickly skimming through alex's patch and yours, i have no opinion which is 'better'.

If you provide me a testcase with a file/url to play, and a clear list of patches to apply on top of a given branch, i can give it a shot, but i can promise nothing, sorry.

Btw i'd be also interested by --with-system-dav1d but from my limited understanding this wont happen as long as the lib is a moving target.

(In reply to Jan Beich from comment #26)

• Bug 1515933 doesn't affect FreeBSD amd64 (aka x86_64 or x64), so forcing 16-byte (re)alignment with AVX2 may affect performance

Both Linux and FreeBSD use the same ABI here, so the LTO bug will affect both.

Alex, can you adjust STACK_ALIGNMENT in media/libdav1d/config.h as well?

(In reply to Thomas Daede [:TD-Linux] from comment #30)

Both Linux and FreeBSD use the same ABI here, so the LTO bug will affect both.

Neither bug 1515933 nor upstream mentioned LTO. After testing I can confirm LTO on FreeBSD is also affected.

I have pushed in phabricator an update if you want to have a look.

Comment on attachment 9052582 [details]
Bug 1536070 - Enable ASM in dav1d for Tier3 platforms.

I confirm, this version (i.e., what landed) works fine on FreeBSD amd64 + i386.

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

LTO crash also affects i386 (aka x86) on BSDs because of 4-byte default. Like amd64 GCC LTO isn't affected.

Comment on attachment 9052582 [details]
Bug 1536070 - Enable ASM in dav1d for Tier3 platforms.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: dav1d support Linux BSD. We need it in order to enable dav1d on beta.
• 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 1535631
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Dav1d on Linux is stable, this patch adds more Linux flavors.
• String changes made/needed:

(In reply to Alex Chronopoulos [:achronop] from comment #38)

• Why is the change risky/not risky? (and alternatives if risky): Dav1d on Linux is stable, this patch adds more Linux flavors.

Can you avoid referring to other non-linux non-windows non-macos non-android operating systems as "Linux flavors" ? I personally don't really care but some ppl are sensitive about that. Thanks!

(In reply to Alex Chronopoulos [:achronop] from comment #38)

Comment on attachment 9052582 [details]
Bug 1536070 - Enable ASM in dav1d for Tier3 platforms.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: dav1d support Linux BSD. We need it in order to enable dav1d on beta.

Wait, we need to enable BSD support on beta so as to also enable dav1d support on our tier1 platforms on beta? Alex can you elaborate on this please? Thanks

(In reply to Pascal Chevrel:pascalc from comment #40)

Wait, we need to enable BSD support on beta so as to also enable dav1d support on our tier1 platforms on beta? Alex can you elaborate on this please? Thanks

This prevents a problem in the compiler and can affect Linux and FreeBDS. If we don't care about building beta with any compiler we can avoid it. However, I would prefer to take it because the following patches have been based on it and it is safer to maintain the same code level between beta and nightly. Also, it's a small patch of low risk.

Comment on attachment 9052582 [details]
Bug 1536070 - Enable ASM in dav1d for Tier3 platforms.

Patch needed for the libdav1d update to 0.2.1 on beta. Uplift accepted for 67 beta 8, thanks.

https://hg.mozilla.org/releases/mozilla-beta/rev/039c28632803