Skip to content

Conversation

bensalcie
Copy link

@bensalcie bensalcie commented Jul 1, 2025

Here is a list of Improvements this PR Tends to Fix.

  • Scoped ExoPlayer to ViewModel to prevent reuse after release
  • Removed singleton usage to avoid dead thread crashes
  • Ensured ExoPlayer releases on screen exit via onCleared()
  • Fixed bug where video continued playing after user left the app
  • Improved separation of concerns via VideoPlayerManager abstraction

Closes #206

Previous Behavior

  1. Initiating a new play, previous play continue and and they play concurrently.
  2. When user Quits the Jetstream app, the video continues playing in background as Audio.
  3. User is unable to tell where to stop it whenever it continues to play after app is quit.

Expected behavior

  1. Player instance is one. One single play at a time. Initiating another play, disposes previous and begins another correctly.
  2. Player releases whenever user quits app and does not continue playing in the background.
image image

Copy link

google-cla bot commented Jul 1, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @bensalcie, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the video player architecture to improve resource management, prevent concurrent playback issues, and ensure proper player lifecycle handling. The changes introduce a dedicated VideoPlayerManager to encapsulate ExoPlayer logic, scope the player to the ViewModel, and ensure its release when the user exits the video screen or the app, thereby fixing the persistent background audio playback bug.

Highlights

  • Player Architecture Refactor: Introduced a new VideoPlayerManager class to centralize ExoPlayer instance creation, media loading, and release. This improves separation of concerns by abstracting player logic away from the UI Composables and ViewModel.
  • ViewModel-Scoped Player Lifecycle: The ExoPlayer instance is now managed within the VideoPlayerScreenViewModel via the VideoPlayerManager. The player is explicitly released in the ViewModel's onCleared() method, ensuring proper resource cleanup when the user navigates away from the video screen or the app is closed, which fixes the bug where video continued playing in the background.
  • Elimination of Concurrent Playback: The updated architecture ensures that only one ExoPlayer instance is active at a time. When a new video is loaded, the previous player state is cleared, preventing multiple videos from playing concurrently.
  • Improved Media Item Handling: The VideoPlayerScreenViewModel now takes responsibility for fetching movie details and subsequently instructing the VideoPlayerManager to load the appropriate media items, centralizing the data flow for video playback.
  • Dependency Injection Update: The VideoPlayerScreenViewModel has been updated to inject the newly introduced VideoPlayerManager, leveraging Hilt for proper dependency management and testability.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the video player, addressing critical bugs related to background playback and player lifecycle management. The introduction of VideoPlayerManager and scoping the player to the ViewModel is a solid architectural improvement. I have provided comments with suggestions for improving the KDoc, seek increment values, exception handling, and separation of concerns.

Comment on lines 55 to 61
try {
val details = repository.getMovieDetails(id)
playerManager.load(details)
VideoPlayerScreenUiState.Done(details)
} catch (e: Exception) {
VideoPlayerScreenUiState.Error
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Catching a generic Exception is risky as it can catch and swallow CancellationException, which is used by Kotlin Coroutines for cancellation. This can lead to unresponsive components, resource leaks, and other hard-to-debug issues. It's best practice to explicitly handle CancellationException by re-throwing it.

                try {
                    val details = repository.getMovieDetails(id)
                    playerManager.load(details)
                    VideoPlayerScreenUiState.Done(details)
                } catch (e: kotlinx.coroutines.CancellationException) {
                    throw e
                } catch (e: Exception) {
                    VideoPlayerScreenUiState.Error
                }

* A manager for the video player.
* This class is responsible for managing the video playback using the ExoPlayer library.
*
* @param VideoPlayerManager Creates an Instance of the player and it available across app for Re-use.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The KDoc for the class contains an incorrect @param tag. The class constructor's parameter is context, not VideoPlayerManager. This should be corrected to accurately document the constructor parameter and improve clarity.

Suggested change
* @param VideoPlayerManager Creates an Instance of the player and it available across app for Re-use.
* @param context The application context, used to create the ExoPlayer instance.

Comment on lines 27 to 28
.setSeekForwardIncrementMs(10)
.setSeekBackIncrementMs(10)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The seek increment values are set to 10 milliseconds, which is too short to be practical for user-controlled seeking. This is likely a typo and should probably be 10 seconds (10,000ms) to provide a better user experience.

Suggested change
.setSeekForwardIncrementMs(10)
.setSeekBackIncrementMs(10)
.setSeekForwardIncrementMs(10000)
.setSeekBackIncrementMs(10000)

VideoPlayerScreenUiState.Done(movieDetails = details)
try {
val details = repository.getMovieDetails(id)
playerManager.load(details)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Calling playerManager.load(details) here introduces a side effect inside a map operator. The map operator is intended for pure data transformations. Mixing side effects can make the code harder to reason about and test. Consider moving this side effect to an onEach operator after the map block to improve separation of concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JetStream: Video Player Continues to Run even when app is Quit/ Page is Removed from Foreground
1 participant