dav1d not being used despite use-dav1d set to true

RESOLVED FIXED in Firefox 67

Status

()

defect
P2
normal
Rank:
12
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: OussamaDanba, Assigned: achronop)

Tracking

(Regressed 1 bug, {regression})

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 unaffected, firefox67 fixed, firefox68 fixed)

Details

Attachments

(1 attachment)

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.

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).

Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core

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.

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

Blocks: 1500596
Status: UNCONFIRMED → NEW
Rank: 12
Ever confirmed: true
Flags: needinfo?(mfroman)
Keywords: regression
Priority: -- → P2

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?

Flags: needinfo?(mfroman) → needinfo?(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.

Flags: needinfo?(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

Flags: needinfo?(achronop) → needinfo?(mfroman)

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

Flags: needinfo?(mfroman) → needinfo?(achronop)

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

Flags: needinfo?(achronop)

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

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.

Flags: needinfo?(achronop)

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

Flags: needinfo?(achronop)

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

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

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

See Also: → 1539449
Depends on: 1538455
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee: nobody → achronop
No longer blocks: 1500596

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:
Attachment #9053304 - Flags: approval-mozilla-beta?

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.

Attachment #9053304 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Regressions: 1549892
You need to log in before you can comment on or make changes to this bug.