Skip to content
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.

Sign up for GitHub

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

[APT-10449] Clients Call From Any Thread #48

Merged
merged 2 commits into from
Sep 20, 2024
Merged

[APT-10449] Clients Call From Any Thread #48

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2f97df
[APT-10449] Clients Call From Any Thread
kabliz Sep 19, 2024
ef6538e
[APT-10449] Rename "helper" to FactoryFactory
kabliz Sep 20, 2024
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
109 changes: 68 additions & 41 deletions Armadillo/src/main/java/com/scribd/armadillo/ArmadilloPlayerChoreographer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@ package com.scribd.armadillo

import android.content.Context
import android.os.Bundle
import android.os.Handler
import android.os.HandlerThread
import android.support.v4.media.MediaBrowserCompat
import android.support.v4.media.session.MediaControllerCompat
import android.support.v4.media.session.MediaSessionCompat
import android.support.v4.media.session.PlaybackStateCompat
import android.util.Log
import androidx.annotation.VisibleForTesting
import androidx.annotation.VisibleForTesting.PRIVATE
import com.scribd.armadillo.actions.Action
import com.scribd.armadillo.actions.ErrorAction
import com.scribd.armadillo.actions.MediaRequestUpdateAction
Expand Down Expand Up @@ -65,6 +68,7 @@ interface ArmadilloPlayer {
fun removeDownload(audioPlayable: AudioPlayable)
fun removeAllDownloads()
fun clearCache()

/**
* Starts playback with the given [AudioPlayable], allowing for configuration through a given [ArmadilloConfiguration]
*/
Expand Down Expand Up @@ -189,22 +193,37 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer {

@Inject
internal lateinit var context: Context

@Inject
internal lateinit var downloadEngine: DownloadEngine

@Inject
internal lateinit var cacheManager: CacheManager

@Inject
internal lateinit var stateProvider: StateStore.Provider

@Inject
internal lateinit var stateModifier: StateStore.Modifier

@Inject
internal lateinit var actionTransmitter: PlaybackActionTransmitter

@Inject
internal lateinit var mediaContentSharer: ArmadilloMediaBrowse.ContentSharer

private companion object {
const val TAG = "ArmadilloChoreographer"
companion object {
private const val TAG = "ArmadilloChoreographer"
private val observerPollIntervalMillis = 500.milliseconds

@VisibleForTesting(otherwise = PRIVATE)
val handlerThread: HandlerThread by lazy {
HandlerThread("ArmadilloChoreographer")
.also { it.start() }
}

@VisibleForTesting(otherwise = PRIVATE)
val handler: Handler by lazy { Handler(handlerThread.looper) }
kschults marked this conversation as resolved.
Show resolved Hide resolved
}

override val playbackCacheSize: Long
Expand All @@ -216,7 +235,7 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer {
* emits the most recently emitted state and all the subsequent states when an observer subscribes to it.
*/
val armadilloStateSubject: BehaviorSubject<ArmadilloState>
get() = stateProvider.stateSubject
get() = stateProvider.stateSubject
override val armadilloStateObservable: Observable<ArmadilloState>
get() = armadilloStateSubject.observeOn(AndroidSchedulers.mainThread())

Expand All @@ -235,34 +254,46 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer {
@VisibleForTesting
internal var playbackConnection: MediaSessionConnection? = null

private fun runHandler(lambda: () -> Unit) {
handler.post { lambda() }
}

private fun runIfPlaybackReady(lambda: (controls: MediaControllerCompat.TransportControls, playbackState: PlaybackState) -> Unit) {
runHandler {
doIfPlaybackReady { controls, playbackState ->
lambda(controls, playbackState)
}
}
}

override fun initDownloadEngine(): ArmadilloPlayer {
isDownloadEngineInit = true
downloadEngine.init()
updateProgressPollTask()
return this
}

override fun beginDownload(audioPlayable: AudioPlayable) {
override fun beginDownload(audioPlayable: AudioPlayable) = runHandler {
if (!isDownloadEngineInit) {
stateModifier.dispatch(ErrorAction(EngineNotInitialized("download engine cannot start download.")))
return
} else {
downloadEngine.download(audioPlayable)
}
downloadEngine.download(audioPlayable)
}

override fun clearCache() {
override fun clearCache() = runHandler {
cacheManager.clearPlaybackCache()
}

override fun removeAllDownloads() {
override fun removeAllDownloads() = runHandler {
if (!isDownloadEngineInit) {
stateModifier.dispatch(ErrorAction(EngineNotInitialized("Cannot remove all the downloads.")))
return
} else {
downloadEngine.removeAllDownloads()
}
downloadEngine.removeAllDownloads()
}

override fun beginPlayback(audioPlayable: AudioPlayable, config: ArmadilloConfiguration) {
override fun beginPlayback(audioPlayable: AudioPlayable, config: ArmadilloConfiguration) = runHandler {
disposables.clear()
actionTransmitter.begin(observerPollIntervalMillis)
val mediaSessionConnection = MediaSessionConnection(context)
Expand All @@ -279,84 +310,80 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer {
})
}

override fun updateMediaRequest(mediaRequest: AudioPlayable.MediaRequest) {
doIfPlaybackReady { controls, _ ->
override fun updateMediaRequest(mediaRequest: AudioPlayable.MediaRequest) =
runIfPlaybackReady { controls, _ ->
stateModifier.dispatch(MediaRequestUpdateAction(mediaRequest))
controls.sendCustomAction(CustomAction.UpdateMediaRequest(mediaRequest))
}
}

override fun updatePlaybackMetadata(title: String, chapters: List<Chapter>) {
override fun updatePlaybackMetadata(title: String, chapters: List<Chapter>) =
doWhenPlaybackReady { controls ->
stateModifier.dispatch(MetadataUpdateAction(title, chapters))
controls.sendCustomAction(CustomAction.UpdatePlaybackMetadata(title, chapters))
}
}

override fun endPlayback() {
override fun endPlayback() = runHandler {
playbackConnection?.transportControls?.stop()
}

override fun deinit() {
override fun deinit() = runHandler {
Log.v(TAG, "deinit")
disposables.clear()
pollingSubscription = null
isDownloadEngineInit = false
actionTransmitter.destroy()
}

override fun playOrPause() {
doIfPlaybackReady { controls, playbackState ->
when (playbackState) {
PlaybackState.PLAYING -> controls.pause()
PlaybackState.PAUSED -> controls.play()
else -> {
stateModifier.dispatch(ErrorAction(
UnexpectedException(cause = IllegalStateException("Neither playing nor paused"),
actionThatFailedMessage = "Trying to play or pause media."))
)
}
override fun playOrPause() = runIfPlaybackReady { controls, playbackState ->
when (playbackState) {
PlaybackState.PLAYING -> controls.pause()
PlaybackState.PAUSED -> controls.play()
else -> {
stateModifier.dispatch(ErrorAction(
UnexpectedException(cause = IllegalStateException("Neither playing nor paused"),
actionThatFailedMessage = "Trying to play or pause media."))
)
}
}
}

// Note: chapter skip and jump-skip behaviours are swapped. See MediaSessionCallback - we are using a jump-skip for skip-forward, as
// most headphones only have a skip-forward button, and this is the ideal behaviour for spoken audio.
override fun nextChapter() = doIfPlaybackReady { controls, _ -> controls.fastForward() }
override fun nextChapter() = runIfPlaybackReady { controls, _ -> controls.fastForward() }

override fun previousChapter() = doIfPlaybackReady { controls, _ -> controls.rewind() }
override fun previousChapter() = runIfPlaybackReady { controls, _ -> controls.rewind() }

override fun skipForward() = doIfPlaybackReady { controls, _ -> controls.skipToNext() }
override fun skipForward() = runIfPlaybackReady { controls, _ -> controls.skipToNext() }

override fun skipBackward() = doIfPlaybackReady { controls, _ -> controls.skipToPrevious() }
override fun skipBackward() = runIfPlaybackReady { controls, _ -> controls.skipToPrevious() }

override fun seekTo(position: Milliseconds) = doIfPlaybackReady { controls, _ ->
override fun seekTo(position: Milliseconds) = runIfPlaybackReady { controls, _ ->
// Add a shift constant to all seeks originating from the client application
// as opposed to system originated, such as from notification
controls.seekTo(position.longValue + Constants.AUDIO_POSITION_SHIFT_IN_MS)
}

override fun seekWithinChapter(percent: Int) {
override fun seekWithinChapter(percent: Int) = runHandler {
val position = stateProvider.currentState.positionFromChapterPercent(percent)
?: run {
stateModifier.dispatch(ErrorAction(
UnexpectedException(cause = KotlinNullPointerException("Current state's position is null"),
actionThatFailedMessage = "seeking within chapter"))
)
return
return@runHandler
}
seekTo(position)
}

override fun removeDownload(audioPlayable: AudioPlayable) {
override fun removeDownload(audioPlayable: AudioPlayable) = runHandler {
if (!isDownloadEngineInit) {
stateModifier.dispatch(ErrorAction(EngineNotInitialized("Cannot remove a download.")))
return
} else {
downloadEngine.removeDownload(audioPlayable)
}
downloadEngine.removeDownload(audioPlayable)
}

override fun addPlaybackActionListener(listener: PlaybackActionListener) {
override fun addPlaybackActionListener(listener: PlaybackActionListener) = runHandler {
PlaybackActionListenerHolder.actionlisteners.add(listener)
}

Expand All @@ -370,7 +397,7 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer {
mediaContentSharer.browseController = null
}

override fun notifyMediaBrowseContentChanged(rootId: String) = mediaContentSharer.notifyContentChanged(rootId)
override fun notifyMediaBrowseContentChanged(rootId: String) = runHandler { mediaContentSharer.notifyContentChanged(rootId) }

/**
* [ArmadilloPlayerChoreographer] polls for updates of playback & downloading
Expand Down
1 change: 0 additions & 1 deletion Armadillo/src/main/java/com/scribd/armadillo/di/PlaybackModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package com.scribd.armadillo.di

import android.app.Application
import android.content.Context
import com.google.android.exoplayer2.drm.DefaultDrmSessionManagerProvider
import com.google.android.exoplayer2.drm.DrmSessionManagerProvider
import com.scribd.armadillo.StateStore
import com.scribd.armadillo.broadcast.ArmadilloNoisyReceiver
Expand Down
36 changes: 20 additions & 16 deletions ...dillo/src/main/java/com/scribd/armadillo/playback/mediasource/DashMediaSourceGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import com.scribd.armadillo.actions.OpeningLicenseAction
import com.scribd.armadillo.download.DownloadEngine
import com.scribd.armadillo.download.DownloadTracker
import com.scribd.armadillo.download.drm.events.WidevineSessionEventListener
import com.scribd.armadillo.extensions.toUri
import com.scribd.armadillo.models.AudioPlayable
import com.scribd.armadillo.models.DrmType
import javax.inject.Inject
Expand Down Expand Up @@ -46,27 +45,32 @@ internal class DashMediaSourceGenerator @Inject constructor(
)

return if (isDownloaded) {
val drmManager = drmSessionManagerProvider.get(mediaItem)
if(request.drmInfo?.drmType == DrmType.WIDEVINE) {
val drmManager = if (request.drmInfo == null) {
drmSessionManagerProvider.get(mediaItem)
} else null
Copy link
Contributor

@griffinfscribd griffinfscribd Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wanted to double check that this is right - that if drmInfo is null, then we get a drmManager, but if it's not null then we set drmManager to null? From the desc, I was expecting the opposite.


if (request.drmInfo?.drmType == DrmType.WIDEVINE) {
downloadEngine.redownloadDrmLicense(id = mediaId, request = request)
}
DownloadHelper.createMediaSource(download!!.request, dataSourceFactory, drmManager)
} else {
DashMediaSource.Factory(dataSourceFactory)
.setDrmSessionManagerProvider(drmSessionManagerProvider)
.createMediaSource(mediaItem).also { source ->
//download equivalent is in DashDrmLicenseDownloader
when (request.drmInfo?.drmType) {
DrmType.WIDEVINE -> {
source.addDrmEventListener(
drmHandler,
WidevineSessionEventListener()
)
}

else -> Unit //no DRM
var factory = DashMediaSource.Factory(dataSourceFactory)
if (request.drmInfo != null) {
factory = factory.setDrmSessionManagerProvider(drmSessionManagerProvider)
}
factory.createMediaSource(mediaItem).also { source ->
//download equivalent is in DashDrmLicenseDownloader
when (request.drmInfo?.drmType) {
DrmType.WIDEVINE -> {
source.addDrmEventListener(
drmHandler,
WidevineSessionEventListener()
)
}

else -> Unit //no DRM
}
}
}
}

Expand Down
5 changes: 3 additions & 2 deletions Armadillo/src/main/java/com/scribd/armadillo/playback/mediasource/DrmMediaSourceHelper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ internal class DrmMediaSourceHelperImpl @Inject constructor(private val secureSt
.setUri(request.url)
.apply {
// Apply DRM config if content is DRM-protected
val drmConfig = request.drmInfo?.let { drmInfo ->
request.drmInfo?.let { drmInfo ->
MediaItem.DrmConfiguration.Builder(drmInfo.drmType.toExoplayerConstant())
.setLicenseUri(drmInfo.licenseServer)
.setLicenseRequestHeaders(drmInfo.drmHeaders)
Expand All @@ -46,8 +46,9 @@ internal class DrmMediaSourceHelperImpl @Inject constructor(private val secureSt
}
}
.build()
}?.let { drmConfig ->
setDrmConfiguration(drmConfig)
}
setDrmConfiguration(drmConfig)
}
.build()
}
3 changes: 2 additions & 1 deletion Armadillo/src/test/java/com/scribd/armadillo/ArmadilloPlayerChoreographerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import org.robolectric.annotation.Config
// Fixes issue in mockito. typealias is insufficient https://github.com/nhaarman/mockito-kotlin/issues/272#issuecomment-513971465
private interface Callback : (MediaControllerCompat.TransportControls) -> Unit

@Config(manifest = Config.NONE)
@Config(manifest = Config.NONE, sdk = [25])
@RunWith(RobolectricTestRunner::class)
class ArmadilloPlayerChoreographerTest {
@Rule
Expand Down Expand Up @@ -90,6 +90,7 @@ class ArmadilloPlayerChoreographerTest {
)
val newRequest = AudioPlayable.MediaRequest.createHttpUri(newUrl, newHeaders)
choreographer.updateMediaRequest(newRequest)
ArmadilloPlayerChoreographer.handler.hasMessages(1) //magic looper processor

verify(choreographer.stateModifier).dispatch(MediaRequestUpdateAction(newRequest))
verify(transportControls).sendCustomAction(eq("update_media_request"), argWhere {
Expand Down
4 changes: 4 additions & 0 deletions RELEASE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Project Armadillo Release Notes

## 1.6.5
- ArmadilloPlayer handles client calls from any thread appropriately, without blocking. For those recently updating since 1.5.1, this
should resolve any strange bugs from client behavior.

## 1.6.4
- Resolves download issue resulting in downloaded storage often being out of alignment

Expand Down
2 changes: 1 addition & 1 deletion gradle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ org.gradle.jvmargs=-Xmx1536m
# org.gradle.parallel=true
PACKAGE_NAME=com.scribd.armadillo
GRADLE_PLUGIN_VERSION=7.2.0
LIBRARY_VERSION=1.6.4
LIBRARY_VERSION=1.6.5
EXOPLAYER_VERSION=2.19.1
RXJAVA_VERSION=2.2.4
RXANDROID_VERSION=2.0.1
Expand Down
Loading