feat: Migrate to Androidx.Media3 Exoplayer.#502
Conversation
This migrates to the Androidx.Media3 from Exoplayer v2. Several minor chnages in setup and playback were done to reduce stuttering and dynamically determine of Tunnelling is supported by the device to help improve playback on lower end devices. Also increased the buffer size for the playback and network optimizations.
WalkthroughThis PR migrates the video player infrastructure from ExoPlayer 2 to AndroidX Media3 across the entire codebase, updating imports, dependencies, and API usage patterns. Additional changes include adjusting HTTP logging levels, enhancing Jellyfin API methods for play session tracking, and implementing tunneling support detection in AndroidHelper. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.kt (1)
58-58: Logging level change improves security.Reducing the logging level from BODY to BASIC prevents sensitive data (authentication tokens, user credentials, personal information) from being logged, which is a good security practice.
Optional: Consider build-variant-specific logging
For better debugging during development while maintaining security in production, you could make the logging level conditional:
-logger.level = HttpLoggingInterceptor.Level.BASIC +logger.level = if (BuildConfig.DEBUG) { + HttpLoggingInterceptor.Level.BODY +} else { + HttpLoggingInterceptor.Level.BASIC +}This allows full request/response logging during debug builds while keeping production logs minimal.
serenity-app/src/test/kotlin/us/nineworlds/serenity/test/ShadowSubtitleView.java (1)
1-30: Consider converting this Java file to Kotlin.This Java file resides in the
kotlin/source directory. As per coding guidelines, Kotlin should be the primary language with Java code converted where feasible. This straightforward Robolectric shadow class is a good candidate for conversion.Based on coding guidelines: "Use Kotlin as the primary language and convert Java code to Kotlin where feasible".
🔎 Example Kotlin conversion
package us.nineworlds.serenity.test import androidx.media3.ui.CaptionStyleCompat import androidx.media3.ui.SubtitleView import org.robolectric.annotation.Implementation import org.robolectric.annotation.Implements import org.robolectric.annotation.RealObject import org.robolectric.shadows.ShadowViewGroup @Implements(SubtitleView::class) class ShadowSubtitleView : ShadowViewGroup() { @RealObject private lateinit var subtitleView: SubtitleView var setUserDefaultTextSize = false private set @Implementation fun setUserDefaultTextSize() { setUserDefaultTextSize = true } fun isSetUserDefaultTextSizeCalled(): Boolean = setUserDefaultTextSize @Implementation fun setUserDefaultStyle() { subtitleView.setStyle(CaptionStyleCompat.DEFAULT) } }serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/HttpDataSourceFactoryProvider.java (1)
1-21: Consider converting to Kotlin per coding guidelines.As per coding guidelines, this simple provider class should be converted from Java to Kotlin. The straightforward structure makes it highly feasible for migration.
🔎 Proposed Kotlin conversion
Create a new file
HttpDataSourceFactoryProvider.kt:package us.nineworlds.serenity.injection.modules.providers import androidx.media3.datasource.HttpDataSource import androidx.media3.datasource.okhttp.OkHttpDataSource import okhttp3.OkHttpClient import javax.inject.Inject import javax.inject.Provider class HttpDataSourceFactoryProvider @Inject constructor( private val okHttpClient: OkHttpClient ) : Provider<HttpDataSource.Factory> { override fun get(): HttpDataSource.Factory { return OkHttpDataSource.Factory(okHttpClient).setUserAgent(USER_AGENT) } companion object { private const val USER_AGENT = "SerenityAndroid" } }Then delete this Java file.
Based on coding guidelines: "Use Kotlin as the primary language and convert Java code to Kotlin where feasible."
serenity-app/src/main/java/us/nineworlds/serenity/core/util/AndroidHelper.java (1)
75-87: Tunneling detection logic is sound.The API level guard and MIME type checks for H.265/H.264 are appropriate. The logic correctly determines tunneling support for common video codecs.
However, per coding guidelines, this file should be converted to Kotlin. Since this is new functionality being added as part of the Media3 migration, consider converting
AndroidHelper.javatoAndroidHelper.kt.As per coding guidelines: "Use Kotlin as the primary language and convert Java code to Kotlin where feasible."
serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/EventLoggerProvider.java (1)
1-21: Consider converting to Kotlin per coding guidelines.This Provider class is written in Java, but the project's coding guidelines specify using Kotlin as the primary language and converting Java code to Kotlin where feasible. This straightforward Provider class with minimal boilerplate would be a good candidate for Kotlin conversion.
As per coding guidelines, Kotlin is preferred for new and refactored code.
🔎 Example Kotlin conversion
package us.nineworlds.serenity.injection.modules.providers import androidx.media3.common.util.UnstableApi import androidx.media3.exoplayer.trackselection.MappingTrackSelector import us.nineworlds.serenity.ui.video.player.EventLogger import javax.inject.Inject import javax.inject.Provider @UnstableApi class EventLoggerProvider @Inject constructor( private val mappingTrackSelector: MappingTrackSelector ) : Provider<EventLogger> { override fun get(): EventLogger = EventLogger(mappingTrackSelector) }serenity-app/src/main/java/us/nineworlds/serenity/ui/video/player/EventLogger.java (3)
100-102: Method signature correctly updated for Media3.The
onAudioInputFormatChangedsignature now matches the Media3AudioRendererEventListenerinterface by including the@Nullable DecoderReuseEvaluationparameter.Note: The
decoderReuseEvaluationparameter is currently unused in the logging output. If enhanced debugging information would be helpful, you could optionally log decoder reuse details (e.g., whether a decoder was reused or reinitialized).
119-121: Method signature correctly updated for Media3.The
onVideoInputFormatChangedsignature now matches the Media3VideoRendererEventListenerinterface by including the@Nullable DecoderReuseEvaluationparameter.Note: The
decoderReuseEvaluationparameter is currently unused in the logging output. If enhanced debugging information would be helpful, you could optionally log decoder reuse details (e.g., whether a decoder was reused or reinitialized).
1-204: Consider converting to Kotlin per coding guidelines.This EventLogger class is written in Java, but the project's coding guidelines specify using Kotlin as the primary language and converting Java code to Kotlin where feasible. While this is a more substantial class than EventLoggerProvider, it would still benefit from Kotlin's concise syntax, null safety, and improved string formatting capabilities.
As per coding guidelines, Kotlin is preferred for new and refactored code.
serenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.kt (1)
103-103: Consider using @OptIn instead of @UnstableApi.In Kotlin, the idiomatic way to opt-in to unstable APIs is using
@OptIn(UnstableApi::class)rather than annotating the method with@UnstableApidirectly.🔎 Proposed refactor
- @UnstableApi + @OptIn(UnstableApi::class) override fun onCreate() {serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/DefaultMappingTrackSelectorProvider.java (1)
22-24: Consider converting to Kotlin per coding guidelines.The
@UnstableApiannotation is correctly applied to mark usage of unstable Media3 APIs. However, the coding guidelines specify to "Use Kotlin as the primary language and convert Java code to Kotlin where feasible."This provider class is a good candidate for Kotlin conversion, which would align with similar providers like
DataSourceFactoryProvider.ktalready using Kotlin.🔎 Proposed Kotlin conversion
Create a new file
DefaultMappingTrackSelectorProvider.kt:package us.nineworlds.serenity.injection.modules.providers import android.content.Context import androidx.media3.common.util.UnstableApi import androidx.media3.exoplayer.trackselection.AdaptiveTrackSelection import androidx.media3.exoplayer.trackselection.DefaultTrackSelector import androidx.media3.exoplayer.trackselection.MappingTrackSelector import us.nineworlds.serenity.common.android.injection.ApplicationContext import javax.inject.Inject import javax.inject.Provider class DefaultMappingTrackSelectorProvider : Provider<MappingTrackSelector> { @Inject @ApplicationContext lateinit var context: Context @OptIn(UnstableApi::class) override fun get(): MappingTrackSelector { return DefaultTrackSelector(context, AdaptiveTrackSelection.Factory()) } }Then delete the Java file.
serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/ExoplayerVideoModule.java (1)
18-21: Migration looks correct; consider class-level annotation for consistency.The
@OptIn(markerClass = UnstableApi.class)annotation is correctly applied to opt into Media3's unstable APIs. However, the related providers (EventLoggerProvider,EventLogger) use@UnstableApiat the class level. For consistency, consider moving the annotation to the class level:+@OptIn(markerClass = UnstableApi.class) public class ExoplayerVideoModule extends Module { - @OptIn(markerClass = UnstableApi.class) public ExoplayerVideoModule() {As per coding guidelines, this Java file could be converted to Kotlin in a future cleanup.
jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinAPIClient.kt (1)
428-434: Unimplemented progress overload with playSessionId.The new
progressoverload that accepts aplaySessionIdparameter is currently unimplemented. This method is likely needed for proper play session tracking with the Media3 migration.Would you like me to generate an implementation based on the existing
progressmethod at lines 414-426? The implementation would need to pass theplaySessionIdto theusersService.progress()call. Please verify whether this should be implemented now or tracked as a follow-up task.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.ktgradle/libs.versions.tomljellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinAPIClient.ktserenity-app/build.gradle.ktsserenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.ktserenity-app/src/main/java/us/nineworlds/serenity/core/util/AndroidHelper.javaserenity-app/src/main/java/us/nineworlds/serenity/injection/modules/ExoplayerVideoModule.javaserenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/DataSourceFactoryProvider.ktserenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/DefaultMappingTrackSelectorProvider.javaserenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/EventLoggerProvider.javaserenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/HttpDataSourceFactoryProvider.javaserenity-app/src/main/java/us/nineworlds/serenity/ui/video/player/EventLogger.javaserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivity.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegate.ktserenity-app/src/main/res/layout-large/activity_exoplayer_video.xmlserenity-app/src/main/res/layout/exo_playback_control_view.xmlserenity-app/src/main/res/layout/exo_player_view.xmlserenity-app/src/main/res/values/legacy_colors.xmlserenity-app/src/test/kotlin/us/nineworlds/serenity/test/ShadowSubtitleView.javaserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivityTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.kt
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{java,kt}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{java,kt}: Use Kotlin as the primary language and convert Java code to Kotlin where feasible
Enforce proper formatting using./gradlew spotlessApply
Use Toothpick for dependency injection
Files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/test/ShadowSubtitleView.javaserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.ktserenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.ktserenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/EventLoggerProvider.javaserenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/DefaultMappingTrackSelectorProvider.javaserenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/DataSourceFactoryProvider.ktserenity-app/src/main/java/us/nineworlds/serenity/injection/modules/ExoplayerVideoModule.javaserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivity.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegate.ktserenity-app/src/main/java/us/nineworlds/serenity/ui/video/player/EventLogger.javaemby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.ktjellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinAPIClient.ktserenity-app/src/main/java/us/nineworlds/serenity/core/util/AndroidHelper.javaserenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/HttpDataSourceFactoryProvider.javaserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivityTest.kt
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
Follow Kotlin naming conventions and style guidelines
Files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.ktserenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.ktserenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/DataSourceFactoryProvider.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivity.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegate.ktemby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.ktjellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinAPIClient.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivityTest.kt
**/*Test.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*Test.kt: All mocking operations must use MockK and no other mocking libraries are permitted
DO NOT USE MockK annotations; prefer field level creation of mocks directly
Write unit tests for all new features, bug fixes, or critical code changes
Define mocks as properties, initialized withmockk(relaxed = true), and useeveryfor expectations andverifyfor assertions
Define setup and cleanup routines explicitly in unit tests, ensuring all mocks are cleared in@Afterblocks usingclearAllMocks()andToothpick.reset()
Use@BeforeClassand@AfterClassfor static mocks in unit tests
Extendus.nineworlds.serenity.test.InjectingTestfor Toothpick-based tests and overridesetUpwith a call tosuper.setUp()as the first action
Bind mocks to instances ininstallTestModulesusing the Module pattern:bind(MainPresenter::class.java).toInstance(mockPresenter)
Files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivityTest.kt
**/*Activity.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*Activity.kt: DO NOT use@InjectPresenteron fields; instead inject Providers and usemoxyPresenterdelegate when converting Moxy MVP Activities to Kotlin
Inject Providers using@Inject lateinit var presenterProvider: Provider<MainPresenter>and use moxyPresenter delegate:internal val presenter by moxyPresenter { presenterProvider.get() }
Always replacefindViewByIdwith View Binding in Activities
Use the moxyPresenter delegate pattern for presenter injection in Activities
Files:
serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivity.kt
🧠 Learnings (11)
📚 Learning: 2025-12-16T18:16:58.793Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.793Z
Learning: Applies to **/*Test.kt : DO NOT USE MockK annotations; prefer field level creation of mocks directly
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivityTest.kt
📚 Learning: 2025-12-16T18:16:58.793Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.793Z
Learning: Applies to **/*Test.kt : All mocking operations must use MockK and no other mocking libraries are permitted
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivityTest.kt
📚 Learning: 2025-12-16T18:16:58.793Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.793Z
Learning: Applies to **/*Test.kt : Define mocks as properties, initialized with `mockk(relaxed = true)`, and use `every` for expectations and `verify` for assertions
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivityTest.kt
📚 Learning: 2025-12-16T18:16:58.793Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.793Z
Learning: Applies to **/*Test.kt : Define setup and cleanup routines explicitly in unit tests, ensuring all mocks are cleared in `After` blocks using `clearAllMocks()` and `Toothpick.reset()`
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.kt
📚 Learning: 2025-12-16T18:16:58.793Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.793Z
Learning: Applies to **/*Activity.kt : Inject Providers using `Inject lateinit var presenterProvider: Provider<MainPresenter>` and use moxyPresenter delegate: `internal val presenter by moxyPresenter { presenterProvider.get() }`
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivity.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegate.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.kt
📚 Learning: 2025-12-16T18:16:58.793Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.793Z
Learning: Applies to **/*Test.kt : Bind mocks to instances in `installTestModules` using the Module pattern: `bind(MainPresenter::class.java).toInstance(mockPresenter)`
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivityTest.kt
📚 Learning: 2025-12-16T18:16:58.793Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.793Z
Learning: Applies to **/*Activity.kt : Use the moxyPresenter delegate pattern for presenter injection in Activities
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivity.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegate.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.kt
📚 Learning: 2025-12-16T18:16:58.793Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.793Z
Learning: Applies to **/*Activity.kt : DO NOT use `InjectPresenter` on fields; instead inject Providers and use `moxyPresenter` delegate when converting Moxy MVP Activities to Kotlin
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivity.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegate.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.kt
📚 Learning: 2025-12-16T18:16:58.793Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.793Z
Learning: Applies to **/*Test.kt : Use `BeforeClass` and `AfterClass` for static mocks in unit tests
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivityTest.kt
📚 Learning: 2025-12-16T18:16:58.793Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.793Z
Learning: Applies to **/*.kt : Follow Kotlin naming conventions and style guidelines
Applied to files:
gradle/libs.versions.toml
📚 Learning: 2025-12-16T18:16:58.793Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.793Z
Learning: Applies to **/*Activity.kt : Always replace `findViewById` with View Binding in Activities
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivityTest.kt
🧬 Code graph analysis (5)
serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/EventLoggerProvider.java (1)
serenity-app/src/main/java/us/nineworlds/serenity/ui/video/player/EventLogger.java (1)
UnstableApi(55-204)
serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/DefaultMappingTrackSelectorProvider.java (3)
serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/EventLoggerProvider.java (1)
UnstableApi(12-21)serenity-app/src/main/java/us/nineworlds/serenity/ui/video/player/EventLogger.java (1)
UnstableApi(55-204)serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/DataSourceFactoryProvider.kt (1)
get(24-32)
serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/ExoplayerVideoModule.java (1)
serenity-app/src/main/java/us/nineworlds/serenity/ui/video/player/EventLogger.java (1)
UnstableApi(55-204)
serenity-app/src/main/java/us/nineworlds/serenity/ui/video/player/EventLogger.java (1)
serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/EventLoggerProvider.java (1)
UnstableApi(12-21)
jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinAPIClient.kt (1)
emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.kt (2)
fetchUserId(547-547)headerMap(160-169)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (32)
serenity-app/src/test/kotlin/us/nineworlds/serenity/test/ShadowSubtitleView.java (1)
4-5: LGTM! Import migration correctly updates to AndroidX Media3.The imports have been properly migrated from ExoPlayer v2 to AndroidX Media3, aligning with the PR's migration objectives.
serenity-app/src/main/res/values/legacy_colors.xml (1)
31-31: Color resource is correctly defined and actively used.The semi-transparent black (
#AA000000) is appropriate for the error message background overlay. The resource is referenced inserenity-app/src/main/res/layout/exo_player_view.xmlat line 45 and used as intended.serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/DataSourceFactoryProvider.kt (3)
4-10: LGTM! Clean migration to Media3 imports.The import statements have been correctly updated from ExoPlayer v2 (
com.google.android.exoplayer2.*) to AndroidX Media3 (androidx.media3.*) packages. The migration is consistent with the PR objectives.
24-24: LGTM! Correct opt-in for unstable API.The
@OptIn(UnstableApi::class)annotation properly acknowledges the use of unstable Media3 APIs, which is the recommended approach for this migration.
26-28: Use DefaultDataSource.Factory instead of deprecated DefaultDataSourceFactory.DefaultDataSourceFactory is deprecated in Media3; use DefaultDataSource.Factory instead. DefaultBandwidthMeter.getSingletonInstance(Context) usage is correct and remains the recommended pattern in Media3.
serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/HttpDataSourceFactoryProvider.java (2)
4-5: LGTM! Imports correctly updated for AndroidX Media3.The imports have been properly migrated from legacy ExoPlayer to
androidx.media3.datasourcepackages.
19-19: The OkHttpDataSource.Factory API usage is correct. ThesetUserAgent()method properly configures the user agent in OkHttpDataSource.Factory, and this is the recommended approach, as OkHttpDataSourceFactory is deprecated. No changes are needed unless your use case specifically requires additional configuration such as transfer listeners or cache control settings.serenity-app/src/main/java/us/nineworlds/serenity/core/util/AndroidHelper.java (2)
30-38: LGTM: Media3 imports are appropriate.The new imports correctly support the migration from ExoPlayer v2 to AndroidX Media3 and provide the necessary classes for tunneling capability detection.
114-136: LGTM: Audio passthrough migration to Media3 is correct.The
@OptInannotation is properly applied, and the codec encoding mappings correctly use Media3'sC.ENCODING_*constants. The logic is sound for checking audio passthrough support.serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/EventLoggerProvider.java (1)
3-12: LGTM: Media3 migration correctly applied.The import updates and
@UnstableApiannotation are properly implemented for the AndroidX Media3 migration. The annotation is required when working with Media3's unstable APIs likeMappingTrackSelector.serenity-app/src/main/java/us/nineworlds/serenity/ui/video/player/EventLogger.java (1)
21-55: LGTM: Media3 imports and annotation correctly applied.The comprehensive import migration from ExoPlayer to
androidx.media3.*packages is properly implemented. The@UnstableApiannotation correctly marks this class's usage of Media3 unstable APIs.serenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.kt (1)
35-38: LGTM! Media3 migration imports are correct.The imports correctly replace the ExoPlayer v2 equivalents with AndroidX Media3 components for cache management.
serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegate.kt (1)
7-7: LGTM!The import migration from
com.google.android.exoplayer2.ExoPlayertoandroidx.media3.exoplayer.ExoPlayeris correct. The Media3ExoPlayerinterface maintains API compatibility for the methods used in this class (playWhenReady,currentPosition,duration,stop(),seekTo()).serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/VideoKeyCodeHandlerDelegateTest.kt (1)
5-5: LGTM on the migration changes!The import and mock type migration from
SimpleExoPlayertoExoPlayercorrectly aligns with the production code changes. In Media3,SimpleExoPlayerwas removed andExoPlayeris now the primary interface.Also applies to: 29-29
serenity-app/src/main/res/layout/exo_playback_control_view.xml (1)
75-79: LGTM!The
DefaultTimeBarwidget class is correctly migrated fromcom.google.android.exoplayer2.ui.DefaultTimeBartoandroidx.media3.ui.DefaultTimeBar. The widget attributes and behavior remain unchanged.gradle/libs.versions.toml (1)
43-43: No action needed. Media3 version 1.9.0 is a valid, stable release.serenity-app/src/main/res/layout-large/activity_exoplayer_video.xml (1)
12-12: LGTM! Clean migration to Media3 PlayerView.The namespace update from ExoPlayer v2 to AndroidX Media3 is correct and aligns with the project-wide migration.
serenity-app/src/main/res/layout/exo_player_view.xml (1)
4-4: LGTM! UI component namespace migration is correct.The update of AspectRatioFrameLayout and SubtitleView to androidx.media3.ui namespace is consistent with the Media3 migration.
Also applies to: 25-25, 48-48
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivityTest.kt (4)
9-14: LGTM! Import migration to Media3 is correct.The test imports have been correctly updated to use androidx.media3.* packages, aligning with the ExoPlayer to Media3 migration.
51-51: LGTM! Mock updated to ExoPlayer type.The mock correctly uses ExoPlayer (Media3) instead of SimpleExoPlayer (ExoPlayer v2). The relaxed mode is appropriate for testing.
Based on learnings, mock definitions follow the project guidelines.
74-74: LGTM! Test name updated to reflect Media3 terminology.The test name change from
bindsSimpleExoPlayerViewtobindsExoPlayerViewcorrectly reflects the migration to Media3's ExoPlayer type.
135-135: LGTM! Type assertion updated for Media3.The assertion correctly checks for ExoPlayer::class.java instead of SimpleExoPlayer::class.java, aligning with the Media3 API.
serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerVideoActivity.kt (5)
15-30: LGTM! Import migration to Media3 is correct.All ExoPlayer v2 imports have been correctly replaced with their androidx.media3.* equivalents, maintaining the necessary classes for media playback, track selection, and UI components.
50-50: LGTM! UnstableApi annotation added.The @UnstableApi annotation is correctly applied since Media3 APIs used in this activity are marked as unstable and require opt-in.
220-223: Verify buffer configuration for memory impact.The buffer configuration has been significantly increased:
- Max buffer duration: 80 seconds (up from default ~50s)
- Target buffer size: 40 MB
While this may help reduce stuttering on lower-end devices (as mentioned in PR objectives), it could also:
- Increase memory pressure on devices with limited RAM
- Cause OOM issues on very low-end hardware
- Delay initial playback start
Consider testing on the lowest-end target device to ensure this doesn't cause memory issues.
226-232: LGTM! ExoPlayer construction updated to Media3 Builder pattern.The player construction correctly uses the new ExoPlayer.Builder API with setRenderersFactory, replacing the old constructor-based approach. This is the proper way to configure ExoPlayer in Media3.
199-216: Audio offload configuration is appropriate but verify device support.Devices may support offload playback of MP3 and AAC on Android 10+, and Opus on Android 11+. The configuration correctly disables speed change support—offload limits the ability to apply audio effects, like speed changes. Disabling gapless support is an intentional trade-off: offload may result in silence gaps between tracks in order to prioritize battery saving. If your target devices require playback speed adjustment, consider using
setIsSpeedChangeSupportRequired(true), though this may cause issues on some devices like Galaxy Z Fold.serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.kt (4)
4-5: LGTM! Import migration to Media3 is correct.The imports have been correctly updated to use androidx.media3.* packages for Player and PlayerControlView.
78-78: LGTM! Player.Listener callback signature updated for Media3.The onPositionDiscontinuity callback signature has been correctly updated to match Media3's Player.Listener interface, which now includes oldPosition and newPosition parameters. The no-op implementation (returning Unit) is appropriate since the functionality isn't needed.
82-89: LGTM! onPlaybackStateChanged correctly implements Media3 callback.The method signature and implementation correctly handle the Media3 Player.Listener callback for playback state changes. The STATE_ENDED handling logic remains functionally equivalent.
91-91: LGTM! onIsLoadingChanged callback signature updated.The callback signature has been correctly updated to match Media3's Player.Listener interface. The no-op implementation is appropriate.
serenity-app/build.gradle.kts (1)
125-127: Media3 dependencies are correctly using the latest stable version.The version catalog specifies Media3 1.9.0, which is the latest stable release. All three Media3 modules (exoplayer, ui, datasource-okhttp) are properly centralized through the version catalog and will benefit from recent bug fixes and performance improvements.
| override fun startPlaying(itemId: String): String? { | ||
| if (userId == null) { | ||
| userId = fetchUserId() | ||
| } | ||
|
|
||
| val call = usersService.startPlaying(headerMap(), userId!!, itemId) | ||
|
|
||
| call.execute() | ||
| return UUID.randomUUID().toString() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "JellyfinAPIClient.kt" -type fRepository: NineWorlds/serenity-android
Length of output: 165
🏁 Script executed:
head -n 500 ./jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinAPIClient.kt | tail -n 100Repository: NineWorlds/serenity-android
Length of output: 3551
🏁 Script executed:
rg -nP --type=kt "fun startPlaying\s*\(" --context=3 jellyfin-lib/Repository: NineWorlds/serenity-android
Length of output: 97
🏁 Script executed:
rg -n "fun startPlaying" --context=3 jellyfin-lib/Repository: NineWorlds/serenity-android
Length of output: 1886
🏁 Script executed:
sed -n '120,130p' jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinUsersService.ktRepository: NineWorlds/serenity-android
Length of output: 450
🏁 Script executed:
rg -n "fun progress" jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinUsersService.kt -A 5Repository: NineWorlds/serenity-android
Length of output: 336
🏁 Script executed:
rg -n "playSessionId\|PlaySessionId" jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/ -A 2 -B 2Repository: NineWorlds/serenity-android
Length of output: 53
🏁 Script executed:
rg -rn "playSessionId\|PlaySessionId" jellyfin-lib/Repository: NineWorlds/serenity-android
Length of output: 53
🏁 Script executed:
rg -n "startPlaying" jellyfin-lib/src/main/kotlin/ -B 2 -A 2Repository: NineWorlds/serenity-android
Length of output: 1936
🏁 Script executed:
sed -n '425,445p' ./jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinAPIClient.ktRepository: NineWorlds/serenity-android
Length of output: 845
🏁 Script executed:
sed -n '460,480p' ./jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinAPIClient.ktRepository: NineWorlds/serenity-android
Length of output: 975
🏁 Script executed:
rg -n "fun progress" jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinUsersService.kt -A 8Repository: NineWorlds/serenity-android
Length of output: 414
🏁 Script executed:
sed -n '100,140p' ./jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinUsersService.ktRepository: NineWorlds/serenity-android
Length of output: 1392
🏁 Script executed:
rg -n "PlaySessionId" ./jellyfin-lib/src/main/kotlin/ -B 3 -A 3Repository: NineWorlds/serenity-android
Length of output: 1072
🏁 Script executed:
rg -n "startPlaying" ./jellyfin-lib/ --type-list -A 1 | head -50Repository: NineWorlds/serenity-android
Length of output: 460
🏁 Script executed:
rg -n "startPlaying" ./jellyfin-lib/ -A 1 -B 1Repository: NineWorlds/serenity-android
Length of output: 1258
Generate UUID before the API call and add error handling.
The current implementation has two critical issues:
-
UUID timing: The UUID is generated on line 494 after
call.execute(), which means it's never sent to the server. If this session ID is needed for tracking, it must be generated before the API call. -
Missing error handling: Unlike similar methods (
watched,unwatched), theexecute()result is not checked. The method should verify success before returning the session ID.
Note: The usersService.startPlaying() endpoint does not accept a playSessionId query parameter. The progress() overload that accepts playSessionId is currently marked TODO("Not yet implemented"), so those must be addressed separately. For now, focus on generating the UUID before the call and properly handling the API response:
override fun startPlaying(itemId: String): String? {
if (userId == null) {
userId = fetchUserId()
}
+
+ val playSessionId = UUID.randomUUID().toString()
- val call = usersService.startPlaying(headerMap(), userId!!, itemId)
+ val call = usersService.startPlaying(headerMap(), userId!!, itemId)
- call.execute()
- return UUID.randomUUID().toString()
+ val result = call.execute()
+ return if (result.isSuccessful) playSessionId else null
}🤖 Prompt for AI Agents
In
jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinAPIClient.kt
around lines 486–495, generate the play session UUID before making the
usersService.startPlaying call, then pass it along if the request supports it
(or keep it locally for tracking), and call execute() into a val response; check
response.isSuccessful and only return the UUID on success; on failure log or
handle the error and return null. Ensure userId is fetched the same way as
before and avoid generating the UUID after the network call.
| try { | ||
| List<MediaCodecInfo> decoderInfos = MediaCodecUtil.getDecoderInfos(mimeType, false, false); | ||
| for (MediaCodecInfo info : decoderInfos) { | ||
| if (info.capabilities != null && info.capabilities.isFeatureSupported(android.media.MediaCodecInfo.CodecCapabilities.FEATURE_TunneledPlayback)) { | ||
| return true; | ||
| } | ||
| } | ||
| } catch (Exception e) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Silent exception handling hinders debugging.
The broad catch (Exception e) silently swallows all errors without logging. If MediaCodecUtil throws exceptions due to device issues or unsupported codecs, you'll have no visibility into the failure cause.
🔎 Suggested improvement: Add logging for exceptions
+import android.util.Log;
+
// ... in method body
try {
List<MediaCodecInfo> decoderInfos = MediaCodecUtil.getDecoderInfos(mimeType, false, false);
for (MediaCodecInfo info : decoderInfos) {
if (info.capabilities != null && info.capabilities.isFeatureSupported(android.media.MediaCodecInfo.CodecCapabilities.FEATURE_TunneledPlayback)) {
return true;
}
}
} catch (Exception e) {
+ Log.w("AndroidHelper", "Failed to check tunneling support for " + mimeType, e);
return false;
}Alternatively, narrow the catch to MediaCodecUtil.DecoderQueryException if you only expect that specific exception type.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| List<MediaCodecInfo> decoderInfos = MediaCodecUtil.getDecoderInfos(mimeType, false, false); | |
| for (MediaCodecInfo info : decoderInfos) { | |
| if (info.capabilities != null && info.capabilities.isFeatureSupported(android.media.MediaCodecInfo.CodecCapabilities.FEATURE_TunneledPlayback)) { | |
| return true; | |
| } | |
| } | |
| } catch (Exception e) { | |
| return false; | |
| } | |
| try { | |
| List<MediaCodecInfo> decoderInfos = MediaCodecUtil.getDecoderInfos(mimeType, false, false); | |
| for (MediaCodecInfo info : decoderInfos) { | |
| if (info.capabilities != null && info.capabilities.isFeatureSupported(android.media.MediaCodecInfo.CodecCapabilities.FEATURE_TunneledPlayback)) { | |
| return true; | |
| } | |
| } | |
| } catch (Exception e) { | |
| Log.w("AndroidHelper", "Failed to check tunneling support for " + mimeType, e); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In
serenity-app/src/main/java/us/nineworlds/serenity/core/util/AndroidHelper.java
around lines 96-105, the current catch block swallows all exceptions which hides
failures from MediaCodecUtil; change the catch to either catch the specific
MediaCodecUtil.DecoderQueryException (if available) or keep Exception but log
the exception before returning false (use the class TAG or Android Log.e with a
short message and the exception) so the error is visible for debugging while
preserving the existing false return behavior.
|
|
||
| import androidx.media3.common.util.UnstableApi; | ||
| import androidx.media3.exoplayer.trackselection.AdaptiveTrackSelection; | ||
| import androidx.media3.exoplayer.trackselection.DefaultTrackSelector; | ||
| import androidx.media3.exoplayer.trackselection.MappingTrackSelector; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Convert file to Kotlin and verify Media3 API compliance.
The import migration from ExoPlayer v2 to AndroidX.Media3 is correct. However, this file should be converted to Kotlin per the coding guidelines that require Kotlin as the primary language. When converting, use @OptIn annotation for unstable Media3 APIs at the method level in Kotlin, rather than the @UnstableApi annotation used in Java. The DefaultTrackSelector constructor requires Context context, TrackSelectionParameters parameters, and ExoTrackSelection.Factory trackSelectionFactory—verify that the provider passes the correct parameters. Toothpick usage with @Inject is appropriate and can be migrated to Kotlin incrementally while maintaining compatibility.
🤖 Prompt for AI Agents
In
serenity-app/src/main/java/us/nineworlds/serenity/injection/modules/providers/DefaultMappingTrackSelectorProvider.java
around lines 5–9, convert this Java provider to Kotlin and update it to use
Media3 APIs correctly: rewrite the class as a Kotlin provider, annotate the
method that constructs the DefaultTrackSelector with @OptIn(UnstableApi::class)
(at method level) instead of using the Java @UnstableApi, and ensure you call
the DefaultTrackSelector constructor with the correct parameters (Context,
TrackSelectionParameters, and
ExoTrackSelection.Factory/AdaptiveTrackSelection.Factory as required by the
Media3 version you target) passing in the current context, appropriate
TrackSelectionParameters, and a proper trackSelectionFactory; keep the Toothpick
@Inject wiring equivalent in Kotlin to maintain DI compatibility and migrate
incrementally if needed.
| val leastRecentlyUsedCacheEvictor = | ||
| LeastRecentlyUsedCacheEvictor((200 * 1024 * 1024).toLong()) | ||
| val exoDatabaseProvider = ExoDatabaseProvider(this) | ||
| simpleCache = SimpleCache(cacheDir, leastRecentlyUsedCacheEvictor, exoDatabaseProvider) |
There was a problem hiding this comment.
Call simpleCache.release() for proper resource cleanup, but not in onTerminate().
SimpleCache requires calling .release() to properly clean up database resources. However, onTerminate() is only for testing and is not called in production. Consider releasing the cache during app lifecycle events that are reliably called, such as onTrimMemory() or when the last activity is destroyed, to ensure cleanup happens in production scenarios.
🤖 Prompt for AI Agents
In serenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.kt
around lines 111 to 114, SimpleCache is created but not released; calling
simpleCache.release() in onTerminate() is insufficient because onTerminate() is
not called in production. Add reliable cleanup: implement
Application.ActivityLifecycleCallbacks to track activity count (or override
onTrimMemory) and call simpleCache.release() when the app is
backgrounded/trimmed to a critical level (e.g., TRIM_MEMORY_COMPLETE/UiHidden)
or when the last activity is destroyed; ensure you null out the simpleCache
reference after release, guard against multiple releases, and wrap release in a
try/catch to avoid crashing.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #502 +/- ##
=============================================
+ Coverage 50.05% 50.27% +0.22%
Complexity 467 467
=============================================
Files 63 63
Lines 1996 2005 +9
Branches 273 273
=============================================
+ Hits 999 1008 +9
Misses 878 878
Partials 119 119
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
This migrates to the Androidx.Media3 from Exoplayer v2. Several minor chnages in setup and playback were done to reduce stuttering and dynamically determine of Tunnelling is supported by the device to help improve playback on lower end devices.
Also increased the buffer size for the playback and network optimizations.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.