win32: add Media Control support #14338
Conversation
kasper93
force-pushed
the
smtc
branch
2 times, most recently
from
08572fa
to
1cfa0a2
Compare
|
Download the artifacts for this pull request: |
|
TO fix #14007 ? |
|
Side note, it is possible to make it work in C using only COM interfaces, but it results in significantly more boilerplate code and is completely not worth the effort. As always patches welcome, if someone wants to rewrite it. |
|
Since this implementation is just a libmpv client, can you consider making it a C plugin instead? This would avoid adding any dependency to mpv core. |
kasper93
force-pushed
the
smtc
branch
2 times, most recently
from
c0cd6f5
to
fac1869
Compare
I've considered it, but since it is pretty core player functionality to integrate into system and macOS does the same. I decided to integrate it inside. |
In this case I would expect proper integration. At least the thumbnail needs to work, which doesn't seem to be the case for now. If this is not possible from a libmpv client, a different approach is needed. |
Thumbnails are not supported on macOS either, I don't think this is a blocker. Generally thumbnail support in mpv is another big topic and out of the scope of this PR. Note that we don't support thumbnails on OSC timeline either. Either way, thumbnail or more likely periodic screenshot of current frame is possible in the future. (and not that difficult) |
kasper93
force-pushed
the
smtc
branch
2 times, most recently
from
7e6fbd8
to
0d36d87
Compare
|
Currently the SMTC interface in Windows 11 doesn't display the mpv icon and program name. The built-in Windows media player does that, and once the Windows media player is run and closed, mpv is stuck with the wrong program icon and name. |
It works the same way with Firefox, if you have ideas how Windows expects this information to be given, let me know. It clearly doesn't get that information from hwnd. EDIT: It is possible that it works only for installed application that have app id defined. |
I have experimented a little and it works pretty good now. I need to glue everything together, because there are multiple cases, local files, external files, embedded files, etc. I will finish it when I get time, but you can expect proper support.' EDIT: To be honest, I find thumbnail itself not that useful. Compared to other features of this PR, so not sure why this is required, but it will work. |
From a look at the API this seems to be the case.
Personally I expect this feature to behave in a similar way as on mobile platforms. So I at least expect album arts to work. For videos, I find most apps don't attempt to do this, or only show a random frame, so missing thumbnail isn't too bad. The above problem of icon and program name might also be alleviated with the "thumbnail" by displaying the mpv icon when no thumbnail is applicable. |
|
Perhaps related, would it be possible to add thumbnail-toolbars(prev,play/pause,next) to the taskbar?
|
I found it "cheap", if you get what I mean. Like pushing logo in the place where it shouldn't have been.
I've taken a look and I'm almost certain they are getting app info for installed packages. Something like // id for mpv is `mpv.exe`
using winrt::Windows::ApplicationModel::AppInfo;
using winrt::Windows::Foundation::Size;
auto app = AppInfo::GetFromAppUserModelId(id);
auto di = app.DisplayInfo();
auto name = di.DisplayName();
auto logo = di.GetLogo(Size(96, 96));There is no app registered for mpv. There are possibilities like https://blogs.windows.com/windowsdeveloper/2019/10/29/identity-registration-and-activation-of-non-packaged-win32-apps/ but it still would need to be installed and have dedicated package. I don't think it is feasible for mpv in any way.
No, this is completely not related. This is completely different interface, with basically custom buttons that you can put there. I'm not interested in supporting this. Maybe one day, this taskbar stuff could be merged into one place, but this is out of the scope of this PR. |
But this is feasible for libmpv. In addition, it is possible to use |
|
In this case it will possibly work or not. I don't know. |
I don't aim directly towards you. Just describing the situation. As for macOS comparison, I compared mostly the usage of client api. Which I think it a good thing, as it abstract a lot of synchronization details that would have to be handled otherwise. |
|
C++ is not some meme language and all of us already have c++ compilers installed anyway so I have no problem with it being in the codebase if it's needed for something. I don't know anything about windows so I don't think my review here would be of any real use. |
It doesn't increase the complexity on the user side if properly done. VLC for example has everything as plugins, yet it doesn't affect its "just works" reputation because they are all bundled in the official distribution. mplayer also had lots of parts "modular" by making About C++ usage, in this particular case it's well separated and optional, and is needed to reasonably interface with OS APIs, so I think it's OK. But I would avoid adding it to platform-independent code for now, especially since a significant portion of *nix users would be certainly driven away by it. |
Well of course. I'm saying in the context of current mpv ecosystem. We do not provide any official binaries/packages. All plugins are fully on the user side to find/copy/install. Also in this case it would be cplugin, so user has to build it or download some pre-compiled binary. I think it is clear that the process is not that accessible. Especially for Windows users, who generally are less tech-savvy, than *nix users. Of course there are solutions to everything I say here, but we should take and step back first and think what is the real problem that we are solving here. About the modularization of mpv. It would be possible, but in the same time not sure there is much gain. We are just to small to partition things into smaller entities. And wm4 really disliked this idea, looking back at his opinion about libplacebo.
Agreed, generally mpv structure doesn't fit C++, it is C code base at is heart. Usage of it should be limited to where strictly beneficial, like imho in the current PR. Spreading this further is not on the table. |
|
Almost every windows user is using mpv.exe single static executable, which makes mpv/libmpv easy to distribute and maintain, so we want to avoid non-system dlls as much as possible, especially for core media functions like SMTC. |
No offense to anyone who might feel strongly about this, but I think "significant portion" is an exaggeration. Additionally, libplacebo already uses C++, so I hope this "significant portion" of users are already not using mpv, for better or worse. Sorry. |
|
My post was mostly about potential contributors. I doubt normal users care about this much. *nix has most of its API in C so modern C++ knowledge isn't required to make the most use out of *nix. If mpv code were all converted to modern C++ they will simply stop contributing. Just saying this since most mpv developers have *nix background.
It's also in a limited fashion, and doesn't make the rest of code require modern C++ knowledge for contribution. |
Fair, but no one suggesting that. As you noticed *nix has most of its API in C, so it is natural to use C as a base language.
Exactly, and that's the idea. Everything needs context when talking about those things. |
It solves no problems. The point of this discussion is how careful we want to be with pulling bigger dependencies into the mpv codebase.
Sure, that's good. I didn't check the changes closely before commenting.
Well if I look into smtc.cc right now I can see that it includes a bunch of internal headers instead of just
The inconvenience relating to mpris is that AFAIR interacting with dbus requires pulling in big dependencies and there's no good standalone library for it.
Aside from technical limitations that make this the only workable solution I think this argument misses its mark. mpv-android is an mpv frontend and we're discussing what to include in mpv itself here.
This is indeed the main question and we don't even have any design or direction document on it. My personal opinion is that mpv is something inbetween. Pretty bare-bones with some obvious tweaks/plugins/scripts you can add to make it into more of a full featured player. (Though this was more true years ago than it is today).
I don't have a strong opinion on whether mpv should or should not ship this feature, but as someone who's been with mpv for a few years my aim is for mpv to keep its identity the way it was.
I suppose convenience of distribution was considered important, since on Unix it's not unusual at all for a program to ship data files separately and to require them to run.
Good point. I think it's due to the close interworking that is required for some formats.
But as @na-na-hi now pointed out this is not necessarily a conflict. These things could be separated and be the default. |
I had that in mind, which is why it mostly uses the standard client.h and a few helper functions, like thread, path normalization, and node helpers. I wanted to keep also the style of mpv code, even though it is C++. I would easily use std::thread, but since it is internal currently I used our helpers as much as possible. Side note, I think node.h should even be a public header for libmpv, as they are nice wrappers over the node_map thing.
Understandable. I was also a little reluctant with cppwinrt, even though it is header-only. While I would prefer to avoid it, as I mentioned before, it greatly simplifies our code.
¯\(ツ)/¯ I get the minimalism, but at the same time, it shouldn't hold us back on features. Generally, I think we should follow common sense. In this case, I don't think the burden of the added code/dependency is significant. It may look like it, but in practice, it's just another language mode and a few headers. Of course, it would be different if it added Boost or Qt as a dependency or something equally insane. To complement this, SMTC plugins actually exist for mpv, although I have never tested any of them. Since there was no cplugin support for mpv on Windows, they resort to a lua script talking to another application that talks to the SMTC. I feel like this is completely unnecessary bloat. Is it required to be inside mpv.exe? No, but I think it is a cleaner way to handle it, both from a code and user perspective. I would expect mpv.exe to be a minimalistic yet powerful player.
This sounds perfectly reasonable, and I don't want to be this guy who topples the sandcastle built over the years. At the same time, I think we should evaluate things individually as they come and decide accordingly. That said, I'm okay with moving this to a plugin in another repository separate from mpv. I would prefer to have it inside mpv, but if that's not compatible with mpv core values, I'm okay with that too. My responses before were mostly motivated by a desire to understand the reasoning behind this.
It's similar on Windows, where mpv is often distributed as a single binary. To make it easier, it would require an installer, which is the common Windows way to install programs with their dependencies. However, we already know that we are not going to ship or maintain any packaging solutions. Hence, having it directly inside is for convenience of distribution too.
I think the main reason is that it is a highly desired feature and makes mpv much more convenient to use with internet videos. Some features just deserve to be included even in a minimalistic player video. |
- Move C only warnings to own test - Add missing native arg for add_languages - Undef __STRICT_ANSI__ only for c/objc
Not really needed currently for our limited use of winrt.
Root directory is added to include directories and `VERSION` conflicts with `#include <version>` which is standard library header.
Will be useful for future commits.
kasper93
force-pushed
the
smtc
branch
2 times, most recently
from
6b489af
to
c9c5873
Compare
osdep/win32/smtc.cc
Outdated
| event->event_id == MPV_EVENT_PLAYBACK_RESTART) | ||
| { | ||
| if (ctx.hwnd) | ||
| SendMessageW(ctx.hwnd, WM_MP_EVENT, WPARAM(event), 0); |
There was a problem hiding this comment.
Two issues:
- The convention for window messages is to use
lparamfor pointers. Usingwparamlooks weird to me. - TOCTOU with
ctx.hwnd: by the time this function is called the HWND might be no longer valid, or even anullptr.SendMessageWmight block indefinitely here.
There was a problem hiding this comment.
The convention for window messages is to use lparam for pointers. Using wparam looks weird to me.
I've been using both of them before, changed.
TOCTOU
I don't see how. This check only performs a check once the window is created on the other thread. The only way to initiate window destruction is from this thread, so no way it disappears while we are sending message. Unless destroyed externally, but this is not supported case.
I can add a mutex for initialization though, so we don't miss some events from mpv, but frankly, I doubt we manage to load file faster than this window is created, so I don't think it is really necessary.
There was a problem hiding this comment.
The only way to initiate window destruction is from this thread, so no way it disappears while we are sending message.
True, but it's by posting a Then WM_CLOSE message which is processed asynchronously in the win event thread. ctx.close will be set only after it exits from the message loop, which can be after the ctx.close check in the mpv event thread. If this happens, ctx.hwnd is read from the mpv event thread, which the win event thread no longer processes. If the ctx.hwnd is set to null by the win event thread between it's null checked and used in the mpv event thread, SendMessageW will be called with nullptr as HWND.
EDIT: I saw that the mpv event loop is ended after the WM_CLOSE message so it shouldn't be a problem if the window is terminated by mpv and not from some outside source.
There was a problem hiding this comment.
I saw that the mpv event loop is ended after the WM_CLOSE message
Exactly, there is a break there. And yes, if something externally closes this window, thing would break, but I don't consider this a valid scenario. Else we would need to notify the other thread about it and in this case probably whole thing would need to be refactored to work.
kasper93
force-pushed
the
smtc
branch
2 times, most recently
from
0cbcae3
to
d08e182
Compare
Add support for SystemMediaTransportControls interface. This allows to control mpv from Windows media control ui.
Add support for SystemMediaTransportControls interface. This allows to
control mpv from Windows media control ui.
Fixes: #9336
Fixes: #13813
Fixes: #14007