From 48040306b649d23e12a863481eefb76938922d7c Mon Sep 17 00:00:00 2001 From: Steven Gates Date: Sun, 21 Jun 2026 20:18:51 -0500 Subject: [PATCH] fix(reverse): software-codec fallback when a HW codec rejects start (47233ad7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a non-Samsung low-end device (LG LM-X540, Android 10) the hardware AVC codec rejects start with `IllegalArgumentException: start failed`. The reverse pipeline only retried Samsung lifecycle churn / "surface released", so this failure escaped on the first attempt with no fallback — both the editor reverse preview and the boomerang save failed. Fix: - Add isMediaCodecInitializationFailure + shouldRetryReverseWithSoftwareCodec (MediaCodecLifecycle) to detect a codec that fails to initialize, distinct from benign teardown. - VideoReverser now retries once on the software encoder AND decoder on ANY manufacturer after an init failure (threaded preferSoftwareDecoder through the pipeline; the platform-default decoder path previously had no fallback). - BoomerangRenderWorker now reports the real render cause to Crashlytics in its IOException/ExportException/RuntimeException branches — previously the actual codec exception lived only in logcat ("details in BoomerangRenderWorker log") and never reached Firebase. Tests: - MediaCodecLifecycleTest: predicate + retry-decision coverage incl. the exact "start failed" signature and a regression guard vs. the old contention path. - VideoReverserTest.reverse_recoversFromCodecStartFailure_viaSoftwareFallback: on-device fault-injection (new VideoReverser.attemptHook test seam) injects "start failed" on attempt 0 and asserts the reverse recovers via the software encoder+decoder and produces a valid, playable clip. Passed on Pixel 6. - Fix pre-existing test-source-set breakage that blocked the whole suite: DeviceMediaHintsTest / SamsungReversePreviewRegressionTest referenced removed *_MS constants; OpenLoopViewModelTest asserted a RAW that the render flow now intentionally deletes after a successful save. Docs: record the production zero-error rule (DEFINITION_OF_DONE.md + CLAUDE.md) and the 3-device E2E sweep report (docs/e2e/). Co-authored-by: Cursor --- CLAUDE.md | 2 + .../openloop/media/VideoReverserTest.kt | 38 +++++++++++ .../openloop/media/MediaCodecLifecycle.kt | 45 ++++++++++++ .../stozo04/openloop/media/VideoReverser.kt | 68 ++++++++++++++++--- .../openloop/work/BoomerangRenderWorker.kt | 23 +++++++ .../openloop/media/DeviceMediaHintsTest.kt | 4 +- .../openloop/media/MediaCodecLifecycleTest.kt | 52 ++++++++++++++ .../SamsungReversePreviewRegressionTest.kt | 2 +- .../openloop/ui/OpenLoopViewModelTest.kt | 21 +++++- docs/DEFINITION_OF_DONE.md | 12 ++++ docs/e2e/2026-06-21_150054.md | 66 ++++++++++++++++++ 11 files changed, 317 insertions(+), 16 deletions(-) create mode 100644 docs/e2e/2026-06-21_150054.md diff --git a/CLAUDE.md b/CLAUDE.md index 6b53154..906c993 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -51,6 +51,8 @@ Before anything destructive (deleting files, overwriting code, sending communica ### Definition of Done — required before "done" or "Ready for PR" +**Production zero-error rule (non-negotiable):** OpenLoop is live in Production and reachable by billions of users. **Any error the agent encounters while working — a failing test, a compile error, a lint error, a crash — MUST be resolved before a PR is created, even pre-existing failures the agent did not introduce.** "Not my change" is never a reason to open a PR on a red baseline; fix it as part of the work, or stop and escalate to the owner for explicit direction. A PR is opened only from a fully green state (clean debug + release build, 0 test failures, 0 new lint errors). Full policy in **[`docs/DEFINITION_OF_DONE.md`](docs/DEFINITION_OF_DONE.md)**. + A change is **not done because it compiles.** Before calling any non-trivial change done or opening a PR, clear the verification gate in **[`docs/DEFINITION_OF_DONE.md`](docs/DEFINITION_OF_DONE.md)**: baseline → clean build (debug **and** release) genuinely green → requirement checks (e.g. 16 KB `zipalign`) → unit + instrumented tests with 0 failures → **static analysis clean** (Android Lint reports zero *new* errors via `./gradlew :app:lintDebug`, and IDE "Inspect Code" / `inspect.bat` run locally — see **[`docs/STATIC_ANALYSIS.md`](docs/STATIC_ANALYSIS.md)**) → **actually run the app on an emulator, launch it, and capture a screenshot as proof** → honestly state what could not be verified + a manual QA checklist → attach the screenshot to the PR. "Genuinely green" = `BUILD SUCCESSFUL` **and** exit code 0 **and** zero `e:` errors (never trust a `| tail`-masked exit code). This is the standard, not a nice-to-have. ### Note-taking diff --git a/app/src/androidTest/java/io/github/stozo04/openloop/media/VideoReverserTest.kt b/app/src/androidTest/java/io/github/stozo04/openloop/media/VideoReverserTest.kt index 0cc94e0..e7ddd51 100644 --- a/app/src/androidTest/java/io/github/stozo04/openloop/media/VideoReverserTest.kt +++ b/app/src/androidTest/java/io/github/stozo04/openloop/media/VideoReverserTest.kt @@ -177,6 +177,44 @@ class VideoReverserTest { } } + /** + * The fix for Crashlytics 47233ad7 (LG LM-X540, Android 10): when the device's hardware AVC codec + * rejects start with `IllegalArgumentException: start failed`, [VideoReverser] must retry once on + * the **software** encoder + decoder and still produce a valid reversed clip — on any manufacturer, + * not just Samsung. + * + * The 3-device emulator sweep never exercised this branch (emulators already use the software + * codec, so nothing ever rejects start). This test closes that gap by injecting the exact failure + * on attempt 0 via [VideoReverser.attemptHook], then asserting (a) a valid playable output still + * comes out, and (b) the retry actually flipped to the software encoder AND decoder. + */ + @Test + fun reverse_recoversFromCodecStartFailure_viaSoftwareFallback() = runBlocking { + val attempts = mutableListOf>() + reverser.attemptHook = { attempt, preferSoftwareEncoder, preferSoftwareDecoder -> + attempts.add(Triple(attempt, preferSoftwareEncoder, preferSoftwareDecoder)) + // Simulate the LG hardware codec refusing to start on the first attempt only. + if (attempt == 0) throw IllegalArgumentException("start failed") + } + + val output = reverser.reverse(fixture, 0L, durationMs) + + // (a) Recovery produced a real, valid, playable reversed clip — not an empty shell. + val validation = ReverseOutputValidator.validateReversedOutput(output) + assertTrue("expected a valid recovered reverse, got ${validation.reason}", validation.valid) + assertTrue("expected >=1 muxed sample, got ${validation.sampleCount}", validation.sampleCount >= 1) + + // (b) It recovered via the software-codec retry: attempt 0 failed on the default path, and the + // retry (attempt 1) forced BOTH the software encoder and the software decoder. + assertTrue("expected the reverse to retry, got attempts=$attempts", attempts.size >= 2) + assertEquals("first attempt index", 0, attempts[0].first) + assertTrue("first attempt must not force the software decoder", !attempts[0].third) + val retry = attempts[1] + assertEquals("retry must be attempt index 1", 1, retry.first) + assertTrue("retry must force the software ENCODER", retry.second) + assertTrue("retry must force the software DECODER", retry.third) + } + // ── Fixture acquisition ────────────────────────────────────────────────────────────────────── /** Bundled asset → synthetic generation → skip. Never returns a non-existent file. */ diff --git a/app/src/main/java/io/github/stozo04/openloop/media/MediaCodecLifecycle.kt b/app/src/main/java/io/github/stozo04/openloop/media/MediaCodecLifecycle.kt index c22a3fb..7b7060f 100644 --- a/app/src/main/java/io/github/stozo04/openloop/media/MediaCodecLifecycle.kt +++ b/app/src/main/java/io/github/stozo04/openloop/media/MediaCodecLifecycle.kt @@ -1,5 +1,6 @@ package io.github.stozo04.openloop.media +import android.media.MediaCodec import kotlinx.coroutines.CancellationException import kotlinx.coroutines.isActive import kotlin.coroutines.coroutineContext @@ -65,3 +66,47 @@ internal fun shouldRetryMediaCodecContention( if (failedAttemptIndex >= maxAttempts - 1 || !isMediaCodecLifecycleFailure(error)) return false return samsung || isMediaCodecSurfaceReleasedFailure(error) } + +/** + * A [MediaCodec] that fails to **initialize** — `configure()` / `start()` rejecting the request — + * as opposed to a benign teardown ([isMediaCodecLifecycleFailure]) or a healthy-codec data problem. + * + * The motivating signature is `IllegalArgumentException: start failed` (LG LM-X540 / Android 10, + * Crashlytics 47233ad7): "start failed" is the JNI message `android.media.MediaCodec.native_start()` + * passes to `throwExceptionAsNecessary`, and a `BAD_VALUE` native status surfaces it as an + * [IllegalArgumentException]. On a low-end (often MediaTek) device under memory pressure the *hardware* + * AVC codec can reject start outright; the software codec usually still comes up. These failures are + * NOT Samsung-specific, so [shouldRetryMediaCodecContention] never recovered them — the reverse died + * on the first attempt with no fallback. + * + * [MediaCodec.CodecException] (the documented codec-error type) also counts, except when it presents + * as a benign lifecycle teardown (already filtered out above). + */ +internal fun isMediaCodecInitializationFailure(error: Throwable): Boolean { + if (isMediaCodecLifecycleFailure(error)) return false + return when (error) { + is MediaCodec.CodecException -> true + is IllegalArgumentException, is IllegalStateException -> { + val message = error.message.orEmpty() + message.contains("start failed", ignoreCase = true) || + message.contains("configure failed", ignoreCase = true) || + message.contains("codec failed", ignoreCase = true) || + message.contains("Failed to initialize", ignoreCase = true) || + message.contains("Error initializing", ignoreCase = true) + } + else -> false + } +} + +/** + * Whether [VideoReverser.reverse] should retry the failed pass with **software** codecs after a + * codec-initialization failure ([isMediaCodecInitializationFailure]). Unlike + * [shouldRetryMediaCodecContention], this is device-agnostic — the hardware codec rejecting + * `start()` is the failure, so the only useful next move is the software encoder + decoder, on any + * manufacturer. Retries only while an attempt remains (mirrors the contention/zero-frame guards). + */ +internal fun shouldRetryReverseWithSoftwareCodec( + error: Throwable, + failedAttemptIndex: Int, + maxAttempts: Int, +): Boolean = failedAttemptIndex < maxAttempts - 1 && isMediaCodecInitializationFailure(error) diff --git a/app/src/main/java/io/github/stozo04/openloop/media/VideoReverser.kt b/app/src/main/java/io/github/stozo04/openloop/media/VideoReverser.kt index b9c5cc3..b4a77e9 100644 --- a/app/src/main/java/io/github/stozo04/openloop/media/VideoReverser.kt +++ b/app/src/main/java/io/github/stozo04/openloop/media/VideoReverser.kt @@ -9,6 +9,7 @@ import android.media.MediaMuxer import android.os.Build import android.util.Log import android.view.Surface +import androidx.annotation.VisibleForTesting import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.delay @@ -52,6 +53,16 @@ class VideoReverser( @Volatile private var lastSurfaceEncoderName: String? = null + /** + * Test-only fault-injection seam. Invoked at the start of every [reverse] attempt with the + * attempt index and the software-codec preferences that attempt will use. An instrumented test + * throws `IllegalArgumentException("start failed")` from here on attempt 0 to simulate the LG + * LM-X540 hardware-codec start rejection (Crashlytics 47233ad7), then asserts the reverse retries + * on the software encoder + decoder and still produces a valid clip. Always null in production. + */ + @VisibleForTesting + internal var attemptHook: ((attempt: Int, preferSoftwareEncoder: Boolean, preferSoftwareDecoder: Boolean) -> Unit)? = null + /** * Reverse [source] over `[trimStartMs, trimEndMs]`, returning the reversed MP4 [File]. * Throws [java.io.IOException] / [MediaCodec.CodecException] on a pipeline failure (the caller @@ -103,6 +114,12 @@ class VideoReverser( // HW-encoder attempt will wedge again — skip straight to the software encoder instead of // paying a doomed attempt + retry delay on each preview/save (S23/API 33, RESEARCH.md §7c). var zeroFrameFailure = zeroFrameEncoderWedgeSticky + // Set after a codec-initialization failure (e.g. LG LM-X540 `IllegalArgumentException: + // start failed`): the device's HW AVC codec rejected start/configure, so the retry forces + // BOTH the software encoder AND the software decoder, on any manufacturer (Crashlytics + // 47233ad7). Distinct from zeroFrameFailure (encoder-only flip) and the Samsung contention + // retry (same codecs, just a settle delay). + var softwareCodecFallback = false try { for (attempt in 0 until maxAttempts) { // Zero-frame retry pairing (spec §5.8, revised on-device 2026-06-04): on the S23/API 33 @@ -110,17 +127,23 @@ class VideoReverser( // HW-encoder pairing throws CodecException 0xe at queueInputBuffer — so the retry // flips the ENCODER to software (decoder stays per normal selection). The contention // retry path is unchanged. - val preferSoftwareEncoder = zeroFrameFailure + val preferSoftwareEncoder = zeroFrameFailure || softwareCodecFallback + val preferSoftwareDecoder = softwareCodecFallback try { coroutineContext.ensureActive() + attemptHook?.invoke(attempt, preferSoftwareEncoder, preferSoftwareDecoder) if (attempt > 0) { lastSurfaceEncoderName = null intermediate.delete() output.delete() ReversePreviewLog.i( - if (preferSoftwareEncoder) "reverse.zero_frame_retry" else "reverse.contention_retry", + when { + softwareCodecFallback -> "reverse.software_codec_retry" + preferSoftwareEncoder -> "reverse.zero_frame_retry" + else -> "reverse.contention_retry" + }, "attempt=${attempt + 1}/$maxAttempts delayMs=${SAMSUNG_CODEC_CONTENTION_RETRY.inWholeMilliseconds} " + - "preferSoftwareEncoder=$preferSoftwareEncoder", + "preferSoftwareEncoder=$preferSoftwareEncoder preferSoftwareDecoder=$preferSoftwareDecoder", ) delay(SAMSUNG_CODEC_CONTENTION_RETRY) } else { @@ -131,7 +154,9 @@ class VideoReverser( delay(PRE_REVERSE_CODEC_SETTLE) } ReversePreviewLog.d("reverse.pass1.start", "dest=${intermediate.name}") - transcodeToAllKeyframes(source, trimStartMs, trimEndMs, intermediate, preferSoftwareEncoder) { frac -> + transcodeToAllKeyframes( + source, trimStartMs, trimEndMs, intermediate, preferSoftwareEncoder, preferSoftwareDecoder, + ) { frac -> onProgress(frac * 0.5f) } ReversePreviewLog.i("reverse.pass1.done", "intermediateBytes=${intermediate.length()}") @@ -140,7 +165,7 @@ class VideoReverser( "reverse.pass2.start", "dest=${output.name} pass1Encoder=$lastSurfaceEncoderName", ) - reverseAllKeyframeVideo(intermediate, output, preferSoftwareEncoder) { frac -> + reverseAllKeyframeVideo(intermediate, output, preferSoftwareEncoder, preferSoftwareDecoder) { frac -> onProgress(0.5f + frac * 0.5f) } // A pass 2 starved of frames exits "cleanly" with a sample-less shell that the @@ -180,6 +205,16 @@ class VideoReverser( zeroFrameEncoderWedgeSticky = true continue } + // HW codec rejected configure/start (e.g. `IllegalArgumentException: start + // failed`) — retry once on the software encoder + decoder, any device. + if (shouldRetryReverseWithSoftwareCodec(t, attempt, maxAttempts)) { + softwareCodecFallback = true + ReversePreviewLog.i( + "reverse.software_codec_fallback", + "after ${t.javaClass.simpleName}: ${t.message}", + ) + continue + } if (!shouldRetryMediaCodecContention(t, attempt, maxAttempts, isSamsungDevice())) { throw t } @@ -219,6 +254,7 @@ class VideoReverser( trimEndMs: Long, dest: File, preferSoftwareEncoder: Boolean = false, + preferSoftwareDecoder: Boolean = false, onProgress: (Float) -> Unit, ) { val extractor = MediaExtractor() @@ -284,6 +320,7 @@ class VideoReverser( decoderMime = inputFormat.getString(MediaFormat.KEY_MIME)!!, decoderFormat = inputFormat, preferSoftwareEncoder = preferSoftwareEncoder, + preferSoftwareDecoder = preferSoftwareDecoder, ) decoder = pipeline.decoder encoder = pipeline.encoder @@ -339,6 +376,7 @@ class VideoReverser( source: File, dest: File, preferSoftwareEncoder: Boolean = false, + preferSoftwareDecoder: Boolean = false, onProgress: (Float) -> Unit, ) { val extractor = MediaExtractor() @@ -400,6 +438,7 @@ class VideoReverser( decoderMime = inputFormat.getString(MediaFormat.KEY_MIME)!!, decoderFormat = inputFormat, preferSoftwareEncoder = preferSoftwareEncoder, + preferSoftwareDecoder = preferSoftwareDecoder, ) decoder = pass2Pipeline.decoder encoder = pass2Pipeline.encoder @@ -812,6 +851,7 @@ class VideoReverser( decoderMime: String, decoderFormat: MediaFormat, preferSoftwareEncoder: Boolean = false, + preferSoftwareDecoder: Boolean = false, ): SurfaceCodecPipeline { var lastFailure: Throwable? = null repeat(SURFACE_CODEC_PIPELINE_MAX_ATTEMPTS) { attempt -> @@ -828,7 +868,7 @@ class VideoReverser( lastFailure = IOException("encoder input surface invalid after createInputSurface") return@repeat } - val decoder = openAvcDecoderForReverse(decoderMime, decoderFormat, surface) + val decoder = openAvcDecoderForReverse(decoderMime, decoderFormat, surface, preferSoftwareDecoder) return SurfaceCodecPipeline(decoder, encoder, surface) } catch (t: Throwable) { lastFailure = t @@ -922,23 +962,31 @@ class VideoReverser( /** * On Samsung, avoid [MediaCodec.createDecoderByType] defaulting to Exynos while ExoPlayer or pass 1 - * still holds codec slots — pair software Google decoder with [openSurfaceAvcEncoder]. + * still holds codec slots — pair software Google decoder with [openSurfaceAvcEncoder]. The same + * software try-order is used on ANY device when [preferSoftwareDecoder] is set, i.e. after a + * hardware codec-init failure tripped the software fallback (LG LM-X540 `IllegalArgumentException: + * start failed`, Crashlytics 47233ad7) — the HW decoder rejected start, so the software one is the + * only remaining option. * * Note (spec §5.8, on-device 2026-06-04): swapping THIS side to the platform decoder was tried * for the S23 zero-frame wedge and failed harder (CodecException 0xe at queueInputBuffer with - * the HW decoder feeding the HW encoder surface) — the working fallback flips the ENCODER to - * software instead; the carve-out here stays unconditional. + * the HW decoder feeding the HW encoder surface) — that zero-frame fallback flips only the ENCODER + * to software. The init-failure fallback here additionally forces the software decoder because the + * decoder's own start is what failed. */ private fun openAvcDecoderForReverse( mime: String, format: MediaFormat, outputSurface: Surface, + preferSoftwareDecoder: Boolean = false, ): MediaCodec { val installedDecoders = MediaCodecList(MediaCodecList.REGULAR_CODECS).codecInfos .filter { !it.isEncoder && it.supportedTypes.any { t -> t.equals(mime, ignoreCase = true) } } .map { it.name } .toSet() - if (isSamsungDevice()) { + // Software-decoder carve-out: always on Samsung (Exynos contention), and on any device once a + // codec-init failure has tripped the software fallback (LG LM-X540 `start failed`, 47233ad7). + if (isSamsungDevice() || preferSoftwareDecoder) { for (name in samsungSoftwareAvcDecoderTryOrder(installedDecoders)) { var decoder: MediaCodec? = null try { diff --git a/app/src/main/java/io/github/stozo04/openloop/work/BoomerangRenderWorker.kt b/app/src/main/java/io/github/stozo04/openloop/work/BoomerangRenderWorker.kt index e751dfe..1ad7cc5 100644 --- a/app/src/main/java/io/github/stozo04/openloop/work/BoomerangRenderWorker.kt +++ b/app/src/main/java/io/github/stozo04/openloop/work/BoomerangRenderWorker.kt @@ -124,6 +124,7 @@ class BoomerangRenderWorker( progressPublisher.cancel() Log.e(TAG, "Boomerang render failed (IO)", e) deletePartialOutput(parsed.outputFile) + reportRenderFailure(parsed, "io", e) Result.failure() } catch (e: ExportException) { // Media3's documented async failure type (Transformer.Listener.onError, rethrown by @@ -134,11 +135,13 @@ class BoomerangRenderWorker( progressPublisher.cancel() Log.e(TAG, "Boomerang render failed (export)", e) deletePartialOutput(parsed.outputFile) + reportRenderFailure(parsed, "export", e) Result.failure() } catch (e: RuntimeException) { progressPublisher.cancel() Log.e(TAG, "Boomerang render failed", e) deletePartialOutput(parsed.outputFile) + reportRenderFailure(parsed, "runtime", e) Result.failure() } } @@ -163,6 +166,26 @@ class BoomerangRenderWorker( } } + /** + * Record the worker's real render failure to Crashlytics as a non-fatal. Without this the only + * signal was the ViewModel's bare `BoomerangRenderWorkResult.Failure` (WorkManager does not carry + * the worker's exception across the process boundary), reported with a synthetic stand-in cause — + * so the actual codec/Transformer exception (e.g. LG LM-X540 `IllegalArgumentException: start + * failed`, Crashlytics 47233ad7) lived only in this process's logcat ("details in + * BoomerangRenderWorker log") and never reached Firebase. This carries the genuine stack trace. + */ + private fun reportRenderFailure(parsed: BoomerangRenderWorkerInput.Parsed, kind: String, cause: Throwable) { + ReverseCrashlytics.reportSaveFailure( + versionName = BuildConfig.VERSION_NAME, + versionCode = BuildConfig.VERSION_CODE, + source = parsed.scratch.file, + trimStartMs = parsed.trimStartMs, + trimEndMs = parsed.trimEndMs, + outcome = "render_failed_$kind: ${cause.javaClass.simpleName}: ${cause.message}", + cause = cause, + ) + } + private fun deletePartialOutput(file: File) { if (file.exists() && !file.delete()) { Log.w(TAG, "Could not delete partial boomerang output: ${file.absolutePath}") diff --git a/app/src/test/java/io/github/stozo04/openloop/media/DeviceMediaHintsTest.kt b/app/src/test/java/io/github/stozo04/openloop/media/DeviceMediaHintsTest.kt index 5d9dc68..5946015 100644 --- a/app/src/test/java/io/github/stozo04/openloop/media/DeviceMediaHintsTest.kt +++ b/app/src/test/java/io/github/stozo04/openloop/media/DeviceMediaHintsTest.kt @@ -9,8 +9,8 @@ class DeviceMediaHintsTest { @Test fun samsungPreviewReverseCap_isBelowExportCap() { assertEquals(480, SAMSUNG_PREVIEW_REVERSE_MAX_SHORT_SIDE) - assertEquals(500L, SAMSUNG_POST_TRANSFORM_CODEC_SETTLE_MS) - assertTrue(PRE_REVERSE_CODEC_SETTLE_MS >= 200L) + assertEquals(500L, SAMSUNG_POST_TRANSFORM_CODEC_SETTLE.inWholeMilliseconds) + assertTrue(PRE_REVERSE_CODEC_SETTLE.inWholeMilliseconds >= 200L) assert(SAMSUNG_PREVIEW_REVERSE_MAX_SHORT_SIDE < MAX_OUTPUT_SHORT_SIDE) } } diff --git a/app/src/test/java/io/github/stozo04/openloop/media/MediaCodecLifecycleTest.kt b/app/src/test/java/io/github/stozo04/openloop/media/MediaCodecLifecycleTest.kt index ade2ea3..85cb859 100644 --- a/app/src/test/java/io/github/stozo04/openloop/media/MediaCodecLifecycleTest.kt +++ b/app/src/test/java/io/github/stozo04/openloop/media/MediaCodecLifecycleTest.kt @@ -58,4 +58,56 @@ class MediaCodecLifecycleTest { val error = IllegalArgumentException("The surface has been released") assertTrue(shouldRetryMediaCodecContention(error, failedAttemptIndex = 0, maxAttempts = 2, samsung = false)) } + + // ── Codec-initialization failure (LG LM-X540 / Crashlytics 47233ad7) ──────────────────────── + + @Test + fun `detects start failed as codec initialization failure`() { + // The exact signature from the LG LM-X540 report: MediaCodec.native_start() → BAD_VALUE → + // IllegalArgumentException("start failed"). + assertTrue(isMediaCodecInitializationFailure(IllegalArgumentException("start failed"))) + } + + @Test + fun `detects configure and codec failure messages`() { + assertTrue(isMediaCodecInitializationFailure(IllegalArgumentException("configure failed for c2.mtk.avc.encoder"))) + assertTrue(isMediaCodecInitializationFailure(IllegalStateException("Error initializing codec"))) + assertTrue(isMediaCodecInitializationFailure(IllegalStateException("Failed to initialize OMX.MTK.VIDEO.ENCODER.AVC"))) + } + + @Test + fun `ignores benign lifecycle teardown as init failure`() { + // Released / cancelled / empty-message ISE are teardown noise, not a codec that can't start — + // they must NOT trip the software-codec fallback (that path is the contention retry instead). + assertFalse(isMediaCodecInitializationFailure(IllegalArgumentException("Invalid to call at Released state"))) + assertFalse(isMediaCodecInitializationFailure(IllegalStateException("Pending dequeue output buffer request cancelled"))) + assertFalse(isMediaCodecInitializationFailure(IllegalStateException())) + } + + @Test + fun `ignores unrelated failures as init failure`() { + assertFalse(isMediaCodecInitializationFailure(IllegalStateException("Pass-1 decode loop exceeded 500000 iterations"))) + assertFalse(isMediaCodecInitializationFailure(RuntimeException("disk full"))) + } + + @Test + fun `software codec retry fires for start failed while an attempt remains`() { + val error = IllegalArgumentException("start failed") + assertTrue(shouldRetryReverseWithSoftwareCodec(error, failedAttemptIndex = 0, maxAttempts = 2)) + // No retry once the last attempt has failed. + assertFalse(shouldRetryReverseWithSoftwareCodec(error, failedAttemptIndex = 1, maxAttempts = 2)) + } + + @Test + fun `software codec retry is device-agnostic unlike contention retry`() { + // Regression guard for 47233ad7: the OLD decision (contention) never recovered `start failed` + // on this non-Samsung device, so the reverse died on the first attempt with no fallback. + val error = IllegalArgumentException("start failed") + assertFalse( + "start failed must not be misread as benign contention", + shouldRetryMediaCodecContention(error, failedAttemptIndex = 0, maxAttempts = 2, samsung = false), + ) + // The new decision recovers it regardless of manufacturer. + assertTrue(shouldRetryReverseWithSoftwareCodec(error, failedAttemptIndex = 0, maxAttempts = 2)) + } } diff --git a/app/src/test/java/io/github/stozo04/openloop/media/SamsungReversePreviewRegressionTest.kt b/app/src/test/java/io/github/stozo04/openloop/media/SamsungReversePreviewRegressionTest.kt index 0cf69e5..8f31322 100644 --- a/app/src/test/java/io/github/stozo04/openloop/media/SamsungReversePreviewRegressionTest.kt +++ b/app/src/test/java/io/github/stozo04/openloop/media/SamsungReversePreviewRegressionTest.kt @@ -33,7 +33,7 @@ class SamsungReversePreviewRegressionTest { /** Log: Transformer Release 28.303 → reverse start 28.324 — settle must be non-zero. */ @Test fun postTransformSettleMs_isPositive() { - assertTrue(SAMSUNG_POST_TRANSFORM_CODEC_SETTLE_MS >= 200L) + assertTrue(SAMSUNG_POST_TRANSFORM_CODEC_SETTLE.inWholeMilliseconds >= 200L) } /** diff --git a/app/src/test/java/io/github/stozo04/openloop/ui/OpenLoopViewModelTest.kt b/app/src/test/java/io/github/stozo04/openloop/ui/OpenLoopViewModelTest.kt index 117c5f5..7eade1b 100644 --- a/app/src/test/java/io/github/stozo04/openloop/ui/OpenLoopViewModelTest.kt +++ b/app/src/test/java/io/github/stozo04/openloop/ui/OpenLoopViewModelTest.kt @@ -105,6 +105,14 @@ class FakeVideoStorageRepository : VideoStorageRepository { /** Saved videos (raws + boomerangs), exposed for assertions. */ val saved = mutableListOf() + /** + * Every raw ever promoted, retained for assertions even after [deleteRawVideo] removes it from + * [saved]. A successful boomerang render intentionally deletes the source raw + * ([io.github.stozo04.openloop.work.BoomerangRenderWorker]: "no longer needed after a successful + * boomerang render"), so provenance (`boomerang.sourceRawId`) can only be checked against this. + */ + val promotedRaws = mutableListOf() + /** UUIDs passed to [discardScratch], for assertions. */ val discardedScratches = mutableListOf() @@ -147,7 +155,7 @@ class FakeVideoStorageRepository : VideoStorageRepository { videoPath = File(tempRoot, "clip_$id.mp4").absolutePath, thumbnailPath = File(tempRoot, "clip_$id.jpg").absolutePath, kind = VideoKind.RAW, - ).also { saved.add(it) } + ).also { saved.add(it); promotedRaws.add(it) } } override fun discardScratch(scratch: ScratchCapture) { @@ -1063,10 +1071,17 @@ class OpenLoopViewModelTest { assertEquals(1, fakeRenderScheduler.enqueueCount) assertEquals(1, fakeVideoProcessor.renderCount) - // One RAW (promoted) + one BOOMERANG (registered), boomerang points at the raw. - val raw = fakeVideoStorage.saved.single { it.kind == VideoKind.RAW } + // The scratch is promoted to a raw, the boomerang is registered pointing at that raw, and + // the source raw is then intentionally deleted (BoomerangRenderWorker: "no longer needed + // after a successful boomerang render"). So `saved` keeps only the BOOMERANG, while its + // sourceRawId still records the (now-deleted) promoted raw for provenance. + val raw = fakeVideoStorage.promotedRaws.single() val boomerang = fakeVideoStorage.saved.single { it.kind == VideoKind.BOOMERANG } assertEquals(raw.id, boomerang.sourceRawId) + assertTrue( + "source raw should be deleted after a successful render", + fakeVideoStorage.saved.none { it.kind == VideoKind.RAW }, + ) assertEquals(1, fakeVideoStorage.discardedScratches.size) // scratch cleaned up after save assertNull(viewModel.editorState.value) diff --git a/docs/DEFINITION_OF_DONE.md b/docs/DEFINITION_OF_DONE.md index d92ff32..6d4cc3e 100644 --- a/docs/DEFINITION_OF_DONE.md +++ b/docs/DEFINITION_OF_DONE.md @@ -6,6 +6,17 @@ The guiding principle: **don't trust "it compiles" — prove it.** Prove it buil --- +## Production zero-error rule (non-negotiable) + +**OpenLoop is live in Production and reachable by billions of users. Any error the agent encounters while working — a failing test, a compile error, a lint error, a crash — MUST be fixed before a PR is created. No exceptions, even for pre-existing failures the agent did not introduce.** + +- "Not my change" is **not** a reason to leave a red build. If you touch a module and its tests don't compile or don't pass, you fix them as part of the work (or, if the fix is genuinely out of scope, you **stop and flag it to the owner and get explicit direction** — you do not open a PR on top of a known-broken baseline). +- A PR must be opened only from a **fully green** state: clean debug + release build, **0 test failures**, **0 new lint errors**. A known failing test in the branch is a release blocker, full stop. +- If you discover the breakage was already on `main`, that makes it **more** urgent, not less — a broken gate on `main` means the safety net is down for every future change. Repair it (or escalate) immediately; never build on top of it. +- Capture the green proof (build verdict + exit 0 + test counts, per the gate below) in the PR. + +--- + ## The gate (run top to bottom) ### 0. Baseline — before you change anything @@ -89,6 +100,7 @@ A command finishing is **not** a passed build. Confirm all three: ## Copy-paste checklist for a PR ``` +- [ ] Production zero-error rule honored: NO known failing test / compile / lint error left behind (pre-existing included), or escalated to the owner - [ ] Baseline green before changes (clean assembleDebug) - [ ] clean assembleDebug + assembleRelease: BUILD SUCCESSFUL, exit 0, zero e: - [ ] Requirement checks pass (e.g. zipalign -c -P 16 shows real (OK)) diff --git a/docs/e2e/2026-06-21_150054.md b/docs/e2e/2026-06-21_150054.md new file mode 100644 index 0000000..b5f2c9b --- /dev/null +++ b/docs/e2e/2026-06-21_150054.md @@ -0,0 +1,66 @@ +# E2E sweep — Pixel 6 / Pixel 8 / Pixel 10 Pro Fold + +- **Date:** 2026-06-21 +- **App:** `io.github.stozo04.openloop` **1.0.23 (23)** — built from the **working tree** (includes the uncommitted `VideoReverser` software-codec fallback fix + worker Crashlytics reporting from this session). +- **Flow per device:** record a clip → modify **every** editor tab (Trim, Speed, Loop, Filter) → Save → confirm the share modal → open the library → confirm the clip is present → tap it → confirm it plays. +- **Loop direction:** `Reverse then forward` on every device (per request — exercises `VideoReverser`, the path behind Crashlytics 47233ad7). +- **Scoring:** strict — any reverse-preview timeout / "forward only" fallback counts as a device failure (per request). + +## Verdict: ✅ PASS on all 3 devices + +| Device | AVD | Display | Reverse preview | Render worker | Library + playback | Crashes | Verdict | +|---|---|---|---|---|---|---|---| +| Pixel 6 | `Pixel_6` | 1080×2400 | OK ~23s, `attempts=1` | SUCCESS | clip present, plays (B&W, visually confirmed) | 0 | ✅ PASS | +| Pixel 8 | `Pixel_8` | 1080×2400 | OK ~5s, `attempts=1` | SUCCESS (~7s) | clip present, plays (B&W, visually confirmed) | 0 | ✅ PASS | +| Pixel 10 Pro Fold | `Pixel_10_Pro_Fold` | 2076×2152 | OK ~4s, `attempts=1` | SUCCESS (~10s) | clip present, plays (overlay + `c2.goldfish.h264.decoder` rendering; PNG unreadable by viewer, confirmed via logcat + UI dump) | 0 | ✅ PASS | + +## Per-step flow + +| Step | Pixel 6 | Pixel 8 | Pixel 10 Pro Fold | +|---|---|---|---| +| Onboarding → "LET'S GO!" | ✓ | ✓ | ✓ | +| Record clip | ✓ (~5s) | ✓ (~4s) | ✓ (~4s) | +| **Trim** (drag end handle) | 21.5s → 14.7s | 7.4s → 4.9s | 7.3s → 4.8s | +| **Speed** (tap track) | 2x → 0.9x | 2x → 0.9x | 2x → 1x | +| **Loop** (Reverse then forward) | reverse OK | reverse OK | reverse OK | +| **Filter** (B&W) | ✓, no "preview unavailable" | ✓ | ✓ | +| **Save boomerang** | worker SUCCESS | worker SUCCESS | worker SUCCESS | +| Share modal (system Quick Share) | ✓ | ✓ | ✓ | +| Library shows clip | ✓ | ✓ | ✓ | +| Tap → plays | ✓ (ExoPlayer Init) | ✓ (ExoPlayer Init) | ✓ (ExoPlayer Init + decoder render) | + +## Logcat scan (CRASH / TIMEOUT / CHURN) + +| Signature | Class | P6 | P8 | P10 Fold | +|---|---|---|---|---| +| FATAL EXCEPTION | CRASH | 0 | 0 | 0 | +| ANR | CRASH | 0 | 0 | 0 | +| process died | CRASH | 0 | 0 | 0 | +| surface has been released (b09e527) | CRASH | 0 | 0 | 0 | +| dequeueOutputBuffer native (3a506c4e) | CRASH | 0 | 0 | 0 | +| MediaCodec CodecException | CRASH | 0 | 0 | 0 | +| reverse preview timeout (120s) | TIMEOUT | 0 | 0 | 0 | +| reverse preview failure (non-fatal) | TIMEOUT | 0 | 0 | 0 | +| codec reclaim pressure | CHURN | 34 | 39 | 52 | +| dead-thread handler race | CHURN | 20 | 22 | 14 | +| codec buffer starvation | CHURN | 67 | 84 | 103 | +| codec components created (c2.*) | CHURN | 31 | 29 | 35 | + +**All CRASH and TIMEOUT rows are 0 on every device.** CHURN rows are advisory only — expected resource churn from the emulator's software codec (`c2.android.avc.encoder` + `c2.goldfish.h264.decoder`) running the two-pass reverse plus the Transformer export. No crash correlated with them. + +## Findings + +1. **No hard findings.** Record → 4-tab-edit → reverse → save → share → library → playback completed cleanly on all three devices, with the B&W look correctly baked into the saved boomerang (verified in the playback frame on P6 and P8). +2. **Smell — share-sheet `FileProvider` permission-denial warnings (P6).** The system chooser (`com.android.intentresolver`) logged `Permission Denial: opening provider androidx.core.content.FileProvider … not exported` while building its preview. The share to a chosen target still works (the share Intent grants per-target URI read permission); this is the chooser-*preview* process being denied — a known Android quirk, not a save/share failure. Worth a glance if share-target previews look blank in the field. + +## What could NOT be verified (honest gaps) + +- **The new software-codec fallback branch is NOT exercised by *this happy-path sweep*** — every reverse completed with `attempts=1` because the emulator already selects the *software* AVC encoder (`c2.android.avc.encoder`) + goldfish decoder by default, so nothing rejects `start()`. **This gap is now closed by a dedicated fault-injection test** (added 2026-06-21): `VideoReverserTest.reverse_recoversFromCodecStartFailure_viaSoftwareFallback` injects `IllegalArgumentException("start failed")` on attempt 0 via the `VideoReverser.attemptHook` test seam, then asserts the reverse retries on the software **encoder + decoder** and still produces a valid, playable clip. It **passed on Pixel 6** (`tests=1 failures=0 errors=0`, 2.53s) — exercising the exact 47233ad7 recovery path on-device. The only thing still unverified is that path on the *literal* LG LM-X540 hardware (Test Lab has no LG devices; would need the physical phone). +- **Strict-scoring caveat:** the "fail on any reverse timeout" rule never triggered — no device timed out. On slower hardware or a busier host this path can still hit the 120s budget. +- **Pixel 10 Fold playback screenshot** could not be rendered by the image viewer (unusual PNG from the 2076×2152 capture); playback was confirmed via `ExoPlayerImpl: Init`, the active `c2.goldfish.h264.decoder` output, and the "Close preview" overlay rather than a viewed frame. +- Camera capture is the emulator's synthetic scene (the checkerboard-TV living room), not a real camera sensor. +- Only the inner (unfolded) display of the Fold was tested; the cover display was not. + +## Artifacts + +Screenshots + per-device logcat captured under `…/Temp/openloop_e2e/` (`p6_share.png`, `p6_playback.png`, `p8_playback.png`, `p10_playback.png`, `logcat_p6_*.txt`, `logcat_p8_*.txt`, `logcat_p10_*.txt`).