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

android: Pass song information over the JNI bridge #1951

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

Conversation

DDRBoxman
Copy link
Contributor

No description provided.

This will allow the android client to directly make calls to the mpd process to change tracks

I went with camel case on the function names here, if you use an underscore
javac generates a function tht looks like this:
 JNIEXPORT void JNICALL Java_org_musicpd_Bridge_play_1previous

I figured what we ended up with looks a little nicer:
JNIEXPORT void JNICALL Java_org_musicpd_Bridge_playPrevious
This class will allow us to send up the equivalent of print_song_info to the
bridge class.
This moves all the song tags into the hashmap and moves that into the song info object along with duration and the song uri.

It makes sure to clean up local references except for the values passed through in return.

In the future we could prefetch or cache the jni field IDs to get a bit of a performance improvement.
@DDRBoxman
Copy link
Contributor Author

I'm not quite sure what the calls to previous and next should look like. I tried making some locking versions like the pause function but seemed to be running into issues with that.

I also wasn't sure which of these would be preferred either.

partition.PlayNext();
partition.playlist.PlayNext(partition.pc);

@MaxKellermann
Copy link
Member

This looks a bit overengineered. Does the Java part really need all the tag values? I thought it might be enough to just have a Bridge method:

public static native void onSongChange(String artist, String title, int durationMilliseconds);

I don't know because there's no code for actually using it, and that's one of the problems of this PR: it adds code for no visible purpose, the code is completely unused.
I also don't like adding empty method declarations (seen in the first commit, "android: add next and previous track to the jni bridge"). This way of making incremental commits is useless - each commit must work, but your first commit doesn't work.

@DDRBoxman
Copy link
Contributor Author

The MediaMetadata object that gets passed into the media session actually accepts a ton of metadata information and I think it would be great if we could fill that out as much as we can: https://developer.android.com/reference/android/media/MediaMetadata

What's the preferred way to route events back to the JNI bridge. Looks to me like the Partition internally creates a PlayerControl which then calls back to the Partition which emits events into the event loop which get invoked on the list of clients the partition has. Should there be an event callback class that can be passed into the partition or should the client be abstracted and allow passing in a new say jni bridge client?

I also wondered if you had any thoughts about having the JNI bridge make next and previous calls, the few things I tried ended up with the mpd process in what seemed like a deadlock.

@MaxKellermann
Copy link
Member

Okay, so you want to pass something to Java that will be converted to MediaMetadata - since your C++ code is so complicated already, would it be useful to create MediaMetadata in C++, instead of this custom class?
(btw. your code doesn't work well because it overwrites previous values if a song has multiple instances of a tag, e.g multiple artists.)

Should there be an event callback class that can be passed into the partition or should the client be abstracted and allow passing in a new say jni bridge client?

I don't quite get it - do you want to supported multiple partitions? In that case, I guess we should have a Java object for each partition; maybe Bridge, but maybe a separate class Partition - I'm not quite sure yet.

I also wondered if you had any thoughts about having the JNI bridge make next and previous calls, the few things I tried ended up with the mpd process in what seemed like a deadlock.

When you call native methods from Java, you're inside some random thread, from which you must not access most data structures or even invoke most methods. Most methods are only supposed to be called from MPD's main thread. But you can always inject calls into that thread via BlockingCall().

@DDRBoxman
Copy link
Contributor Author

Building MediaMetadata on the native side is probably even more gross than the current code to build that new class since it uses the builder pattern. This ends up with a ton of calls to GetMethodID and then calling those methods. It probably makes sense to just turn that hash map into something like an array of tuples to correct the duplicate entry issue.

I guess I'm not quite clear on what multiple partitions are used for. If we only end up with one player on android having callbacks for say song change events from that sounds fine. I just wasn't sure what hooking that would look like.

@MaxKellermann
Copy link
Member

I don't think building a MediaMetadata is really harder, and neither does the Builder pattern make it any harder. You only need to look up the Builder class and some of its methods. Then you have a TagType to string table, containing the documented values of MediaMetadata.METADATA_KEY_*. Your code iterates this table, looks up all values of a certain TagType, concatenates them (separated by comma or semicolon?), call build(), return it.
(Does MediaMetadata support multiple values for one key or are applications supposed to concatenate them into one string?)

I guess I'm not quite clear on what multiple partitions are used for.

Then let's just forget that feature and only support the first partition. If somebody ever finds this feature useful on Android, you can add it, but don't think about the complexity right now.

@DDRBoxman DDRBoxman marked this pull request as draft January 6, 2024 01:00
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.

2 participants