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-10204] pass loadcontrol to exoplayer #35

Merged
merged 4 commits into from
Jun 27, 2024
Merged

[APT-10204] pass loadcontrol to exoplayer #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d937af
[APT-10204] Pass loadControl values to exoplayer instance
griffinfscribd Jun 27, 2024
6f655d4
[APT-10204] Update tests
griffinfscribd Jun 27, 2024
c13dca7
[APT-10204] Revert previous 1.3.2 fix
griffinfscribd Jun 27, 2024
9bf48ab
[APT-10204] Add release notes and bump version
griffinfscribd Jun 27, 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
26 changes: 25 additions & 1 deletion Armadillo/src/main/java/com/scribd/armadillo/ArmadilloConfiguration.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
package com.scribd.armadillo

import com.google.android.exoplayer2.DefaultLoadControl.DEFAULT_BUFFER_FOR_PLAYBACK_AFTER_REBUFFER_MS
import com.google.android.exoplayer2.DefaultLoadControl.DEFAULT_BUFFER_FOR_PLAYBACK_MS
import com.google.android.exoplayer2.DefaultLoadControl.DEFAULT_MIN_BUFFER_MS
import com.google.android.exoplayer2.DefaultLoadControl.DEFAULT_MAX_BUFFER_MS
import com.google.android.exoplayer2.DefaultLoadControl.DEFAULT_PRIORITIZE_TIME_OVER_SIZE_THRESHOLDS
import com.google.android.exoplayer2.DefaultLoadControl.DEFAULT_TARGET_BUFFER_BYTES
import com.scribd.armadillo.models.AudioPlayable
import com.scribd.armadillo.time.milliseconds
import java.io.Serializable

/**
* Used to specify various settings when starting playback on a new [AudioPlayable]
Expand All @@ -11,10 +18,27 @@ import com.scribd.armadillo.time.milliseconds
* @property maxDurationDiscrepancy Armadillo will output errors if the metadata for the audio duration doesn't match the
* actual duration of playback. This value can be used to set the allowed maximum difference in seconds between stated vs. actual duration.
* Can also be set to a negative value to ignore any discrepancies.
* @property minBufferMs The minumum amount of audio attempted to be buffered at all times in milliseconds.
* @property maxBufferMs The maximum amount of audio attempted to be buffered at all times in milliseconds.
* @property bufferForPlaybackMs The duration of media that must be buffered for playback to start or
* resume following a user action such as a seek, in milliseconds.
* @property bufferForPlaybackAfterRebufferMs The duration of media that must be buffered for playback
* to resume after a rebuffer, in milliseconds. A rebuffer is defined to be caused by buffer depletion
* rather than a user action.
* @property targetBufferSize The desired size of the media buffer in bytes. An unset buffer size will
* will be calculated based on the selected tracks.
* @property prioritizeTimeOverSizeThresholds Whether the load control prioritizes buffer time constraints
* over buffer size constraints.
*/
data class ArmadilloConfiguration(val initialOffset: Milliseconds = 0.milliseconds,
val isAutoPlay: Boolean = true,
val maxDurationDiscrepancy: Int = MAX_DISCREPANCY_DEFAULT) {
val maxDurationDiscrepancy: Int = MAX_DISCREPANCY_DEFAULT,
val minBufferMs: Int = DEFAULT_MIN_BUFFER_MS,
val maxBufferMs: Int = DEFAULT_MAX_BUFFER_MS,
val bufferForPlaybackMs: Int = DEFAULT_BUFFER_FOR_PLAYBACK_MS,
val bufferForPlaybackAfterRebufferMs: Int = DEFAULT_BUFFER_FOR_PLAYBACK_AFTER_REBUFFER_MS,
val targetBufferSize: Int = DEFAULT_TARGET_BUFFER_BYTES,
val prioritizeTimeOverSizeThresholds: Boolean = DEFAULT_PRIORITIZE_TIME_OVER_SIZE_THRESHOLDS): Serializable {

companion object {
// Default duration discrepancy values in seconds
Expand Down
4 changes: 1 addition & 3 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 @@ -268,10 +268,8 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer {
mediaSessionConnection.connectToMediaSession(object : MediaSessionConnection.Listener {
override fun onConnectionCallback(transportControls: MediaControllerCompat.TransportControls) {
val bundle = Bundle()
bundle.putSerializable(Constants.Keys.KEY_ARMADILLO_CONFIG, config)
bundle.putSerializable(Constants.Keys.KEY_AUDIO_PLAYABLE, audioPlayable)
bundle.putSerializable(Constants.Keys.KEY_INITIAL_OFFSET, config.initialOffset)
bundle.putBoolean(Constants.Keys.KEY_IS_AUTO_PLAY, config.isAutoPlay)
bundle.putInt(Constants.Keys.KEY_MAX_DURATION_DISCREPANCY, config.maxDurationDiscrepancy)
transportControls.playFromUri(audioPlayable.request.url.toUri(), bundle)
updateProgressPollTask()
}
Expand Down
6 changes: 1 addition & 5 deletions Armadillo/src/main/java/com/scribd/armadillo/Constants.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,8 @@ object Constants {
private val APP_NAME = R.string.arm_app_name

internal object Keys {
const val KEY_ARMADILLO_CONFIG = "armadillo_config"
const val KEY_AUDIO_PLAYABLE = "audio_playable"
const val KEY_IS_AUTO_PLAY = "is_auto_play"
const val KEY_MAX_DURATION_DISCREPANCY = "max_duration_discrepancy"
const val KEY_PLAYBACK_READY = "playback_ready"
const val KEY_INITIAL_OFFSET = "initial_offset"
const val KEY_ERROR = "error"
}

internal object DI {
Expand Down
4 changes: 3 additions & 1 deletion Armadillo/src/main/java/com/scribd/armadillo/playback/ExoplayerExt.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package com.scribd.armadillo.playback
import android.content.Context
import com.google.android.exoplayer2.C
import com.google.android.exoplayer2.ExoPlayer
import com.google.android.exoplayer2.LoadControl
import com.google.android.exoplayer2.RenderersFactory
import com.google.android.exoplayer2.audio.AudioAttributes
import com.google.android.exoplayer2.audio.AudioCapabilities
Expand Down Expand Up @@ -41,8 +42,9 @@ internal fun ExoPlayer.playerDuration(): Milliseconds? = if (duration == C.TIME_
*
* We provide our own renderers factory so that Proguard can remove any non-audio rendering code.
*/
internal fun createExoplayerInstance(context: Context, attributes: AudioAttributes): ExoPlayer =
internal fun createExoplayerInstance(context: Context, attributes: AudioAttributes, loadControl: LoadControl): ExoPlayer =
ExoPlayer.Builder(context, createRenderersFactory(context))
.setLoadControl(loadControl)
.build().apply {
setAudioAttributes(attributes, true)
}
Expand Down
11 changes: 4 additions & 7 deletions Armadillo/src/main/java/com/scribd/armadillo/playback/MediaSessionCallback.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import androidx.annotation.VisibleForTesting
import com.scribd.armadillo.ArmadilloConfiguration
import com.scribd.armadillo.Constants
import com.scribd.armadillo.Constants.AUDIO_POSITION_SHIFT_IN_MS
import com.scribd.armadillo.Milliseconds
import com.scribd.armadillo.StateStore
import com.scribd.armadillo.actions.CustomMediaSessionAction
import com.scribd.armadillo.actions.UpdateProgressAction
Expand Down Expand Up @@ -125,9 +124,6 @@ internal class MediaSessionCallback(private val onMediaSessionEventListener: OnM

override fun onPlayFromUri(uri: Uri, extras: Bundle) {
val newAudioPlayable = extras.getSerializable(Constants.Keys.KEY_AUDIO_PLAYABLE) as AudioPlayable
val isAutoPlay = extras.getBoolean(Constants.Keys.KEY_IS_AUTO_PLAY, false)
val maxDurationDiscrepancy = extras.getInt(Constants.Keys.KEY_MAX_DURATION_DISCREPANCY,
ArmadilloConfiguration.MAX_DISCREPANCY_DEFAULT)

if (newAudioPlayable == stateProvider.currentState.playbackInfo?.audioPlayable && isPlaying) {
Log.v(TAG, "onPlayFromUri: already playing audioPlayable: ${newAudioPlayable.id} - Skipping setup")
Expand All @@ -138,10 +134,11 @@ internal class MediaSessionCallback(private val onMediaSessionEventListener: OnM
onStop()
}

@Suppress("UNCHECKED_CAST") val initialOffset = extras.getSerializable(Constants.Keys.KEY_INITIAL_OFFSET) as Milliseconds
val config = extras.getSerializable(Constants.Keys.KEY_ARMADILLO_CONFIG) as ArmadilloConfiguration
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way this could be missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe so, but better to err on the safe side. In case it's missing from the bundle, it'll come back as null, so I'll provide a default value for the config.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, depends on what direction you wanna go with it

If there is no way it could be missing, can probably go with what you had before and we'll just get hit with a NPE or typecast exception we can investigate if there's a problem. Keeps the code simpler and confidently shows clear intent of how it's working/what should be happening

Having a fallback is safe, but it also obscures when something is wrong, so usually good to pair with logging an event to our dashboard. Not sure if we do that in Armadillo without it throwing an error that might stop the player

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applying that to the config, I feel like it makes sense that I should always be expecting the configuration for the audio playable so I'll go with the former. And good point about fallback possibly obscuring errors.

Log.v(TAG, "ArmadilloConfiguration: $config")
playbackEngine = playbackEngineFactory.createPlaybackEngine(newAudioPlayable)
playbackEngine?.beginPlayback(isAutoPlay, maxDurationDiscrepancy, initialOffset)
playbackEngine?.seekTo(initialOffset)
playbackEngine?.beginPlayback(config)
playbackEngine?.seekTo(config.initialOffset)

isPlaying = true

Expand Down
26 changes: 20 additions & 6 deletions Armadillo/src/main/java/com/scribd/armadillo/playback/PlaybackEngine.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ package com.scribd.armadillo.playback

import android.content.Context
import androidx.annotation.VisibleForTesting
import com.google.android.exoplayer2.DefaultLoadControl
import com.google.android.exoplayer2.ExoPlayer
import com.google.android.exoplayer2.PlaybackParameters
import com.google.android.exoplayer2.Player
import com.google.android.exoplayer2.upstream.cache.Cache
import com.scribd.armadillo.ArmadilloConfiguration
import com.scribd.armadillo.ArmadilloPlayerChoreographer
import com.scribd.armadillo.Constants
import com.scribd.armadillo.Milliseconds
Expand Down Expand Up @@ -46,7 +48,7 @@ internal interface AudioPlaybackEngine {
*/
var offloadAudio: Boolean

fun beginPlayback(isAutoPlay: Boolean, maxDurationDiscrepancy: Int, initialOffset: Milliseconds = 0.milliseconds)
fun beginPlayback(config: ArmadilloConfiguration)

/**
* Updates the request for the currently playing media. This could be to change the request headers.
Expand Down Expand Up @@ -118,18 +120,30 @@ internal class ExoplayerPlaybackEngine(private var audioPlayable: AudioPlayable)
}
}

override fun beginPlayback(isAutoPlay: Boolean, maxDurationDiscrepancy: Int, initialOffset: Milliseconds) {
stateModifier.dispatch(NewAudioPlayableAction(audioPlayable, maxDurationDiscrepancy, initialOffset))
override fun beginPlayback(config: ArmadilloConfiguration) {
stateModifier.dispatch(NewAudioPlayableAction(
audioPlayable = audioPlayable,
maxDurationDiscrepancy = config.maxDurationDiscrepancy,
initialOffset = config.initialOffset))

exoPlayer = createExoplayerInstance(context, audioAttributes.exoPlayerAttrs)
val loadControl = DefaultLoadControl.Builder()
.setBufferDurationsMs(config.minBufferMs,
config.maxBufferMs,
config.bufferForPlaybackMs,
config.bufferForPlaybackAfterRebufferMs)
.setTargetBufferBytes(config.targetBufferSize)
.setPrioritizeTimeOverSizeThresholds(config.prioritizeTimeOverSizeThresholds)
.build()

exoPlayer = createExoplayerInstance(context, audioAttributes.exoPlayerAttrs, loadControl)

val mediaSource = mediaSourceRetriever.generateMediaSource(audioPlayable.request, context)
exoPlayer.setMediaSource(mediaSource)
exoPlayer.prepare()

exoPlayer.addListener(playerEventListener)

exoPlayer.playWhenReady = isAutoPlay
exoPlayer.playWhenReady = config.isAutoPlay

stateModifier.dispatch(PlaybackEngineReady(true))
stateModifier.dispatch(PlayerStateAction(PlaybackState.PAUSED))
Expand Down Expand Up @@ -243,7 +257,7 @@ internal class ExoplayerPlaybackEngine(private var audioPlayable: AudioPlayable)
}

private fun seekToExo(position: Milliseconds) {
stateModifier.dispatch(PlaybackProgressAction(position, exoPlayer.playerDuration()))
exoPlayer.seekTo(exoPlayer.currentMediaItemIndex, position.longValue)
stateModifier.dispatch(PlaybackProgressAction(position, exoPlayer.playerDuration()))
}
}
6 changes: 3 additions & 3 deletions Armadillo/src/test/java/com/scribd/armadillo/playback/MediaSessionCallbackTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ class MediaSessionCallbackTest {
mediaSessionCallback.onPlayFromUri(URL.toUri(), bundleTwo)
verify(mediaSessionCallback.playbackEngine!!, times(1)).deinit()
verify(mediaSessionCallback.playbackEngine!!, times(1))
.beginPlayback(false, ArmadilloConfiguration.MAX_DISCREPANCY_DEFAULT, 0.milliseconds)
.beginPlayback(ArmadilloConfiguration())
}

@Test
Expand All @@ -147,7 +147,7 @@ class MediaSessionCallbackTest {
whenever(playbackInfo.audioPlayable).thenReturn(audiobookTwo)
mediaSessionCallback.onPlayFromUri(URL.toUri(), bundleOne)
verify(mediaSessionCallback.playbackEngine!!, times(1))
.beginPlayback(false, ArmadilloConfiguration.MAX_DISCREPANCY_DEFAULT, 0.milliseconds)
.beginPlayback(ArmadilloConfiguration())
}

@Test
Expand Down Expand Up @@ -321,6 +321,6 @@ class MediaSessionCallbackTest {

private fun Bundle.addAudiobook(audiobook: AudioPlayable) {
putSerializable(Constants.Keys.KEY_AUDIO_PLAYABLE, audiobook)
putSerializable(Constants.Keys.KEY_INITIAL_OFFSET, 0.milliseconds)
putSerializable(Constants.Keys.KEY_ARMADILLO_CONFIG, ArmadilloConfiguration())
}
}
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.3.3
- Reverted fix in 1.3.2 as it may have affected listening progress not being correctly reported
- Added support for passing in load control parameters via ArmadilloConfiguration to the exo player instance

## 1.3.2
- Added a fix for order of dispatched actions during seek related events

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 @@ -12,7 +12,7 @@ org.gradle.jvmargs=-Xmx1536m
# http://www.gradle.org/docs/current/userguide/multi_project_builds.html#sec:decoupled_projects
# org.gradle.parallel=true
PACKAGE_NAME=com.scribd.armadillo
LIBRARY_VERSION=1.3.2
LIBRARY_VERSION=1.3.3
EXOPLAYER_VERSION=2.17.1
RXJAVA_VERSION=2.2.4
RXANDROID_VERSION=2.0.1
Expand Down
Loading