Closed Bug 1559213 Opened 6 years ago Closed 11 months ago

support building against system av1 libs instead of bundled.

Categories

(Core :: Audio/Video, enhancement, P5)

Product:
Core
Component:
Audio/Video
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
136 Branch
Tracking Flags:
Tracking Status
firefox69 --- wontfix
firefox136 --- fixed

People

(Reporter: anarchy, Assigned: gaston)

Details

Attachments

(5 files, 2 obsolete files)

Bug 1559213 Allow to use system av1 libs instead of bundled r?glandium
47 bytes, text/x-phabricator-request
Details | Review
0005-bmo-1559213-Support-system-av1.patch
3.90 KB, patch
Details | Diff | Splinter Review
0016-bmo-1559213-fix-system-av1-libs.patch
728 bytes, patch
Details | Diff | Splinter Review
Bug 1559213: add method to conditionally include system av1 headers r?ng,glandium
48 bytes, text/x-phabricator-request
Details | Review
Bug 1559213: commit generated moz.build files r?ng,glandium
48 bytes, text/x-phabricator-request
Details | Review

Will attach patch to phabricator in just a minute.

Type: defect → enhancement

This simply allows us to link to system libraries for av1 support
instead of using the bundled libs

Jory, are you in search of a reviewer for these patches?

Flags: needinfo?(anarchy)

(In reply to Bryce Seager van Dyk (:bryce) from comment #2)

Jory, are you in search of a reviewer for these patches?

Of course, it doesn't make much sense to submit a patch if I am not wanting to see it included in upstream tree.

Flags: needinfo?(anarchy)
Assignee: nobody → anarchy
Status: NEW → ASSIGNED
Priority: -- → P5

Is this patch/review stale ? Would be interesting to have, since we ship dav1d 0.4.0 in OpenBSD ports i could use it.

(In reply to Landry Breuil (:gaston) from comment #4)

Is this patch/review stale ? Would be interesting to have, since we ship dav1d 0.4.0 in OpenBSD ports i could use it.

I have stepped down from Mozilla products in Gentoo. They still use a patch you would be most inclined to have a view of it if your wanting to use it with OpenBSD.

The bug assignee didn't login in Bugzilla in the last 7 months.
:jimm, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: anarchy → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
Severity: normal → S3

This is the rebased patch for 116.0.

Will look at it.

Flags: needinfo?(stransky)

And to clarify and be clear, these two patches are currently needed to get system-av1 support. I'll add both patches after this message. These have been rebased and apply on 118.0.1 currently.

Rebased version of anarchy's initial patch.

Attachment #9346640 - Attachment is obsolete: true
Assignee: nobody → stransky
Status: NEW → ASSIGNED
Attachment #9071974 - Attachment description: Allow to use system av1 libs instead of bundled → Bug 1559213 Allow to use system av1 libs instead of bundled r?glandium
Flags: needinfo?(stransky)

say i'd like to unstall https://phabricator.services.mozilla.com/D34921, since i'm not the original author, i guess i need to create a new rebased revision taking into account glandium's comments ?

(In reply to Landry Breuil (:gaston) from comment #12)

say i'd like to unstall https://phabricator.services.mozilla.com/D34921, since i'm not the original author, i guess i need to create a new rebased revision taking into account glandium's comments ?

Please use 'Commandeer revision' action and you'll become owner of the patch and you can submit new one. You can download/apply the patch locally by 'moz-phab patch D34921 --apply-to rev_num' so it preserves phabricator link to the revision.

i've tried the moz-phab patch trick, but it failed to apply the patch and just left the broken pieces aroud, so i'll try to manually preserve the phabricator link in the commit message.. i guess..

Attachment #9428462 - Attachment is obsolete: true

Awesome to have you working on this! As far as I'm aware, only Gentoo and FreeBSD allow using system-av1 and system-libvpx through a similar patch. I compared the one you updated to the one we're carrying and there are slight differences - yours seem better, so I'm eager to test them out during a next version bump!

Of course hoping these land straight upstream to ease up package maintenance in distribution level.

kind review ping.. :)

Flags: needinfo?(mh+mozilla)
Flags: needinfo?(alwu)

r+ on the patch, sorry for delay!

Flags: needinfo?(alwu)
Flags: needinfo?(mh+mozilla)

trying to rebase the patch on top of m-c to take into account mike's review, i now stumble on a new build failure in libwebrtc :

 1:20.32 In file included from Unified_cpp_dav1d_decoder_gn0.cpp:2:                                                                      
 1:20.32 In file included from /home/landry/src/m-c/third_party/libwebrtc/modules/video_coding/codecs/av1/dav1d_decoder.cc:21:           
 1:20.32 In file included from /home/landry/src/m-c/third_party/libwebrtc/third_party/dav1d/libdav1d/include/dav1d/dav1d.h:22:           
 1:20.32 /usr/obj/m-c/dist/system_wrappers/dav1d/dav1d.h:3:15: fatal error: 'dav1d/dav1d.h' file not found                               
 1:20.33 #include_next <dav1d/dav1d.h>                                                                                                   
 1:20.33               ^~~~~~~~~~~~~~~                                                                                                   
 1:20.39 1 error generated.                                                                                                              
 1:20.56 gmake[4]: *** [/home/landry/src/m-c/config/rules.mk:676: Unified_cpp_dav1d_decoder_gn0.o] Error 1                               
 1:20.57 gmake[4]_: on quitte le r_pertoire __/usr/obj/m-c/third_party/libwebrtc/modules/video_coding/codecs/av1/dav1d_decoder_gn__      
 1:20.57 gmake[3]: *** [/home/landry/src/m-c/config/recurse.mk:72: third_party/libwebrtc/modules/video_coding/codecs/av1/dav1d_decoder_gn/target-objects] Error 2
 1:20.58 gmake[3]: *** Attente des t_ches non termin_es....                                                                              
 1:21.61 gmake[4]_: on quitte le r_pertoire __/usr/obj/m-c/third_party/libwebrtc/modules/video_coding/timing/frame_delay_variation_kalman_filter_gn__
 1:22.11 In file included from Unified_cpp_ibaom_av1_encoder_gn0.cpp:2:                                                                  
 1:22.11 In file included from /home/landry/src/m-c/third_party/libwebrtc/modules/video_coding/codecs/av1/libaom_av1_encoder.cc:42:      
 1:22.12 In file included from /home/landry/src/m-c/third_party/libwebrtc/third_party/libaom/source/libaom/aom/aom_codec.h:22:           
 1:22.12 /usr/obj/m-c/dist/system_wrappers/aom/aom_codec.h:3:15: fatal error: 'aom/aom_codec.h' file not found                           
 1:22.12 #include_next <aom/aom_codec.h>                                                                                                 
 1:22.13               ^~~~~~~~~~~~~~~~~                                                                                                 
 1:22.19 1 error generated.

This is probably due to bug #1921154 landing in the meantime, as previously the tree built with the wip diff which didnt touch anything under third_party/libwebrtc.

Also, since adding the necessary bits so that CONFIG["MOZ_SYSTEM_LIBAOM_CFLAGS"] and CONFIG["MOZ_SYSTEM_DAV1D_CFLAGS"] end up in CPPFLAGS involves dealing with the gn machinery generating the moz.build files, im... not sure i want to lose my sanity there. Unless there's a way to circumvent this whack-a-mole ?

Flags: needinfo?(ngrunbaum)
Flags: needinfo?(mh+mozilla)

i guess i'll need to look at what had be done in bug #1875201 for --with-system-libvpx... and find out again how to generate the moz.build files from gn files.

my build apparently goes further after manually adding

if CONFIG["MOZ_SYSTEM_AV1"]:
    CXXFLAGS += CONFIG["MOZ_SYSTEM_LIBAOM_CFLAGS"]
    CXXFLAGS += CONFIG["MOZ_SYSTEM_DAV1D_CFLAGS"]

to third_party/libwebrtc/modules/video_coding/codecs/av1/dav1d_decoder_gn/moz.build and third_party/libwebrtc/modules/video_coding/codecs/av1/libaom_av1_encoder_gn/moz.build. Now i just need to figure out the right magic to make gn files do that, and in a way that doesnt break the build with the bundled dav1d.

no matter what i try in the mozbuild gn processor bits, it thinks im on android (?) and fails:

[13:17] c64:~/src/m-c/ $mach python python/mozbuild/mozbuild/gn_processor.py dom/media/webrtc/third_party_build/gn-configs/webrtc.json 
Loading preprocessor dom/media/webrtc/third_party_build/gn-configs/webrtc_preprocessor.py
Running "/usr/local/bin/gn gen /usr/obj/ports/tmp1kjxds23 --args=host_cpu="x64" is_debug=true target_cpu="x64" target_os="android" build_mozilla_webrtc=true concurrent_links=1 action_pool_depth=1 --ide=json --root=./ --dotfile=libwebrtc/.gn"
ERROR at //chromium/build/config/BUILDCONFIG.gn:222:3: Assertion failed.
  assert(host_os == "linux" || host_os == "mac",
  ^-----
Android builds are only supported on Linux and Mac hosts.

the wip diff i have is this:

diff --git a/python/mozbuild/mozbuild/gn_processor.py b/python/mozbuild/mozbuild/gn_processor.py
--- a/python/mozbuild/mozbuild/gn_processor.py
+++ b/python/mozbuild/mozbuild/gn_processor.py
@@ -570,16 +570,26 @@ def write_mozbuild(
                     relsrcdir
                     in write_mozbuild_variables["INCLUDE_SYSTEM_LIBVPX_HANDLING"]
                 ):
                     mb.write('if not CONFIG["MOZ_SYSTEM_LIBVPX"]:\n')
                     mb.write('    LOCAL_INCLUDES += [ "/media/libvpx/libvpx/" ]\n')
                     mb.write('    CXXFLAGS += CONFIG["MOZ_LIBVPX_CFLAGS"]\n')
             except KeyError:
                 pass
+            try:
+                if (
+                    relsrcdir
+                    in write_mozbuild_variables["INCLUDE_SYSTEM_DAV1D_HANDLING"]
+                ):
+                    mb.write('if CONFIG["MOZ_SYSTEM_AV1"]:\n')
+                    mb.write('    CXXFLAGS += CONFIG["MOZ_SYSTEM_DAV1D_CFLAGS"]\n')
+                    mb.write('    CXXFLAGS += CONFIG["MOZ_SYSTEM_LIBAOM_CFLAGS"]\n')
+            except KeyError:
+                pass
 
             all_args = [args for args, _ in configs]
 
             # Start with attributes that will be a part of the mozconfig
             # for every configuration, then factor by other potentially useful
             # combinations.
             # FIXME: this is a time-bomb. See bug 1775202.
             for attrs in (
diff --git a/dom/media/webrtc/third_party_build/gn-configs/webrtc.json b/dom/media/webrtc/third_party_build/gn-configs/webrtc.json
--- a/dom/media/webrtc/third_party_build/gn-configs/webrtc.json
+++ b/dom/media/webrtc/third_party_build/gn-configs/webrtc.json
@@ -12,16 +12,20 @@
   },
   "mozilla_flags": ["-fobjc-arc", "-mavx2", "-mfma", "-mfpu=neon", "-msse2"],
   "write_mozbuild_variables": {
     "INCLUDE_TK_CFLAGS_DIRS": [
       "third_party/libwebrtc/modules/desktop_capture/desktop_capture_gn",
       "third_party/libwebrtc/modules/portal/portal_gn",
       "third_party/libwebrtc/modules/video_capture/video_capture_internal_impl_gn"
     ],
+    "INCLUDE_SYSTEM_DAV1D_HANDLING": [
+      "third_party/libwebrtc/modules/video_coding/codecs/av1/dav1d_decoder_gn/",
+      "third_party/libwebrtc/modules/video_coding/codecs/av1/libaom_av1_encoder_gn/"
+    ],
     "INCLUDE_SYSTEM_LIBVPX_HANDLING": [
       "third_party/libwebrtc/modules/video_coding/webrtc_libvpx_interface_gn",
       "third_party/libwebrtc/modules/video_coding/webrtc_vp8_gn",
       "third_party/libwebrtc/modules/video_coding/webrtc_vp9_gn",
       "third_party/libwebrtc/third_party/libvpx/libvpx_gn"
     ]
   },
   "non_unified_sources": [


@@ -765,17 +775,17 @@ def main():
 
     with open(args.config, "r") as fh:
         config = json.load(fh)
 
     topsrcdir = Path(__file__).parent.parent.parent.parent.resolve()
 
     vars_set = []
     for is_debug in (True, False):
-        for target_os in ("android", "linux", "mac", "openbsd", "win"):
+        for target_os in ("linux", "mac", "openbsd", "win"):

allows mach python python/mozbuild/mozbuild/gn_processor.py dom/media/webrtc/third_party_build/gn-configs/webrtc.json to run... im a bit afraid at what will come out of it.

edit well, that drops wayys too many android-related bits from the generated moz.build files, so someone (tm) on linux with the android sdk will have to run it for me ?

No need for an android sdk. You just need a linux or macos system as it told you in comment 22

Flags: needinfo?(mh+mozilla)

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

No need for an android sdk. You just need a linux or macos system as it told you in comment 22

i ... dont have such a thing setupped for mozilla development.

oh well, apparently adding || host_os == 'openbsd' allows me to run the whole gn whack-a-mole :) guess i probably had the same the last time i had to deal with it.

diff --git a/third_party/chromium/build/config/BUILDCONFIG.gn b/third_party/chromium/build/config/BUILDCONFIG.gn
--- a/third_party/chromium/build/config/BUILDCONFIG.gn
+++ b/third_party/chromium/build/config/BUILDCONFIG.gn
@@ -214,17 +214,17 @@ if (host_toolchain == "") {
   } else {
     assert(false, "Unsupported host_os: $host_os")
   }
 }
 
 _default_toolchain = ""
 
 if (target_os == "android") {
-  assert(host_os == "linux" || host_os == "mac",
+  assert(host_os == "linux" || host_os == "mac" || host_os == "openbsd",
          "Android builds are only supported on Linux and Mac hosts.")
   _default_toolchain = "//chromium/build/toolchain/android:android_clang_$target_cpu"
 } else if (target_os == "chromeos" || target_os == "linux" || target_os == "openbsd") {
   # See comments in build/toolchain/cros/BUILD.gn about board compiles.
   if (is_clang) {
     _default_toolchain = "//chromium/build/toolchain/linux:clang_$target_cpu"
   } else {
     _default_toolchain = "//chromium/build/toolchain/linux:$target_cpu"

nah, false premature joy..

ERROR at //chromium/build/config/android/config.gni:303:5: Assertion failed.
    assert(false, "Need Android toolchain support for your build OS.")
    ^-----
Need Android toolchain support for your build OS.
See //chromium/build/config/sysroot.gni:34:5: whence it was imported.
    import("//chromium/build/config/android/config.gni")
    ^--------------------------------------------------
See //chromium/build/config/linux/pkg_config.gni:5:1: whence it was imported.
import("//chromium/build/config/sysroot.gni")
^-------------------------------------------
See //libwebrtc/BUILD.gn:24:1: whence it was imported.
import("//chromium/build/config/linux/pkg_config.gni")
^----------------------------------------------------
See //BUILD.gn:11:14: which caused the file to be included.
    deps = [ "libwebrtc:webrtc" ]
             ^-----------------
Traceback (most recent call last):

EDIT2 after more BUILDCONFIG.gn hacking i have something that works and generates the expected bits in the expected moz.build files. will do a pair of build with/without --with-system-av1 and post the diffs to phabricator tmrw hopefully

generated with

./mach python python/mozbuild/mozbuild/gn_processor.py
dom/media/webrtc/third_party_build/gn-configs/webrtc.json

and some hacks in third_party/chromium/build/config to run
gn_processor.py on OpenBSD

just for the record, this is the local diff i had to be able to regen the moz.build files:

--- a/third_party/chromium/build/config/BUILDCONFIG.gn
+++ b/third_party/chromium/build/config/BUILDCONFIG.gn
@@ -214,17 +214,17 @@ if (host_toolchain == "") {
   } else {
     assert(false, "Unsupported host_os: $host_os")
   }
 }
 
 _default_toolchain = ""
 
 if (target_os == "android") {
-  assert(host_os == "linux" || host_os == "mac",
+  assert(host_os == "linux" || host_os == "mac" || host_os == "openbsd",
          "Android builds are only supported on Linux and Mac hosts.")
   _default_toolchain = "//chromium/build/toolchain/android:android_clang_$target_cpu"
 } else if (target_os == "chromeos" || target_os == "linux" || target_os == "openbsd") {
   # See comments in build/toolchain/cros/BUILD.gn about board compiles.
   if (is_clang) {
     _default_toolchain = "//chromium/build/toolchain/linux:clang_$target_cpu"
   } else {
     _default_toolchain = "//chromium/build/toolchain/linux:$target_cpu"
diff --git a/third_party/chromium/build/config/android/config.gni b/third_party/chromium/build/config/android/config.gni
--- a/third_party/chromium/build/config/android/config.gni
+++ b/third_party/chromium/build/config/android/config.gni
@@ -295,17 +295,18 @@ if (is_android || is_chromeos) {
 
   # Defines the name the Android build gives to the current host CPU
   # architecture, which is different than the names GN uses.
   if (host_os == "linux") {
     android_host_os = "linux"
   } else if (host_os == "mac") {
     android_host_os = "darwin"
   } else {
-    assert(false, "Need Android toolchain support for your build OS.")
+    android_host_os = "linux"
+    # assert(false, "Need Android toolchain support for your build OS.")
   }
 
   # Directories and files ------------------------------------------------------
   #
   # We define may of the dirs strings here for each output architecture (rather
   # than just the current one) since these are needed by the Android toolchain
   # file to define toolchains for all possible targets in one pass.

thanks :glandium: for the x-mas r+ on https://phabricator.services.mozilla.com/D232488, can you have a final look at https://phabricator.services.mozilla.com/D34921 so that we can try getting everything landed before the next 135 cycle ?

Flags: needinfo?(mh+mozilla)
Flags: needinfo?(mh+mozilla)

can someone with lando access push those accepted patches ? i dont have access due to whatever more mfa madness being required there ..

Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/6ca0fbdbcdfc Allow to use system av1 libs instead of bundled r=glandium,media-playback-reviewers,aosmond,stransky,alwu
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/63d0e7430da5 add method to conditionally include system av1 headers r=ng,glandium,mjf https://hg.mozilla.org/integration/autoland/rev/b55ed69b8e36 commit generated moz.build files r=ng,webrtc-reviewers

https://hg.mozilla.org/mozilla-central/rev/6ca0fbdbcdfc
https://hg.mozilla.org/mozilla-central/rev/63d0e7430da5
https://hg.mozilla.org/mozilla-central/rev/b55ed69b8e36

Status: ASSIGNED → RESOLVED
Closed: 11 months ago
status-firefox136: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 136 Branch
Assignee: stransky → landry
Flags: needinfo?(ngrunbaum)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: