Skip to content

fix(player): harden player lifecycle behavior - #102

Open
bee-san wants to merge 1 commit into
sohilsayed:mainfrom
bee-san:agent/player-lifecycle-hardening
Open

fix(player): harden player lifecycle behavior#102
bee-san wants to merge 1 commit into
sohilsayed:mainfrom
bee-san:agent/player-lifecycle-hardening

Conversation

@bee-san

@bee-san bee-san commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

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:

  1. Playback could issue loadfile before 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.

  2. 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.

  3. 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.

  4. Feature detection did not make picture-in-picture calls safe.
    Android can still reject setPictureInPictureParams() or enterPictureInPictureMode() with IllegalStateException because of current Activity/framework state. The previous code repeatedly called the framework after such a rejection.

  5. 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 loadfile and let MPV queue it”

There were two earlier behaviors, each solving only half the problem:

  • falling back to player.playFile() avoided the missing-surface case but could bypass the MPV start position already configured for resume;
  • replacing that with unconditional loadfile <url> replace preserved start-position behavior but reintroduced the surface race.

Leaving loadfile to native timing can run across surfaceCreated, surfaceDestroyed, resume, and player destruction. A delayed callback can also load an obsolete episode after a newer request.

Chosen instead: retain the required loadfile ... replace semantics 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 IllegalStateException repeats a rejected operation.

Chosen instead: a PictureInPictureGuard catches only the framework's IllegalStateException, 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

SurfacePlaybackLoadGate is a small state machine with four facts: surface readiness, closed state, one pending URL, and a loadNow callback.

  • before a surface: retain the URL;
  • on surface creation/resume: retry it;
  • on a newer request: replace the old pending URL;
  • on surface destruction: stop immediate loads but retain pending state;
  • on player destruction: close and clear everything;
  • if loadNow reports 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 through runOnUiThread before 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:

  • leaves the player visible;
  • respects safe drawing insets;
  • exposes progress through an accessibility live region;
  • has a 48 dp cancel target during the cancellable phase;
  • removes the cancel action once commit begins;
  • prevents taps on the overlay from leaking through to player controls.

Scope

This PR includes only player and UI lifecycle hardening:

  • surface/load sequencing;
  • main-thread dispatch for video changes;
  • PiP rejection behavior;
  • MPV config-directory fallback;
  • scene-progress overlay behavior.

It deliberately excludes:

Verification

  • One commit directly on upstream main.
  • git diff --check passes.
  • ./gradlew :app:compileDebugKotlin passes.
  • Focused unit coverage was added for:
    • load before surface creation;
    • latest pending URL replacement;
    • retry after failed load;
    • surface recreation and close/destroy behavior;
    • PiP disabled/unavailable, successful entry, framework rejection, and no retry after rejection;
    • internal config selection plus null, blank, and throwing external lookups.
  • Upstream PR CI currently stops at repository-wide Spotless failures in untouched presentation-core baseline code before build/tests.

Remaining device checks

  • cold player startup before surface creation
  • rapid episode changes while the surface is created/recreated
  • background/resume with a pending load
  • Activity destruction with queued/off-main callbacks
  • PiP supported, unsupported, user-disabled, and framework-rejected states across Android versions
  • revoked/missing external MPV config permissions
  • scene preparation cancellation versus non-cancellable commit
  • overlay insets, accessibility announcement, rotation, and player-control interaction

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant