Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Meson for mpv instead of Waf #2359

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Conversation

RivenSkaye
Copy link
Contributor

Pull request that translates Waf options to their Meson equivalents at runtime, fixing #2346
Also ensures any flags guaranteed to be illegal on Windows are disabled, with a warning sent to the user.

Still has one call to waf distclean, since that doesn't hang up and will allow for users to keep using existing envs. That call can be removed later, when it can be reasonably assumed nobody who's remotely up-to-date has Waf artifacts left that might interfere with the build.
A side effect of this change is the removal of the option to build mruby support into mpv, as the relevant branch has been dead since 2017 and can't be built with Meson.
Work on this also brought to light something else, due to Meson being a lot less forgiving when it comes to impossible combinations and configure flags. There is currently no way to explicitly enable d3d11 and spirv-cross flags due to #2358 blocking the build of spirv-cross-c-shared. It seems like all of the Vulkan stuff that relies on SPIR-V works so long as the shared tools are built, which still happens with the spirv-meson patch.

I'm opening this PR as a draft because this still needs some considerable cleanup. Things like the extra LDFLAGS being set in order to allow linking mpv to libarchive without undefined symbol references. I'm not exactly sure why these changes were needed to make mpv build properly, as I got it to work just fine standalone. This only seemed to be a problem within the context of the Suite.

elif [[ $option =~ ^--enable-(cocoa|drm|egl-android|*-drm|*-wayland|*-x11|*-cocoa|gbm|rpi|vdpau|vdpau-*|vaapi|vaapi-*|wayland|x11|xv) ]]; then
meson_opts+=("${option/--enable-/-D}=disabled")
do_simple_print "${orange}Disabling option '${option}' as it's not usable on Windows"'!'"${reset}"
elif [[ $option =~ ^--enable-(caca|d3d11|direct3d|egl|egl-*|plain-gl|gl|gl-*|jpeg|libplacebo|sdl2-video|shaderc|sixel|spirv-cross|vulkan) ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to change this one a bit, as the suite doesn't include sixel (yet) and there's no release that can be grabbed from the repos

meson_opts+=("${option/--enable-/-D}=enabled")
elif [[ $option =~ ^--disable-(html-build|manpage-build) ]]; then
meson_opts+=("${option/--disable-/-D}=disabled")
elif [[ $option =~ ^--enable-pdf-build ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remnant of the old code, which silently and unconditionally disabled PDF doc builds, but this needs to be tested

Copy link
Contributor Author

@RivenSkaye RivenSkaye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial feedback for myself, some questions for the maintainers.
I turned on maintainer edits for the PR, so if there's things you'd like to tackle, feel free to add it to the PR

RST2PDF="${MINGW_PREFIX}/bin/rst2pdf2"
mkdir build
PKG_CONFIG="pkgconf --keep-system-libs --keep-system-cflags" CC=${CC/ccache /}.bat CXX=${CXX/ccache /}.bat \
log "meson.setup" meson setup build "${meson_opts[@]}" --prefix="${LOCALDESTDIR}" --bindir="${LOCALDESTDIR}/bin-video" --default-library="${default_lib}" --buildtype=release
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log names are non-standard atm, but I expect there to be more changes required to the Meson calls.
Kept this order for now so that it

  1. doesn't break existing mpv_extra.sh setups
  2. I wanted it to work first, and then adapt it to use the helper functions later

log install /usr/bin/python waf -j1 install ||
log install /usr/bin/python waf -j1 install
cpuCount=1 log "meson.install" meson install -C build &&
mv "${LOCALDESTDIR}/bin-video/libmpv-2.dll" "$LOCALDESTDIR/bin-video/mpv-2.dll"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find a way to tell Meson I don't want the output file to be named libmpv-2.dll, hence the manual move

LDFLAGS="$old_ldf"
replace=" ${mpv_cflags[*]} -Wno-int-conversion"
CFLAGS="${CFLAGS/$replace}"
unset mpv_ldflags replace PKGCONF_STATIC meson_opts default_lib old_ldf
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely happy with this cleanup, but it works:tm:
I'll look into extra-cflags options and similar for Meson

for option in "${MPV_OPTS[@]}"; do
# Process Waf flags into Meson options.
# starting with boolean flags
if [[ $option =~ ^--enable-(gpl|cplayer|build-date|tests|ta-leak-report) ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TA Leak can only be turned off by also adjusting an env var; perhaps make some accessible entry point for people to inject the option from a custom patch instead of setting it here?

elif [[ $option =~ ^--enable-(cocoa|drm|egl-android|*-drm|*-wayland|*-x11|*-cocoa|gbm|rpi|vdpau|vdpau-*|vaapi|vaapi-*|wayland|x11|xv) ]]; then
meson_opts+=("${option/--enable-/-D}=disabled")
do_simple_print "${orange}Disabling option '${option}' as it's not usable on Windows"'!'"${reset}"
elif [[ $option =~ ^--enable-(caca|d3d11|direct3d|egl|egl-*|plain-gl|gl|gl-*|jpeg|libplacebo|sdl2-video|shaderc|sixel|spirv-cross|vulkan) ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also some things to note about SPIRV-Cross, see issue linked in the initial comment on the PR

@Quackdoc
Copy link

Quackdoc commented Jan 13, 2023

PR to deprecate waf in mpv

mpv-player/mpv#11143

@vt-idiot
Copy link
Contributor

vt-idiot commented Apr 19, 2024

Does this work?

It might. I am fairly certain openal will have to be explicitly disabled and the meson options actually have it disabled by default

mpv added some more fluff to make wasapi work better, but I think some ifndefs are missing. I can't really see a reason to use OpenAL over WASAPI on Windows to begin with...

FAILED: mpv.exe 
"gcc.bat" @mpv.exe.rsp
J:/media-autobuild_suite-master/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/13.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: J:/media-autobuild_suite-master/local64/lib/libopenal.a(uiddefs.cpp.o):uiddefs.cpp:(.rdata$KSDATAFORMAT_SUBTYPE_IEEE_FLOAT[KSDATAFORMAT_SUBTYPE_IEEE_FLOAT]+0x0): multiple definition of `KSDATAFORMAT_SUBTYPE_IEEE_FLOAT'; libmpv-2.dll.p/audio_out_ao_wasapi_utils.c.obj:ao_wasapi_util:(.rdata+0xcb0): first defined here
J:/media-autobuild_suite-master/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/13.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: J:/media-autobuild_suite-master/local64/lib/libopenal.a(uiddefs.cpp.o):uiddefs.cpp:(.rdata$KSDATAFORMAT_SUBTYPE_PCM[KSDATAFORMAT_SUBTYPE_PCM]+0x0): multiple definition of `KSDATAFORMAT_SUBTYPE_PCM'; libmpv-2.dll.p/audio_out_ao_wasapi_utils.c.obj:ao_wasapi_util:(.rdata+0xcc0): first defined here

collect2.exe: error: ld returned 1 exit status
[265/265] Linking target libmpv-2.dll
FAILED: libmpv-2.dll 
"gcc.bat" @libmpv-2.dll.rsp
J:/media-autobuild_suite-master/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/13.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: J:/media-autobuild_suite-master/local64/lib/libvulkan.a(loader_windows.c.obj):loader_windows:(.text+0x270): multiple definition of `DllMain'; J:/media-autobuild_suite-master/msys64/mingw64/lib/libmingwex.a(lib64_libmingwex_a-dllmain.o):C:/M/B/src/mingw-w64/mingw-w64-crt/crt/dllmain.c:10: first defined here
J:/media-autobuild_suite-master/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/13.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: J:/media-autobuild_suite-master/local64/lib/libopenal.a(uiddefs.cpp.o):uiddefs.cpp:(.rdata$KSDATAFORMAT_SUBTYPE_IEEE_FLOAT[KSDATAFORMAT_SUBTYPE_IEEE_FLOAT]+0x0): multiple definition of `KSDATAFORMAT_SUBTYPE_IEEE_FLOAT'; libmpv-2.dll.p/audio_out_ao_wasapi_utils.c.obj:ao_wasapi_util:(.rdata+0xcb0): first defined here
J:/media-autobuild_suite-master/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/13.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: J:/media-autobuild_suite-master/local64/lib/libopenal.a(uiddefs.cpp.o):uiddefs.cpp:(.rdata$KSDATAFORMAT_SUBTYPE_PCM[KSDATAFORMAT_SUBTYPE_PCM]+0x0): multiple definition of `KSDATAFORMAT_SUBTYPE_PCM'; libmpv-2.dll.p/audio_out_ao_wasapi_utils.c.obj:ao_wasapi_util:(.rdata+0xcc0): first defined here

collect2.exe: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
INFO: autodetecting backend as ninja
INFO: calculating backend command to run: J:\media-autobuild_suite-master\msys64\mingw64\bin/ninja.EXE -C J:/media-autobuild_suite-master/build/mpv/build

This one might be a showstopper though

J:/media-autobuild_suite-master/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/13.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: J:/media-autobuild_suite-master/local64/lib/libvulkan.a(loader_windows.c.obj):loader_windows:(.text+0x270): multiple definition of `DllMain'; J:/media-autobuild_suite-master/msys64/mingw64/lib/libmingwex.a(lib64_libmingwex_a-dllmain.o):C:/M/B/src/mingw-w64/mingw-w64-crt/crt/dllmain.c:10: first defined here

@RivenSkaye
Copy link
Contributor Author

RivenSkaye commented Apr 19, 2024

I mean, last I touched it I managed to produce a build. If that counts as working, then yes. But for all intents and purposes it needs a fair amount of TLC and I currently just cannot spare the time to wrangle with upstream changes on mpv at the moment

@vt-idiot
Copy link
Contributor

I did get past the OpenAL issues by disabling it in ffmpeg, rebuilding, and going again. libvulkan's DllMain is going to be an issue for building libmpv-2.dll unless there's a different way of patching vulkan-loader. I put a couple hours into it and couldn't make heads or tails of it.

@RivenSkaye
Copy link
Contributor Author

Huh, last time I ran a build off upstream I couldn't get vulkan to work properly either, and it has also been a bit iffy on Arch to get everything to play nice.

And yes, spending a few hours on it sounds like pretty much every step of the way I already had into this PR. Hence why time is an issue, I'd almost miss my student years with no household of my own to run...

@vt-idiot
Copy link
Contributor

Might be worth looking at https://github.com/shinchiro/mpv-winbuild-cmake/tree/master/packages if you ever get the chance.

@1480c1
Copy link
Member

1480c1 commented Apr 20, 2024

I'm thinking perhaps we just go the nuke route and just move the old option file and make a new one.

The main thing I'm concerned about is the whole enable/disable and using that logic with stuff that might not adhere to it with meson, like instead of enable/disable, some options might be bools. We may instead need to introduce a different set of stuff for meson and like mpv_enable() and mpv_true() or something with different arrays for the different types.

@vt-idiot
Copy link
Contributor

I'm thinking perhaps we just go the nuke route and just move the old option file and make a new one.

Not a bad idea. One issue (maybe 2?) I did run into is that even in a fresh environment, libvulkan causes it to fail because of the DllMain definition.

Also the way spirv-cross currently builds probably still needs to be addressed, I'm not sure why it still uses all that meson patchwork when there is now a build system in-place upstream.

@1480c1
Copy link
Member

1480c1 commented May 2, 2024

realized there may be more pains with meson considering the user could do something like

-D option=enable

Need to do a lot more worrying about parsing on bash's side, past just slurping up the arguments

@RivenSkaye
Copy link
Contributor Author

That's why my current rough draft is just parsing the existing options files. I say we butcher the config generation in three places.

  1. Initial setup, where we only write non-default features (and explicit non-options due to Windows constraints)
  2. Before we finalize setup and start the first build, based on relevant environment variables and args passed with a -D prefix
  3. Before the actual build of mpv itself, once all patches and additional user scripts have ran and set their info. If this changes settings in a breaking way, we ask the user to check their custom scripts.

If we don't yet, it might be worthwhile to set a flag to see if the user added any custom scripts (perhaps echo `cygpath -wa $script_path` >> .userscripts.log or something?)

We'd then probably end up with a glorified commandline in a file, but we at least wouldn't have to worry about parsing the contents anymore

@RivenSkaye
Copy link
Contributor Author

Burned a few hours trying to merge upstream changes into my fork, but the amount of work needed to get it merged is a nightmare and all it takes is a small mistake to leave the script in a horribly broken state.
At this point I'm wondering if it would be better to clean my fork and redo my changes, taking into account other changes made since I opened the PR, or if I should somehow piece together the merge so I can move forward on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants