fix(player): harden animated scene capture portability - #101
Closed
bee-san wants to merge 1 commit into
Closed
Conversation
Contributor
Author
Local verification at
|
This was referenced Aug 1, 2026
bee-san
marked this pull request as ready for review
August 1, 2026 08:40
Owner
|
closed as we are going in a different direction |
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 a merge gate. Local Kotlin compilation and focused scene tests pass, but JVM tests mock the native executor and cannot prove vendor MediaCodec behavior. Unsupported or failed animation generation deliberately returns the already-captured still image.
The problem
Animated Anki scene capture worked on the device and media shape it was first built around, but the implementation made assumptions that Android's codec API does not guarantee:
Real devices vary in alignment, minimum/maximum dimensions, block budgets, conditional width ranges, rate support, color formats, CQ support, and vendor behavior. Real media also carries rotation, sample-aspect ratio, 10-bit SDR, HDR metadata, embedded subtitle streams, SAF inputs, and remote URLs.
The result was not one isolated bug. Depending on device/input, capture could select an unusable encoder, distort portrait or anamorphic video, request unsupported dimensions, produce an AVIF with an invalid first frame, fail on a harmless subtitle stream, lose cancellation, leak/delete files at the wrong time, or simply return a still with no useful diagnostic.
Concrete failures that drove the design
These were not hypothetical codec-matrix concerns:
/proc/self/fd/497, reopened it by path, lost the SAF grant, and failed withPermission deniedbefore MediaCodec ran. The fix is FFmpegKit's nativesaf:protocol rather than reopening a FUSE-backed descriptor path. Diagnosis640×640 @ 8 fpscheck, so capture returned in roughly 13 ms even though the actual 16:9 source could be encoded at640×360. Source-aware selection later produced a validated 32-frame, four-second looping AVIF. Failure · follow-up passmov_textstream was missing from the bounded decoder whitelist. The subtitle was not output, but libavformat still initialized it while probing. Addingmov_textallowed the same input to produce animated AVIF and sentence audio. Diagnosis and follow-upThese runs establish the actual failure modes and validate predecessor integrated builds. They are not exact-head coverage of this standalone upstream PR; that is why the vendor matrix remains open below.
Approaches tried before this one
Fixed 640×640 capability check
This was simple, but it asked the wrong question. An encoder may support a useful portrait or landscape canvas while rejecting 640×640, or advertise 640×640 while imposing alignment/rate/conditional-range constraints elsewhere. Forcing visible content into that square also risks distortion.
Why rejected: codec capability is a constraint-solving problem, not a single
isSizeSupported(640, 640)check.First advertised AV1 encoder
Codec enumeration order is not a quality or compatibility guarantee. The first encoder may lack planar YUV420, constant-quality mode, a usable quality range, 8 fps support, or enough resolution for the source while a later candidate works.
Why rejected: it turns vendor ordering into application behavior and misses valid alternatives.
Normalize raw AV1 OBUs in Kotlin, then remux in a second FFmpeg pass
An earlier implementation wrote an intermediate AV1 stream, normalized OBUs in Kotlin, and remuxed that file into AVIF.
Why rejected: it duplicated AV1 syntax handling outside FFmpeg, added a second native session and temporary file, complicated cancellation/ownership, and still could not safely repair MediaCodec's internal GOP/reference state. The native invariants are handled by the narrowly patched dependency in #99 instead.
Accept any file that FFmpeg reports as successful
A non-empty file and zero exit code do not prove a valid animated AVIF. Container boxes can point outside the file, sample sizes can disagree, timing can be unusable, and frame 1 may not be independently decodable.
Why rejected: animation is optional; failing closed to the still image is safer than handing corrupt media to Anki.
Retry broadly or permit every input
Blind retries repeat deterministic codec/container failures. Passing extension arguments, credentials, DRM/transient sources, or unrestricted remote protocols into FFmpeg expands both the failure and security surface.
Why rejected: select capabilities before encoding and reject unsafe inputs before native execution.
Why this solution
The final path is probe → select → encode once → validate → deliver or fall back.
1. Freeze and inspect the request
The request carries the resolved scene timing and a stable video-input description. Capture rejects missing/changed state rather than guessing at a different stream or time range.
ffprobe extracts the facts needed for encoding:
Invalid dimensions, protected/HDR/BT.2020 content, unsafe sources, and unusable probe output fail before an output file is committed. 8-bit and 10-bit SDR input are accepted; 10-bit input is converted to
yuv420p, not advertised as preserved 10-bit output.2. Evaluate every usable hardware AV1 encoder
SceneAv1EncoderSelectorexamines each MediaCodec AV1 candidate for:Display geometry is derived from coded dimensions, SAR, and rotation. Visible content is scaled without intentional distortion and then black-padded into a supported 16-pixel-aligned canvas. Candidate ranking first preserves content resolution, then avoids an unnecessarily large canvas.
This separates content size from encoder canvas size—the key distinction the fixed-square approach lacked.
3. Use one native encode/mux command
The selected stream is sampled at 8 fps, scaled/padded, converted to
yuv420p, encoded with the chosenav1_mediacodecencoder, and muxed directly into looping AVIF.The two-pass Kotlin OBU/remux pipeline is gone. #99 supplies the native parser/MediaCodec reset/configuration/first-keyframe fixes required for this direct path.
The command remains bounded:
mov_textallowed so an embedded MP4 subtitle track does not invalidate otherwise safe video.4. Validate before ownership transfer
A successful native return is necessary but not sufficient.
AnimatedAvifValidatorchecks the output's:stssis present.If command construction, probing, encoding, validation, or delivery fails, partial output is removed and the caller keeps the still-image fallback.
5. Make cancellation and cleanup native-aware
Kotlin cancellation does not mean native FFmpeg has already stopped. The implementation therefore separates Kotlin ownership from native-use leases:
6. Produce useful diagnostics without leaking inputs
The original path had many early returns and almost no explanation; one device test could return a still without revealing which gate fired.
Tagged
SceneMininglogs now cover request rejection, probe output, encoder selection, native failure, validation, cancellation, and cleanup. Remote paths and embedded URLs are reduced/redacted, and FFmpegKit's own unredacted logcat output remains suppressed.Input policy
The path supports seekable local inputs and constrained public HTTP(S)/HLS sources. It intentionally rejects or falls back for:
Those are explicit safety/portability decisions, not accidental unsupported cases.
Scope
This PR includes MediaCodec selection, geometry, probe/command construction, capture ownership, AVIF validation, safe input handling, redacted diagnostics, and focused tests.
It deliberately excludes:
Verification
Verified at exact head
f0624257e6237e7fd796f08104f9e4ac4086abbf:./gradlew :app:compileDebugKotlin— BUILD SUCCESSFULeu.kanade.tachiyomi.ui.player.scene.*tests — 69 passed, 0 failed, 0 errors, 0 skippedgit diff --checkpassesThe focused test task initially encountered an unrelated baseline test-compile defect:
ReaderOcrSourceTestomits the now-requiredmokuroAvailableargument. The same mismatch exists on upstreammain; it was temporarily supplied only in the isolated verification worktree, then reverted. No such change is in this PR.Upstream PR CI currently stops earlier at repository-wide Spotless failures in untouched
presentation-corecode, so the red check does not represent a compile/test failure in this diff.What the tests cover
Focused coverage exercises:
Remaining device work
This PR is the selected design because it asks the platform what it can encode, preserves display geometry, performs one bounded native operation, validates the artifact before delivery, and treats animation as optional. It does not claim universal vendor compatibility until the device matrix is complete.