Rework media player coordination

vector/src/main/java/im/vector/app/core/error/ErrorFormatter.kt

@@ -157,8 +157,7 @@ class DefaultErrorFormatter @Inject constructor(
             RecordingError.BlockedBySomeoneElse -> stringProvider.getString(R.string.error_voice_broadcast_blocked_by_someone_else_message)
             RecordingError.NoPermission -> stringProvider.getString(R.string.error_voice_broadcast_permission_denied_message)
             RecordingError.UserAlreadyBroadcasting -> stringProvider.getString(R.string.error_voice_broadcast_already_in_progress_message)
-            is VoiceBroadcastFailure.ListeningError.UnableToPlay,
+            is VoiceBroadcastFailure.ListeningError -> stringProvider.getString(R.string.error_voice_broadcast_unable_to_play)
-            is VoiceBroadcastFailure.ListeningError.DownloadError -> stringProvider.getString(R.string.error_voice_broadcast_unable_to_play)
         }
     }
 

vector/src/main/java/im/vector/app/features/voicebroadcast/VoiceBroadcastFailure.kt

@@ -31,6 +31,6 @@ sealed class VoiceBroadcastFailure : Throwable() {
          * @property extra an extra code, specific to the error, see [MediaPlayer.OnErrorListener.onError].
          */
         data class UnableToPlay(val what: Int, val extra: Int) : ListeningError()
-        data class DownloadError(override val cause: Throwable?) : ListeningError()
+        data class PrepareMediaPlayerError(override val cause: Throwable? = null) : ListeningError()
     }
 }

vector/src/main/java/im/vector/app/features/voicebroadcast/listening/VoiceBroadcastPlayerImpl.kt

@@ -18,7 +18,6 @@ package im.vector.app.features.voicebroadcast.listening
 
 import android.media.AudioAttributes
 import android.media.MediaPlayer
-import android.media.MediaPlayer.OnPreparedListener
 import androidx.annotation.MainThread
 import im.vector.app.core.di.ActiveSessionHolder
 import im.vector.app.core.extensions.onFirst
@@ -33,10 +32,13 @@ import im.vector.app.features.voicebroadcast.model.VoiceBroadcast
 import im.vector.app.features.voicebroadcast.model.VoiceBroadcastEvent
 import im.vector.app.features.voicebroadcast.usecase.GetVoiceBroadcastStateEventLiveUseCase
 import im.vector.lib.core.utils.timer.CountUpTimer
+import kotlinx.coroutines.CancellationException
+import kotlinx.coroutines.CompletableDeferred
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.flow.launchIn
 import kotlinx.coroutines.flow.onEach
 import kotlinx.coroutines.launch
+import org.matrix.android.sdk.api.extensions.orFalse
 import org.matrix.android.sdk.api.extensions.tryOrNull
 import org.matrix.android.sdk.api.session.room.model.message.MessageAudioContent
 import timber.log.Timber
@@ -63,8 +65,29 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
     private var voiceBroadcastStateObserver: Job? = null
 
     private var currentMediaPlayer: MediaPlayer? = null
+        private set(value) {
+            Timber.d("## Voice Broadcast | currentMediaPlayer changed: old=${field.hashCode()}, new=${value.hashCode()}")
+            field = value
+        }
     private var nextMediaPlayer: MediaPlayer? = null
-    private var isPreparingNextPlayer: Boolean = false
+        private set(value) {
+            Timber.d("## Voice Broadcast | nextMediaPlayer changed:    old=${field.hashCode()}, new=${value.hashCode()}")
+            field = value
+        }
+
+    private var prepareCurrentPlayerJob: Job? = null
+        set(value) {
+            if (field?.isActive.orFalse()) field?.cancel()
+            field = value
+        }
+    private var prepareNextPlayerJob: Job? = null
+        set(value) {
+            if (field?.isActive.orFalse()) field?.cancel()
+            field = value
+        }
+
+    private val isPreparingCurrentPlayer: Boolean get() = prepareCurrentPlayerJob?.isActive.orFalse()
+    private val isPreparingNextPlayer: Boolean get() = prepareNextPlayerJob?.isActive.orFalse()
 
     private var mostRecentVoiceBroadcastEvent: VoiceBroadcastEvent? = null
 
@@ -83,7 +106,7 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
         @MainThread
         set(value) {
             if (field != value) {
-                Timber.d("## Voice Broadcast | playingState: $field -> $value")
+                Timber.d("## Voice Broadcast | playingState: ${field::class.java.simpleName} -> ${value::class.java.simpleName}")
                 field = value
                 onPlayingStateChanged(value)
             }
@@ -174,10 +197,11 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
     }
 
     private fun onPlaylistUpdated() {
+        if (isPreparingCurrentPlayer || isPreparingNextPlayer) return
         when (playingState) {
             State.Playing,
             State.Paused -> {
-                if (nextMediaPlayer == null && !isPreparingNextPlayer) {
+                if (nextMediaPlayer == null) {
                     prepareNextMediaPlayer()
                 }
             }
@@ -206,19 +230,16 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
         val content = playlistItem?.audioEvent?.content ?: run { Timber.w("## Voice Broadcast | No content to play at position $position"); return }
         val sequence = playlistItem.sequence ?: run { Timber.w("## Voice Broadcast | Playlist item has no sequence"); return }
         val sequencePosition = position - playlistItem.startTime
-        sessionScope.launch {
+        prepareCurrentPlayerJob = sessionScope.launch {
             try {
-                prepareMediaPlayer(content) { mp ->
+                val mp = prepareMediaPlayer(content)
-                    currentMediaPlayer = mp
+                playlist.currentSequence = sequence - 1 // will be incremented in onNextMediaPlayerStarted
-                    playlist.currentSequence = sequence
                 mp.start()
                 if (sequencePosition > 0) {
                     mp.seekTo(sequencePosition)
                 }
-                    playingState = State.Playing
+                onNextMediaPlayerStarted(mp)
-                    prepareNextMediaPlayer()
+            } catch (failure: VoiceBroadcastFailure.ListeningError) {
-                }
-            } catch (failure: VoiceBroadcastFailure.ListeningError.DownloadError) {
                 playingState = State.Error(failure)
             }
         }
@@ -261,12 +282,10 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
 
     private fun prepareNextMediaPlayer() {
         val nextItem = playlist.getNextItem()
-        if (nextItem != null) {
+        if (!isPreparingNextPlayer && nextMediaPlayer == null && nextItem != null) {
-            isPreparingNextPlayer = true
+            prepareNextPlayerJob = sessionScope.launch {
-            sessionScope.launch {
                 try {
-                    prepareMediaPlayer(nextItem.audioEvent.content) { mp ->
+                    val mp = prepareMediaPlayer(nextItem.audioEvent.content)
-                        isPreparingNextPlayer = false
                     nextMediaPlayer = mp
                     when (playingState) {
                         State.Playing,
@@ -280,9 +299,7 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
                         is State.Error,
                         State.Idle -> stopPlayer()
                     }
-                    }
+                } catch (failure: VoiceBroadcastFailure.ListeningError) {
-                } catch (failure: VoiceBroadcastFailure.ListeningError.DownloadError) {
-                    isPreparingNextPlayer = false
                     // Do not change the playingState if the current player is still valid,
                     // the error will be thrown again when switching to the next player
                     if (playingState == State.Buffering || tryOrNull { currentMediaPlayer?.isPlaying } != true) {
@@ -293,18 +310,30 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
         }
     }
 
-    private suspend fun prepareMediaPlayer(messageAudioContent: MessageAudioContent, onPreparedListener: OnPreparedListener): MediaPlayer {
+    /**
+     * Create and prepare a [MediaPlayer] instance for the given [messageAudioContent].
+     * This methods takes care of downloading the audio file and returns the player when it is ready to use.
+     *
+     * Do not forget to release the resulting player when you don't need it anymore, in case you cancel the job related to this method, the player will be
+     * automatically released.
+     */
+    private suspend fun prepareMediaPlayer(messageAudioContent: MessageAudioContent): MediaPlayer {
         // Download can fail
         val audioFile = try {
             session.fileService().downloadFile(messageAudioContent)
         } catch (failure: Throwable) {
             Timber.e(failure, "Voice Broadcast | Download has failed: $failure")
-            throw VoiceBroadcastFailure.ListeningError.DownloadError(failure)
+            throw VoiceBroadcastFailure.ListeningError.PrepareMediaPlayerError(failure)
         }
 
-        return audioFile.inputStream().use { fis ->
+        val latch = CompletableDeferred<MediaPlayer>()
-            MediaPlayer().apply {
+        val mp = MediaPlayer()
-                setOnErrorListener(mediaPlayerListener)
+        return try {
+            mp.apply {
+                setOnErrorListener { mp, what, extra ->
+                    mediaPlayerListener.onError(mp, what, extra)
+                    latch.completeExceptionally(VoiceBroadcastFailure.ListeningError.PrepareMediaPlayerError())
+                }
                 setAudioAttributes(
                         AudioAttributes.Builder()
                                 // Do not use CONTENT_TYPE_SPEECH / USAGE_VOICE_COMMUNICATION because we want to play loud here
@@ -312,12 +341,16 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
                                 .setUsage(AudioAttributes.USAGE_MEDIA)
                                 .build()
                 )
-                setDataSource(fis.fd)
+                audioFile.inputStream().use { fis -> setDataSource(fis.fd) }
                 setOnInfoListener(mediaPlayerListener)
-                setOnPreparedListener(onPreparedListener)
+                setOnPreparedListener(latch::complete)
                 setOnCompletionListener(mediaPlayerListener)
                 prepareAsync()
             }
+            latch.await()
+        } catch (e: CancellationException) {
+            mp.release()
+            throw e
         }
     }
 
@@ -328,7 +361,9 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
 
         nextMediaPlayer?.release()
         nextMediaPlayer = null
-        isPreparingNextPlayer = false
+
+        prepareCurrentPlayerJob = null
+        prepareNextPlayerJob = null
     }
 
     private fun onPlayingStateChanged(playingState: State) {
@@ -440,7 +475,10 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
             if (currentMediaPlayer == mp) {
                 currentMediaPlayer = null
             } else {
-                error("The media player which has completed mismatches the current media player instance.")
+                Timber.w(
+                        "## Voice Broadcast | onCompletion: The media player which has completed mismatches the current media player instance.\n" +
+                                "currentMediaPlayer=${currentMediaPlayer.hashCode()}, mp=${mp.hashCode()}"
+                )
             }
 
             // Next media player is already attached to this player and will start playing automatically
@@ -459,7 +497,7 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
         }
 
         override fun onError(mp: MediaPlayer, what: Int, extra: Int): Boolean {
-            Timber.d("## Voice Broadcast | onError: what=$what, extra=$extra")
+            Timber.w("## Voice Broadcast | onError: what=$what, extra=$extra")
             // Do not change the playingState if the current player is still valid,
             // the error will be thrown again when switching to the next player
             if (playingState == State.Buffering || tryOrNull { currentMediaPlayer?.isPlaying } != true) {