maleszka/chronotrim
Watch
1
0
Fork
You've already forked chronotrim
 0
Code Issues Pull requests 1 Projects Releases Packages 1 Wiki Activity Actions

WIP: Video rotation feature #3

Draft
maleszka wants to merge 37 commits from 001-video-trim-editor into main
pull from: 001-video-trim-editor
merge into: maleszka:main
Conversation 0 Commits 37 Files changed 38 +2454 -72
maleszka commented 2026-07-21 21:29:18 +02:00
Owner
Copy link
No description provided. 
T039: covers requiresReencode and the NONE→ROT_90→ROT_180→ROT_270→NONE
cycling helper ahead of its implementation (T041).
T040: any non-NONE rotation must select FULL_REENCODE with no fallback
reason, and the combined final orientation written onto the export must
not double or cancel against the source's own orientation flag — exercised
against effectiveOutputMetadata (T043), not yet implemented.
T041: advances NONE→ROT_90→ROT_180→ROT_270→NONE per 90°-clockwise tap
of the single rotate button (FR-004).
T042: PreviewControllerImpl.setPreviewTransform now drives ExoPlayer's
video-effects pipeline with a ScaleAndRotateTransformation matching the
user's rotation, reusing the same decode/effects pipeline Transformer
uses for export so the source's own orientation flag combines correctly
without extra math here. Adds a shared Rotation.counterClockwiseDegrees
helper (clockwise Rotation -> the counterclockwise degrees Media3's
rotation effects expect) for reuse by the export path (T043).
T043: TranscodeServiceImpl now attaches a ScaleAndRotateTransformation
to the EditedMediaItem whenever the selection requires re-encoding, via
a new buildEditedMediaItem helper (kept separate to avoid growing
startExportAttempt further past its existing length). finalizeExport now
writes effectiveOutputMetadata's orientation instead of the source's raw
flag: a full re-encode already bakes the combined source+user rotation
into pixels, so copying the source's original flag on top would apply it
a second time; only the lossless stream-copy path still needs it, since
those bytes are untouched.

Also tightens a line in T042's setPreviewTransform that exceeded the
100-column style limit.
T045: advances EditSelection.rotation via the cycling helper and drives
the live preview through PreviewController.setPreviewTransform. No
separate re-encode-required field is stored — the UI reads
EditSelection.requiresReencode directly (FR-007), matching how
validation is already derived on demand rather than cached.

Also has FakePreviewController record the last rotation it was asked to
preview, for the rotate-button screen test (T046).
T044: a single rotate button sits alongside Play and the frame-nudge
controls, advancing rotation one 90°-clockwise step per tap and driving
the live preview via EditorViewModel.rotate() (FR-004). Also surfaces a
re-encode notice near the Export button whenever the current selection
requires it (FR-007).
T046: taps the rotate button four times and asserts the preview
transform cycles ROT_90->ROT_180->ROT_270->NONE (via
FakePreviewController.lastPreviewTransform, since there's no real GL
preview to inspect under Robolectric), then verifies a rotated export
carries the same rotation through to TranscodeService.export's selection.
T039-T046 done and covered by passing tests; T047 (on-device manual
verification) deferred to the user to run against a physical device.
Splits the single 4-item centered Row into the prev/play/next
cluster (still centered) plus Rotate pinned to the trailing edge, so
the playback cluster's center matches where it sat before rotate was
added.
Replaces the "⟳ Rotate" text label with Icons.Default.RotateRight
and a content description, matching Android's own landscape/portrait
rotate iconography instead of a bespoke text glyph.
Rewords the near-Export re-encode notice so it only describes what
exporting will do, since it no longer needs to double as
acknowledgement that a rotation was applied (the rotate button's own
animation covers that instead).
Adds a content-description assertion covering the icon-only rotate
button's accessible name, since the existing tap/cycle assertions
already went through ROTATE_BUTTON_TAG rather than button text.
Re-encode-required feedback belongs in the export options dialog
(planned separately) rather than as static text on the main editor
screen; drop it here instead of rewording it further.
Stage 1 (on-device log spike) empirically confirmed the Category C
root-cause hypothesis against the real my.mp4 on the actual Xperia
device. Stage 0 (real-file instrumented test infra) independently
confirms it again from real decoded/muxed bytes rather than logs,
though its first run needs re-verification after being rebased onto
this branch instead of main.
Wires up withDeviceTestBuilder alongside the existing withHostTestBuilder in
the AGP 9 Kotlin Multiplatform Android library DSL, giving transcoding an
instrumented test source set for tests that need real Transformer/Muxer
behavior instead of Robolectric shadows - ShadowMediaExtractor is already
known to be insufficient for rotation/sample-flag behavior (see the note in
MetadataServiceTest.kt). Adds androidx.test:runner and androidx.test:core to
the version catalog at the same 1.7.0 generation as the already-pinned
androidx.test.ext:junit 1.3.0.
Adds a small set of real (ffmpeg-generated) MP4 clips, plus the generation
script: no_rotation.mp4, rotation_90/180/270.mp4 (real container Display
Matrix rotation, matched byte-for-byte against my.mp4's own confirmed +90
matrix), and non_square_par.mp4 (2:1 sample aspect ratio, no rotation). Each
carries a bright marker box at a known raw pixel position so a test can
decode a frame and check where the marker physically is, independent of
what the container's rotation tag claims.

The rotation_* fixtures are derived from no_rotation.mp4 via ffmpeg's
-display_rotation (stream copy), so their encoded samples are bit-identical;
verified via ffprobe side_data and by hashing raw (-noautorotate) decoded
frames. ffmpeg's -metadata:s:v:0 rotate=N did not reliably write a Display
Matrix on this ffmpeg build combined with -c copy; -display_rotation does,
and needed a sign flip (it's counter-clockwise) to match Android/Media3's
clockwise convention.
The AGP 9 Kotlin Multiplatform Android library plugin's androidDeviceTest
source set does not wire a src/androidDeviceTest/assets/ directory into the
APK's true AAPT assets/ folder (confirmed empirically: packaging with that
layout produced an empty asset merge, nothing readable via context.assets).
Its Kotlin-style resources/ folder does get packaged, as plain JVM classpath
resources at the APK root - readable via ClassLoader.getResourceAsStream and
good enough once copied to a real file for MediaExtractor/MediaCodec/
Transformer to open directly.
Unlike androidHostTest, which inherits kotlin.test transitively via its
commonTest association, the device test source set has no such association
and needs it declared explicitly.
FixtureAssets copies a bundled fixture's classpath resource bytes out to
context.cacheDir (idempotent) and builds a file:// URI plus a SourceVideo
matching what a real ingestion/scan step would have populated, so tests can
feed fixtures straight into TranscodeServiceImpl.export().

FrameCornerInspector does the real-bytes inspection: readRotationDegrees
reads the container's real MediaFormat.KEY_ROTATION via a real
MediaExtractor; locateMarkerCorner decodes the first real frame via a real
MediaCodec onto an ImageReader-backed surface and reports which corner the
fixture's marker box lands in from the raw (unrotated) decoded luma -
deliberately not adjusted for whatever the container rotation claims, so a
test can independently combine the two and catch exactly the "metadata says
one thing, pixels show another" bug class this test tier exists for.
Corner.rotatedClockwiseBy composes them.

RealExportRunner drives TranscodeService.export's Flow to its terminal
ExportResult with a timeout, plus a best-effort MediaStore cleanup helper,
since real exports here land in real (test) storage via the SAVE output
method - shareInstead is not implemented yet, so SAVE is the only usable
path.
[transcoding] add instrumented rotate/trim export tests against real fixtures
All checks were successful
CI / build (pull_request) Successful in 8m34s
Details
04db412cd0
 
FixtureRotationSanityTest checks the bundled fixtures themselves, independent
of any chronotrim code: a real MediaExtractor reads back each fixture's
intended container rotation, and all rotation_* variants share
no_rotation.mp4's exact raw marker position, confirming they really are
stream copies that differ only in the Display Matrix.

TranscodeServiceRealExportTest runs a real TranscodeServiceImpl.export (real
Transformer, real Mp4Muxer, no mocks) for pure trim, pure rotate, and
trim+rotate on an already-rotated source, then combines the real decoded
(unadjusted) marker corner with the real output container rotation to get
the effective visual corner - independent of whether Transformer chooses to
physically re-encode the rotation or only tag it in the container.

Run on a real device (Xperia XQ-CC72): pure trim and trim+rotate pass;
pureRotateWithNoTrimRotatesTheOutput fails - the raw marker stays at its
unrotated position and the output container rotation is 0, i.e. Transformer
transmuxed the video track untouched (per lossless.md 1.2) but
TranscodeServiceImpl's metadata step discarded the correct combined
rotation instead of writing it, exactly matching the Category C root cause
already confirmed by the Stage 1 on-device log spike in lossless.md. This
is the instrumented, real-bytes confirmation of that same gap; fixing it is
out of scope here.
[docs] record re-verified Stage 0 results after rebasing onto this branch
All checks were successful
CI / build (pull_request) Successful in 8m26s
Details
e5bb10d879
 
The instrumented suite's first run was against a worktree accidentally
forked from main, predating this branch's rotation feature entirely.
Re-run against the actual 001-video-trim-editor code on the real Xperia
device: only the pure-rotate case still fails, and its failure now
matches the Stage 1 log-spike finding exactly, giving two independent
confirmations of the same Category C root cause.
Stage 2 of lossless.md: a container/crop-driven pure function deciding
whether the lossless-cut path can be requested at all, separate from
whether the user wants it. Crop is stubbed at NONE until cropping ships.

Claude-Session: https://claude.ai/code/session_01AhBKeJt5KRbqQJn1yY33F4
Stage 2 of lossless.md: user-facing lossless-cut preference, default
ON, distinct from LosslessCutFeasibility (whether it's possible at all).

Claude-Session: https://claude.ai/code/session_01AhBKeJt5KRbqQJn1yY33F4
Stage 2 of lossless.md: a resolved-outcome type distinct from the
requested ExportPath, carrying the trim-optimization outcome and the
video-track transcode-vs-transmux outcome separately since Media3
resolves each independently and they can diverge. Not yet wired into
TranscodeServiceImpl — that's lossless.md Stage 4.

Claude-Session: https://claude.ai/code/session_01AhBKeJt5KRbqQJn1yY33F4
Stage 2 of lossless.md: EditSelection.requiresReencode conflated "does
this edit need pixels rewritten" with "did the user ask to rotate" —
per the Media3 transmux-with-rotation-hint optimization (lossless.md
1.2), rotation alone doesn't require re-encoding. planExport now
requests LOSSLESS_TRIM_OPTIMIZED whenever LosslessCutPreference is ON
and LosslessCutFeasibility is Feasible, regardless of rotation, and
reports the feasibility reason through ExportPlan.fallbackReason only
when the toggle wanted lossless but couldn't get it (a preference of
OFF is an intentional choice, not a fallback).

Note: this alone does not yet fix the actual rotation-metadata bug
described in lossless.md Category C (that requires the resolved-signal
rewrite in Stage 4) — this stage is model/planning only.

Claude-Session: https://claude.ai/code/session_01AhBKeJt5KRbqQJn1yY33F4
[docs] mark Stage 2 done in lossless.md
All checks were successful
CI / build (pull_request) Successful in 8m30s
Details
e88b2e131a
 
Claude-Session: https://claude.ai/code/session_01AhBKeJt5KRbqQJn1yY33F4
Stage 3 of lossless.md: Media3's transmux-with-rotation-hint shortcut
(1.2) fires whenever the only video effects are no-ops or pure 90-degree
rotations, regardless of which ExportPath was requested — so a
FULL_REENCODE plan that only asks for a rotation could still be
silently transmuxed, leaving pixels unbaked when the user explicitly
turned lossless cut off (or it wasn't feasible).

Checked current Media3 1.10.1 sources (TransformerUtil.shouldTranscodeVideo,
Composition.Builder.setTransmuxVideo) rather than guessing: the first,
highest-priority check in shouldTranscodeVideo() is
encoderFactory.videoNeedsEncoding(), a public Codec.EncoderFactory
extension point documented for exactly this purpose.
Composition.Builder.setTransmuxVideo() was considered and rejected —
its own javadoc says the value is ignored for a single-MediaItem
composition, which is always chronotrim's case.

ForceVideoTranscodeEncoderFactory wraps a real DefaultEncoderFactory,
overriding only videoNeedsEncoding() to report true, and is set on the
Transformer whenever plan.path is FULL_REENCODE.

Claude-Session: https://claude.ai/code/session_01AhBKeJt5KRbqQJn1yY33F4
[docs] mark Stage 3 done in lossless.md
All checks were successful
CI / build (pull_request) Successful in 8m24s
Details
24d8ba87ff
 
Claude-Session: https://claude.ai/code/session_01AhBKeJt5KRbqQJn1yY33F4
Stage 4 of lossless.md: effectiveOutputMetadata() previously branched on
the requested ExportPath, which is only a prediction — Media3 resolves
the video track's actual transcode-vs-transmux outcome independently
per export (1.2/1.3/2.3) and can silently transmux a rotate-only
FULL_REENCODE plan, or transcode only part of a LOSSLESS_TRIM_OPTIMIZED
plan's video track (the trim-optimization lead-in segment).

Now branches on the resolved ExportResult.videoConversionProcess
instead:
- TRANSCODED: pixels are fully baked, orientation metadata is zeroed
  (unchanged from before, just gated on the real signal).
- TRANSMUXED / TRANSMUXED_AND_TRANSCODED: read the staged file's own
  tkhd rotation back via a MediaExtractor peek and copy that value
  through verbatim, rather than assuming the source's raw value or
  zero. Verified against current Media3 1.10.1 sources
  (MuxerWrapper.addTrackFormat, TrimOptimizedExportOperation) that the
  muxer writes one correctly-combined rotation for the whole track
  even when only part of it was transcoded, so both outcomes are
  handled identically on purpose.
- NOT_APPLICABLE: no video track, metadata passed through untouched.

Also added resolveTrimOptimizationOutcome/resolveVideoTrackConversionOutcome,
small pure mappers from Media3's raw ExportResult ints to the
ResolvedExportOutcome vocabulary Stage 2 introduced, and reworked
resolveActualPath to consume the mapped enum instead of a raw int.

Claude-Session: https://claude.ai/code/session_01AhBKeJt5KRbqQJn1yY33F4
[docs] mark Stage 4 done in lossless.md
All checks were successful
CI / build (pull_request) Successful in 8m21s
Details
9eeaceec53
 
Claude-Session: https://claude.ai/code/session_01AhBKeJt5KRbqQJn1yY33F4
A non-default QualityTier or ResolutionPreset produces a re-encoded track,
so it must disqualify the lossless trim-optimized path just like an
unsupported container or a crop does (lossless.md 2.4/Stage 5).
Backed by LosslessCutPreference on EditorViewModel's Loaded state
(default ON, toggleLosslessCut() flips it) and greyed out with a
visible reason whenever losslessCutFeasibility says the current
source can't take the lossless path (lossless.md 2.1/Stage 5).
[docs] mark Stage 5 done in lossless.md
All checks were successful
CI / build (pull_request) Successful in 8m38s
Details
84ef58c4a6
 
Row lacked verticalAlignment, so the label sat top-aligned against the
taller Switch. Also bumped LoadedContent's outer horizontal padding
from 16dp to 24dp so every control sits further from the screen edge.
[transcoding] retry NO_MEMORY codec exceptions even when isTransient is false (T075)
All checks were successful
CI / build (pull_request) Successful in 8m28s
Details
7891d27699
 
A captured on-device failure (msm_vidc "H/W is overloaded" with two
concurrent HEVC decode sessions) showed the encoder's CodecException
reported with actionCode=FATAL despite errorCode=ERROR_INSUFFICIENT_RESOURCE
- a genuinely transient resource race the vendor HAL just doesn't flag
as transient. Broaden isTransientResourceError to catch this by
errorCode independent of isTransient, and apply it uniformly rather
than gating on FULL_REENCODE, since Media3's trim optimization can
need its own concurrent decode+encode under LOSSLESS_TRIM_OPTIMIZED
too (lossless.md 1.2).

Also: mark Category C resolved in follow-up.md (superseded by
lossless.md, user-confirmed on-device) and record this diagnosis
against Category D/T075.
[ui] animate preview rotation entirely in Compose, off the real player
All checks were successful
CI / build (pull_request) Successful in 8m23s
Details
2bc4cb8cc8
 
Driving the rotate animation by repeatedly calling
PreviewController.setPreviewTransform (player.setVideoEffects) turned out
to be the wrong mechanism on-device: the preview surface was a
SurfaceView-backed PlayerView, which is composited as its own hardware
hole-punch that ignores ancestor view transforms entirely, so the
Compose overlay never visibly rotated the actual video (only the
letterbox frame around it moved); and repeatedly re-driving the GL
effects pipeline while paused reliably wedged playback after as few as
two rotate taps.

Removes PreviewController.setPreviewTransform from the contract (nothing
else needed it — export reads EditSelection.rotation directly) and
switches VideoRenderSurface/VideoSurface's Android actual to a raw
TextureView, which does respect Compose transforms. RotatingVideoSurface
now applies the user's rotation purely as a graphicsLayer rotate+scale,
driven by a spring Animatable, so playback is never touched by rotating
and rapid taps can't wedge it.

Claude-Session: https://claude.ai/code/session_01AhBKeJt5KRbqQJn1yY33F4
All checks were successful
CI / build (pull_request) Successful in 8m23s
Details
This pull request is marked as a work in progress.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 001-video-trim-editor:001-video-trim-editor
git switch 001-video-trim-editor

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff 001-video-trim-editor
git switch 001-video-trim-editor
git rebase main
git switch main
git merge --ff-only 001-video-trim-editor
git switch 001-video-trim-editor
git rebase main
git switch main
git merge --no-ff 001-video-trim-editor
git switch main
git merge --squash 001-video-trim-editor
git switch main
git merge --ff-only 001-video-trim-editor
git switch main
git merge 001-video-trim-editor
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
maleszka/chronotrim!3
No description provided.