Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package com.google.jetstream.presentation.screens.videoPlayer

import android.content.Context
import androidx.media3.common.C
import androidx.media3.common.Player
import androidx.media3.common.util.UnstableApi
import androidx.media3.datasource.DefaultDataSource
import androidx.media3.exoplayer.ExoPlayer
import androidx.media3.exoplayer.source.ProgressiveMediaSource
import com.google.jetstream.data.entities.MovieDetails
import dagger.hilt.android.qualifiers.ApplicationContext
import javax.inject.Inject

@UnstableApi
// Do not make this a singleton to prevent Released player from Being invoked for play

/**
* 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.

Copy link
Copy Markdown

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.

*/
class VideoPlayerManager @Inject constructor(
@ApplicationContext private val context: Context
) {
private var _exoPlayer: ExoPlayer? = ExoPlayer.Builder(context)
.setSeekForwardIncrementMs(10)
.setSeekBackIncrementMs(10)

Copy link
Copy Markdown

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)

.setMediaSourceFactory(
ProgressiveMediaSource.Factory(DefaultDataSource.Factory(context))
)
.setVideoScalingMode(C.VIDEO_SCALING_MODE_SCALE_TO_FIT_WITH_CROPPING)
.build().apply {
playWhenReady = true
repeatMode = Player.REPEAT_MODE_OFF
}

val player: ExoPlayer
get() = _exoPlayer ?: throw IllegalStateException("Player has been released")

fun load(movieDetails: MovieDetails) {
player.apply {
stop()
clearMediaItems()
addMediaItem(movieDetails.intoMediaItem())
movieDetails.similarMovies.forEach { addMediaItem(it.intoMediaItem()) }
prepare()
}
}

fun release() {
_exoPlayer?.release()
_exoPlayer = null
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package com.google.jetstream.presentation.screens.videoPlayer

import android.net.Uri
import androidx.activity.compose.BackHandler
import androidx.annotation.OptIn
import androidx.compose.foundation.focusable
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.fillMaxSize
Expand Down Expand Up @@ -50,10 +51,10 @@ import com.google.jetstream.presentation.screens.videoPlayer.components.VideoPla
import com.google.jetstream.presentation.screens.videoPlayer.components.VideoPlayerPulse.Type.FORWARD
import com.google.jetstream.presentation.screens.videoPlayer.components.VideoPlayerPulseState
import com.google.jetstream.presentation.screens.videoPlayer.components.VideoPlayerState
import com.google.jetstream.presentation.screens.videoPlayer.components.rememberPlayer
import com.google.jetstream.presentation.screens.videoPlayer.components.rememberVideoPlayerPulseState
import com.google.jetstream.presentation.screens.videoPlayer.components.rememberVideoPlayerState
import com.google.jetstream.presentation.utils.handleDPadKeyEvents
import androidx.core.net.toUri

object VideoPlayerScreen {
const val MovieIdBundleKey = "movieId"
Expand All @@ -63,40 +64,37 @@ object VideoPlayerScreen {
* [Work in progress] A composable screen for playing a video.
*
* @param onBackPressed The callback to invoke when the user presses the back button.
* @param videoPlayerScreenViewModel The view model for the video player screen.
* @param VideoPlayerScreenViewModel The view model for the video player screen.
*/
@OptIn(UnstableApi::class)
@Composable
fun VideoPlayerScreen(
onBackPressed: () -> Unit,
videoPlayerScreenViewModel: VideoPlayerScreenViewModel = hiltViewModel()
viewModel: VideoPlayerScreenViewModel = hiltViewModel()
) {
val uiState by videoPlayerScreenViewModel.uiState.collectAsStateWithLifecycle()

// TODO: Handle Loading & Error states
when (val s = uiState) {
is VideoPlayerScreenUiState.Loading -> {
Loading(modifier = Modifier.fillMaxSize())
}

is VideoPlayerScreenUiState.Error -> {
Error(modifier = Modifier.fillMaxSize())
}
val uiState by viewModel.uiState.collectAsStateWithLifecycle()

when (val state = uiState) {
is VideoPlayerScreenUiState.Loading -> Loading(modifier = Modifier.fillMaxSize())
is VideoPlayerScreenUiState.Error -> Error(modifier = Modifier.fillMaxSize())
is VideoPlayerScreenUiState.Done -> {
VideoPlayerScreenContent(
movieDetails = s.movieDetails,
movieDetails = state.movieDetails,
exoPlayer = viewModel.player,
onBackPressed = onBackPressed
)
}
}
}

@androidx.annotation.OptIn(UnstableApi::class)
@Composable
fun VideoPlayerScreenContent(movieDetails: MovieDetails, onBackPressed: () -> Unit) {
val context = LocalContext.current
val exoPlayer = rememberPlayer(context)

@OptIn(UnstableApi::class)
@Composable
fun VideoPlayerScreenContent(
movieDetails: MovieDetails,
onBackPressed: () -> Unit,
exoPlayer: ExoPlayer
) {
val videoPlayerState = rememberVideoPlayerState(
hideSeconds = 4,
)
Expand Down Expand Up @@ -177,7 +175,7 @@ private fun Modifier.dPadEvents(
}
)

private fun MovieDetails.intoMediaItem(): MediaItem {
fun MovieDetails.intoMediaItem(): MediaItem {
return MediaItem.Builder()
.setUri(videoUri)
.setSubtitleConfigurations(
Expand All @@ -186,7 +184,7 @@ private fun MovieDetails.intoMediaItem(): MediaItem {
} else {
listOf(
MediaItem.SubtitleConfiguration
.Builder(Uri.parse(subtitleUri))
.Builder(subtitleUri.toUri())
.setMimeType("application/vtt")
.setLanguage("en")
.setSelectionFlags(C.SELECTION_FLAG_DEFAULT)
Expand All @@ -196,7 +194,7 @@ private fun MovieDetails.intoMediaItem(): MediaItem {
).build()
}

private fun Movie.intoMediaItem(): MediaItem {
fun Movie.intoMediaItem(): MediaItem {
return MediaItem.Builder()
.setUri(videoUri)
.setSubtitleConfigurations(
Expand All @@ -205,7 +203,7 @@ private fun Movie.intoMediaItem(): MediaItem {
} else {
listOf(
MediaItem.SubtitleConfiguration
.Builder(Uri.parse(subtitleUri))
.Builder(subtitleUri.toUri())
.setMimeType("application/vtt")
.setLanguage("en")
.setSelectionFlags(C.SELECTION_FLAG_DEFAULT)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,39 +16,66 @@

package com.google.jetstream.presentation.screens.videoPlayer

import androidx.annotation.OptIn
import androidx.compose.runtime.Immutable
import androidx.lifecycle.SavedStateHandle
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import androidx.media3.common.util.UnstableApi
import androidx.media3.exoplayer.ExoPlayer
import com.google.jetstream.data.entities.MovieDetails
import com.google.jetstream.data.repositories.MovieRepository
import dagger.hilt.android.lifecycle.HiltViewModel
import javax.inject.Inject
import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.stateIn

@UnstableApi
@HiltViewModel
class VideoPlayerScreenViewModel @Inject constructor(
@OptIn(UnstableApi::class)
class VideoPlayerScreenViewModel
@Inject constructor(
savedStateHandle: SavedStateHandle,
repository: MovieRepository,
private val repository: MovieRepository,
private val playerManager: VideoPlayerManager
) : ViewModel() {
val uiState = savedStateHandle
.getStateFlow<String?>(VideoPlayerScreen.MovieIdBundleKey, null)

private val movieIdFlow = savedStateHandle.getStateFlow<String?>(
VideoPlayerScreen.MovieIdBundleKey,
null
)

val uiState: StateFlow<VideoPlayerScreenUiState> = movieIdFlow
.map { id ->
if (id == null) {
VideoPlayerScreenUiState.Error
} else {
val details = repository.getMovieDetails(movieId = id)
VideoPlayerScreenUiState.Done(movieDetails = details)
try {
val details = repository.getMovieDetails(id)
playerManager.load(details)

Copy link
Copy Markdown

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.

VideoPlayerScreenUiState.Done(details)
} catch (e: Exception) {
VideoPlayerScreenUiState.Error
}
Comment on lines +55 to +68

Copy link
Copy Markdown

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
                }

}
}.stateIn(
}
.stateIn(
scope = viewModelScope,
started = SharingStarted.WhileSubscribed(5_000),
initialValue = VideoPlayerScreenUiState.Loading
)

val player: ExoPlayer get() = playerManager.player

override fun onCleared() {
super.onCleared()
playerManager.release()
}
}


@Immutable
sealed class VideoPlayerScreenUiState {
data object Loading : VideoPlayerScreenUiState()
Expand Down