fix(android): HLS DVR media should update playable duration in the background #4105
Conversation
…ation channel until onDestroy
See https://developer.android.com/develop/background-work/services/foreground-services\#fgs-prerequisites
See rationale here https://stackoverflow.com/questions/45525214/are-there-any-benefits-to-using-context-startforegroundserviceintent-instead-o
Deleting the notification channel while the foreground service is still running is not permitted.
| @@ -95,6 +96,10 @@ class VideoPlaybackService : MediaSessionService() { | |||
|
|
|||
| override fun onDestroy() { | |||
| cleanup() | |||
| val notificationManager: NotificationManager = getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager | |||
There was a problem hiding this comment.
won't it be better to move this part of code inside cleanup function ? clean up is used more intensivelly then onDestroy ...
There was a problem hiding this comment.
I think that would cause a problem because the foreground service/notification could be still alive when cleanup() is ran. I can test this though and report back.
There was a problem hiding this comment.
In fact I am disturbed by the logic.
Notification channel is created in onUpdateNotification and destroyed in onDestroy
For exemple shouldn't you add this code also in : (which calls cleanup)
unregisterPlayer(player: ExoPlayer) {
in fact (maybe), you need to replace cleanup() by hideAllNotifications() in onTaskRemoved and unregisterPlayer() ?
Notice that if you unregisterPlayer and registerPlayer just after the notification channel is not re created...
onDestroy seems ok for me to cleanup.
There was a problem hiding this comment.
@paulrinaldi gentle reminder on this point, It would be great to merge this PR for next release !
|
should decrease repeatability of #3897 |
|
@freeboub @paulrinaldi my team has had tons of users reporting this issue, could we please get this fix merged so we can update with the fix? Thanks |
|
Our app is experiencing this issue and we really need this to improve our UX. Any idea when we can expect a fix? |
|
Hi, |
|
I have one remaining comment, but I am Ok to merge it. |
|
Currently using a patch with this as the fix. In a submission to the play store, we did see some crashes when a test user from google play seemed to create a race between reusing and destroying a media session and when trying to play two videos at once. I would recommend we carefully consider the destruction and creation method details. |
|
@paulrinaldi Thank you for the feedback! I think the 2 video in parallel is not necessary a usual use case. |
There was a problem hiding this comment.
It Okay for me, would be nice to resolve @freeboub comment but not required for this PR
|
The patch I'm using is still equivalent to the changes in these commits. |
…ation channel until onDestroy (TheWidlarzGroup#4105) See https://developer.android.com/develop/background-work/services/foreground-services\#fgs-prerequisites See rationale here https://stackoverflow.com/questions/45525214/are-there-any-benefits-to-using-context-startforegroundserviceintent-instead-o Deleting the notification channel while the foreground service is still running is not permitted.
e.g. wait until onDestroy
When unmounting a component holding the the videoplayer from RNV component, I saw an error that crashed my app: deleting the notification channel while the foreground service is still running is not allowed.
Summary
Use service starting methods properly. Remove notification channel properly.
Motivation
The exploration started from looking in to #4039 which describes that HLS DVR streams stop loading playable duration when in the backgrond.
Add
startForegroundServicesince the foreground service seemed to not be working correctly, warning logs I saw about buffer deallocations and windows closing and observations above in #4039, and https://stackoverflow.com/questions/45525214/are-there-any-benefits-to-using-context-startforegroundserviceintent-instead-o. AddstartForeground(required by usingstartForegroundService).Changes
See PR diff.
Test plan
Test on physical android devices with the diff below and HLS DVR streams ("https://democracynow-hls.secdn.net/democracynow-live/play/democracynow.smil/playlist.m3u8", "https://5b44cf20b0388.streamlock.net:8443/dvr/dvr/playlist.m3u8?DVR").
Open the
examples/basicvideoplayer project, edit with this diff:background.patch
Also add HLS DVR streams to
general.tsto view other than the HLS nonDVR redbull stream.