fix(player): restore embedded mpv playback - #64
Conversation
📝 WalkthroughWalkthroughThe PR removes the experimental GL playback pipeline, standardizes ExoPlayer wiring, migrates legacy backend values, adds GLES-aware MPV output selection, updates FFmpeg filter syntax, and simplifies TV player surface handling. ChangesPlayback backend migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PlayerScreen
participant ExoPlaybackController
participant StandardExoPlayer
PlayerScreen->>ExoPlaybackController: start playback
ExoPlaybackController->>StandardExoPlayer: prepare and play
ExoPlaybackController->>StandardExoPlayer: release standard player
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerScreen.kt (1)
448-481: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
currentPositionas an effect key can cancel the capture delay forever.
currentPositionchanges on every position tick. Each change restarts thisLaunchedEffectand cancelsdelay(250). The code writeslastStandardDebugCaptureAttemptbefore the delay, so restarts return early until the position advances bySTANDARD_DEBUG_CAPTURE_RETRY_INTERVAL_MS, and then the same cancellation repeats. If the position tick interval is below 250 ms,captureCurrentFramenever runs.Key the effect on the pending label and the boolean gate instead of the raw position.
🐛 Proposed fix for the capture scheduling
LaunchedEffect( - playbackState, - currentPosition, + pendingDebugCaptureLabel, + shouldCaptureStandardDebugFrame, playerViewRef, ) { val pendingLabel = viewModel.pendingGlFrameCaptureLabel() ?: return@LaunchedEffectRun the following script to check how often the position state emits:
#!/bin/bash # Description: Find the player position tick interval that drives currentPosition. fd -e kt -g '*PlayerViewModel*' | while IFS= read -r file; do rg -n -C 6 'currentPosition|_currentPosition|delay\(' "$file" done🤖 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 448 - 481, Update the LaunchedEffect around pendingGlFrameCaptureLabel to key only on the pending capture label and shouldCaptureStandardDebugFrame, removing raw currentPosition and playerViewRef from the effect keys. Continue reading the latest currentPosition and resolving playerViewRef inside the effect so the 250 ms delay can complete without position-tick cancellation.
🤖 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 `@player-core/src/main/kotlin/com/miruplay/tv/player/DeviceGlCapabilities.kt`:
- Around line 7-12: Update resolveGlEsMajorVersion so the decoded upper-16-bit
major version cannot return zero: treat a zero result from reqGlEsVersion shr as
ES2 while preserving the existing undefined and non-positive fallbacks. Add a
test case to EmbeddedMpvSessionOptionsTest covering a positive packed value
whose decoded major version is zero.
In
`@player-core/src/main/kotlin/com/miruplay/tv/player/EmbeddedMpvSessionOptions.kt`:
- Line 16: Update the call to effectiveEmbeddedMpvVideoOutput to pass the
resolved glEsMajorVersion from EmbeddedMpvSessionOptions, ensuring ES2 devices
select mediacodec_embed instead of gpu-next.
---
Outside diff comments:
In `@ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerScreen.kt`:
- Around line 448-481: Update the LaunchedEffect around
pendingGlFrameCaptureLabel to key only on the pending capture label and
shouldCaptureStandardDebugFrame, removing raw currentPosition and playerViewRef
from the effect keys. Continue reading the latest currentPosition and resolving
playerViewRef inside the effect so the 250 ms delay can complete without
position-tick cancellation.
🪄 Autofix
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: 405bd919-5808-4798-8c43-650621431a5e
📒 Files selected for processing (29)
app/src/test/kotlin/com/miruplay/tv/LaunchTestSourceContentModeTest.ktcore/model/src/main/kotlin/com/miruplay/tv/model/PlaybackUiConventions.ktcore/model/src/main/kotlin/com/miruplay/tv/model/ToneMappingModels.ktdata/src/test/kotlin/com/miruplay/tv/data/preferences/PlaybackPreferencesManagerTest.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/AudioDspMpvOptions.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/DeviceGlCapabilities.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/DiModule.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/ExperimentalHdrSdrVideoGraphFactory.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/ExperimentalHdrSurfaceMediaCodecVideoRenderer.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/ExperimentalRenderersFactory.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/ExperimentalVideoPipelineMode.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/PlaybackPlayerQualifiers.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/PlayerFactoryImpl.ktplayer-core/src/test/kotlin/com/miruplay/tv/player/AudioDspMpvOptionsTest.ktplayer-core/src/test/kotlin/com/miruplay/tv/player/EmbeddedMpvSessionOptionsTest.ktplayer-core/src/test/kotlin/com/miruplay/tv/player/ExoPlaybackControllerLazyInitTest.ktplayer-core/src/test/kotlin/com/miruplay/tv/player/ExperimentalHdrSdrVideoGraphFactoryTest.ktplayer-core/src/test/kotlin/com/miruplay/tv/player/ExperimentalHdrSurfaceMediaCodecVideoRendererTest.ktplayer-core/src/test/kotlin/com/miruplay/tv/player/ExperimentalVideoPipelineModeTest.ktplayer-mpv-android/src/main/kotlin/is/xyz/mpv/BaseMPVView.ktplayer-mpv-android/src/main/kotlin/is/xyz/mpv/MiruMpvSurfaceView.ktui-tv/src/main/kotlin/com/miruplay/tv/ui/player/GLVideoSurfaceView.ktui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerScreen.ktui-tv/src/main/kotlin/com/miruplay/tv/ui/settings/AddSourceScreen.ktui-tv/src/test/kotlin/com/miruplay/tv/ui/player/GLVideoSurfaceViewAttachmentPolicyTest.ktweb-control/src/main/kotlin/com/miruplay/tv/webcontrol/PlaybackDebugConfigMapping.ktweb-control/src/test/kotlin/com/miruplay/tv/webcontrol/PlaybackDebugConfigMappingTest.kt
💤 Files with no reviewable changes (13)
- player-core/src/test/kotlin/com/miruplay/tv/player/ExperimentalHdrSdrVideoGraphFactoryTest.kt
- player-core/src/main/kotlin/com/miruplay/tv/player/ExperimentalHdrSdrVideoGraphFactory.kt
- player-core/src/main/kotlin/com/miruplay/tv/player/ExperimentalRenderersFactory.kt
- player-core/src/main/kotlin/com/miruplay/tv/player/ExperimentalHdrSurfaceMediaCodecVideoRenderer.kt
- player-core/src/test/kotlin/com/miruplay/tv/player/ExperimentalHdrSurfaceMediaCodecVideoRendererTest.kt
- ui-tv/src/test/kotlin/com/miruplay/tv/ui/player/GLVideoSurfaceViewAttachmentPolicyTest.kt
- player-core/src/test/kotlin/com/miruplay/tv/player/ExperimentalVideoPipelineModeTest.kt
- player-core/src/main/kotlin/com/miruplay/tv/player/ExperimentalVideoPipelineMode.kt
- player-core/src/main/kotlin/com/miruplay/tv/player/PlaybackPlayerQualifiers.kt
- player-core/src/main/kotlin/com/miruplay/tv/player/PlayerFactoryImpl.kt
- ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/GLVideoSurfaceView.kt
- player-core/src/main/kotlin/com/miruplay/tv/player/DiModule.kt
- web-control/src/main/kotlin/com/miruplay/tv/webcontrol/PlaybackDebugConfigMapping.kt
| fun resolveGlEsMajorVersion(reqGlEsVersion: Int): Int = | ||
| when { | ||
| reqGlEsVersion == ConfigurationInfo.GL_ES_VERSION_UNDEFINED -> 2 | ||
| reqGlEsVersion <= 0 -> 2 | ||
| else -> reqGlEsVersion shr 16 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file=$(git ls-files | grep -F 'player-core/src/main/kotlin/com/miruplay/tv/player/DeviceGlCapabilities.kt' | head -n1)
printf '%s\n' "$file"
wc -l "$file"
cat -n "$file"
printf '\nTest references:\n'
rg -n -C 4 'resolveGlEsMajorVersion|EmbeddedMpvSessionOptionsTest|reqGlEsVersion|GL_ES_VERSION_UNDEFINED' .Repository: ModerRAS/MiruPlay
Length of output: 6816
🌐 Web query:
Android SDK ConfigurationInfo reqGlEsVersion "major" "minor" 16 bits GL_ES_VERSION_UNDEFINED
💡 Result:
In the Android SDK, the property reqGlEsVersion represents the OpenGL ES version required by an application, using a 32-bit integer format where the upper 16 bits define the major version and the lower 16 bits define the minor version [1][2][3]. GL_ES_VERSION_UNDEFINED is a constant defined as 0, which serves as the default value for reqGlEsVersion when no specific version has been set [1][2][3]. Key details regarding this configuration include: Bitwise Representation: The value is encoded such that the major and minor versions are separated into the high and low 16-bit segments, respectively [1][3]. For example, a value of 0x00010002 corresponds to OpenGL ES version 1.2 [2][3]. You can extract these values using the getGlEsVersion method, which parses this integer into a readable "major.minor" string [1][2][3]. Usage in Manifest: Developers declare these requirements in their AndroidManifest.xml using the uses-feature element with the android:glEsVersion attribute [4][5]. If an application does not specify this attribute, the system defaults to assuming the app requires OpenGL ES 1.0, which is supported by all Android devices [4]. Context in Data Structures: This field appears in both the ConfigurationInfo and FeatureInfo classes [1][6]. In FeatureInfo, the reqGlEsVersion is only considered valid if the name field of the object is null, indicating that the feature represents an OpenGL ES version requirement [6][3].
Citations:
- 1: https://developer.android.com/reference/kotlin/android/content/pm/ConfigurationInfo
- 2: https://developer.android.com/reference/android/content/pm/ConfigurationInfo
- 3: https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/content/pm/FeatureInfo.java
- 4: https://developer.android.com/guide/topics/manifest/uses-feature-element
- 5: https://developer.android.google.cn/develop/ui/views/graphics/opengl/about-opengl
- 6: https://developer.android.com/reference/kotlin/android/content/pm/FeatureInfo
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' 'Relevant test section:'
cat -n player-core/src/test/kotlin/com/miruplay/tv/player/EmbeddedMpvSessionOptionsTest.kt | sed -n '145,170p'
printf '\nCall sites and related GL-version logic:\n'
rg -n -C 3 'resolveDeviceGlEsMajorVersion|resolveGlEsMajorVersion|glEsMajor|GLES' player-core/src/main player-core/src/test
printf '\nBehavioral probe for packed values:\n'
python3 - <<'PY'
values = [0, -1, 0x0000FFFF, 0x00010002, 0x00030000, 0x00030002]
for value in values:
signed = value if value < 2**31 else value - 2**32
major = signed >> 16
fallback = 2 if signed == 0 or signed <= 0 else major
corrected = 2 if signed <= 0 else (major if major > 0 else 2)
print(f"{value:`#010x`}: encoded major={(value & 0xffffffff) >> 16}, current={fallback}, corrected={corrected}")
PYRepository: ModerRAS/MiruPlay
Length of output: 11017
Fallback to ES2 when the packed major version is zero.
ConfigurationInfo.reqGlEsVersion stores the major version in the upper 16 bits. A positive value such as 0x0000FFFF currently resolves to 0, but invalid values must fall back to ES2. Coerce a zero decoded major version to 2 and add this case to EmbeddedMpvSessionOptionsTest.
🤖 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/DeviceGlCapabilities.kt`
around lines 7 - 12, Update resolveGlEsMajorVersion so the decoded upper-16-bit
major version cannot return zero: treat a zero result from reqGlEsVersion shr as
ES2 while preserving the existing undefined and non-positive fallbacks. Add a
test case to EmbeddedMpvSessionOptionsTest covering a positive packed value
whose decoded major version is zero.
| shaderPaths: List<String>, | ||
| speed: Float = 1.0f, | ||
| runtimeAbiIs32Bit: Boolean = isEmbeddedMpvRuntime32Bit(), | ||
| glEsMajorVersion: Int = 3, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 --glob '*.kt' \
'buildEmbeddedMpvSessionOptions\s*\(|effectiveEmbeddedMpvVideoOutput\s*\(' \
player-core player-mpv-android ui-tvRepository: ModerRAS/MiruPlay
Length of output: 26245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all references ---'
rg -n -C 5 --hidden --glob '!build/**' --glob '!node_modules/**' \
'buildEmbeddedMpvSessionOptions|effectiveEmbeddedMpvVideoOutput|deviceGlEsMajorVersion|resolveGlEsMajorVersion' .
printf '%s\n' '--- candidate source files ---'
git ls-files | rg 'EmbeddedMpvSessionOptions|ExoPlaybackController|\.kt$|\.java$' | head -200Repository: ModerRAS/MiruPlay
Length of output: 41830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- WebControlService production call ---'
sed -n '1075,1145p' web-control/src/main/kotlin/com/miruplay/tv/webcontrol/WebControlService.kt
printf '%s\n' '--- Embedded MPV option functions ---'
sed -n '1,45p;125,145p' player-core/src/main/kotlin/com/miruplay/tv/player/EmbeddedMpvSessionOptions.kt
printf '%s\n' '--- GLES capability resolution ---'
cat -n player-core/src/main/kotlin/com/miruplay/tv/player/DeviceGlCapabilities.kt
printf '%s\n' '--- all production uses of the diagnostic value ---'
rg -n -C 4 --glob 'src/main/**/*.kt' \
'effectiveEmbeddedMpvVo|embeddedMpvVo|effectiveEmbeddedMpvVideoOutput' web-control player-core appRepository: ModerRAS/MiruPlay
Length of output: 7856
Pass the resolved GLES version to effectiveEmbeddedMpvVideoOutput. The production debug snapshot omits glEsMajorVersion, so ES2 devices report gpu-next instead of mediacodec_embed.
🤖 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/EmbeddedMpvSessionOptions.kt`
at line 16, Update the call to effectiveEmbeddedMpvVideoOutput to pass the
resolved glEsMajorVersion from EmbeddedMpvSessionOptions, ensuring ES2 devices
select mediacodec_embed instead of gpu-next.
f32a82b to
9eb81c4
Compare
ModerRAS
changed the title
1 participant
Summary
mediacodec_embedon GLES 2 devices and keep the cached VO synchronized across Surface lifecycle changesgain_entrysyntax so audio DSP initializes correctlyEXPERIMENTAL_GLserialization token and migrate existing preferences to standard ExomasterVerification
./gradlew test :app:assembleDebuggit diff --checkVO: [mediacodec_embed], and audio/video playingVersioning
Patch-level player fix. The branch is rebased onto the current
2.6.xmaster baseline; no major/minor version bump is needed.