Closed Bug 1493400 Opened 11 months ago Closed 9 months ago

Vendor dav1d and create MediaDataDecoder wrapper

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox64 --- wontfix
firefox65 --- fixed

People

(Reporter: TD-Linux, Assigned: achronop)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(5 files, 5 obsolete files)

46 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Parse ninja files and generate mozbuild files.
Blocks: dav1d
No longer depends on: dav1d
Priority: -- → P2
Thomas, is this something you're currently looking at?
Flags: needinfo?(tdaede)
I'm not looking at it quite yet. That said, it should be significantly easier than libaom, because dav1d uses meson which can output ninja files, and we can use the same vendoring system as ANGLE, I think.
Flags: needinfo?(tdaede)
(In reply to Thomas Daede [:TD-Linux] from comment #2)
> I'm not looking at it quite yet. That said, it should be significantly
> easier than libaom, because dav1d uses meson which can output ninja files,
> and we can use the same vendoring system as ANGLE, I think.

Unfortunately, it looks like ANGLE is relying upon 'gn' to generate json output to get the list of files to build [1], so I don't think meson's ninja backend will be enough to get this working. Adding a json or moz.build backend to Meson might be one way ahead, or perhaps we could parse the ninja output itself.

[1] https://searchfox.org/mozilla-central/rev/3a54520d8d2319a4116866371ed3d9ed2ec0cc2b/gfx/angle/update-angle.py#159
In order to engage the fuzzy team we need to file a PI request (better in advance). We also need to mention that the existing AV1 decoder already has a fuzzing target in-tree and the new decoder may need a second one.
:decoder pointed out that dav1d comes with a fuzzing target and it can be used with minor adaptations:
https://code.videolan.org/videolan/dav1d/blob/master/tests/libfuzzer/dav1d_fuzzer.c
Attached patch add-media-libdav1d (obsolete) — Splinter Review
WIP: This is for building in Linux. I am using config files from ninja parsing.
Attachment #9019754 - Flags: feedback?(ted)
Attachment #9019754 - Attachment mime type: application/octet-stream → text/patch
Attachment #9019754 - Attachment is patch: true
Attachment #9019754 - Attachment mime type: text/patch → text/plain
See Also: → 1501796, 1501995
Depends on D9607
Assignee: nobody → achronop
Depends on: 1501796
See Also: 1501796
:achronop, don't you plan to write the MediaDataDecoder wrapper for it?
pushing just dav1d to the tree like that would just take space for no reason.
Summary: Vendor dav1d → Vendor dav1d and create MediaDataDecoder wrapper
Try run from comment 16 has NASM enabled as implemented in Bug 1501796. 

There are several build errors in *.asm files in Linux64. I suspect that the errors occur due to different NASM version used by try (2.10 on try, 2.13 locally). Locally I build successfully on Linux64.

Also, for windows errors, NASM is not enabled yet in window platform.
dav1d issue about android link error:
https://code.videolan.org/videolan/dav1d/issues/136
Because dav1d currently doesn't have arm assembly, we can probably just rely on libaom on Android for now.
Also more link errors in Android:

[task 2018-10-31T17:53:43.520Z] 17:53:43     INFO -  /builds/worker/workspace/build/src/android-ndk/toolchains/x86-4.9/prebuilt/linux-x86_64/lib/gcc/i686-linux-android/4.9.x/../../../../i686-linux-android/bin/ld: error: read-only segment has dynamic relocations
[task 2018-10-31T17:53:43.520Z] 17:53:43     INFO -  /builds/worker/workspace/build/src/android-ndk/toolchains/x86-4.9/prebuilt/linux-x86_64/lib/gcc/i686-linux-android/4.9.x/../../../../i686-linux-android/bin/ld: error: hidden symbol 'posix_memalign' is not defined locally

We build with `D__ANDROID_API__=16` which corresponds to 4.1

According to https://github.com/android-ndk/ndk/issues/647 the method `posix_memalign` is available in 4.2 (api 17).
I have filed an issue in dav1d to track adding a fallback for API 16: https://code.videolan.org/videolan/dav1d/issues/140
Attachment #9019982 - Attachment is obsolete: true
Attachment #9019983 - Attachment is obsolete: true
The wrapper implementation is blocked on https://code.videolan.org/videolan/dav1d/merge_requests/246/diffs
 We are not blocked by the merge request in previous comment, apparently.

This is a 1st version of the wrapper working with 8-bitdepth files:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=210390981&revision=c2e7add68a2d83d1ebdecc935e326e9baae5f475
New try run for every platform (to help testing):
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=210653492&revision=8c2a59afaea28f27619d53aff1b659b022949bbc
In order to use dav1d you need to flip the pref media.av1.use-dav1d (on the top of media.av1.enabled)

P.S. the OSX failures seem unrelated to my changes.
For the record, I tested a local build with asm on. Initially I hit the issue described in [1]. It proved to be a mistake in our moz.build files. After corrected that the build was successful and all 8 and 10 bitdepth videos played without a problem. Also, playback was very smooth, even on a debug build, without frames drops.

[1] https://code.videolan.org/videolan/dav1d/issues/157
Hmm I am getting the same OSX failures after a rebase, which cannot be a coincident:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5c925e62a69ce335d171edc1e67dae9cfcfcd6a

Error:
/builds/worker/workspace/build/src/dom/media/platforms/apple/AppleVTDecoder.cpp:434:5: error: unknown type name 'IOSurfacePtr'
/builds/worker/workspace/build/src/dom/media/platforms/apple/AppleVTDecoder.cpp:434:28: error: use of undeclared identifier 'MacIOSurfaceLib'
/builds/worker/workspace/build/src/dom/media/platforms/apple/AppleVTDecoder.cpp:437:12: error: unknown type name 'MacIOSurface'; did you mean '__IOSurface'?
/builds/worker/workspace/build/src/dom/media/platforms/apple/AppleVTDecoder.cpp:437:43: error: unknown type name 'MacIOSurface'; did you mean '__IOSurface'?
/builds/worker/workspace/build/src/dom/media/platforms/apple/AppleVTDecoder.cpp:439:39: error: unknown type name 'MacIOSurfaceImage'
/builds/worker/workspace/build/src/dom/media/platforms/apple/AppleVTDecoder.cpp:641:35: error: use of undeclared identifier 'MacIOSurfaceLib'
/builds/worker/workspace/build/src/dom/media/platforms/apple/AppleVTDecoder.cpp:643:17: error: no matching function for call to 'ArrayLength'
/builds/worker/workspace/build/src/dom/media/platforms/apple/AppleVTDecoder.cpp:651:24: error: no matching function for call to 'ArrayLength'
make[4]: *** [Unified_cpp_dom_media_platforms1.o] Error 1
Attachment #9019754 - Attachment is obsolete: true
Attachment #9019754 - Flags: feedback?(ted)
DecoderDoctorLifeLogger is used as a base class in many classes thus the destructor has to be virtual.
Attachment #9025754 - Attachment description: Bug 1493400 - Create build files for dav1d. r?ted → Bug 1493400 - Create build files for dav1d. r?chmanchester
Blocks: 1509327
Attachment #9026924 - Attachment is obsolete: true
Attachment #9026374 - Attachment is obsolete: true
Pushed by achronopoulos@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ddfa91686df0
Import dav1d into tree. r=glob
https://hg.mozilla.org/integration/autoland/rev/94a729d77bc5
Update dav1d from upstream to d27598e. r=TD-Linux
https://hg.mozilla.org/integration/autoland/rev/5c4bf474ddb3
Create build files for dav1d. r=firefox-build-system-reviewers,chmanchester
https://hg.mozilla.org/integration/autoland/rev/8b2be3d7ece9
Implement platform decoder for dav1d. r=jya
https://hg.mozilla.org/integration/autoland/rev/04d915d32eea
Test av1 video using dav1d has the correct number of frames. r=jya
I can't build firefox since this landed.  Here's the error I see:

 0:02.56 In file included from /home/paul/dev/moz/dont-poison/third_party/dav1d/src/data.c:39:
 0:02.56 /home/paul/dev/moz/dont-poison/third_party/dav1d/src/ref.h:39:5: error: unknown type name 'atomic_int'
 0:02.56     atomic_int ref_cnt;
 0:02.56     ^

I'm using clang6 on amd64 Linux.
(In reply to Paul Bone [:pbone] from comment #44)
> I can't build firefox since this landed.  Here's the error I see:
> 
>  0:02.56 In file included from
> /home/paul/dev/moz/dont-poison/third_party/dav1d/src/data.c:39:
>  0:02.56 /home/paul/dev/moz/dont-poison/third_party/dav1d/src/ref.h:39:5:
> error: unknown type name 'atomic_int'
>  0:02.56     atomic_int ref_cnt;
>  0:02.56     ^
> 
> I'm using clang6 on amd64 Linux.

Can you please open a new bug about it? Can you please build dav1d library alone from [1] and provide the config.h file found in the build directory.

[1] https://code.videolan.org/videolan/dav1d
Blocks: 1511263
You need to log in before you can comment on or make changes to this bug.