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

[RFC] modular driver support #11145

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

[RFC] modular driver support #11145

wants to merge 10 commits into from

Conversation

t-8ch
Copy link
Contributor

@t-8ch t-8ch commented Jan 14, 2023

This implements modular drivers. These allow shipping a binary mpv package that provides all drivers but does not require them to be installed.

The commits are split logically for easier review.

Limitations:

  • only AOs supported in this PoC
  • Not all AOs supported, pipewire and jack were chosen at random
  • The logging in osdep/module-posix.c seems not to work
  • only POSIX platforms are supported
  • only meson is supported
  • All modules are loaded very early and eagerly. Could be changed to load lazily in the future
  • No validation is done if the modules do match the binary

Fixes #11138

@t-8ch t-8ch changed the title [WIP] modular driver supports [WIP] modular driver support Jan 14, 2023
@t-8ch
Copy link
Contributor Author

t-8ch commented Jan 16, 2023

I think this can be reviewed.

@t-8ch t-8ch marked this pull request as ready for review January 16, 2023 19:36
@t-8ch
Copy link
Contributor Author

t-8ch commented Jan 17, 2023

Changes in current version:

  • Make the option a meson feature instead of a plain boolean
  • Instead of renaming the driver struct itself with a macro just create a pointer with a well-known name.

@t-8ch
Copy link
Contributor Author

t-8ch commented Jan 17, 2023

Logging now works, too.

@t-8ch t-8ch force-pushed the modules branch 6 times, most recently from 0ed8a82 to 3133aef Compare January 19, 2023 16:08
@t-8ch t-8ch changed the title [WIP] modular driver support [RFC] modular driver support Jan 27, 2023
@t-8ch t-8ch requested a review from Dudemanguy January 27, 2023 04:08
@t-8ch
Copy link
Contributor Author

t-8ch commented Jan 27, 2023

@philipl maybe you are interested in taking a look at this? If not feel free to ignore.

@philipl
Copy link
Member

philipl commented Jan 27, 2023

I think this broke when combined with #11216. The ao modules won't load due to missing symbols. I guess it's because of the default hidden visibility of the symbols. You'd need to export more symbols from the lib, or handle them in some other way to make them accessible to modules without being visible from libmpv.

@Dudemanguy
Copy link
Member

or handle them in some other way to make them accessible to modules without being visible from libmpv.

That would be preferred. Worst case scenario, we would just have to build these files twice again (once for libmpv and again for cplayer).

@philipl
Copy link
Member

philipl commented Jan 28, 2023

There will probably be handful of files that need to be compiled with different symbol visibility, so it will still allow most of the files to benefit from being compiled once.

@t-8ch
Copy link
Contributor Author

t-8ch commented Jan 28, 2023

we would just have to build these files twice again

I also want this feature to work in libmpv, so one compilation should still be possible.

Also if mpv consists of multiple libraries anyways (because of the modules) it wouldn't make a difference to ship cplayer as just another client of libmpv.

@philipl
Copy link
Member

philipl commented Feb 2, 2023

Ok. Then can you update this PR to export the necessary symbols from libmpv so we can see what it looks like in practice? Thanks.

@t-8ch
Copy link
Contributor Author

t-8ch commented Feb 2, 2023

I am trying to think of something better.

Manually having to export random parts of the internals sounds like a nightmare to maintain.

@t-8ch
Copy link
Contributor Author

t-8ch commented Feb 8, 2023

What about this:

libmpv.so becomes libmpvcore.so and exports all symbols.

A new shim libmpv.so is added that loads libmpvcore.so via dlopen(RTLD_LOCAL)/dlmopen().
This shim exports all MPV_EXPORT symbols, probably via generated stubs during build (using pycparser).
These stub functions then call into the real functions from libmpvcore.so.

This gives us:

  • Only external API symbols are exported form libmpv.so
  • VO and AO authors do not have to care about which internal symbols they are using
  • Can be completely handled in the buildsystem, no code changes are necessary (for this specific aspect)

Disadvantages:

  • More complexity in the buildsystem
  • Introduces another function pointer indirection. But there are already plenty of those.

@Dudemanguy
Copy link
Member

Would it be easier to build the executable first and extract the objects from there to build libmpv?

@t-8ch
Copy link
Contributor Author

t-8ch commented Feb 8, 2023

Would it be easier to build the executable first and extract the objects from there to build libmpv?

Then all the symbols would be exported to users of libmpv.
Which could lead to conflicts or users depending on mpv internals.

Also I think there would be issues with PIE/PIC.

@Dudemanguy
Copy link
Member

Dudemanguy commented Feb 8, 2023

Shouldn't the gnu_symbol_visibility handle that? I thought the whole reason the reusing objects PR breaking this was because the library function didn't export all the symbols. Or I guess you're saying that extracting the objects automatically means that all of the symbols always get exported.

@t-8ch
Copy link
Contributor Author

t-8ch commented Feb 9, 2023

The problem is that we need both at the same time: no internal symbols exported to users of libmpv; (all) internal symbols exported to the modules.

(let's focus on libmpv for now, cplayer is easy)

@t-8ch
Copy link
Contributor Author

t-8ch commented Feb 16, 2023

The new version introduces the proposed shim library.
It will need to be made more robust but should be enough for a PoC and design feedback.

@t-8ch t-8ch force-pushed the modules branch 3 times, most recently from b6cccb1 to f75e2ec Compare February 18, 2023 19:09
@t-8ch
Copy link
Contributor Author

t-8ch commented Feb 19, 2023

The aproach with dlopen(libmpvcore, RTLD_LOCAL) which in turn does dlopen(driver.so, RTLD_LOCAL) does not work because the drivers still can't see libmpvcores symbols.

Doing dlmopen(LM_ID_NEW, ...) does also not work because of https://sourceware.org/bugzilla/show_bug.cgi?id=18684 .

Back to the drawing board.

@t-8ch t-8ch force-pushed the modules branch 4 times, most recently from 49b9088 to f4269fa Compare February 25, 2023 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFE: modular drivers (AOs, VOs, ...)
3 participants