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

Removed Android service #903

Closed
wants to merge 2 commits into from
Closed

Conversation

Guichaguri
Copy link
Collaborator

@Guichaguri Guichaguri commented Apr 21, 2020

This PR removes the Android service in favor of the in-app playback.

Pros

Cons

  • Android may possibly kill the app while playing in order to reduce system resource usage. We'll have to figure out how frequently it happens and how to prevent that from happening.
  • The player won't be able to play while the app is closed. This is a feature that is usually disabled through the stopWithApp option by the majority of the apps anyway.
  • This was a big differential from other music modules, as it was closer to a native music app, but I figured that react-native doesn't work properly with services nor the community wants to work with native behavior like this.

As this is a big change on Android, I'd love to see testing here from all sorts of apps.

@Guichaguri Guichaguri added this to the v2 milestone Apr 21, 2020
@Guichaguri Guichaguri changed the base branch from dev to v2 April 21, 2020 00:27
@puckey
Copy link
Collaborator

puckey commented Apr 21, 2020

I would be happy to test this out on some Android phones to see how long it takes for the app to be killed.

However, I do wonder if this is the right direction. For a music player having music stop at random moments feels like a more existential issue than intermittent crashes for a small subset of users. (0.8% in our radio-player case)

Our use-case is a radio app, where I know users can listen to a station for up to 8 hours at a time - the chance that the app would be killed by the system during that time is pretty high.. and for a user it is not a nice experience to have to wonder why their music stopped playing.

Here is a related issue about background-audio on expo, where users report apps being killed after several minutes in the background: expo/expo#4850

@afkcodes
Copy link

@puckey i will love to try your radio app, also i will also be testing the online streaming. i would like to know does your app streams hls content or not as its more prominent on hls (m3u8) links

@puckey
Copy link
Collaborator

puckey commented Apr 21, 2020

@ashishfeels here you go: https://play.google.com/store/apps/details?id=com.jonathanpuckey.radiogarden&hl=nl  We don't support HLS content right now, but it is on our roadmap.

@afkcodes
Copy link

@Guichaguri can we make the error reporting better. i mean the type of error we get while playing the audio. like i was trying to play an hls content and it was giving me liveBehindWindow Error , but on js side it was only giving me playback error. that will help the issue in debugging the problems.

@Guichaguri
Copy link
Collaborator Author

@ashishfeels we definitely should, but that's for another PR.

@afkcodes
Copy link

yes @Guichaguri that's just an idea as i tried to fix in my fork i don't know its correct or not but it works :P

@kyleclegg
Copy link

Happy to test this as well 👍

@samkuhn
Copy link

samkuhn commented May 7, 2020

Sorry left a bit of a snarky message thinking this was the Expo repo lol.

This was a big differential from other music modules, as it was closer to a native music app, but I figured that react-native doesn't work properly with services nor the community wants to work with native behavior like this.

I'm not sure I agree with this and would definitely advise pushing on for full background audio. Expo has been dallying for 3 years on this subject much to everyone's dismay. I created this issue a year ago but ended up just ejecting from Expo and coding the foreground service myself following this example and video.

Our background playback has been working flawlessly since.

@puckey
Copy link
Collaborator

puckey commented May 7, 2020

@samkuhn any chance you could delete your earlier message? It would be nice to keep the tone friendly and encouraging here..

@samkuhn
Copy link

samkuhn commented May 7, 2020

@samkuhn any chance you could delete your earlier message? It would be nice to keep the tone friendly and encouraging here..

Yes I did delete it? I've been through the wars with Expo and I mistakenly thought that this was a pull request for that repo. I apologise.

@Guichaguri
Copy link
Collaborator Author

@samkuhn Removing the Android service doesn't mean background mode won't work anymore, it will still play while the app is open and minimized, but it will stop as soon as the app is swiped from the task list (basically the same behavior as having the old stopWithApp option enabled)

@samkuhn
Copy link

samkuhn commented May 7, 2020

Hi Guichaguri, Wouldn't the audio intermittently cut out while minimized though? That is what I found in my experience with no foreground service running to keep the audio alive (as per ExoPlayer team recommendations).

I'd also like to apologise again for my tone this morning I totally thought this was a different repo and I've been a bit scarred with my android background audio experience there. But even so it was wrong to go in so hard and not my normal style. And definitely wasn't targeted at you @Guichaguri! Good luck with the implementation.

@Anitaislam
Copy link

open pulling

@kyleclegg
Copy link

kyleclegg commented May 27, 2020

How stable is this branch? Should we try shipping it to users from the commit and monitor? Normally I wouldn't suggest shipping from a dev branch but in this case the change addresses a background crash that's flooding our logs, so I'm looking forward to seeing if this fixes it. 🤞

@afkcodes
Copy link

@kyleclegg i am unfortunately not able to test this , but you can really test it by doing an A/B Test may be as the sample size will be quite bigger and if anything comes up up we can fix that before making the changes available to everyone. but its totally your choice. But this will helpful for devs & maintainers.

@Drazail
Copy link

Drazail commented Jun 7, 2020

@Guichaguri

We used a rather similar approach to this at the start of our development cycle, however, Android used to be very aggressive with killing the app so we decided to go with your previous implementation. and so far ( after some minor tweaks) we haven't received any crash reports related to this library in months.

If you got any free time, please checkout my fork, it may provide some context to this comment. ( all of our media endpoints are mp3 files on s3, so this has not been tested for any other media types.)

@biomancer
Copy link
Contributor

biomancer commented Jun 13, 2020

Hi @Guichaguri! For us these changes would mean that we would have to stick to some old version or fork forever, as it seems as there is no way to get tracks to reliably play in background if not using service approach. As you have pointed out, it is a big differential from other music modules and it was actually the main (or even the single) reason, why we started using react-native-track-player in the first place - the other solutions just could not provide reliable background playback.

@bemnet4u
Copy link

Happy to test this.

I already set stopWithApp=true since when the user kills the app, we need to kill audio as it seams the right thing to do...so not too worried about that.

@ashokkumar88
Copy link

I am getting the following error whey trying to play a track.

com.facebook.react.bridge.ReadableNativeMap cannot be cast to com.facebook.react.bridge.JavaOnlyMap

@abdelmagied94
Copy link

@Guichaguri Is the notification capabilities still working? As mentioned in the android docs, MediaButtonReceiver needs a service to receive ACTION_MEDIA_BUTTON intents.

@dcvz dcvz force-pushed the v2 branch 2 times, most recently from f4082ef to cef7d73 Compare May 28, 2021 12:30
@dcvz dcvz removed this from the v2 milestone May 31, 2021
@dcvz
Copy link
Contributor

dcvz commented May 31, 2021

I will close this for now.. at the moment I don't believe the future of this library should be to disable the service.

Overall this is what allows us to build applications that can work in the background and not be killed by the system.. if there's such a need in the future, we'll re-evaluate.

@dcvz dcvz closed this May 31, 2021
@dcvz dcvz deleted the chore/remove-android-service branch April 24, 2022 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment