Attempt at fixing media3 bugs (#413)

Author: qnga

readium/lcp/src/main/java/org/readium/r2/lcp/LcpDownloadsRepository.kt

@@ -9,8 +9,10 @@ package org.readium.r2.lcp
 import android.content.Context
 import java.io.File
 import java.util.LinkedList
+import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Deferred
 import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.MainScope
 import kotlinx.coroutines.async
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.withContext
@@ -20,31 +22,35 @@ import org.readium.r2.shared.util.CoroutineQueue
 internal class LcpDownloadsRepository(
     context: Context
 ) {
-    private val queue = CoroutineQueue()
+    private val coroutineScope: CoroutineScope =
+        MainScope()
+
+    private val queue: CoroutineQueue =
+        CoroutineQueue()
 
     private val storageDir: Deferred<File> =
-        queue.scope.async {
+        coroutineScope.async {
             withContext(Dispatchers.IO) {
                 File(context.noBackupFilesDir, LcpDownloadsRepository::class.qualifiedName!!)
                     .also { if (!it.exists()) it.mkdirs() }
             }
         }
 
     private val storageFile: Deferred<File> =
-        queue.scope.async {
+        coroutineScope.async {
             withContext(Dispatchers.IO) {
                 File(storageDir.await(), "licenses.json")
                     .also { if (!it.exists()) { it.writeText("{}", Charsets.UTF_8) } }
             }
         }
 
     private val snapshot: Deferred<MutableMap<String, JSONObject>> =
-        queue.scope.async {
+        coroutineScope.async {
             readSnapshot().toMutableMap()
         }
 
     fun addDownload(id: String, license: JSONObject) {
-        queue.scope.launch {
+        coroutineScope.launch {
             val snapshotCompleted = snapshot.await()
             snapshotCompleted[id] = license
             writeSnapshot(snapshotCompleted)

readium/navigators/media/tts/src/main/java/org/readium/navigator/media/tts/TtsNavigator.kt

@@ -229,6 +229,7 @@ public class TtsNavigator<S : TtsEngine.Settings, P : TtsEngine.Preferences<P>,
 
     override fun close() {
         player.close()
+        sessionAdapter.release()
     }
 
     override val currentLocator: StateFlow<Locator> =

readium/navigators/media/tts/src/main/java/org/readium/navigator/media/tts/TtsPlayer.kt

@@ -194,10 +194,26 @@ internal class TtsPlayer<S : TtsEngine.Settings, P : TtsEngine.Preferences<P>,
     }
 
     fun play() {
+        // This can be called by the session adapter with a pending intent for a foreground service
+        // if playWhenReady is false or the state is Ended.
+        // We must keep or transition to a state which will be translated by media3 to a
+        // foreground state.
+        // If the state was State.Ended, it will get back to its initial value later.
+
+        if (playbackMutable.value.playWhenReady && playback.value.state == State.Ready) {
+            return
+        }
+
+        playbackMutable.value =
+            playbackMutable.value.copy(state = State.Ready, playWhenReady = true)
+
         coroutineScope.launch {
-            // WORKAROUND to get the media buttons correctly working.
-            fakePlayingAudio()
-            playAsync()
+            mutex.withLock {
+                // WORKAROUND to get the media buttons correctly working when an audio player was
+                // running before.
+                fakePlayingAudio()
+                playIfReadyAndNotPaused()
+            }
         }
     }
 
@@ -240,30 +256,19 @@ internal class TtsPlayer<S : TtsEngine.Settings, P : TtsEngine.Preferences<P>,
             ?: run { Timber.e("Couldn't fake playing audio.") }
     }
 
-    private suspend fun playAsync() = mutex.withLock {
-        if (isPlaying()) {
-            return
-        }
-
-        playbackMutable.value = playbackMutable.value.copy(playWhenReady = true)
-        playIfReadyAndNotPaused()
-    }
-
     fun pause() {
-        coroutineScope.launch {
-            pauseAsync()
-        }
-    }
-
-    private suspend fun pauseAsync() = mutex.withLock {
         if (!playbackMutable.value.playWhenReady) {
             return
         }
 
         playbackMutable.value = playbackMutable.value.copy(playWhenReady = false)
         utteranceMutable.value = utteranceMutable.value.copy(range = null)
-        playbackJob?.cancelAndJoin()
-        Unit
+
+        coroutineScope.launch {
+            mutex.withLock {
+                playbackJob?.cancelAndJoin()
+            }
+        }
     }
 
     fun tryRecover() {
@@ -308,19 +313,16 @@ internal class TtsPlayer<S : TtsEngine.Settings, P : TtsEngine.Preferences<P>,
     }
 
     fun restartUtterance() {
-        coroutineScope.launch {
-            restartUtteranceAsync()
-        }
-    }
-
-    private suspend fun restartUtteranceAsync() = mutex.withLock {
-        playbackJob?.cancel()
         if (playbackMutable.value.state == State.Ended) {
             playbackMutable.value = playbackMutable.value.copy(state = State.Ready)
         }
-        utteranceMutable.value = utteranceMutable.value.copy(range = null)
-        playbackJob?.join()
-        playIfReadyAndNotPaused()
+
+        coroutineScope.launch {
+            playbackJob?.cancel()
+            playbackJob?.join()
+            utteranceMutable.value = utteranceMutable.value.copy(range = null)
+            playIfReadyAndNotPaused()
+        }
     }
 
     fun hasNextUtterance() =
@@ -541,9 +543,6 @@ internal class TtsPlayer<S : TtsEngine.Settings, P : TtsEngine.Preferences<P>,
         contentIterator.overrideContentLanguage = engineFacade.settings.value.overrideContentLanguage
     }
 
-    private fun isPlaying() =
-        playbackMutable.value.playWhenReady && playback.value.state == State.Ready
-
     private fun TtsUtteranceIterator.Utterance.ttsPlayerUtterance(): Utterance =
         Utterance(
             text = utterance,

readium/shared/src/main/java/org/readium/r2/shared/util/CoroutineQueue.kt

@@ -6,50 +6,74 @@
 
 package org.readium.r2.shared.util
 
-import kotlin.coroutines.resume
-import kotlinx.coroutines.CancellableContinuation
 import kotlinx.coroutines.CancellationException
+import kotlinx.coroutines.CompletableDeferred
+import kotlinx.coroutines.CoroutineDispatcher
 import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.MainScope
+import kotlinx.coroutines.Deferred
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.SupervisorJob
 import kotlinx.coroutines.cancel
 import kotlinx.coroutines.channels.Channel
 import kotlinx.coroutines.channels.trySendBlocking
 import kotlinx.coroutines.launch
-import kotlinx.coroutines.suspendCancellableCoroutine
+import kotlinx.coroutines.supervisorScope
 import org.readium.r2.shared.InternalReadiumApi
 
 /**
- * Executes coroutines in a sequential order (FIFO).
+ * CoroutineScope-like util to execute coroutines in a sequential order (FIFO).
+ * As with a SupervisorJob, children can be cancelled or fail independently one from the other.
  */
 @InternalReadiumApi
 public class CoroutineQueue(
-    public val scope: CoroutineScope = MainScope()
+    dispatcher: CoroutineDispatcher = Dispatchers.Main
 ) {
+    private val scope: CoroutineScope =
+        CoroutineScope(dispatcher + SupervisorJob())
+
     init {
         scope.launch {
             for (task in tasks) {
-                task()
+                // Don't fail the root job if one task fails.
+                supervisorScope {
+                    task()
+                }
             }
         }
     }
 
     /**
      * Launches a coroutine in the queue.
+     *
+     * Exceptions thrown by [block] will be ignored.
      */
-    public fun launch(task: suspend () -> Unit) {
-        tasks.trySendBlocking(Task(task)).getOrThrow()
+    public fun launch(block: suspend () -> Unit) {
+        tasks.trySendBlocking(Task(block)).getOrThrow()
+    }
+
+    /**
+     * Creates a coroutine in the queue and returns its future result
+     * as an implementation of Deferred.
+     *
+     * Exceptions thrown by [block] will be caught and represented in the resulting [Deferred].
+     */
+    public fun <T> async(block: suspend () -> T): Deferred<T> {
+        val deferred = CompletableDeferred<T>()
+        val task = Task(block, deferred)
+        tasks.trySendBlocking(task).getOrThrow()
+        return deferred
     }
 
     /**
      * Launches a coroutine in the queue, and waits for its result.
+     *
+     * Exceptions thrown by [block] will be rethrown.
      */
-    public suspend fun <T> await(task: suspend () -> T): T =
-        suspendCancellableCoroutine { cont ->
-            tasks.trySendBlocking(Task(task, cont)).getOrThrow()
-        }
+    public suspend fun <T> await(block: suspend () -> T): T =
+        async(block).await()
 
     /**
-     * Cancels all the coroutines in the queue.
+     * Cancels this coroutine queue, including all its children with an optional cancellation cause.
      */
     public fun cancel(cause: CancellationException? = null) {
         scope.cancel(cause)
@@ -59,11 +83,15 @@ public class CoroutineQueue(
 
     private class Task<T>(
         val task: suspend () -> T,
-        val continuation: CancellableContinuation<T>? = null
+        val deferred: CompletableDeferred<T>? = null
     ) {
         suspend operator fun invoke() {
-            val result = task()
-            continuation?.resume(result)
+            try {
+                val result = task()
+                deferred?.complete(result)
+            } catch (e: Exception) {
+                deferred?.completeExceptionally(e)
+            }
         }
     }
 }

test-app/src/main/java/org/readium/r2/testapp/reader/MediaService.kt

@@ -13,8 +13,8 @@ import android.content.Intent
 import android.content.ServiceConnection
 import android.os.Build
 import android.os.IBinder
+import androidx.core.app.NotificationManagerCompat
 import androidx.core.app.ServiceCompat
-import androidx.core.content.ContextCompat
 import androidx.media3.session.MediaSession
 import androidx.media3.session.MediaSessionService
 import kotlinx.coroutines.*
@@ -54,18 +54,21 @@ class MediaService : MediaSessionService() {
             sessionMutable.asStateFlow()
 
         fun closeSession() {
-            ServiceCompat.stopForeground(this@MediaService, ServiceCompat.STOP_FOREGROUND_REMOVE)
-            session.value?.mediaSession?.release()
-            session.value?.navigator?.close()
-            session.value?.coroutineScope?.cancel()
-            sessionMutable.value = null
+            Timber.d("closeSession")
+            session.value?.let { session ->
+                session.mediaSession.release()
+                session.coroutineScope.cancel()
+                session.navigator.close()
+                sessionMutable.value = null
+            }
         }
 
         @OptIn(FlowPreview::class)
         fun <N> openSession(
             navigator: N,
             bookId: Long
         ) where N : AnyMediaNavigator, N : Media3Adapter {
+            Timber.d("openSession")
             val activityIntent = createSessionActivityIntent()
             val mediaSession = MediaSession.Builder(applicationContext, navigator.asMedia3Player())
                 .setSessionActivity(activityIntent)
@@ -107,6 +110,12 @@ class MediaService : MediaSessionService() {
 
             return PendingIntent.getActivity(applicationContext, 0, intent, flags)
         }
+
+        fun stop() {
+            closeSession()
+            ServiceCompat.stopForeground(this@MediaService, ServiceCompat.STOP_FOREGROUND_REMOVE)
+            this@MediaService.stopSelf()
+        }
     }
 
     private val binder by lazy {
@@ -135,9 +144,18 @@ class MediaService : MediaSessionService() {
     override fun onTaskRemoved(rootIntent: Intent) {
         super.onTaskRemoved(rootIntent)
         Timber.d("Task removed. Stopping session and service.")
-        // Close the navigator to allow the service to be stopped.
+        // Close the session to allow the service to be stopped.
         binder.closeSession()
-        stopSelf()
+        binder.stop()
+    }
+
+    override fun onDestroy() {
+        Timber.d("Destroying MediaService.")
+        binder.closeSession()
+        // Ensure one more time that all notifications are gone and,
+        // hopefully, pending intents cancelled.
+        NotificationManagerCompat.from(this).cancelAll()
+        super.onDestroy()
     }
 
     companion object {
@@ -146,7 +164,12 @@ class MediaService : MediaSessionService() {
 
         fun start(application: Application) {
             val intent = intent(application)
-            ContextCompat.startForegroundService(application, intent)
+            application.startService(intent)
+        }
+
+        fun stop(application: Application) {
+            val intent = intent(application)
+            application.stopService(intent)
         }
 
         suspend fun bind(application: Application): Binder {
@@ -162,10 +185,14 @@ class MediaService : MediaSessionService() {
 
                 override fun onServiceDisconnected(name: ComponentName) {
                     Timber.d("MediaService disconnected.")
-                    // Should not happen, do nothing.
                 }
 
                 override fun onNullBinding(name: ComponentName) {
+                    if (mediaServiceBinder.isCompleted) {
+                        // This happens when the service has successfully connected and later
+                        // stopped and disconnected.
+                        return
+                    }
                     val errorMessage = "Failed to bind to MediaService."
                     Timber.e(errorMessage)
                     val exception = IllegalStateException(errorMessage)
@@ -179,11 +206,6 @@ class MediaService : MediaSessionService() {
             return mediaServiceBinder.await()
         }
 
-        fun stop(application: Application) {
-            val intent = intent(application)
-            application.stopService(intent)
-        }
-
         private fun intent(application: Application) =
             Intent(SERVICE_INTERFACE)
                 // MediaSessionService.onBind requires the intent to have a non-null action