-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add support for Android Auto #9592
base: dev
Are you sure you want to change the base?
Conversation
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 "waze navigation" map app you are using, but they just don't show it anywhere as then, users will start abandoning even waze.
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. |
bc2ba86
to
bec3408
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
This is similar to how they are shown in the app UI.
Also improve parser code to simplify passing URLs within a media ID.
@haggaie great to see this PR active again. I would love to review final changes. I have some updates in mind but will wait for your changes to finalize first. |
Thanks @snaik20, I think the code is in a good place to finalize, at least in terms of features. I fixed the tab color issue, added remote playlists to the playlist tab, and added a search function. I don't think there is other functionality missing for a first PR of this feature. Of course, there's always room for improvement, and I imagine the implementation could be made cleaner by integrating it deeper into the rest of NewPipe. For that, I'm sure I'll need feedback from the codebase's experts. |
This comment was marked as resolved.
This comment was marked as resolved.
Ahh, got busy elsewhere. Will get to it today, thanks. |
Sorry for the delay. I have gone through the changes. Looking great. I just updated the PR with some kotlin conversions for new files and fixed own comments. I have not been able to test the PR though. Feel free to update my changes if found any issues in testing. |
No problem, thanks for the review. I've tested it locally (with the desktop head unit) and fixed a few issues. I'll try testing it in my car in the coming days. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Hi @snaik20, I've tested the PR again both with the desktop head unit and in my car, and it worked fine. |
I've been using it in my car, as well, with no problems. |
I've experienced a few bugs, but I think they are the same behavior as the current public release, and not directly related to the Android Auto integration. Nothing new. Works great. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and testing. Approved the changes.
Had a couple of errors over the last several days, but I can't repeat the errors. First one was an error on starting it with Android Auto, the second was when swiping to the suggested list. Haven't happened since. Everything is working great. |
Here is a summary of the current bugs I found: Coming from #9592 (comment):
Other issues:
Everything else I can test seem to work properly. I can't test search, since it seems to require Google Assistant and voice to do so, and I don't want to use it. If you have an other way to do a search with the DHU, please let me know. For improvements, here is what should be done in others PRs:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for maintaining this PR for so much time!
I have some comments on the current code changes, a part of them concern @snaik20's changes in this PR.
It would be really great if you can update your PR description to mention what is supported, what is not supported, what are the limitations and the known bugs, so we can open issues to track and fix/implement them in an easier way.
Maybe important code changes should be added too to the description.
The changes look good overall for me.
val service: PlayerService? | ||
get() = playerService.get() | ||
|
||
fun getPlayer(): Player = service?.player ?: throw Error("Player service is null") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why a simple Error
is thrown in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose NullPointerException would be more appropriate? Or returning null?
app/src/main/java/org/schabi/newpipe/player/mediabrowser/MediaBrowserConnector.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/player/mediabrowser/MediaBrowserConnector.kt
Show resolved
Hide resolved
private fun handleSearchError(throwable: Throwable) { | ||
Log.e(TAG, "Search error: $throwable") | ||
disposePrepareOrPlayCommands() | ||
playbackError(R.string.content_not_supported, PlaybackStateCompat.ERROR_CODE_NOT_SUPPORTED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is a content not supported error raised when a search error occurs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure how to translate the possible internal errors, so I just picked one. I suppose we could generate ERROR_CODE_ACTION_ABORTED or another value as a general error for search errors, or alternatively try to translate the individual potential errors. Do you know if the extractor has a list of potential exceptions?
app/src/main/java/org/schabi/newpipe/player/mediabrowser/MediaBrowserConnector.kt
Outdated
Show resolved
Hide resolved
Thanks @AudricV, I'll update the PR text with a list of open issues.
I'm afraid I haven't seen this. When I connect the DHU the playback simply stops, but playback controls are immediately shown. Perhaps the error depends on some specific state of the app or the media session.
Could you explain the expected behavior, and where this square format setting is configured in the DHU?
The search I added to the PR doesn't work with Google Assistant, as Google Assistant seems to only work with Google Play Store's apps (I'll add that to the PR description). I added a search that appears from inside the app's browser view within Android Auto. Basically, you click the app icon and then the magnifying glass at the top. I haven't tried it recently, but I believe you could test the same interfaces used by Android Auto and Google Assistant also with the Media Controller Test app (https://developer.android.com/media/optimize/mct), but note that it should have separate features prepare/playFromSearch for the Google Assistant interface and "test results" that I hope cover the android auto search feature. |
Quality Gate passedIssues Measures |
What is it?
Description of the changes in your PR
Issues, bugs, and missing features
Future enhancements
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