mpv-player / mpv

🎥 Command line video player
https://mpv.io
Other
26.67k stars 2.83k forks source link

win32: add Media Control support #14338

Open kasper93 opened 2 weeks ago

kasper93 commented 2 weeks ago

Add support for SystemMediaTransportControls interface. This allows to control mpv from Windows media control ui.

Fixes: #9336 Fixes: #13813 Fixes: #14007

github-actions[bot] commented 2 weeks ago

Download the artifacts for this pull request:

Windows * [mpv-i686-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1629263674.zip) * [mpv-x86_64-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1629265084.zip) * [mpv-x86_64-windows-msvc](https://nightly.link/mpv-player/mpv/actions/artifacts/1629270273.zip)
macOS * [mpv-macos-12-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1629263460.zip) * [mpv-macos-13-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1629264776.zip) * [mpv-macos-14-arm](https://nightly.link/mpv-player/mpv/actions/artifacts/1629263068.zip)
hooke007 commented 2 weeks ago

TO fix https://github.com/mpv-player/mpv/issues/14007 ?

kasper93 commented 2 weeks ago

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.

na-na-hi commented 2 weeks ago

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 commented 2 weeks ago

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.

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.

na-na-hi commented 2 weeks ago

but since it is pretty core player functionality to integrate into system and macOS does the same

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.

kasper93 commented 2 weeks ago

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)

na-na-hi commented 2 weeks ago

This also fixes #13813 and #9336.

na-na-hi commented 2 weeks ago

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.

kasper93 commented 2 weeks ago

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.

kasper93 commented 2 weeks ago

At least the thumbnail needs to work, which doesn't seem to be the case for now.

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.

na-na-hi commented 2 weeks ago

It is possible that it works only for installed application that have app id defined.

From a look at the API this seems to be the case.

To be honest, I find thumbnail itself not that useful.

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.

zhongfly commented 2 weeks ago

Perhaps related, would it be possible to add thumbnail-toolbars(prev,play/pause,next) to the taskbar? https://learn.microsoft.com/en-us/windows/win32/shell/taskbar-extensions#thumbnail-toolbars

image

kasper93 commented 2 weeks ago

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.

I found it "cheap", if you get what I mean. Like pushing logo in the place where it shouldn't have been.

From a look at the API this seems to be the case.

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.

Perhaps related, would it be possible to add thumbnail-toolbars(prev,play/pause,next) to the taskbar?

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.

Andarwinux commented 2 weeks ago

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.

But this is feasible for libmpv. In addition, it is possible to use Add-AppxPackage -Register to register mpv in-place without signature, and mpv-packaging can do this for end-users.

kasper93 commented 2 weeks ago

In this case it will possibly work or not. I don't know.

kasper93 commented 2 weeks ago

Added thumbnail support per @na-na-hi request. It tries to use external file/url directly if available (track marked as image or albumart), if not found it tries to use Windows thumbnail for the file and finally uses screenshot-raw if needed.

na-na-hi commented 2 weeks ago

Thanks for the thumbnail support. Here are some problems I found during testing:

kasper93 commented 2 weeks ago

Embedded album art doesn't work, even when it shows up in Explorer thumbnail.

I will double check that.

If I click the info/thumbnail area (which brings the Windows built-in media player to the foreground, but does nothing to mpv)

I didn't even know it does that. Unfortunately since they cannot recognize mpv as an application it doesn't work.

and then click the play/pause button a few times, the SMTC interface won't be updated anymore, and I can't close the mpv window with the X button afterwards. This might be related to HWND or app id.

This sounds like we crashed one of the event threads and no longer can close / join them cleanly. I need to review the code if all error paths would behave correctly. But if you can give more info, it can help too, I cannot reproduce this one.

Side note about this (Windows 11) media control thingy, it is really buggy. It can leave phantom statuses, from various apps, it can just stop working, etc. It is not very robust.

EDIT:

I might disable screenshot-raw fallback, it is not fast enough and with higher resolution videos can produce small hitch at start when screenshot is taken. Our screenshots are taken with all the processing, which can be really slow. Maybe doing it is possible to query last swapchain image instead, but not sure.

kasper93 commented 2 weeks ago

Embedded album art doesn't work, even when it shows up in Explorer thumbnail.

@na-na-hi could you show example of a file like that?

na-na-hi commented 2 weeks ago

But if you can give more info, it can help too, I cannot reproduce this one.

I reproduced with the following: cap4 log.txt

could you show example of a file like that?

None of the mp3 files with embedded album art works for me. Example: 01_Shuwa_Shuwa Parfait_ななひら.zip

Thumbnail works for external album arts.

kasper93 commented 2 weeks ago

I reproduced with the following:

Can you check the verbose log? It should print errors.

EDIT:

I can see exception, after clicking on title thingy.

SystemMediaTransportControls: update_thumbnail: 0x8001010d - An outgoing call cannot be made since the application is dispatching an input-synchronous call.

Maybe it is dispatched after all and we could do bring to front on our window. But I didn't see this before.

Looking into it, what is going on.

EDIT2:

Also seems to fail after opening directory, because of mixed forward slashes.

update_thumbnail: 0x800700a1 - The specified path (...) contains one or more invalid characters.

None of the mp3 files with embedded album art works for me.

Works for me, but I changed few things, try latest version.

na-na-hi commented 2 weeks ago

Works for me, but I changed few things, try latest version.

The latest version works now.

kasper93 commented 2 weeks ago

@na-na-hi: I've added path normalization, which helps with exceptions. Also fixed window focusing. Do you see any other issues?

na-na-hi commented 2 weeks ago

A few more suggestions:

kasper93 commented 2 weeks ago

Changed thumbnail get to do it asynchronous, because it was introducing lag on the buttons for me.

The stray app icon/name problem is because mpv isn't installed, but third-party builds can integrate their own installers. I think it's better for mpv to define and use an appid here so that third-party installers can properly integrate and solve this problem.

I don't know what is required to make it work. So I would rather wait for someone to test and add needed changes.

For example, to even use mpv.exe from installed package, you have to add, something like the following to manifest.

    <msix xmlns="urn:schemas-microsoft-com:msix.v1"
        publisher="CN=mpv"
        packageName="mpv.exe"
        applicationId="mpv.exe"
    />

For libmpv, the story is different and appid, would be of parent app anyway.

I would imagine, we would need smtc-appid property that users can set and it would be set as appid in our code. But I'm not sure it is even needed, so I would like to defer to upstream integrators for those changes.

Add an option to disable SMTC. For example, Firefox and Spotify have options to disable this so that they won't hook and steal media key inputs.

Added --media-controls, so other platforms can connect to it too.

na-na-hi commented 2 weeks ago

I don't know what is required to make it work. So I would rather wait for someone to test and add needed changes.

I think this is OK for now. We can wait for the third-party who cares to submit a PR for that.

sfan5 commented 2 weeks ago

Honestly given the need to pull in C++ and some winrt stuff I do agree with @na-na-hi's suggestion of keeping this as an external plugin. It doesn't even use anything other than the client API.

Note that this must not mean that we can't have it in the mpv repo. It could even be built and installed by default, just keep it in a separate folder and somewhat separate from the main meson.build.

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.

On Linux mpris is the de-facto standard and does not have integration in mpv either (unless you install a plugin). If you wanted to import it into mpv it would be a similar level of annoying because it builds on D-Bus.

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 absolutely doable from the client API. I do it on mpv-android. I think it's also generally useful to add features to the client API whenever possible (e.g. if we introduce a way to get a nearly live view of the rendered video).

kasper93 commented 2 weeks ago

Honestly given the need to pull in C++ and some winrt stuff I do agree with @na-na-hi's suggestion of keeping this as an external plugin. It doesn't even use anything other than the client API.

The same can be said for our macOS integration, and yet it is included here.

Note that this must not mean that we can't have it in the mpv repo. It could even be built and installed by default, just keep it in a separate folder and somewhat separate from the main meson.build.

I don't see the real benefit of this approach. Wouldn't this only increase the complexity of the solution from both a development and user perspective? I might be missing something, so please let me know what issues or problems this solution would resolve.

keep it in a separate folder and somewhat separate from the main meson.build.

EDIT: It is in separate meson.build in osdep/win32. And only if it is enabled C++ language is added. Just in case, it wasn't clear. Because it is literally separated as much as possibly can if we would like to link into mpv.exe still.

On Linux mpris is the de-facto standard and does not have integration in mpv either (unless you install a plugin). If you wanted to import it into mpv it would be a similar level of annoying because it builds on D-Bus.

I don't understand the inconvenience you are referring to. Focusing on this PR, it only adds an optional dependency on a few headers, which greatly simplifies the code. As for C++, we already have all the necessary components; it's not like we're adding Rust or Swift.

Thumbnails are absolutely doable from the client API. I do it on mpv-android.

Thumbnails are implemented in this PR. Since you have it integrated within mpv-android too, it seems you agree that it's a feature deserving of integration in the media player frontend. I assume you don't require users to install an external application to handle this basic feature.

I think the main question is, what is mpv.exe? Is it a media player or just a toolkit to DIY one?

Regarding modularizing things, it would indeed be nice to have a clean separation of core mpv code, libmpv API, scripting engines, video outputs, and so on. However, it has never been done that way. We even embed lua source into the mpv binary. Why isn't youtube-dl an external feature? I'm not sure where this modularization requirement is coming from. Not to mention the entire macOS integration, but that's another topic.

tl;dr: I believe media controls (and thumbnails) are fundamental media player features and should be integrated into the media player (mpv). Users should not have to DIY these features. It is prohibitively difficult to know how to enable these media controls. How are users supposed to know that? They just want to download a player and have it work. I love that mpv is customizable, but customizability should extend beyond basic features and target people who like to tinker.

Akemi commented 2 weeks ago

The same can be said for our macOS integration, and yet it is included here.

just some additional info, back when i added support for some of the macOS components that are application wide the conclusion was to use libmpv/the client API even for internal usage.

if we are comparing the pulling of c++ in with the objective-c usage in the macOS parts, the latter is a hard requirement since there is no basic c api for a lot of the needed things.

anyway don't misunderstand me, i am not trying to argue against the inclusion, i am more on the side of including such features.

kasper93 commented 2 weeks ago

if we are comparing the pulling of c++ in with the objective-c usage in the macOS parts, the latter is a hard requirement since there is no basic c api for a lot of the needed things.

Programming language is only a tool and tools are designed to make given task possible or easier. C++ is not some boogeyman, the usage of it is friction-less. While it is possible to use winrt directly from C, after all those are all COM interfaces underneath, the overhead of handling everything manually, keeping track of everything is not something I'm interested in doing, hence why C++ projection of winrt has been used here. Note that alternative would be to essentially rewrite manually parts of what's done in C++ "wrapper", especially for async operations. We are developing desktop application here, not embedded one.

Akemi commented 2 weeks ago

Programming language is only a tool and tools are designed to make given task possible or easier. C++ is not some boogeyman, the usage of it is friction-less. While it is possible to use winrt directly from C, after all those are all COM interfaces underneath, the overhead of handling everything manually, keeping track of everything is not something I'm interested in doing, hence why C++ projection of winrt has been used here. Note that alternative would be to essentially rewrite manually parts of what's done in C++ "wrapper", especially for async operations. We are developing desktop application here, not embedded one.

not sure why you direct that towards me. i didn't argue against this, neither did i say it's a boogeyman, nor did i say it's bad in any way. i am just saying that it's not completely comparable to the macOS situation.

kasper93 commented 2 weeks ago

not sure why you direct that towards me. i didn't argue against this, neither did i say it's a boogeyman, nor did i say it's bad in any way. i am just saying that it's not completely comparable to the macOS situation.

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.

Dudemanguy commented 2 weeks ago

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.

na-na-hi commented 2 weeks ago

Wouldn't this only increase the complexity of the solution from both a development and user perspective?

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 libao and libvo separate libraries, but mpv moved away from it. Efforts like https://github.com/mpv-player/mpv/pull/11145 are needed if mpv wants to be modular again.

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.

kasper93 commented 2 weeks ago

It doesn't increase the complexity on the user side if properly done.

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.

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.

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.

Andarwinux commented 2 weeks ago

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.

kasper93 commented 2 weeks ago

especially since a significant portion of *nix users would be certainly driven away by it.

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.

na-na-hi commented 2 weeks ago

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.

Additionally, libplacebo already uses C++

It's also in a limited fashion, and doesn't make the rest of code require modern C++ knowledge for contribution.

kasper93 commented 2 weeks ago

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.

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.

It's also in a limited fashion, and doesn't make the rest of code require modern C++ knowledge for contribution.

Exactly, and that's the idea. Everything needs context when talking about those things.

sfan5 commented 2 weeks ago

Wouldn't this only increase the complexity of the solution from both a development and user perspective? I might be missing something, so please let me know what issues or problems this solution would resolve.

It solves no problems. The point of this discussion is how careful we want to be with pulling bigger dependencies into the mpv codebase.

EDIT: It is in separate meson.build in osdep/win32. And only if it is enabled C++ language is added. Just in case, it wasn't clear.

Sure, that's good. I didn't check the changes closely before commenting.

Because it is literally separated as much as possibly can if we would like to link into mpv.exe still.

Well if I look into smtc.cc right now I can see that it includes a bunch of internal headers instead of just libmpv/client.h. This means we have to ensure that a bunch of internal headers are also valid C++20 (not that I expect this to be a problem). But this is the kind of separation that can be useful to keep to avoid other code being "infected".

I don't understand the inconvenience you are referring to. Focusing on this PR, it only adds an optional dependency on a few headers, which greatly simplifies the code. As for C++, we already have all the necessary components; it's not like we're adding Rust or Swift.

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. I guess calling C++ an inconvenience is inaccurate. You indeed get it for free with the compiler that is already installed.

Since you have it integrated within mpv-android too, it seems you agree that it's a feature deserving of integration in the media player frontend. I assume you don't require users to install an external application to handle this basic feature.

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. I do agree that for media control integration thumbnails are an obvious feature that should exist.

I think the main question is, what is mpv.exe? Is it a media player or just a toolkit to DIY one?

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'm not sure where this modularization requirement is coming from. Not to mention the entire macOS integration, but that's another topic.

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. mpv during the wm4 era held (perceived) code quality/architecture/cleanliness in high regard, so much that otherwise useful features suffered (see: DVD support). The macOS sitation is indeed an exception... This sounds vague, might be obvious to state but I wanted illustrate where my opinion came from. Maybe trying to stay on the "old ways" is also irrational.

We even embed lua source into the mpv binary.

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.

Why isn't youtube-dl an external feature?

Good point. I think it's due to the close interworking that is required for some formats. I think the ytdl_hook is a good example of how a feature is nicely separated from the rest of the player.

How are users supposed to know that? They just want to download a player and have it work.

But as @na-na-hi now pointed out this is not necessarily a conflict. These things could be separated and be the default.

kasper93 commented 1 week ago

Well if I look into smtc.cc right now I can see that it includes a bunch of internal headers instead of just libmpv/client.h. This means we have to ensure that a bunch of internal headers are also valid C++20 (not that I expect this to be a problem). But this is the kind of separation that can be useful to keep to avoid other code being "infected".

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.

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.

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.

This is indeed the main question and we don't even have any design or direction document on it.

¯\(ツ)/¯ 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 vague, might be obvious to state but I wanted illustrate where my opinion came from. Maybe trying to stay on the "old ways" is also irrational.

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.

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.

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.

Good point. I think it's due to the close interworking that is required for some formats. I think the ytdl_hook is a good example of how a feature is nicely separated from the rest of the player.

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.