Cocoa restructuring work for libmpv #721
Conversation
|
Hello I still have to review your code. For embedding, even if we are in the same process, maybe creating a custom vo_opengl backend that renders to IOSurface could be the way that gives most freedom to someone embedding mpv? I think we should also provide a way to simply embed the mpv openglview in another view or window (similar to --wid on other platforms). How would you want to structure the video rendering in the client application? |
|
Yep, please do review it! That's why it's here. I want to go the IOSurface route, yeah. Should the IOSurface pointer be passed through the client API? Video rendering happens on a textured quad in an OpenGL buffer. |
Yes, somehow it needs to be passed through. I see some possibilities:
All of these are fine for now. By the way, maybe we need a ringbuffer of IOSurfaces, instead of a single one. Not sure yet. Also, I think it would be nice if we could add support for DisplayLink at one point for presentation - I'm not sure how this would affect the client API. I'm still thinking that we should maybe export some sort of cocoa widget, but I don't know enough about cocoa to know how reasonable this is. |
|
DisplayLink is an interesting tangent. I'm guessing the display link needs to be bound to the client's GL context with Where does the --wid code live? Also, 2) or 3) make sense, because I think there's some extra data like pixel formats we'll want to be passing too. Passing an IOSurface around as a number scares me a little since it's refcounted, but careful policy could deal with that I suppose. |
Nowhere. It's just an int64 option that sets |
|
@pigoz A custom vo_opengl backend sounds good. Or can corevideo+vdpau work, since that produces the IOSurface? I don't know which is easier to integrate and maintain. |
What does vdpau do here? |
|
Whoops, I meant vda-hwaccel. I'm confusing all these formats. |
|
vo_corevideo is useless. It will be removed the next time it is problematic :) |
|
Ah, good to know!
|
|
Any news? |
|
Well, the GL integration is going to take a lot of figuring out and time... the code in this PR could use a review in the meantime though! |
|
This is the target to generate libmpv: https://github.com/frau/mpv/blob/0e0f59cd5af45d34296fcec4762da81e8639740b/wscript_build.py#L445 As you can see, it doesn't add main_fn.c to the sources. What problem are you running into exactly? (Sorry, lost track of the context.) |
|
@wm4 If I revert commit b6f8017 on my branch, then Is main_fn getting linked in because both ctx() calls have target="mpv"? I'm not really familiar with waf. What's the proper way to do this? |
|
Note that the command line mpv is also built in this mode, with a different target (and separate object files etc.!). Maybe that's the cause. The configure stuff in wscript doesn't really separate them. If it's not possible to do so properly, then we'll probably have make building the command line player and libmpv completely separate, i.e. you should add an if that enables either the command line mpv or libmpv, but never both. Sorry for the mess. |
ghost
mentioned this pull request
|
Right, uh, isn't that what b6f8017 accomplishes? With --enable-libmpv-shared being the toggle? |
Unfortunately, you're right. (I guess my thought process went full-circle.) |
|
FWIW, making it impossible to build mpv and libmpv at the same time will make the life of Linux packagers who want both (i.e. me) harder. Could it be made os x specific if it really needs to be done? |
|
I don't think them be mutually exclusive to be beneficial to any platform. |
Not sure if this was the same problem, but reverting 21287ca too fixed the build for me. |
|
...on Linux that is. |
|
We probably need something to build the two targets (mpv and libmpv) with different configuration. Could this be fixed with some waf magic? Currently, they produce a different set of object files anyway, so they're already very different. They merely share the configuration (config.h) and the file list. |
|
For targets you mean run configure for libmpv and run configure for mpv itself and build them separately? |
|
Yes, essentially. |
|
Ping? |
|
I'm back! 🔉 Rebased the PR onto master, dealt with bitrot, and removed that controversial main_fn.c-omitting commit. As a result, |
|
Welcome back... I hope pigoz can take a look. |
|
Hello! Welcome back. Take a look at the branch pr/721, it should solve your problems (and can also be used by mpv developers to easily develop against different versions of FFmpeg or other libraries without having to rerun configure and build steps all the time). Example usage would be: |
|
I went another route and instead of hiding It seems to work fine here (linux), altough maybe a bit ugly (it require the |
"global" as in non-static, it's already a global variable... |
|
@ghedo if you see my branch, I reverted that particular commit which hides mpv_main since it's not needed at all. |
|
Oh, right... I missed it (I only looked at the variants thingy). Still though, |
|
@pigoz Awesome! Thanks for the waf work. That is nice. I will try building my sample project against it later today. |
I had to put it after video options because it depends on cocoa; is there anywhere better to put it rather than making a new group?
This builds but both the libmpv example and the cplayer block infinitely when building libmpv. That's because we wait inifinitely in `dispatch_sync` as there's no event loop in the main thread that allows for libdispatch to work.. Whiel we are at it, we should probably investigate how to use mp_dispatch instead since it is a little lower level and could give us higher control in building and event loop.
This allows the user to execute multiple configuration and build steps. It can be used for several scenarios where you need different compiler flags.
|
So, great! pr/721 works for my sample app. Rebased it into this PR, removing the commit-revert pair, and doing a minor reorg. So, how can we get this merged? Do we need to deal with those Cocoa deadlocks first, or can those be dealt with in other PRs later? Or do we need to write the whole feature end-to-end before merging? |
|
@frau please squash 1e5df72 with e621967. We can merge immediately, the only issue is that using the default variant from |
|
@frau btw I'd love to see your sample application (does it deadlock)? I am thinking about some major restructuring of the Cocoa parts of mpv and it would be cool to have an idea how are you using it. |
|
The zsh completion script works fine here with the patches applied, and mpv-build uses |
|
@pigoz Okay, squashed! The sample project is not really different than the client api example shipped with MPV, and is just for making sure that I can actually link to libmpv and play a video; the basic source is at https://gist.github.com/frau/10841304 My OpenGL stuff is nowhere near ready for integrating with mpv, but maybe soon... |
|
I guess I will merge the branch this evening or tomorrow morning unless someone objects ^^ |
That sounds like a very bad idea. The old build system just put the mpv binary into the top-level directory. Everyone could find it. Then waf moved it to build/... That's already slightly harder to find. And now it's build/default/? I don't think a user would even expect it to find it there. What is "default" even? And how often is the binary path going to change? Will it be in build/default/no/look/harder/ next? Short version: no, I don't like it. |
|
Well it's the name of the default variant. We can use empty string Would that be better / acceptable? |
Sounds like it'd be the same as before. So, very acceptable. |
|
Hmm... the more I work on integrating with OpenGL, the more it feels like that I'm just going to replace all |
|
I pushed everything after fixing the issues @wm4 suggested. I'm going to close this PR. Please open a new issue (even just for discussion) for the OpenGL changes! |
|
🎊 Thanks all! |
Here's some preliminary work for your consideration. It's not ready to merge, just a progress report.
With these changes, I can link and run a super simple client test that plays video (in a window created by mpv though, not the client's window) and the audio.
Now the question I have is how to rework cocoa_common to accommodate libmpv... cocoa_common is full of standalone-specific stuff, like most of
struct vo_cocoa_state, that libmpv isn't going to want.I want to be able to access the IOSurface thatHave to pass the GL texture through the client API somehow.gl_cocoagenerates. Not sure what a good way forward is from here.This is part of #556.