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

initial MPRIS support #5255

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

initial MPRIS support #5255

wants to merge 2 commits into from

Conversation

Gottox
Copy link
Contributor

@Gottox Gottox commented Sep 1, 2024

This PR adds support for the MPRIS Dbus protocol as described in #4080. My first approach was to directly use the low level API, but unfortunately the usage of the library is discouraged:

This manual documents the low-level D-Bus C API. If you use this low-level API directly, you're signing up for some pain.

So I felt back on the glib API and tried to get an glib event loop running in a worker thread.

For now the only event that has been wired up is the Raise method.

The goal for the first iteration is to only get the media keys working. Later iterations could also update the current track metadata if they can be extracted from the device. I haven't looked at the server implementation yet.

This implementation is unfinished and meant as an opportunity to see if @rom1v considers the approach acceptable.

addresses #1824, #4080

@rom1v
Copy link
Collaborator

rom1v commented Sep 1, 2024

Thank you for your PR, and for posting an early draft 👍

AFAIU, dbus/mpris is useful to control a player with a feedback, i.e. the client knows the state of the media player and get asynchronous updates.

Here, IIUC this is not the case (we are not able to receive the Android player state to synchronize with the client). In that case, maybe shortcuts are sufficient (#1824)?

Also, how does it interact with a local media player? For example, in my case, I don't want the media keys to control the Android device but my audio player on the computer.

So I felt back on the glib API and tried to get an glib event loop running in a worker thread.

I would like to avoid adding a dependency to glib. The low-level API can still be used, like in VLC:

https://code.videolan.org/videolan/vlc/-/blob/master/modules/control/dbus/dbus.c

But without state feedback from the device, I'm not sure the feature is useful. What do you think?

@Gottox
Copy link
Contributor Author

Gottox commented Sep 1, 2024

My usecase: Since scrcpy supports audio forwarding (❤️❤️❤️), I really enjoy putting my phone into the dock and my desktop and seamlessly continue listening to music/podcasts on my desktop. While scrcpy is pretty good at forwarding, the integration of my android players into my desktop environment would be a real cherry on top.

Also, how does it interact with a local media player? For example, in my case, I don't want the media keys to control the Android device but my audio player on the computer.

That depends on the desktop environment. Gnome for example can handle multiple sources:

image

Also, in the final version, I would hide it behind a cmdline option: --mpris

I would like to avoid adding a dependency to glib. The low-level API can still be used, like in VLC:

Thanks! It looks quite massive, but I'll have a look.

But without state feedback from the device, I'm not sure the feature is useful. What do you think?

I believe that it would already have a benefit if a user can control the music on the android device without having scrcpy in focus. At least that would be a nice start for me.

I'm currently looking into the MediaSessionManager (src) API of android.

Up to now I'm unable to instantiate it, but if I understand correctly, it would allow control over all MediaSessions of a device and also can request metainformation about these sessions. My knowledge of the Android API is pretty basic and I have no real idea how to instantiate system services without the default getSystemService() interface.

Do you think that's feasible?

@Gottox
Copy link
Contributor Author

Gottox commented Oct 3, 2024

@rom1v

Here, IIUC this is not the case (we are not able to receive the Android player state to synchronize with the client).

I finally got access to a functional MediaSessionManager instance. That would indeed make it possible to synchronize the state between the scrcpy client and the Android Device. I still need to work out how to a) get events from this instance (and its MediaController children) and b) synchronize the state through scrcpy.

I haven't looked into the libdbus itself. So for now it'll remain in libgdbus.

Now to figure out how to wire everything up.

@Gottox
Copy link
Contributor Author

Gottox commented Oct 4, 2024

I updated the PR. Currently I'm facing the issue, that the MediaSessionManager and the MediaController do not emit any events. Any pointers what the issue could be here?

Edit: fixed. I needed to start a new HandlerThread.

Copy link

@MultisampledNight MultisampledNight left a comment

Choose a reason for hiding this comment

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

First of all: Thanks!! This is amazing!! 🎉
Then: noted a few small things I noticed while skimming through. I don't know C that well though, so take them with some caution!

Comment on lines +17 to +20
data[size] = '\0';
memcpy(data, src, size);
}
*target = data;

Choose a reason for hiding this comment

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

Doesn't this leave target pointing at uninitialized memory if size == 0? Below, the null terminator write was done unconditionally. Putting this here would result in:

Suggested change
data[size] = '\0';
memcpy(data, src, size);
}
*target = data;
memcpy(data, src, size);
}
data[size] = '\0';
*target = data;

Comment on lines +101 to +102
}
}

Choose a reason for hiding this comment

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

Is it intentional that these closing brackets don't match their indentation level? The first one closes the case of DEVICE_MSG_TYPE_MEDIA_REMOVE (henceforth m) while the second one closes the case of DEVICE_MSG_TYPE_UHID_OUTPUT (henceforth o).

(I'm not even sure if the case of m works since it's inside the block opened by o, but I don't know C well enough to say for sure. Maybe cases inside a switch can be in nested blocks and blocks aren't hard scopes.)

Comment on lines +165 to +166
if (g_strcmp0(method_name, "Pause") == 0) {
g_dbus_method_invocation_return_value(invocation, NULL);
Copy link

@MultisampledNight MultisampledNight Nov 19, 2024

Choose a reason for hiding this comment

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

Is there a reason the Pause method is not logged like the others?

Suggested change
if (g_strcmp0(method_name, "Pause") == 0) {
g_dbus_method_invocation_return_value(invocation, NULL);
if (g_strcmp0(method_name, "Pause") == 0) {
LOGD("mpris: Pause");
g_dbus_method_invocation_return_value(invocation, NULL);

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.

3 participants