fix(player): harden player lifecycle behavior

[bee-san]

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:

• FFmpegKit dependency/native fixes (build: use patched ffmpeg-kit for animated AVIF #99);
• scene worker-process isolation (refactor(player): isolate scene FFmpeg processing #100);
• MediaCodec probing, AVIF generation, validation, and diagnostics (fix(player): harden animated scene capture portability #101).

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.

[bee-san]

Rebased onto upstream/main (e5b03663b5) to clear merge conflicts

Rebased from the old base (2f648f0a68, v2.3.1) onto current main. One commit, still: c8e9cb872a.

One slice was dropped, not carried forward

The original PR included a fifth change — replacing the modal PlayerSceneMiningProgressDialog with a non-modal top overlay (PlayerSceneMiningProgressOverlay). That change no longer has anything to attach to:

• 65bbe37a (revert: remove animated AVIF scene mining (#78)) deleted PlayerSceneMiningUi.kt and the whole ui/player/scene/ package.
• e5b03663b5 re-implemented scene mining as PlayerMediaCaptureService, which has no progress UI, no sceneMiningProgress state, and no cancelSceneMiningPreCommit.

So rather than resurrect a reverted file, I dropped that hunk. If the new capture path grows a progress indicator later, the non-modal overlay design (safe-drawing insets, polite live region, 48 dp cancel target only before the commit boundary) is worth revisiting then — but it belongs with that UI, not here.

What remains — the four lifecycle fixes, unchanged

1. Surface-aware playback load gate (SurfacePlaybackLoadGate, AniyomiMPVView.loadFileWhenSurfaceReady)
2. Main-thread boundary for PlayerActivity.setVideo()
3. PiP rejection guard (PictureInPictureGuard)
4. MPV config-directory fallback (MpvConfigDirectoryResolver)

The PlayerControls.kt diff is now a single line (the enterPictureInPictureIfAvailable() call site) instead of two.

Verification at c8e9cb872a

• ./gradlew :app:compileDebugKotlin — BUILD SUCCESSFUL
• SurfacePlaybackLoadGateTest, PictureInPictureGuardTest, MpvConfigDirectoryResolverTest — 16 tests, 0 failures
• git diff --check clean

One caveat on the test run: the debug unit-test source set does not compile on current main as-is — ReaderOcrSourceTest calls availableSources(...) without the now-required mokuroAvailable argument (two call sites, main only, outside this diff). I supplied that argument locally to get the test source set compiling, ran the focused tests, then reverted it; the pushed branch does not contain that edit.

The device checks listed in the PR description are still outstanding — PiP across Android versions, surface recreation timing, and revoked external MPV config permissions are framework behavior JVM tests can't prove.

[bee-san]

CI note: the Build app failure is pre-existing on main, not from this branch

The run stops at :data:spotlessKotlinCheck before it ever reaches compile or tests:

Execution failed for task ':data:spotlessKotlinCheck'.
> The following files had format violations:
      src/main/java/tachiyomi/data/chapter/ChapterRepositoryImpl.kt
      src/main/java/tachiyomi/data/source/anime/AnimeSourceRepositoryImpl.kt

Neither file is in this PR's diff — it touches only ui/player/. I checked out e5b03663b5 (current main) clean and ran the same task:

Execution failed for task ':data:spotlessKotlinCheck'.
> The following files had format violations:
      src/main/java/tachiyomi/data/chapter/ChapterRepositoryImpl.kt
      src/main/java/tachiyomi/data/source/anime/AnimeSourceRepositoryImpl.kt

Byte-identical, so every PR against main will fail the same way until ./gradlew spotlessApply lands on main. Both are trivial (a trailing blank line, and an import-ordering swap for an aliased import).

I've deliberately not fixed them here — unrelated formatting churn in :data would make this player-lifecycle diff harder to review, and it would collide with whoever runs spotlessApply on main. Happy to send that as a separate one-commit PR if useful.

Separately, and worth flagging since it's also main-only: the debug unit-test source set doesn't compile either, because ReaderOcrSourceTest calls availableSources(...) without the required mokuroAvailable argument (2 call sites). CI never gets far enough to surface it, but it will bite as soon as Spotless is green.