fix(player): harden player lifecycle behavior - #102
Open
bee-san wants to merge 1 commit into
Open
Conversation
This was referenced Aug 1, 2026
bee-san
marked this pull request as ready for review
August 1, 2026 08:40
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
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.
Important
Ready for code review; device validation remains before merge. These fixes make lifecycle decisions explicit and unit-testable, but player surfaces and picture-in-picture remain framework/device behavior that JVM tests cannot fully prove.
The problem
Animated scene testing exposed several player failures that were adjacent to capture but not caused by the AVIF container code:
Playback could issue
loadfilebefore MPV had a surface, during surface recreation, or while teardown was in progress.Depending on timing, a cold start or episode change could be lost, run against a destroyed player, or leave the UI and native player disagreeing about the current video.
setVideo()could be called off the main thread.It touches Activity/player state, so relying on every upstream caller to arrive on the UI thread leaves a race at the boundary.
External MPV config resolution assumed storage state could not change.
getMPVConfigDirectory()!!.filePath!!could fail when a configured directory was missing, blank, revoked, or threw during storage lookup—even though the internal config directory was a valid fallback.Feature detection did not make picture-in-picture calls safe.
Android can still reject
setPictureInPictureParams()orenterPictureInPictureMode()withIllegalStateExceptionbecause of current Activity/framework state. The previous code repeatedly called the framework after such a rejection.Scene generation used a modal dialog.
The dialog obscured the player and made the difference between “still cancellable” and “already committing” awkward. It was too heavy for optional background media preparation.
These issues were split from the MediaCodec/FFmpeg changes so they can be reviewed and merged independently.
Why the obvious fixes were not enough
“Call
loadfileand let MPV queue it”There were two earlier behaviors, each solving only half the problem:
player.playFile()avoided the missing-surface case but could bypass the MPVstartposition already configured for resume;loadfile <url> replacepreserved start-position behavior but reintroduced the surface race.Leaving
loadfileto native timing can run acrosssurfaceCreated,surfaceDestroyed, resume, and player destruction. A delayed callback can also load an obsolete episode after a newer request.Chosen instead: retain the required
loadfile ... replacesemantics while keeping only the latest pending URL in a small surface-aware gate. Flush it when a surface becomes ready or the Activity resumes; close the gate before destroying MPV so no later retry can reach a dead player.“Post a retry with a delay”
A timer guesses when the surface will exist, can outlive the Activity, and introduces ordering races between episode changes.
Chosen instead: react to the actual surface lifecycle and keep deterministic latest-request-wins semantics.
“The PiP preference and feature flag are true, so the call is safe”
Those checks describe capability, not current framework state. Catching every exception would hide programming errors, while retrying after
IllegalStateExceptionrepeats a rejected operation.Chosen instead: a
PictureInPictureGuardcatches only the framework'sIllegalStateException, logs it, disables subsequent PiP attempts for that Activity, and uses the normal finish/back path. Unexpected exception types are not swallowed.“Keep asserting the external config path”
External storage permissions and providers are mutable runtime state. Crashing the player because an optional config location disappeared is worse than using the app's internal config.
Chosen instead: a resolver accepts only a nonblank external path and otherwise returns the internal directory. Exceptions are logged and fallback remains deterministic.
“Keep the modal progress dialog”
The operation has two phases: preparation can be cancelled, but commit should not pretend cancellation is still safe. A modal window also blocks context the user may want to see.
Chosen instead: a compact top overlay with a polite accessibility live region. It shows the cancel action only before the commit boundary and consumes taps only inside the overlay.
The selected design
Surface-aware playback gate
SurfacePlaybackLoadGateis a small state machine with four facts: surface readiness, closed state, one pending URL, and aloadNowcallback.loadNowreports failure: keep the latest URL for a later retry.AniyomiMPVView.loadFileWhenSurfaceReady()also posts to the main looper when necessary. This keeps ordering and native player access on the UI thread.Main-thread video boundary
PlayerActivity.setVideo()checks the current looper and redispatches itself throughrunOnUiThreadbefore touching player or Activity state. The guard sits at the mutating boundary rather than depending on every caller.PiP rejection guard
All PiP parameter updates and entry attempts go through one guard. After an
IllegalStateException, PiP becomes unavailable for the rest of that Activity instance; back/finish behavior continues normally instead of repeatedly invoking a framework path that already failed.MPV config fallback
The player prefers the external config only when requested and successfully resolved to a nonblank path. Null, blank, missing, or exceptional lookups use the internal app directory and record the external failure.
Non-modal scene progress
The new top overlay:
Scope
This PR includes only player and UI lifecycle hardening:
It deliberately excludes:
Verification
main.git diff --checkpasses../gradlew :app:compileDebugKotlinpasses.presentation-corebaseline code before build/tests.Remaining device checks
This approach was selected because it replaces timing guesses and scattered exception handling with small stateful boundaries that encode the actual lifecycle rules and can be tested independently.