-
Notifications
You must be signed in to change notification settings - Fork 14
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: support Media Controls #539
Android: support Media Controls #539
Conversation
To implement lock-screen controls, a player instance must be 'kept alive', so that the player is not garbage-collected or destroyed by the system when the view is destroyed. There are 2 ways to do this: - init the player on the service itself (like in the Android SDK sample) - init the player as usual, and somehow pass it to the service, to store an additional reference This first version implements the second option. The app has 1 strong reference, and the service has 1 strong reference to the player. So that whenever the app dies, the background playback still goes on. Current Issues: - Whenever the app gets back, the player instance should get fetched back as well! - Another limitation with this implementation is that we're calling `setupMediaSession` on `player.ts`'s `inizitalize()`. But when changing sources (e.g. playback view, click back, playback view), the source in the media session does not get overridden with the source from the new view.
Via native by using player module Note: The actual media session module can just be a native one (like offline module) as it is just called via native (player module)
This module will be Android-only.
The old player was not getting destroyed when a new one is being put in charge of the media session
This has to be handled via the activity lifecycle. Took `BackgroundPlaybackScreen` from Android SDK samples as the example Also fix `playerEventRelay` being null in some conditions
As modules represent something native that can be re-used from the JS side, but this is not the case here
EVENT_CLASS_TO_REACT_NATIVE_NAME_MAPPING, | ||
::emitEventFromPlayer, | ||
) | ||
|
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 had to move playerEventRelay
initialization here because it was null
during the first invocations of onStop
in the activity lifecycle.
This is due to the custom setPlayer
we have, which also sets playerEventRelay
Limitation section will need an update once the lock-screen implementation into Android SDK is finished
src/lockScreenControlConfig.ts
Outdated
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.
Note that the Limitations section is not modified.
It will be update once the lock-screen feature is fully released on the Android SDK
A 'fake' media session was being initialized here because the Android SDK by a design mistake had mediaSession as not nullable.
android/src/main/java/com/bitmovin/player/reactnative/RNPlayerView.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/bitmovin/player/reactnative/converter/JsonConverter.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/bitmovin/player/reactnative/services/MediaSessionPlaybackService.kt
Show resolved
Hide resolved
android/src/main/java/com/bitmovin/player/reactnative/services/MediaSessionPlaybackService.kt
Show resolved
Hide resolved
Pair-programming session w Mario Improve general player view and player management Fix: - Restore the correct player from media session - Media Session not getting destroyed as the `@ReactMethod onDestroy()` in `PlayerModule.kt` was nullifying the media session manager inside the promise block. However, the main thread was already killed from JS, so that code was unreached Spot and comment a bug in `PlayerView` creation in Android SDK side
As this config is not existing on Native, we have to set a default value directly here Because it may not be there in the Json
Address with the changes to state-management in 36e88e6 |
@@ -25,6 +26,8 @@ class PlayerModule(context: ReactApplicationContext) : BitmovinBaseModule(contex | |||
*/ | |||
private val players: Registry<Player> = mutableMapOf() | |||
|
|||
var mediaSessionPlaybackManager: MediaSessionPlaybackManager? = 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.
As discussed offline, let's init the mediaSessionPlaybackManager
once here, instead of every time there is a player created with media controls support enabled.
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.
@@ -296,6 +297,7 @@ class RNPlayerViewManager(private val context: ReactApplicationContext) : Simple | |||
block() | |||
} catch (e: Exception) { | |||
Log.e(MODULE_NAME, "Error while executing command", e) | |||
// TODO: re throw |
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.
Please remove and create ticket for this
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.
Address in 5549678
@@ -190,6 +215,7 @@ class RNPlayerView( | |||
// is responsible for destroying the player in the end. | |||
playerView.player = null | |||
playerView.onDestroy() | |||
(playerView.parent as? ViewGroup)?.removeView(playerView) |
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.
That was just for debugging, please remove
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.
Address in 5549678
Since 6d62572 a patch is no longer needed for testing as the Media Session API is now part of a stable release |
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.
Great work 💪 Only some nit left from my side.
@@ -4,15 +4,16 @@ | |||
|
|||
### Added | |||
|
|||
- `LockScreenControlConfig` to configure the lock screen information for the application. When `isEnabled` is `true`, the current media information will be shown on the lock-screen and within the control center | |||
- `MediaControlConfig` to configure the media control information for the application. When `isEnabled` is `true`, the current media information will be shown on the lock-screen, in notifications, and within the control center |
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.
Nit: Maybe a bit more specific?
- `MediaControlConfig` to configure the media control information for the application. When `isEnabled` is `true`, the current media information will be shown on the lock-screen, in notifications, and within the control center | |
- `MediaControlConfig` to configure the media control information for the application. When `isEnabled` is `true`, the current media information will be shown on the lock-screen and the Control Center on iOS and as a notification on Android |
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'd prefer to keep it as it is since the widget also pops up in the lock-screen in Android as well
@@ -211,6 +222,7 @@ class PlayerModule(context: ReactApplicationContext) : BitmovinBaseModule(contex | |||
*/ | |||
@ReactMethod | |||
fun destroy(nativeId: NativeId, promise: Promise) { | |||
mediaSessionPlaybackManager.destroy(nativeId) |
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.
Should this be inside the resolveOnUiThreadWithPlayer block?
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.
We spotted a bug here: the media session was not destroyed if put inside the resolve block.
The issue is that, at this point, the main thread is already killed from JS, so if we put this inside the resolve block, the code is not reached
// remove player from view so it does not get paused when entering background | ||
private fun removePlayerForBackgroundPlayback() { | ||
playerInMediaSessionService = null | ||
playerView?.player?.let { |
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.
Nit: I'm not a big fan of using let
for nullability checks as the code gets an extra indentation, consider replacing it with:
val player = playerView?.player ?: return
It's also generally nice to avoid it
and name the parameter if the code is more than 1 line.
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.
Address in b12d6b0
134e11e
into
feature/base-enable-lock-screen-controls
Description
Implement the media controls in Android (via the MediaSession API) to make the system display the current media information on the lock-screen, in notifications, and within the control-center.
Additionally, this PR also introduces background-playback support
PR-planning
Both will be merged together on a base branch
Overview pics
Changes
Implement media controls
MediaControlConfig
Implement Background playback
playerEventRelay
being null in some scenariosChanges to samples
To allows having playback without the main app being opened
Note
Note: both samples will destroy the player instance (hence destroying the media control widgets) when going back to the homepage. That is expected, a more advanced sample could be part of a follw-up (e.g. letting the widget live in the homescreen as well, until we open a sample with media session disabled).
Config Flags Design
MediaControlConfig.isEnabled
decides whether the service starts or notPlayerConfig.PlaybackConfig.isBackgroundPlaybackEnabled
decides whether to detach the player from the view, basically handling whether the playback should automatically pause (when set tofalse
) or not (when set totrue
)MediaControlConfig.isEnabled
has to betrue
for background playback to work, since it needs a notificationTesting
Please do some manual testing, with a physical device if possible 🙏
Things to look for
Media Controls (use any Media Control sample apart from Background Playback)
Background Playback feature (use the new "Background Playback" sample)
MediaControlConfig
)Activity lifecycle changes
Checklist
CHANGELOG
entry -entry added already in base PR