feat(android): remote-first player controls and ijkplayer backend - #55
Merged
Conversation
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds an experimental ijkplayer backend with MediaSession controls, Media3 I/O bridging, HDR/subtitle fallbacks, TV remote and focus-navigation updates, previous/next episode playback, web-configured backend options, tests, and bundled licensing/provenance metadata. ChangesExperimental ijkplayer playback
TV playback controls
Episode navigation
Web backend settings
Release metadata
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
player-core/src/main/kotlin/com/miruplay/tv/player/ExoPlaybackController.kt (1)
290-319: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
updateRemoteControlPlaybackState()may touch the IJKViewoff the main thread.In
pause()/resume(),updateRemoteControlPlaybackState()runs after thewithContext(Dispatchers.Main) {}block returns, i.e. on whatever dispatcher the caller used to invokepause()/resume(). That function callsisPlaying(), which forEXPERIMENTAL_IJKPLAYERcallsijkView?.isPlaybackActive()—ijkViewis aView, and Android Views must only be accessed from the main thread. If any caller invokespause()/resume()off the main dispatcher, this is a realCalledFromWrongThreadException/undefined-behavior risk.🔒 Proposed fix: move the call inside the Main-dispatched block
override suspend fun pause() { withContext(Dispatchers.Main) { ... activeExoPlayer().playWhenReady = false + updateRemoteControlPlaybackState() } - updateRemoteControlPlaybackState() } override suspend fun resume() { withContext(Dispatchers.Main) { ... activeExoPlayer().playWhenReady = true + updateRemoteControlPlaybackState() } - updateRemoteControlPlaybackState() }Also applies to: 321-349, 566-572
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@player-core/src/main/kotlin/com/miruplay/tv/player/ExoPlaybackController.kt` around lines 290 - 319, Move updateRemoteControlPlaybackState() inside the withContext(Dispatchers.Main) block in pause() and resume(), ensuring every return path invokes it before leaving the main-thread context. Apply the same adjustment to the related playback-control paths around the referenced methods, while preserving their existing backend-specific state updates.
🧹 Nitpick comments (5)
web-control/src/test/kotlin/com/miruplay/tv/webcontrol/PlaybackDebugConfigMappingTest.kt (1)
24-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the canonical IJK alias too.
The mapper also accepts
experimentalijkplayeron Line 16, but this test only covers the two shorter aliases.Suggested assertion
assertEquals(PlaybackRenderBackend.EXPERIMENTAL_IJKPLAYER, playbackRenderBackendFromDebugValue("ijk")) assertEquals(PlaybackRenderBackend.EXPERIMENTAL_IJKPLAYER, playbackRenderBackendFromDebugValue("ijkplayer")) + assertEquals( + PlaybackRenderBackend.EXPERIMENTAL_IJKPLAYER, + playbackRenderBackendFromDebugValue("experimentalijkplayer"), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-control/src/test/kotlin/com/miruplay/tv/webcontrol/PlaybackDebugConfigMappingTest.kt` around lines 24 - 25, Add a test assertion in PlaybackDebugConfigMappingTest for playbackRenderBackendFromDebugValue("experimentalijkplayer"), verifying it returns PlaybackRenderBackend.EXPERIMENTAL_IJKPLAYER alongside the existing IJK alias cases.repository-api/src/main/kotlin/com/miruplay/tv/repository/NextPlaybackSourceResolver.kt (1)
40-50: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the current-episode lookup when there is no adjacent episode.
currentPathperforms a repository load before theepisode ?: return nullearly exit, so every boundary call (first/last episode) pays an unnecessary I/O hit.♻️ Proposed reorder
private suspend fun buildAdjacent( currentSource: PlaybackSource, episode: Episode?, ): PlaybackSource? { + val adjacentEpisode = episode ?: return null val currentPath = currentSource.episodeId ?.let { loadEpisode(it)?.filePath } ?: currentSource.uri - val adjacentEpisode = episode ?: return null val version = adjacentEpisode.availableVersions().nearestTo(currentPath) ?: return null return build(adjacentEpisode, version) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@repository-api/src/main/kotlin/com/miruplay/tv/repository/NextPlaybackSourceResolver.kt` around lines 40 - 50, The buildAdjacent method currently loads the current episode before checking whether an adjacent episode exists. Move the episode ?: return null guard before currentPath computation, preserving the existing version selection and build flow for valid adjacent episodes.ui-tv/src/test/kotlin/com/miruplay/tv/ui/player/PlayerKeyHandlingTest.kt (1)
12-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test asserts the forward-seek path.
Key.DirectionRight+KeyDown→onSkipForwardis never exercised; the KeyUp test only checks non-consumption. Worth a short additional case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tv/src/test/kotlin/com/miruplay/tv/ui/player/PlayerKeyHandlingTest.kt` around lines 12 - 31, The PlayerKeyHandlingTest coverage only verifies repeated left-seek handling. Add a focused test for handlePlaybackTimelineKey using Key.DirectionRight with KeyEventType.KeyDown, assert the event is consumed, and verify onSkipForward is invoked while onSkipBackward is not.ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerScreen.kt (2)
1128-1136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDown-target fallback skips the always-enabled picture chip.
When there are no audio tracks and no subtitles,
downjumps to the speed chip even though the picture chip is the first (and always enabled) chip in the row. Consider ending the chain onpictureFocusRequesterfor a predictable leftmost landing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerScreen.kt` around lines 1128 - 1136, Update the downFocusRequester selection in PlayerScreen so the fallback after audioTracks and subtitles uses pictureFocusRequester instead of speedFocusRequester. Preserve the existing audio and subtitle targets when those tracks are available.
1919-1926: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFully-qualified enum references are inconsistent with the file's imports.
MiruPlayInputIntentis already imported at Line 144; thecom.miruplay.tv.design.prefixes here can be dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerScreen.kt` around lines 1919 - 1926, Update the when expression in the event input handling flow to reference the already imported MiruPlayInputIntent enum directly, removing the redundant com.miruplay.tv.design. qualification from MediaPrevious, MediaNext, and Captions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/assets/third_party/ijkplayer/PROVENANCE.md`:
- Around line 29-43: Update the provenance build recipe to pin exact JDK,
Android SDK/build-tools, NDK, CMake, Gradle, and native compiler versions
instead of allowing ranges or platform-dependent toolchains. Provide a
deterministic containerized environment if exact cross-platform pinning is
impractical, and add verification steps requiring the produced AAR and ABI
outputs to match the recorded SHA-256 provenance values.
In `@player-core/src/main/kotlin/com/miruplay/tv/player/ExoPlaybackController.kt`:
- Around line 1060-1078: Update updateRemoteControlPlaybackState() to pass the
player’s current playback position instead of the hardcoded 0L when building
PlaybackState. Reuse the existing current-position source used by IJK/MPV
playback, such as getCurrentPosition() or ijkPositionMs, while preserving the
existing state, speed, and action behavior.
- Around line 1461-1527: Serialize all MiruIjkPlayerListener callbacks from
MiruIjkSurfaceView onto a single controller-owned task queue or actor before
mutating shared IJK state. Ensure onPrepared, onBufferingChanged, onCompletion,
onError, and onAudioTracksChanged update ijk fields, availableAudioTracks, and
_state.value only on that serialized executor, preserving callback ordering and
existing playback-state behavior.
In
`@player-ijkplayer-android/src/main/kotlin/com/miruplay/tv/player/ijk/android/MiruIjkSurfaceView.kt`:
- Around line 14-25: The loadIjkSource flow must preserve resolved playback
headers when opening sources through the Android-I/O bridge. Update the
MiruIjkSurfaceView path and IjkPlaybackAndroidIo.open integration so the source
headers reach DataSpec via withAdditionalHeaders(), while retaining the existing
URI and I/O behavior.
In `@ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerScreen.kt`:
- Around line 1497-1528: The subtitle and audio LazyRows in the PlayerMenu
rendering must scroll the selected track into view before initial focus is
assigned. Add a rememberLazyListState for each row, pass it to the corresponding
LazyRow, and use LaunchedEffect keyed to the selected subtitle/audio index to
scroll to that item (accounting for the subtitles “off” item offset) before
applying initialFocusHandle.modifier().
- Around line 1607-1611: Guard the initial focus request in the LaunchedEffect
associated with infoTabFocusRequester so requestFocus() cannot propagate an
IllegalStateException when PlayerInfoPanel is not yet attached or placed by
AnimatedVisibility. Preserve the existing focus behavior once the Button’s
internal focus target is available, and avoid assuming PlayerOptionButton itself
always provides an attached focusable node.
---
Outside diff comments:
In `@player-core/src/main/kotlin/com/miruplay/tv/player/ExoPlaybackController.kt`:
- Around line 290-319: Move updateRemoteControlPlaybackState() inside the
withContext(Dispatchers.Main) block in pause() and resume(), ensuring every
return path invokes it before leaving the main-thread context. Apply the same
adjustment to the related playback-control paths around the referenced methods,
while preserving their existing backend-specific state updates.
---
Nitpick comments:
In
`@repository-api/src/main/kotlin/com/miruplay/tv/repository/NextPlaybackSourceResolver.kt`:
- Around line 40-50: The buildAdjacent method currently loads the current
episode before checking whether an adjacent episode exists. Move the episode ?:
return null guard before currentPath computation, preserving the existing
version selection and build flow for valid adjacent episodes.
In `@ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerScreen.kt`:
- Around line 1128-1136: Update the downFocusRequester selection in PlayerScreen
so the fallback after audioTracks and subtitles uses pictureFocusRequester
instead of speedFocusRequester. Preserve the existing audio and subtitle targets
when those tracks are available.
- Around line 1919-1926: Update the when expression in the event input handling
flow to reference the already imported MiruPlayInputIntent enum directly,
removing the redundant com.miruplay.tv.design. qualification from MediaPrevious,
MediaNext, and Captions.
In `@ui-tv/src/test/kotlin/com/miruplay/tv/ui/player/PlayerKeyHandlingTest.kt`:
- Around line 12-31: The PlayerKeyHandlingTest coverage only verifies repeated
left-seek handling. Add a focused test for handlePlaybackTimelineKey using
Key.DirectionRight with KeyEventType.KeyDown, assert the event is consumed, and
verify onSkipForward is invoked while onSkipBackward is not.
In
`@web-control/src/test/kotlin/com/miruplay/tv/webcontrol/PlaybackDebugConfigMappingTest.kt`:
- Around line 24-25: Add a test assertion in PlaybackDebugConfigMappingTest for
playbackRenderBackendFromDebugValue("experimentalijkplayer"), verifying it
returns PlaybackRenderBackend.EXPERIMENTAL_IJKPLAYER alongside the existing IJK
alias cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a2cd73e9-5d70-4684-92ae-309c1a32a317
⛔ Files ignored due to path filters (3)
player-ijkplayer-android/libs/ijkplayer-classes.jaris excluded by!**/*.jarplayer-ijkplayer-android/src/main/jniLibs/arm64-v8a/libijkplayer.sois excluded by!**/*.soplayer-ijkplayer-android/src/main/jniLibs/armeabi-v7a/libijkplayer.sois excluded by!**/*.so
📒 Files selected for processing (47)
app/build.gradle.ktsapp/src/main/assets/third_party/ijkplayer/COPYING.LGPLv2.1app/src/main/assets/third_party/ijkplayer/LICENSE.OpenSSL-1.1.1wapp/src/main/assets/third_party/ijkplayer/LICENSE.libyuvapp/src/main/assets/third_party/ijkplayer/NOTICEapp/src/main/assets/third_party/ijkplayer/PROVENANCE.mdcore/model/src/main/kotlin/com/miruplay/tv/model/PlaybackUiConventions.ktcore/model/src/main/kotlin/com/miruplay/tv/model/ToneMappingModels.ktdocs/android-tv-player-controls.mdplayer-core/build.gradle.ktsplayer-core/src/main/kotlin/com/miruplay/tv/player/ExoPlaybackController.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/IjkPlaybackAndroidIo.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/MiruPlayMediaService.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/ToneMappingRuntimeConfigResolver.ktplayer-core/src/test/kotlin/com/miruplay/tv/player/ExoPlaybackControllerLazyInitTest.ktplayer-core/src/test/kotlin/com/miruplay/tv/player/IjkPlaybackAndroidIoTest.ktplayer-core/src/test/kotlin/com/miruplay/tv/player/ToneMappingRuntimeConfigResolverTest.ktplayer-ijkplayer-android/build.gradle.ktsplayer-ijkplayer-android/consumer-rules.proplayer-ijkplayer-android/src/main/AndroidManifest.xmlplayer-ijkplayer-android/src/main/kotlin/com/miruplay/tv/player/ijk/android/MiruIjkSurfaceView.ktrepository-api/src/main/kotlin/com/miruplay/tv/repository/NextPlaybackSourceResolver.ktrepository-api/src/test/kotlin/com/miruplay/tv/repository/NextPlaybackSourceResolverTest.ktsettings.gradle.ktsthird_party/ijkplayer/COPYING.LGPLv2.1third_party/ijkplayer/LICENSE.OpenSSL-1.1.1wthird_party/ijkplayer/LICENSE.libyuvthird_party/ijkplayer/NOTICEthird_party/ijkplayer/README.mdui-design/src/main/kotlin/com/miruplay/tv/design/MiruPlayComposeKeyProfile.ktui-design/src/main/kotlin/com/miruplay/tv/design/MiruPlayInputIntent.ktui-design/src/main/kotlin/com/miruplay/tv/design/MiruPlayKeyInput.ktui-design/src/test/kotlin/com/miruplay/tv/design/MiruPlayComposeKeyProfileTest.ktui-design/src/test/kotlin/com/miruplay/tv/design/MiruPlayInputIntentTest.ktui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerKeyHandling.ktui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerScreen.ktui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerViewModel.ktui-tv/src/test/kotlin/com/miruplay/tv/ui/player/PlayerInfoSanitizationTest.ktui-tv/src/test/kotlin/com/miruplay/tv/ui/player/PlayerKeyHandlingTest.ktui-tv/src/test/kotlin/com/miruplay/tv/ui/player/PlayerOptionsPanelFocusTest.ktui-tv/src/test/kotlin/com/miruplay/tv/ui/player/PlayerTimelineFocusTest.ktweb-control-core/src/main/kotlin/com/miruplay/tv/webcontrol/WebControlModels.ktweb-control-core/src/test/kotlin/com/miruplay/tv/webcontrol/WebControlSettingsRouteTest.ktweb-control/frontend/src/App.vueweb-control/src/main/kotlin/com/miruplay/tv/webcontrol/PlaybackDebugConfigMapping.ktweb-control/src/main/kotlin/com/miruplay/tv/webcontrol/WebControlService.ktweb-control/src/test/kotlin/com/miruplay/tv/webcontrol/PlaybackDebugConfigMappingTest.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
IJK provenance
Validation
The unrelated untracked Windows NUL entry is excluded.
Summary by CodeRabbit