Add external subtitle playback#44
Conversation
|
Warning Review limit reached
Next review available in: 33 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 Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe changes add external subtitle discovery, MLIP import, Room persistence, playback resolution, mpv/Exo handling, and a TV subtitle-off option. The database advances to version 8, and the application fallback version changes to 1.1.0. ChangesExternal subtitle playback
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Scanner
participant MediaIndexRepository
participant PlayerViewModel
participant PlaybackController
participant EmbeddedMpv
Scanner->>MediaIndexRepository: store externalSubtitlePaths
PlayerViewModel->>MediaIndexRepository: resolve indexed playback subtitles
MediaIndexRepository-->>PlayerViewModel: subtitle tracks
PlayerViewModel->>PlaybackController: start resolved PlaybackSource
PlaybackController->>EmbeddedMpv: loadMedia with external subtitle paths
EmbeddedMpv-->>PlaybackController: subtitle track list
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: 2
🧹 Nitpick comments (2)
player-core/src/main/kotlin/com/miruplay/tv/player/ExoPlaybackController.kt (2)
408-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the nullable-index resolution for embedded mpv subtitle selection.
The
trackIndex?.let(embeddedSubtitleTrackIds::getOrNull) ?: if (trackIndex == null) null else return@withContextexpression is correct but hard to follow. Awhenreads more clearly:♻️ Suggested refactor
- val nativeTrackId = trackIndex?.let(embeddedSubtitleTrackIds::getOrNull) ?: if (trackIndex == null) null else return@withContext + val nativeTrackId = when (trackIndex) { + null -> null + else -> embeddedSubtitleTrackIds.getOrNull(trackIndex) ?: return@withContext + }🤖 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 408 - 421, In setSubtitleTrack, replace the complex nullable trackIndex resolution for EXPERIMENTAL_MPV_EMBEDDED with an explicit when expression: resolve null to null, return from the Main context when a non-null index has no matching embedded subtitle ID, and otherwise use the mapped ID. Preserve the existing setSubtitleTrack and selectedSubtitleTrackIndex behavior.
1264-1264: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed 150ms delay before refreshing embedded mpv subtitle tracks is timing-dependent.
postDelayed(::refreshEmbeddedMpvSubtitleTracks, 150L)assumes mpv finishes processing thesub-addcommands issued duringMPV_EVENT_FILE_LOADEDwithin 150ms. On slower storage/larger subtitle files this could race, leavinggetAvailableSubtitles()briefly (or persistently, if never refreshed again) missing tracks. The file already wiresobserveProperty/eventPropertyfortime-pos/duration/full/pause/eof-reached; consider observingtrack-list/countthe same way instead of a fixed delay.Also applies to: 1298-1323
🤖 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` at line 1264, Replace the fixed 150ms refresh scheduling around refreshEmbeddedMpvSubtitleTracks, including the related logic at the referenced later block, with an observeProperty/eventProperty handler for mpv’s track-list/count property. Trigger subtitle-track refresh when the property changes after sub-add processing, while preserving the existing time-pos, duration/full, pause, and eof-reached observers.
🤖 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
`@desktop-app/src/main/kotlin/com/miruplay/tv/desktop/MiruPlayDesktopComposeApp.kt`:
- Around line 1045-1065: Update the subtitle selection logic in the
DesktopPlaybackLaunchRequest construction to preserve an explicit “no subtitle”
state separately from an absent subtitle value. Ensure a deliberately blank
subtitlePath does not trigger indexedSubtitlePaths fallback, while still
allowing automatic indexed subtitle discovery when no explicit override was
provided.
In `@scanner/src/main/kotlin/com/miruplay/tv/scanner/DefaultMediaScanner.kt`:
- Around line 99-101: Update siblingFilePaths in DefaultMediaScanner so it
applies shouldInclude(file.name, config) in addition to excluding directories
before matching subtitles. Ensure matchingExternalSubtitlePaths receives only
scan-approved sibling candidates, including the analogous construction at the
other referenced location, while preserving the existing video-file filtering
flow.
---
Nitpick comments:
In `@player-core/src/main/kotlin/com/miruplay/tv/player/ExoPlaybackController.kt`:
- Around line 408-421: In setSubtitleTrack, replace the complex nullable
trackIndex resolution for EXPERIMENTAL_MPV_EMBEDDED with an explicit when
expression: resolve null to null, return from the Main context when a non-null
index has no matching embedded subtitle ID, and otherwise use the mapped ID.
Preserve the existing setSubtitleTrack and selectedSubtitleTrackIndex behavior.
- Line 1264: Replace the fixed 150ms refresh scheduling around
refreshEmbeddedMpvSubtitleTracks, including the related logic at the referenced
later block, with an observeProperty/eventProperty handler for mpv’s
track-list/count property. Trigger subtitle-track refresh when the property
changes after sub-add processing, while preserving the existing time-pos,
duration/full, pause, and eof-reached observers.
🪄 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
Run ID: c9b9fcb0-5cd7-4feb-89d8-18fc78d2cc94
📒 Files selected for processing (31)
app/build.gradle.ktsapp/src/main/kotlin/com/miruplay/tv/di/AppModule.ktapp/src/test/kotlin/com/miruplay/tv/di/AppModuleTest.ktcore/model/src/main/kotlin/com/miruplay/tv/model/PlaybackUiConventions.ktcore/model/src/main/kotlin/com/miruplay/tv/model/SubtitleTracks.ktcore/model/src/test/kotlin/com/miruplay/tv/model/SubtitleTracksTest.ktdata/schemas/com.miruplay.tv.data.db.MiruPlayDatabase/8.jsondata/src/main/kotlin/com/miruplay/tv/data/dao/IndexDao.ktdata/src/main/kotlin/com/miruplay/tv/data/db/MiruPlayDatabase.ktdata/src/main/kotlin/com/miruplay/tv/data/entity/IndexEntryEntity.ktdata/src/main/kotlin/com/miruplay/tv/data/repository/IndexRepositoryImpl.ktdata/src/test/kotlin/com/miruplay/tv/data/db/MiruPlayDatabaseMigrationTest.ktdata/src/test/kotlin/com/miruplay/tv/data/repository/IndexRepositoryImplTest.ktdesktop-app/src/main/kotlin/com/miruplay/tv/desktop/MiruPlayDesktopComposeApp.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/EmbeddedMpvSessionOptions.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/ExoPlaybackController.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/PlaybackController.ktplayer-core/src/test/kotlin/com/miruplay/tv/player/EmbeddedMpvSessionOptionsTest.ktplayer-core/src/test/kotlin/com/miruplay/tv/player/SubtitleMimeTypeTest.ktplayer-mpv-android/src/main/kotlin/is/xyz/mpv/MiruMpvSurfaceView.ktrepository-api/src/main/kotlin/com/miruplay/tv/repository/MediaIndexRepository.ktrepository-api/src/main/kotlin/com/miruplay/tv/repository/PlaybackSubtitleResolver.ktrepository-api/src/test/kotlin/com/miruplay/tv/repository/PlaybackSubtitleResolverTest.ktscanner-desktop/src/main/kotlin/com/miruplay/tv/scanner/desktop/DesktopMediaLibraryScanner.ktscanner/src/main/kotlin/com/miruplay/tv/scanner/DefaultMediaScanner.ktscanner/src/main/kotlin/com/miruplay/tv/scanner/MlipLibraryIndexImporter.ktscanner/src/main/kotlin/com/miruplay/tv/scanner/ScanCoordinator.ktscanner/src/test/kotlin/com/miruplay/tv/scanner/MlipLibraryIndexImporterTest.ktscanner/src/test/kotlin/com/miruplay/tv/scanner/ScanCoordinatorTest.ktui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerScreen.ktui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerViewModel.kt
1 participant
Summary
media_subtitleentries without WebDAV directory scansValidation
./gradlew test --no-daemon./gradlew :app:assembleDebug --no-daemonMlipLibraryIndexImporterTestNote
./gradlew lintremains blocked by the pre-existing API 29MediaStore.Downloads.EXTERNAL_CONTENT_URIissue inEarlyStartupDiagnostics.kt.Summary by CodeRabbit
New Features
Bug Fixes