1538474 - dav1d not being used despite use-dav1d set to true

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[OussamaDanba]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

I tried to watch an AV1 video on Firefox Nightly with media.av1.use-dav1d set to true (which is the default on nightly) but it's not using dav1d; it's using libaom.

Actual results:

libaom is used for decoding rather than dav1d.

Expected results:

dav1d should be used when the media.av1.use-dav1d is set to true.

Comment 1

[OussamaDanba]

Never reported a bug before so sorry for the separate comment.

I've observed this happen on both linux and windows on Nightly.
Initially I was looking into bad decoding performance and was using perf top (on linux) and noticed the decoding
functions of libaom being used rather than those of dav1d. On Windows using process explorer the thread count for
decoding also matched the settings of AOMDecoder.cpp rather than those of DAV1DDecoder.cpp.

As a final confirmation I build Firefox from source with the thread counts in DAV1DDecoder.cpp being changed
but that had no effect. Changing those in AOMDecoder.cpp did have an effect so it seems the media.av1.use-dav1d pref
is being ignored on both my machines.

Let me know if you need any additional information (or want me to hop on IRC).

Comment 2

[OussamaDanba]

I had a look using a debugger and put breakpoints on the constructor of AOMDecoder and DAV1DDecoder and noticed the DAV1DDecoder constructor is never reached, only the AOMDecoder constructor is reached.

The call stack looks like this which leads to RemoteVideoDecoder.cpp:155 where it always constructs an AOMDecoder. AgnosticDecoderModule which does check for media.av1.use-dav1d is never reached on my machine.

Comment 3

[Alex Chronopoulos [:achronop]]

Great catch! Thank you for noticing!

RemoteVideoDecoder in [1] must check the dav1d pref and trigger dav1d instead of libaom when necessary, similar to [1].

[1] https://searchfox.org/mozilla-central/source/dom/media/ipc/RemoteVideoDecoder.cpp#152-156
[2] https://searchfox.org/mozilla-central/source/dom/media/platforms/agnostic/AgnosticDecoderModule.cpp#50-59

Comment 4

[Michael Froman [:mjf]]

Ah! Alex - back when I added the RDD code, there was no dav1d pref in AgnosticDecoderModule. Would you rather I add it or would you like to add it?

Comment 5

[Alex Chronopoulos [:achronop]]

I don't mind anything you prefer, I don't know what is going on with uplifts etc. We need it soon so if you don't have the time I can do it.

Comment 6

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1538474 - Enable dav1d in RDD process. r?mjf

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ef62825d79f
Enable dav1d in RDD process. r=mjf

Comment 8

[Narcis Beleuzu [:NarcisB]]

Backed out for causing mda failures on test_playback.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/d04ec06581a08f3b487f1becd6ef018424ef8139
Push link: https://hg.mozilla.org/integration/autoland/rev/6ef62825d79f142b9aabd982d84f2e69e4eded06
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=235912121&repo=autoland&lineNumber=2532

Comment 9

[Alex Chronopoulos [:achronop]]

Michael, can you please help me with that issue. This is a report inside the RDD process. I am not sure how it is linked to AV1 change. Also, stack is not very useful.

I've already downloaded and tested locally the opt and debug binaries from autoland but it's green there.

I have pushed on try including a debug asan run in case we have better luck there.

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

Comment 10

[Michael Froman [:mjf]]

Alex, would you be able to do that same debug asan run, but change security.sandbox.rdd.win32k-disable pref[1] to false?

[1] https://searchfox.org/mozilla-central/source/browser/app/profile/firefox.js#1050

Comment 11

[Alex Chronopoulos [:achronop]]

Here is the try run with security.sandbox.rdd.win32k-disable off:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa0729c153b87a86c99ca1392307ddfc5219e233

Comment 12

[Michael Froman [:mjf]]

While we're waiting on those results, I'm going to try to get an asan build here on my Win VM.

Comment 13

[Michael Froman [:mjf]]

Alex, dav1d is crashing on windows asan during the mochitest dom/media/test/test_playback.html even when not running on RDD. You can see the relevant log output here:
2:12.02 INFO [finished A4.ogv-72] remaining= flac-sample.mp4-71
2:12.02 PASS [finished A4.ogv-72 t=111.516] Length of array should match number of running tests
2:12.02 PASS [started av1.mp4-74 t=111.518] Length of array should match number of running tests
2:12.07 PASS av1.mp4: Name should match #3
2:12.07 PASS av1.mp4: Name should match #1
2:12.07 PASS av1.mp4 duration (0.999984) should be around 1
2:12.07 PASS av1.mp4 isEncrypted should be true if we have decryption keys
2:12.54 GECKO(9756) ###!!! [Parent][MessageChannel] Error: (msgtype=0x1F0099,name=PBrowser::Msg_UpdateNativeWindowHandle) Channel error: cannot send/recv
2:12.54 GECKO(9756) ###!!! [Parent][MessageChannel] Error: (msgtype=0x1F0088,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
2:12.68 GECKO(9756) A content process crashed and MOZ_CRASHREPORTER_SHUTDOWN is set, shutting down

In fact, just running the local asan build and loading the file dom/media/test/av1.mp4 will crash the content process.

Comment 14

[Alex Chronopoulos [:achronop]]

Thank you Michael, I will create my own asan build and test it here.

Comment 15

[Alex Chronopoulos [:achronop]]

Unfortunately my local build does not crash ... Is your build debug or opt?

Comment 16

[Michael Froman [:mjf]]

Here is the .mozconfig I used on Windows.
ac_add_options --target=x86_64-pc-mingw32
ac_add_options --host=x86_64-pc-mingw32

ac_add_options --enable-address-sanitizer
ac_add_options --disable-jemalloc

export CC="clang-cl.exe"
export CXX="clang-cl.exe"

export LDFLAGS="clang_rt.asan_dynamic-x86_64.lib clang_rt.asan_dynamic_runtime_thunk-x86_64.lib"
CLANG_LIB_DIR="$(cd ~/.mozbuild/clang/lib/clang/*/lib/windows && pwd)"
export MOZ_CLANG_RT_ASAN_LIB_PATH="${CLANG_LIB_DIR}/clang_rt.asan_dynamic-x86_64.dll"
export LIB=$LIB:$CLANG_LIB_DIR

Comment 17

[Michael Froman [:mjf]]

However, we have a larger problem. The mochitest should be failing on try for dav1d running in content process, but the test stays green even though the content process is crashed by the dav1d decoder. My try run is here[1].

If you look in the log[2] at 02:08:35 timestamp, you'll see:
02:08:35 INFO - GECKO(9096) | A content process crashed and MOZ_CRASHREPORTER_SHUTDOWN is set, shutting down

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3ec9590a68febee64fb38ad4ed4f86cad1ede88&selectedJob=236277480
[2] https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=236277480&repo=try&lineNumber=2551

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/134d1cdc2c83
Enable dav1d in RDD process. r=mjf

Comment 19

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/134d1cdc2c83

Comment 20

[Alex Chronopoulos [:achronop]]

Comment on attachment 9053304 [details]
Bug 1538474 - Enable dav1d in RDD process. r?mjf

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: RDD is a hard requirement for dav1d. Dav1d is allowed only through RDD. This patch add dav1d decoder in RDD.
• 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 1538455
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Dav1d is stable in Nightly, with all previous patches.
• String changes made/needed:

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: RDD is a hard requirement for dav1d. Dav1d is allowed only through RDD. This patch add dav1d decoder in RDD.
• 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 1538455
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Dav1d is stable in Nightly, with all previous patches.
• String changes made/needed:

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: RDD is a hard requirement for dav1d. Dav1d is allowed only through RDD. This patch add dav1d decoder in RDD.
• 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 1538455
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Dav1d is stable in Nightly, with all previous patches.
• String changes made/needed:

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: RDD is a hard requirement for dav1d. Dav1d is allowed only through RDD. This patch add dav1d decoder in RDD.
• 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 1538455
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Dav1d is stable in Nightly, with all previous patches.
• String changes made/needed:

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: RDD is a hard requirement for dav1d. Dav1d is allowed only through RDD. This patch add dav1d decoder in RDD.
• 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 1538455
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Dav1d is stable in Nightly, with all previous patches.
• String changes made/needed:

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: RDD is a hard requirement for dav1d. Dav1d is allowed only through RDD. This patch add dav1d decoder in RDD.
• 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 1538455
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Dav1d is stable in Nightly, with all previous patches.
• String changes made/needed:

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: RDD is a hard requirement for dav1d. Dav1d is allowed only through RDD. This patch add dav1d decoder in RDD.
• 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 1538455
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Dav1d is stable in Nightly, with all previous patches.
• String changes made/needed:

Comment 21

[Alex Chronopoulos [:achronop]]

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

Comment 22

[Pascal Chevrel:pascalc]

Comment on attachment 9053304 [details]
Bug 1538474 - Enable dav1d in RDD process. r?mjf

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

Comment 23

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d1ca56d685a6