Add support for Android Auto #9592
Conversation
SameenAhnaf
added
feature request
haggaie
force-pushed
the
android-auto
branch
from
a4760b6
to
4a28129
Compare
|
Kudos, SonarCloud Quality Gate passed! |
|
Interesting! Is it possible that using |
I'm not familiar with these issues, but I can try testing them if they are reproduced with this patch. Can you point me in their direction?
I wanted to start with media controls, because it seems useful in its own. I can set a play queue in advance and control it from the car. Later I would also like to add see browsing. I think that for Auto it should be limited - maybe things like saved playlists and search history? There's also an API for search queries I am hoping would allow integration with voice search.
Yes, I think the documentation had something to say about it - that the service shouldn't start playing when it starts. It should only start playing when you get a callback from the MediaBrowserServiceCompat. But the service can tell how if it gets started by another UI or by a media browser, can't it? I had a related question: at first I thought it would be better to create a separate service. I then saw that it is easier to use the existing player service, but do you think it is the right approach? Another small problem I noticed with the pull request in its current state, is that the Android Auto's rendering of the media session notification shows the progress circle around the pause button in gray, rather than showing the current position. This is weird, because the notification on my phone does show a seekable progress bar. |
You actually answered my question: the issue with the notification I was referring to is the one related to no seek bar showing up. And apparently no,
Yeah that's fine, you can take care of the rest later if you want. Integration with voice search would also be really nice.
Yes it can, but atm I think that part is broken, because there are user reports of the player popping up from nowhere or crashing at random when connecting or disconnecting bluetooth.
Here are a few considerations:
So I think all in all it's best to keep it in |
|
I'm trying to implement some simple browsing, but I got into lifecycle issues. The How can I handle this? Perhaps closing the notification shouldn't close the entire player, and instead, stop and close the notification UI? |
Closing the notification should definitely release the player. Afterwards any incoming intent to start the player should create a new player, while other intents should be treated separately (and NOT start the player). But it's a big mess. If you can't find out a way to do this, maybe go with the option of creating a separate service. |
Makes sense, I tried doing something along these lines, though I don't differentiate between the intents yet. The code also still creates the player when the service is created, but I can change it as you wrote. I also tried adding some browsing capabilities, but for some reason it only worked on the desktop HUD and not with my car. I'll have to check that out. |
OT: google also bought the
google and others are fiercely anti-competitive and purchase any company doing better than them. Remeber, |
Yes, that's what I've seen too. In Spotify for example the progress is shown as a circle that can't be manipulated, rather than a seek bar. Still, the circle shows playback progress with two colors. I'll try capturing a screenshot sometime.
I know, that's too bad because I liked Waze. Up until now, Google haven't merged it, even if they haven't grown it much. I think it was pretty good at predicting traffic, which is something that requires a popular service, so I might end up with Google Maps if they merge them. |
oh yeah, that thin two coloring of the circle's border stroke, right? i can imagine that. OT:
I know, that's too bad because I liked Waze. same, i found this too while searching for alternates to google maps some years ago. |
|
Hi, I've updated the pull request and tried to keep the MediaSession and its connector in the new helper object so that they outlive the player. I also tried making the media Ids more URI-like, inspired by VLC. By the way, I also tried implementing the APIs for voice search (I think it should just be onPlayFromSearch/onPrepareFromSearch), but it didn't work. As far as I understand, Google Assistant only works with apps from the Google Play Store. I can't even invoke "open NewPipe", and this shouldn't require any code. |
haggaie
force-pushed
the
android-auto
branch
2 times, most recently
from
bc2ba86
to
bec3408
Compare
haggaie
force-pushed
the
android-auto
branch
from
bec3408
to
59f3fd8
Compare
|
Apologies for my unfamiliarity with your development process, but is this likely to be included in the next release? It's a feature I'm very much interested in and looks to the untrained eye like it's ready to be merged? |
There was a problem hiding this comment.
Thank you very much for this feature!
Please apply the following changes and rebase your branch on top of the dev one, so we can test the changes on the latest release. I am trusting you for your usage and your implementation of the feature.
However, there is an issue with this PR: the single ExoPlayer library is being discontinued, as the library has now its place in the Media3 one.
This change removes the media session extension, as media session management is directly done by Media3, so this code would have to be rewritten soon if and when we migrate to Media3 the app.
Even if this migration may happen soon, I don't think we will migrate to Media3 in the next release and I think we can merge this pull request, especially because of the feature it gives to the app.
Finally, I see several changes in this PR related to the player service, especially on command interception. Wouldn't your changes fix the player crash issues we currently have when using a media command on a Bluetooth device and maybe the player ANRs?
app/src/main/java/org/schabi/newpipe/player/mediabrowser/MediaBrowserConnector.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/player/mediabrowser/MediaBrowserConnector.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/player/mediabrowser/MediaBrowserConnector.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/player/mediabrowser/MediaBrowserConnector.java
Outdated
Show resolved
Hide resolved
|
Thanks for your feedback. I'm trying to rebase, but I came across a build issue with the updated dev branch. I'm getting the following error: Do you know what causes this? Regarding moving to media3, I understand it can make most of this code obsolete. Once you are done with the move to media3, I could try to test its support for android auto and see when changes are needed. |
The recurrent Jitpack artifact |
Is there an issue about this I can read? Maybe I could try and reproduce this problem and see if my patches help. |
I tried reproducing #9095, and I believe it still happens with this PR. I tried using my car's console without Android Auto, only functioning as a Bluetooth headset. If I play a video on NewPipe, close it, and press "Play" on my car's console, the application crashes. I got the following in the crash log: So I think closing the player must have left the service running but with the player object null. I haven't tested the |
|
Hi @haggaie, Sorry to get back in touch with you after so much time. Regarding the player service crashes on your last comment, #10232 fixed a memory leak in this service (which is the reason of your conflicts with the You said the following sentences when I raised the issue we will have with this PR to use Media3 in the player:
Are you willing to help us on the Android Auto support (or even better, doing the corresponding part) when we will rewrite the player using Media3? We do not plan to migrate to Media3 with the current player structure, since the player codebase seems to be incompatible with the Media3 approach, although the legacy ExoPlayer library is now officially deprecated and won't receive any more updates. Nobody in the team has worked on supporting Android Auto before, so someone like you would probably add such support in a better and more successful way. Your answer is important for our decision to merge this PR or not. |
| mediaItems.add( | ||
| createRootMediaItem(ID_BOOKMARKS, | ||
| playerService.getResources().getString(R.string.tab_bookmarks))); | ||
| mediaItems.add( | ||
| createRootMediaItem(ID_HISTORY, | ||
| playerService.getResources().getString(R.string.action_history))); |
There was a problem hiding this comment.
NewPipe allows the user to modify the tabs to be shown on the home screen. Can we add similar functionality for the Auto home screen as well. Alternatively, we could decide to reflect the home page tabs for consistency. Everything does not have to be in this CL, just make provision to keep this generic rather than hard-coded options.
@AudricV What do you think
There was a problem hiding this comment.
Yes, this makes sense. I wasn't sure what the best structure for Auto browser would be, so I just started with a couple of examples.
There was a problem hiding this comment.
That's a great idea, but what would happen in the case the user disabled a supported tab or all supported tabs?
There was a problem hiding this comment.
One comment about the root menu: the Android Auto documentation says that in most cases, there's a limit of 3-4 tabs, and additional tabs won't be shown. I haven't tried this, though. It would make it difficult to use the same tab configuration that users have chosen for their home screen on the phone.
| private Single<PlayQueue> extractPlayQueueFromMediaId(final String mediaId) { | ||
| final Uri mediaIdUri = Uri.parse(mediaId); | ||
| if (mediaIdUri == null) { | ||
| return Single.error(new ContentNotAvailableException("Media ID cannot be parsed")); | ||
| } | ||
| if (mediaId.startsWith(ID_BOOKMARKS)) { | ||
| final var path = mediaIdUri.getPathSegments(); | ||
| if (path.size() == 4) { | ||
| final long playlistId = Long.parseLong(path.get(2)); | ||
| final int index = Integer.parseInt(path.get(3)); | ||
|
|
||
| return getPlaylistManager() | ||
| .getPlaylistStreams(playlistId) | ||
| .firstOrError() | ||
| .map(items -> { | ||
| final var infoItems = items.stream() | ||
| .map(PlaylistStreamEntry::toStreamInfoItem) | ||
| .collect(Collectors.toList()); | ||
| return new SinglePlayQueue(infoItems, index); | ||
| }); | ||
| } | ||
| } else if (mediaId.startsWith(ID_STREAM)) { | ||
| final var path = mediaIdUri.getPathSegments(); | ||
| if (path.size() == 3) { | ||
| final long streamId = Long.parseLong(path.get(2)); | ||
| return getDatabase().streamHistoryDAO().getHistory() | ||
| .firstOrError() | ||
| .map(items -> { | ||
| final var infoItems = items.stream() | ||
| .filter(it -> it.getStreamId() == streamId) | ||
| .map(StreamHistoryEntry::toStreamInfoItem) | ||
| .collect(Collectors.toList()); | ||
| return new SinglePlayQueue(infoItems, 0); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return Single.error(new ContentNotAvailableException("Media ID cannot be parsed")); | ||
| } |
There was a problem hiding this comment.
Extraction code is also part of Bookmarks/History Tab fragment classes. Can we put the code in a common util/handler class? That would make future refactoring much easier.
There was a problem hiding this comment.
This extraction code is for the media ID URIs used for interaction between the browser and the player, which I thought are not the same as the URIs NewPipe currently uses for streams. Did NewPipe already have this concept?
I think it makes this code easier if these media IDs were used throughout the app, but it felt like an intrusive change to me.
There was a problem hiding this comment.
I think that's fine. What do you think @Stypox?
|
Hi @AudricV
Sure, I'll be happy to help if I can. |
|
Please do not push any change on your branch associated to this PR, I will push additional changes improving the existing ones and rebasing the PR on top of the upstream (If it wasn't obvious enough, we should merge this PR if it works fine.) |
These objects need to live beyond the player for supporting MediaBrowserServiceCompat and Android Auto, so they need to move outside of the MediaSessionPlayerUi class.
Expose a MediaBrowserService from within the existing PlayerService, and use the existing MediaSession for Auto. Empty media browser for now. To test, one needs to enable "Unknown sources" in Android Auto's developer settings. Issue: TeamNewPipe#1758
Add icon for Auto to manifest, and describe NewPipe as a media app.
… index This happens in the MediaBrowserServiceCompat flow (playing a playlist).
Can be used to play history items.
Also simplify logic in MediaBrowserConnector.extractPlayQueueFromMediaId, suppress some Sonar warnings as they could not be solved, and use explicit types in some variables.
This should avoid some NullPointerExceptions which could be raised in some cases.
They should be displayed only by Android Auto if they are known, i.e. when they are not empty.
|
Apologizes for the late push. I made a few code style changes, including ones that uses explicit types in more places to allow better understanding of the code, annotated more fields and suppressed some Sonar warnings which couldn't be resolved. Regarding the crash described in #9592 (comment), I added more player null checks, which should avoid this I also tried to add icons to root media items, but the icon for the bookmarked playlists one doesn't show on my desktop head unit I used to test my changes. I don't understand why this happen, perhaps that's related to the changes of #7518? This part of the Android Auto documentation looks very related to this issue:
Note that supplying a shorter, an empty or a null media item title doesn't change anything. In addition to the previous changes, I added the uploader name to stream media items as their subtitle, like VLC does for tracks' artist(s). I noticed an important issue with the current implementation: items (playlists and streams) are not updated in Android Auto when they are in the app. I am not sure if the current app architecture allows to fix this issue in a proper way, it would be great if you can investigate this issue and try to fix it @haggaie (do not try hacky things, please, we have already several issues in the codebase of the app, so only fix the issue if that's possible in a proper way). Remote playlists aren't shown in the app, I think I understood your choice to not include them as you seem to hit an API limitation. I don't know if that's possible to add a similar pagination to the one of the app to avoid loading all items of a remote playlist, as we don't want to send thousands of requests within a few seconds for a playlist with 10000 items for instance. Finally, there are two additional issues:
|
|
Kudos, SonarCloud Quality Gate passed! |
I actually see the icon for the history tab on the desktop head unit, but not the one for the playlists tab (see below). It is quite difficult to see, as it is drawn in black over gray. 
|
This might be possible with notifyChildrenChanged. I'll try it out. |
|
It is easy to subscribe to the LocalPlaylistManager's stream and trigger notifications to the media browser when it changes (though I don't know if that's the most efficient way of getting these notifications). However, I don't know which playlists the browser is currently showing, so I can't limit the returned flow to those specific playlists. Can a query be added to the DAO that only provides the playlistId of every modified playlist without the rest of the details? |
I also tested it on my car, and the color of the history tab was fine, it was bright enough. Perhaps the faded color is just an problem with the desktop head unit. The playlist tab icon was still invisible on the car though. |
I checked how VLC does it, and it keeps the last media ID called through onLoadChildren and uses it on the next notification updates. We could do something similar and filter the query on which we subscribe for updates, but it seems unreliable because the browser side could have cached some of the items. I also saw that there are explicit subscribe/unsubscribe events from the browser on media3 (see the onSubscribe method here), so if the plan is to move to media3, it would be simpler to implement these notifications there. |
Hi, I haven't had a chance to work on this PR the last couple of months. I remember there were a couple of comments in the review. If you want to help address them that would be great. |
What is it?
Description of the changes in your PR
Screenshots/Screen Record
Fixes the following issue(s)
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.
Due diligence