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 Auto #578

Merged
merged 48 commits into from
Jun 2, 2024
Merged

Android Auto #578

merged 48 commits into from
Jun 2, 2024

Conversation

puff
Copy link

@puff puff commented Jan 17, 2024

Adds basic Android Auto and Android Automotive support.

Features:

  • Browse & play albums, playlists, and albums in each genre.
  • Artists start an instant mix when playing them.
  • Jump to albums/artists by letter (click A-Z on scroll bar).
  • Seek forwards and backwards in songs.
  • Skip to previous and next songs in queue, as well as directly to any song in the queue.
  • Shuffle button
  • Offline mode support.
    • Even outside of offline mode, it will try to use downloaded items before going online, unless it is fetching all items for root category (e.g. albums, artists, etc).
  • Search (voice and keyboard)

Future features/TODO?

  • Repeat button
  • Favorite button
  • Instant mix button
  • Home tab
  • Genres should start an instant mix (not yet implemented in Finamp)
  • Offline mode artists/genres instant mix (not yet implemented in Finamp)

Currently, there is a hard limit of 100 250 (online mode) or 1000 (offline mode) items displayed so the app doesn't crash on large libraries.

// dev notes

  • Investigate why it doesn't switch to the Now Playing screen sometimes when starting playback, or takes a while before switching to it. This is likely because of the playback states being wrong.
  • Add pagination so we can show more than 100 items. This and This may be useful.

@jmshrv
Copy link
Owner

jmshrv commented Jan 18, 2024

This is awesome to see finally :)

Is the 100 item limit just an awkward thing with Android Auto or a thing that specifically needs to be fixed here? Jellyfin luckily makes pagination very easy when fetching from the server.

Artwork sounds like an annoying issue to fix though 🙃

@puff
Copy link
Author

puff commented Jan 18, 2024

Is the 100 item limit just an awkward thing with Android Auto or a thing that specifically needs to be fixed here? Jellyfin luckily makes pagination very easy when fetching from the server.

I put that limit because someone reported Android Auto crashing on large libraries (1000+) I do have an idea on how to add pagination to it, so I'll probably work on that tonight.

Artwork sounds like an annoying issue to fix though 🙃

Yeah, I'm gonna have to do some experimentation with that.

@Chaphasilor
Copy link
Collaborator

Thanks for the PR! I can try to take a look at it in the coming days, I just need to find a car to test with ^^

I'm guessing there is no built-in pagination support in Android Auto, and I don't think it would be super useful anyway.
I believe focussing on quick actions, like shuffle all songs, instant mixes and playing recently played items is a good approach.

Search and some voice interactions would definitely be useful down the road (hah), but if basic playback and browsing is working then I guess it qualifies for a beta.
Maybe it attracts some additional contributors :P

@puff
Copy link
Author

puff commented Jan 18, 2024

I just need to find a car to test with ^^

You can use the emulator! #24 (comment) It's very convenient for debugging too.

I'm guessing there is no built-in pagination support in Android Auto, and I don't think it would be super useful anyway. I believe focussing on quick actions, like shuffle all songs, instant mixes and playing recently played items is a good approach.

Yeah, I think I agree with you. Not sure that pagination should be a priority. I typically just use a playlist anyway lol.

Search and some voice interactions would definitely be useful down the road (hah)

Yeah, 100%. I wanted to incorporate that into this PR, but I was having an issue I didn't really understand where it crashes when searching. EDIT: fixed the crash, was a problem with changing the package name

@Chaphasilor
Copy link
Collaborator

Yeah I know about the emulator and have used it last time, but in the end I want to make sure everything works on an actual car too ^^

Regarding voice, how flexible is that? Can it only be used for voice search, or could we enable custom actions, like saying "Start playing Hello by Adele" and it will start an instant mix?

@puff
Copy link
Author

puff commented Jan 18, 2024

Can it only be used for voice search, or could we enable custom actions, like saying "Start playing Hello by Adele" and it will start an instant mix?

I know you can do stuff like that with app actions. There's also a version for Android Auto/Automotive.

@Chaphasilor
Copy link
Collaborator

Chaphasilor commented Jan 18, 2024

The second link you provided seems to be geared towards navigation/Points-of-Interest apps. Here's the one for media apps: https://developer.android.com/training/cars/media#support_voice

Seems like it has everything we need. If the user indicates an item type (album, playlist, etc.) we can use that to deal with Jellyfin's poor search, and if they omit the type "Play some music" we could resort to shuffle all for now. Would you like to look into it? :)

@puff
Copy link
Author

puff commented Jan 19, 2024

Would you like to look into it? :)

Sure, I have some time the next few days so I'll see if I can understand this and get something usable.

@Chaphasilor
Copy link
Collaborator

Chaphasilor commented Jan 19, 2024

Awesome. Let me know if you need help with anything!

Edit: Just checked it out on the emulator, here are a few things I noticed:

  • Clicking an item in the "Genres" tab should start an instant mix for that genre. Searching for albums by genre is probably not used that often and not really suitable for driving anyway
    • Same thing for artists maybe?
  • Placeholder images (especially for artists) would be nice to make sure the item names are aligned
  • For albums and playlists I'd really like to either be prompted if I want to play in-order or shuffled (that might not be ideal while driving), or have a way in the settings to set default behavior
  • The sorting is a bit different than in the phone app, not sure if you can do anything about it. Names that start with symbols are sorted by the first non-symbol character in the car, but not on the phone. Examples are "...Baby one more time", "& Friends" is "F", ".me", "(Un)Commentary"
  • The jump-to-letter function is neat, but only works with the loaded items. Is there a way to load items on-demand, like is done with the new "fast scroller" on the phone app?
  • I think we definitely need a "home" tab/section. I'll see if I can get something going for the entire app, since the phone app is supposed to also get a home screen with the redesign
    • There's also a "for you" section in the Android Auto home screen music widget (swiping left on the music widget) where the items from the home screen could be shown. Right now it just shows first 3 items from the albums tab, although strangely in the sorting of the phone app (symbols first)
  • There's a display bug when shuffle is active. The order of the list is correct, but the index of the currently playing song is not shown correctly in the list (the bars next to the active song are on the wrong item). Did you do any implementation for the queue? Otherwise I'll have to dive into just_audio once again to see if I can fix it...

@puff
Copy link
Author

puff commented Jan 20, 2024

Thanks for the feedback, I agree with your observations here.

Clicking an item in the "Genres" tab should start an instant mix for that genre

I was thinking we could replace the genre tab with the home tab. Android Auto by default only allows 4 tabs, and genres don't have instant mixes (though we could shuffle albums within the genre).

For albums and playlists I'd really like to either be prompted if I want to play in-order or shuffled

I don't think this is possible, but a setting for the default would work.

The sorting is a bit different than in the phone app
The jump-to-letter function is neat, but only works with the loaded items

I don't think I can do anything about these :(, they seem to be built into Android Auto.

There's also a "for you" section...

Seems like this is related to that section, looks to be possible.

There's a display bug when shuffle is active

I've done a little troubleshooting, and I'm confused. It seems there may be a bug somewhere causing a conflict between just_audio and the new queue service?

queueIndex: event.currentIndex,

The queue service expects queueIndex to be the non-shuffled, original index:

_queueAudioSourceIndex = event.queueIndex ?? 0;
if (previousIndex != _queueAudioSourceIndex) {
int adjustedQueueIndex = (playbackOrder ==
FinampPlaybackOrder.shuffled &&
_queueAudioSource.shuffleIndices.isNotEmpty)
? _queueAudioSource.shuffleIndices.indexOf(_queueAudioSourceIndex)
: _queueAudioSourceIndex;

int adjustedQueueIndex = (playbackOrder == FinampPlaybackOrder.shuffled &&
_queueAudioSource.shuffleIndices.isNotEmpty)
? _queueAudioSource.shuffleIndices.indexOf(_queueAudioSourceIndex)
: _queueAudioSourceIndex;

I suspect somewhere in just_audio expects it to be the shuffled index and is sending it to the backend (ExoPlayer?), which results in the queue showing the wrong song as selected. To elaborate, the erroneous selected song in queue is in the position of the original (non-shuffled) index, which is why I suspect this.

Changing queueIndex to the shuffled index, and replacing both occurrences of adjustedQueueIndex in queue_service directly to _queueAudioSourceIndex fixes the issue, however I'm not sure of the implications of this.

music_player_background_task.dart

- queueIndex: event.currentIndex,
+ queueIndex: _player.shuffleModeEnabled && (shuffleIndices?.isNotEmpty ?? false) && event.currentIndex != null ? shuffleIndices!.indexOf(event.currentIndex!) : event.currentIndex,

queue_service.dart

-int adjustedQueueIndex = (playbackOrder == FinampPlaybackOrder.shuffled &&
-         _queueAudioSource.shuffleIndices.isNotEmpty)
-     ? _queueAudioSource.shuffleIndices.indexOf(_queueAudioSourceIndex)
-     : _queueAudioSourceIndex;
+ int adjustedQueueIndex = _queueAudioSourceIndex;

@Chaphasilor
Copy link
Collaborator

Chaphasilor commented Jan 21, 2024

We could reduce the number of tabs even further, with a home tab and a library tab? On the library tab we could have a grid with the different categories.

Genre mixes should be possible, at least there's a way to add a genre to a mix on the phone 🤔 But maybe that just adds all individual albums, not sure.

Can we change the sorting within Android Auto? If something like "Recently Added" is possible, then it should be possible to use the BaseItemDto's SortTitle too.

I'll take a look at the modifications to the queue service. just_audio is really opinionated and the layered architecture doesn't make things easier. I've struggled with it for many monhs 🙃

Edit: Genre instant mixes should be a thing, at least it works in the Jellyfin app. No idea why it's hidden in Finamp...

@Chaphasilor
Copy link
Collaborator

Chaphasilor commented Jan 21, 2024

@puff I managed to fix up the queue based on your proposed changes. There were a few other things I needed to adjust, and I fixed a bug that I introduced a while ago (items in next up were not being reported to the external/OS queue).
From my testing everything seems to work as it should, but please let me know if anything is off!
With the beta release approaching fast I would hate to break things, and there have been the strangest bugs with playback and queue management in the past...

@puff
Copy link
Author

puff commented Jan 22, 2024

We could reduce the number of tabs even further, with a home tab and a library tab?

Yeah, I like that idea.

Genre mixes should be possible

I didn't see a way already implemented in Finamp yet, but yeah it should be possible if the Jellyfin app has it.

Can we change the sorting within Android Auto?

Yes, we can sort the MediaItem list before returning from getChildren. While testing, I noticed the offline sort already used doesn't match output from the Jellyfin api when sorting names.

offlineSortedItems!.sort((a, b) {

Looks to be because the Jellyfin api is case-insensitive:
https://github.com/jellyfin/jellyfin/blob/7027bc00fc06314929011735e425763779cb4076/MediaBrowser.Controller/Entities/BaseItem.cs#L897

I've fixed it and added sort to Android Auto. It uses whatever sort option is selected in the phone app.
Before & After:
image image

@Chaphasilor
Copy link
Collaborator

Thanks for fixing that. There are some sorting-related issues that need to be tackled at some point anyway, like #444 or #581, so if you feel like taking a look while you're dealing with sorting anyway, that would be much appreciated.
Otherwise I'll try to fix it after some bigger issues have been fixed...

@puff
Copy link
Author

puff commented Jan 23, 2024

I managed to get artwork to work with offline items and (kind've) Android Automotive using the android_content_provider package.

The white icon in the middle is the placeholder art.
image

Only problem is that for Android Automotive, we can't use http(s) URIs. Because of this, we have to resolve them ourselves. My current implementation hangs until every item has a resolved image. I think the way to fix this is to offload it to the ContentProvider. For now, I've commented it out until I have a better solution.

@Chaphasilor
Copy link
Collaborator

Did you manage to take a look at the voice command stuff? Or is there anything that you need help with? :)

@puff
Copy link
Author

puff commented Feb 1, 2024

Did you manage to take a look at the voice command stuff? Or is there anything that you need help with? :)

I've been having some trouble with it. It doesn't default to finamp even when it's open (maybe I forgot to configure something though?). It really doesn't like to recognize the app's name, you have to say it slowly. Saying "album" or really anything else other than the name of the song/album makes it default to YouTube Music (probably your default music service) so extras is always null.

"play some music on finamp"
image
Seems like some generic phrases are stripped out, odd that the "on finamp" part wasn't though.

"play black ray on finamp"
image

If you want to try it out, you just need to add this to the manifest:

<intent-filter>
  <action android:name="android.media.action.MEDIA_PLAY_FROM_SEARCH" />
  <category android:name="android.intent.category.DEFAULT" />
</intent-filter>

and uncomment playFromSearch in music_player_background_task.dart

@Chaphasilor
Copy link
Collaborator

Sorry for not responding earlier, I was a bit busy. Just tested it out and didn't manage to invoke the playFromSearch() method even once, although I did manage to get it to recognize the app's name a few times. I did manage to open the app by saying "Play some music on Finamp", but I don't think that's related to the manifest changes.
If the problem would just be to properly recognize the app name, then this might help, if it still works. But I think the whole voice command stuff seems to be pretty flawed, with YouTube being the default for a lot of things even after explicitly not selecting a default in the Assistant settings.
I only tested with Google Assistant, not with Android Auto, but I'd expect results to stay the same.

I guess we should skip this feature for now, including regular search. Search is due for a rewrite soon, and it makes little sense to integrate the old version now...

@Chaphasilor Chaphasilor added the feature A new feature label May 15, 2024
@Chaphasilor
Copy link
Collaborator

It's been way to long since I last worked on this, but I'm planning to get this into 0.9.8 or 0.9.9 (depending on how quickly we push out a "hotfix" for the things that broke in 0.9.7).

I guess what's missing right now is the search improvements (search different types + show headers for the different types) and some fixes I wanted to add for voice commands in offline mode, if I remember correctly.

@Chaphasilor
Copy link
Collaborator

@puff could it be that http-only URLs don't work with the content provider? I tried testing while using the local IP of my server, and the images only showed up for downloaded media. Logging in over the public HTTPS address everything worked normally 🤔

@puff
Copy link
Author

puff commented May 26, 2024

It works for my local server over http. Using a Pixel 8 and the DHU emulator. Also works on my Android Automotive emulator.
Maybe there's something in your logs?

@Chaphasilor
Copy link
Collaborator

Nothing in the logs sadly. Only logs I get are Getting children of <xyz> and Couldn't get offline image [...] (for the ones that don't have an image, where the placeholder is correctly being applied).

Here's what it looks like:

image
image
image
image

I can try testing it with an actual car in the coming days, maybe even tomorrow.


// replace with content uri or jellyfin api uri
if (downloadedImage != null) {
artUri = downloadedImage.file?.uri.replace(scheme: "content", host: "com.unicornsonlsd.finamp");

Choose a reason for hiding this comment

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

Will all these content URIs work on iOS? Android seems to have a custom handler we've written.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch, should probably have a platform check here.

@@ -57,6 +57,7 @@ class _MusicScreenTabViewState extends State<MusicScreenTabView>

final _jellyfinApiHelper = GetIt.instance<JellyfinApiHelper>();
final _isarDownloader = GetIt.instance<DownloadsService>();
final _finampUserHelper = GetIt.instance<FinampUserHelper>();

Choose a reason for hiding this comment

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

Is this line used?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not anymore, at least according to the linter ^^

@@ -1263,3 +1264,41 @@ class PreviousTracksSectionHeader extends SliverPersistentHeaderDelegate {
@override
bool shouldRebuild(SliverPersistentHeaderDelegate oldDelegate) => false;
}

class QueueTracksMask extends SingleChildRenderObjectWidget {

Choose a reason for hiding this comment

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

This should be removed.

lib/main.dart Outdated
@@ -111,6 +112,21 @@ void main() async {
: "en_US";
await initializeDateFormatting(localeString, null);

// Load the album image from assets and save it to the documents directory for use in Android Auto
final documentsDirectory = await getApplicationDocumentsDirectory();

Choose a reason for hiding this comment

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

Don't put this in documents. Put it in application support. Also, this whole block should probably be in one of the submethods where we've still got the errorApp wrapper.

lib/main.dart Outdated
@@ -111,6 +112,21 @@ void main() async {
: "en_US";
await initializeDateFormatting(localeString, null);

// Load the album image from assets and save it to the documents directory for use in Android Auto
final documentsDirectory = await getApplicationDocumentsDirectory();
final albumImageFile = File('${documentsDirectory.absolute.path}/images/album_white.png');

Choose a reason for hiding this comment

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

I don't think constructing files from full pathstrings is considered good practice? I'm not sure how much that matters though.

Copy link
Author

Choose a reason for hiding this comment

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

/images/album_white.png could be moved to a constant. Not really sure what else to do here.

Choose a reason for hiding this comment

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

I basically mean using the path_helper to combine the segments. Like I said, I'm not sure how much it matters though.

lib/services/music_player_background_task.dart Outdated Show resolved Hide resolved

// triggers when skipping to specific item in android auto queue
@override
Future<void> skipToQueueItem(int index) async {

Choose a reason for hiding this comment

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

Can't we just rename skipToIndex to match the override?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think skipToIndex() is clearer, and it doesn't really hurt to do it this way?

Choose a reason for hiding this comment

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

I guess not? It's pretty strange to have this pointless redirection though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually don't think so. There are some weird external methods that the OS can decide to call, but unifying this into a more sensible API seems like a good idea to me.
Basically, all we're doing is say "if Android Auto decides it needs to skip to a specific queue item, that just means we skip to the provided index, just like we do in some other cases". That redirection is basically some documentation/explanation saying that the two actions are equivalent, without there being any confusion...

lib/services/android_auto_helper.dart Outdated Show resolved Hide resolved
|| tabContentType == TabContentType.artists || tabContentType == TabContentType.songs;
}

Future<MediaItem> _convertToMediaItem({

Choose a reason for hiding this comment

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

This should probably be unified with the method in queueService. Maybe the id string and playable bool should be optional parameters for that? Alternatively, it looks like this is only used for menu items, so maybe you just unify the art URI fetching and then cut this version down?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like this?

@Chaphasilor
Copy link
Collaborator

Thanks for the review Komodo! The age of this PR is starting to show...

I'll try to go through your comments later and improve the sections as best as I can :)

@Chaphasilor
Copy link
Collaborator

Chaphasilor commented May 27, 2024

Also before I forget, the way Symfonium handles playing in-order vs. shuffled is by making each "leaf" item a node/collection, which has two children, "Play in order" and "Play shuffled". I think that would also be a good solution for us, and we could even add more choices, like starting and instant mix or adding to Next Up.

@Chaphasilor
Copy link
Collaborator

Okay, aside from breaking the whole Android Auto media browser, the review findings should be handled now 😅
I'll try to figure out what exactly broke tomorrow...

@Chaphasilor
Copy link
Collaborator

There we go. Managed to fix it today :D

@Chaphasilor
Copy link
Collaborator

Okay, the enhanced search is also working now. I'd like to get a like button onto the AA player screen / media notification because it's been a long-requested feature, and then this should be ready to merge

@Chaphasilor
Copy link
Collaborator

Also before I forget, the way Symfonium handles playing in-order vs. shuffled is by making each "leaf" item a node/collection, which has two children, "Play in order" and "Play shuffled". I think that would also be a good solution for us, and we could even add more choices, like starting and instant mix or adding to Next Up.

Custom browse actions (https://developer.android.com/training/cars/media#custom_browse_actions) would be a much better solution though...

@Chaphasilor
Copy link
Collaborator

This should be ready to merge now. All the basics are working reasonably well. If someone could confirm that this still works fine on iOS that would also be nice :)

@Chaphasilor
Copy link
Collaborator

Okay, I'll merge this in now for the next update, since most things should be working fine. If not, we won't know without having users test it anyway.
Enhancements and new features can come at a later point, based on user feedback.

@Chaphasilor Chaphasilor merged commit c68cdf0 into jmshrv:redesign Jun 2, 2024
4 checks passed
@Chaphasilor
Copy link
Collaborator

Thank you for getting this started looking into the documentation, etc. @puff!

@Chaphasilor
Copy link
Collaborator

@puff do you have a working Automotive setup? I'm low on disk space, but it seem like we need screenshots from Automotive for our Play Store listing to pass the review.

If you could grab some screenshots in landscape and portrait mode using the official demo server, that would be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants