From f46d7bf676ada9f056fdfa4084613616c243bb93 Mon Sep 17 00:00:00 2001
From: Duncan Tourolle <duncan@tourolle.paris>
Date: Sun, 16 Aug 2026 00:42:56 +0200
Subject: [PATCH] =?UTF-8?q?fix(player):=20make=20native=20Android=20video?=
 =?UTF-8?q?=20opt-in=20again=20=E2=80=94=20it=20shipped=20as=20audio=20wit?=
 =?UTF-8?q?h=20no=20picture?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

DR-161 flipped experimentalNativeVideo on by default so picture-in-picture could
shrink a real video surface. On a device that shipped sound with a blank screen.

The decode path was never at fault. Logcat shows ExoPlayer running and feeding a
live SurfaceView with an active BufferQueue. The compositing was: the SurfaceView
sits behind the WebView, and the step that clears the opaque layers above it
never took effect — `WebView transparent = false` is logged, `= true` never
appears. The video was rendering correctly the whole time, behind an opaque page.

This is precisely the defect the flag existed to contain;
VideoPlayer.scrubRegression.test.ts had already recorded that "the native
SurfaceView has never been visible through the webview". Enabling it by default
shipped a verified decode path on top of an unverified display path.

Reverting costs nothing that matters: PiP does not depend on it — DR-160 drives
PiP from the WebView <video> — and working video outranks PiP showing a native
surface. The flag stays in Settings, now described as incomplete rather than as a
performance win, so anyone helping test it still can.

Fixing the compositing is the prerequisite for trying this default again (DR-172).
---
 docs/requirements.md                          |  3 +-
 scripts/extract-traces.test.ts                |  4 +--
 .../VideoPlayer.scrubRegression.test.ts       | 12 +++----
 .../player/VideoPlayer.touchScrub.test.ts     | 12 +++----
 src/lib/stores/nativeVideo.ts                 | 36 +++++++++++--------
 src/lib/utils/pictureInPicture.ts             |  6 ++--
 src/routes/settings/+page.svelte              |  8 ++---
 7 files changed, 44 insertions(+), 37 deletions(-)

diff --git a/docs/requirements.md b/docs/requirements.md
index 8dbdb114..60699303 100644
--- a/docs/requirements.md
+++ b/docs/requirements.md
@@ -327,6 +327,7 @@ Internal architecture, components, and application logic.
 | DR-168 | Pause and resume actually stop and restart the bytes. `pause_download` wrote `status = 'paused'` and did nothing else, and no cancellation existed anywhere in the download stack — no token, no flag, no abort — so the streaming task ran on, kept writing, and overwrote the row with `completed`/`failed` when it finished: the row flicked to "paused" and undid itself. `resume_download` had the mirror defect, flipping the row to `pending` without calling `pump_download_queue`; the pump runs when something calls it rather than polling, so a resumed download sat untouched until an unrelated event happened to pump the queue. A per-download stop flag (`download::stop`) is the missing half — a module-level registry because the two sides never meet, the command holding Tauri state and the worker running detached in `async_runtime::spawn`. The worker reads it between chunks and on retry (so a pause is not swallowed by a 45-second backoff), flushes, and returns `Stopped`, which is deliberately **not** retryable and **not** recorded as a failure: the `.part` file is left intact because that is exactly what the resume's Range request continues from. Registering returns a *fresh* flag, or a resumed download would inherit the pause that stopped it and halt instantly. Cancel and `clear_stale_downloads` signal it too, so neither deletes a file still being written | Downloads | UR-055 | Done |
 | DR-169 | Partial files are actually reaped. The worker named its sidecar with `Path::with_extension("part")`, which *replaces* the extension — `movie.mp4` became `movie.part` — while every cleanup path deleted `"{file_path}.part"`, i.e. `movie.mp4.part`. The two never matched, so the partial file of every cancelled or failed download stayed on disk indefinitely, invisible to the disk-usage totals because no `downloads` row pointed at it. `partial_path` appends instead, is the single definition both the writer and the cleaners use, and incidentally removes a collision the old form had, where `movie.mp4` and `movie.mkv` mapped to one `movie.part` | Downloads | UR-055 | Done |
 | DR-170 | Downloads at a chosen bitrate are no longer corrupted by their own retries. Only the `original` preset asks for `Static=true`; every other rung requests a **transcode**, which Jellyfin serves chunked, with no `Content-Length`, and cannot byte-seek — so it ignores `Range` and answers `200` with the whole stream from the beginning rather than `206` with the requested tail. The worker sent the Range header whenever a `.part` existed and appended the body unconditionally, so each retry and each resume concatenated a fresh copy of the entire transcode onto the bytes already on disk: the file grew past its real size and would not play, which is why "downloads for different bitrates" stayed broken after the `videoBitRate` casing fix (DR-adc460f3) corrected the *request*. `resume_offset` makes the response decide — append only on a `206`, otherwise truncate and take the stream from the top — and the total size is computed from that offset rather than from a partial length the server never agreed to | Downloads | UR-071 | Done |
+| DR-172 | Native Android video is opt-in again, because as a default it shipped as **audio with no picture**. DR-161 flipped `experimentalNativeVideo` on so picture-in-picture could shrink a real video surface; on a device that produced sound and a blank screen. The decode path was never the problem — logcat showed ExoPlayer running (`Position update` ticks) and feeding a live `SurfaceView` with an active BufferQueue. The compositing was: the SurfaceView sits *behind* the WebView, and the step that clears the opaque layers above it never took effect, with `WebView transparent = false` logged and `= true` never appearing. So the video rendered correctly the whole time, behind an opaque page. This is exactly the defect the flag existed to contain — `VideoPlayer.scrubRegression.test.ts` had recorded that "the native SurfaceView has never been visible through the webview" — and enabling it by default shipped a verified decode path on top of an unverified display path. Reverting costs nothing that matters: PiP does not depend on it (DR-160 drives PiP from the WebView `<video>`), and working video outranks PiP showing a native surface. The flag stays available in Settings, now described as incomplete rather than as a performance win, and the scrub-regression mocks that were made explicit under DR-161 are kept explicit so those tests state which path they guard rather than inheriting a default that has now moved twice. Fixing the compositing is the prerequisite for trying this default again | UI | UR-003, UR-004, UR-041 | Done |
 | DR-171 | A downloaded video keeps audio the device can actually decode. `original` quality asked for `Static=true`, which hands back the source file byte-for-byte — E-AC-3/AC-3/DTS/TrueHD track included — and video is rendered on both platforms by the webview `<video>` element, which decodes none of them. Streaming already knew this: DR-149 judges the track the server would serve against `WEBVIEW_AUDIO_CODECS` and forces a transcode over Jellyfin's own direct-play offer, because 10.11.5 honours a `DirectPlayProfile`'s container and video codec but ignores its audio codec. The download path never consulted that policy, so the *same film* had sound when streamed and played as picture in silence once downloaded — and offline a download is the only source a video has, so there was no working path left to fall back to. The rule is now one rule: `served_audio_codec` picks the track the server will serve (the default, or the first when none is marked) and both callers judge it, the streaming verdict staying a bool and the download path needing the codec itself so it can say what to re-encode. Only the audio is re-encoded — `allowVideoStreamCopy=true` keeps an h264 source's picture byte-for-byte and no bitrate or resolution cap is added, so `original` still means original quality; a source the webview could not have rendered anyway (HEVC) becomes h264 as a side effect, which is the only form of it that would have played. The decision is per item rather than blanket because the transcode costs the byte-range resumability `Static=true` gives the download worker (see DR-170 for what a chunked, length-less response does to a resume), so a file whose audio already plays keeps the direct copy. An unknown codec — item not fetchable, or the server named none — changes nothing: the policy only ever *adds* a transcode, so it cannot make a working download worse. The codec set judged against is the **webview's**, not the platform's, even though DR-161 made ExoPlayer the Android default: `experimentalNativeVideo` is a user setting, a downloaded file outlives whatever it was set to when the file arrived, and the narrow list is the only one that holds on both sides of it — at the cost of a Dolby-licensed device re-encoding a track its ExoPlayer could have played. `resolve_video_download_url` is the single entrance for all three resolution sites (the frontend's per-item command, the bulk series/season enqueue, and the offline-queued resume), since the pure builder cannot look a codec up and a caller that forgets to is exactly how the silent downloads shipped. **Files already downloaded stay silent** — the bytes on disk are the wrong bytes and only a re-download replaces them | Downloads | UR-071, UR-004 | Done |
 | DR-162 | Video streams are opened against a **bandwidth ceiling the user chose**, instead of a fixed allowance nobody could change. Every video URL carried `MaxStreamingBitrate=20000000`/`VideoBitrate=18000000`, `PlaybackInfo` negotiated at 20 Mbps, and the device profile advertised `999999999` — so on a metered or slow connection the only lever was not watching. `StreamingQuality` is a ladder of ceilings (Original, 20/10/8/4/2/1 Mbps, 720 kbps) in which a step is not a label but a bundle of transcode parameters: the total ceiling, the audio share of it, and the resolution that budget can carry. It lives in Rust because those numbers are Jellyfin encoding vocabulary — the frontend names a variant and reads labels back over `player_get_streaming_qualities`, the same arrangement as the EQ preset curves. The video bitrate is the total *minus* the audio share, so the two together honour the cap rather than overshooting it by the size of the audio track, and `MaxHeight` falls with the ladder so a small budget is not spent on pixels it cannot afford. The cap has to reach the **negotiation**, not only the transcode URL: `max_static_bitrate` in the device profile is what makes the server refuse to direct-play a source fatter than the ceiling, and without it a 30 Mbps remux is handed over untouched and every URL parameter downstream is moot — which is why it is applied at all four places that decide bandwidth (the HLS builder, `PlaybackInfo`, `open_live_stream`, and the background-audio handoff, which takes the lower of the cap and its own 384 kbps). The ceiling is process-wide rather than a field on `OnlineRepository`, mirroring `INCLUDE_CATALOG_BROWSE`: it is a preference about *this device's connection*, it must survive a repository rebuilt on re-login, and every builder plus the negotiation have to agree on it or the cap leaks. Settings owns the durable default and is the only writer to `app_settings` — persisted unlike the rest of `VideoSettings`, because a limit set for a metered connection that silently reverts to uncapped on the next launch spends the user's data with no changed setting to show for it — and it is restored at startup from the async runtime, defaulting to uncapped if the read fails so a database problem degrades to the old behaviour rather than to an arbitrary limit. The in-player menu is the per-video override: a cap is a property of the stream the server is producing, so it cannot be applied to one already in flight — `player_set_stream_quality` re-opens the stream at the new quality and resumes at the current position, reloading a native backend itself and handing HTML5 a URL for the same `reloadSource` primitive the audio-track switch uses, so no strategy branch lives in the UI. It deliberately does not persist. This gives UR-070 its resume-at-the-same-point mechanism; the server-offered per-item rendition list that requirement also asks for remains proposed | Playback | UR-074, UR-070 | Done |
 | DR-161 | Native video is the default, so picture-in-picture has a real surface. DR-160 makes PiP work on the HTML5 path, but that path can only ever shrink the *UI* into the PiP window; showing the video itself needs the SurfaceView behind the WebView, which is what `experimentalNativeVideo` gates. The flag now defaults to on when the user has never chosen, with an explicit stored choice still winning in both directions so anyone who turned it off keeps it off. This is a deliberate acceptance of risk: the flag existed because the native path was an unfinished spike, and `VideoPlayer.scrubRegression.test.ts` documents its history — a native init that flipped to HTML5 mid-lifecycle and left seeks going down one path while ExoPlayer played on another. Those tests pin the **flag-off** interim override (native response overridden to HTML5, backend stopped once), which the default no longer selects, so they now mock the flag off rather than inherit it: they still guard that path, but they no longer describe what ships. The native scrub/seek path is consequently not covered by the suite and needs device verification | UI | UR-041, UR-003 | Needs device verification |
@@ -388,7 +389,7 @@ Internal architecture, components, and application logic.
 | UR-038 | IR-010 | DR-043 |
 | UR-039 | - | DR-045, DR-046 |
 | UR-040 | IR-025 | DR-051, DR-052, DR-129, DR-130, DR-159 |
-| UR-041 | IR-026 | DR-053, DR-160, DR-161 |
+| UR-041 | IR-026 | DR-053, DR-160, DR-161, DR-172 |
 | UR-042 | IR-009, IR-014 | DR-054 |
 | UR-043 | IR-027 | DR-055 |
 | UR-044 | - | DR-056 |
diff --git a/scripts/extract-traces.test.ts b/scripts/extract-traces.test.ts
index 0ded1aa1..3f16e7cc 100644
--- a/scripts/extract-traces.test.ts
+++ b/scripts/extract-traces.test.ts
@@ -175,8 +175,8 @@ describe("live requirements.md", () => {
 
     expect(defined.UR).toBe(74);
     expect(defined.IR).toBe(32);
-    expect(defined.DR).toBe(162);
+    expect(defined.DR).toBe(163);
     expect(defined.JA).toBe(35);
-    expect(defined.total).toBe(303);
+    expect(defined.total).toBe(304);
   });
 });
diff --git a/src/lib/components/player/VideoPlayer.scrubRegression.test.ts b/src/lib/components/player/VideoPlayer.scrubRegression.test.ts
index 40f92d89..8e39f125 100644
--- a/src/lib/components/player/VideoPlayer.scrubRegression.test.ts
+++ b/src/lib/components/player/VideoPlayer.scrubRegression.test.ts
@@ -23,12 +23,12 @@ import { describe, it, expect, vi, beforeEach } from "vitest";
 // ---- Mocks (must precede component import) --------------------------------
 
 const channelHandlers: Record<string, (event: any) => void> = {};
-// These tests pin the **flag-off** interim behaviour: when `experimentalNativeVideo`
-// is off, VideoPlayer overrides Android's native backend response to HTML5
-// rendering and stops the native backend. That flag now defaults to *on*
-// (DR-160, so picture-in-picture has a real surface to shrink into), so the
-// default no longer selects this path and the tests have to say which path they
-// are guarding rather than inherit it. (DR-161)
+// These tests pin the **flag-off** behaviour: when `experimentalNativeVideo` is
+// off, VideoPlayer overrides Android's native backend response to HTML5
+// rendering and stops the native backend. That is the default again (DR-172,
+// after native video shipped as audio with no picture), so this mock now agrees
+// with the default rather than opposing it — kept explicit so the tests state
+// which path they guard instead of inheriting whatever the default happens to be.
 vi.mock("$lib/stores/nativeVideo", async (importOriginal) => {
   const actual = await importOriginal<typeof import("$lib/stores/nativeVideo")>();
   return {
diff --git a/src/lib/components/player/VideoPlayer.touchScrub.test.ts b/src/lib/components/player/VideoPlayer.touchScrub.test.ts
index 97c02abc..89b6fcb4 100644
--- a/src/lib/components/player/VideoPlayer.touchScrub.test.ts
+++ b/src/lib/components/player/VideoPlayer.touchScrub.test.ts
@@ -26,12 +26,12 @@ import { describe, it, expect, vi, beforeEach } from "vitest";
 // ---- Mocks (must precede component import) --------------------------------
 
 const channelHandlers: Record<string, (event: any) => void> = {};
-// These tests pin the **flag-off** interim behaviour: when `experimentalNativeVideo`
-// is off, VideoPlayer overrides Android's native backend response to HTML5
-// rendering and stops the native backend. That flag now defaults to *on*
-// (DR-160, so picture-in-picture has a real surface to shrink into), so the
-// default no longer selects this path and the tests have to say which path they
-// are guarding rather than inherit it. (DR-161)
+// These tests pin the **flag-off** behaviour: when `experimentalNativeVideo` is
+// off, VideoPlayer overrides Android's native backend response to HTML5
+// rendering and stops the native backend. That is the default again (DR-172,
+// after native video shipped as audio with no picture), so this mock now agrees
+// with the default rather than opposing it — kept explicit so the tests state
+// which path they guard instead of inheriting whatever the default happens to be.
 vi.mock("$lib/stores/nativeVideo", async (importOriginal) => {
   const actual = await importOriginal<typeof import("$lib/stores/nativeVideo")>();
   return {
diff --git a/src/lib/stores/nativeVideo.ts b/src/lib/stores/nativeVideo.ts
index 3940107a..b39c0df9 100644
--- a/src/lib/stores/nativeVideo.ts
+++ b/src/lib/stores/nativeVideo.ts
@@ -27,26 +27,32 @@ const STORAGE_KEY = "jellytau-experimental-native-video";
 const NATIVE_VIDEO_ATTR = "data-native-video";
 
 /**
- * Whether the native path is on, defaulting to **on** when the user has never
- * chosen.
+ * Whether the native path is on. **Off** unless the user turned it on.
  *
- * It shipped defaulting to off while the native path was a spike. It is now the
- * default because picture-in-picture is built on it: PiP shrinks the *Activity*,
- * so it needs a real video surface behind the WebView to show, and on the HTML5
- * path there is nothing for it to shrink into but the UI itself (DR-160).
+ * DR-161 briefly made this default to on, so picture-in-picture could shrink a
+ * real video surface. On a device that shipped as **audio with no picture**:
+ * ExoPlayer decoded correctly and fed its SurfaceView, but the SurfaceView sits
+ * *behind* the WebView and the compositing that clears the opaque layers above it
+ * never took effect — logcat showed `WebView transparent = false` and never
+ * `= true`. So the video was rendering the whole time, behind the page.
+ *
+ * That is the defect the flag existed to contain, and it is why the default is
+ * back off: video working matters more than PiP showing the native surface, and
+ * PiP still works without it via the HTML5 path (DR-160). Native video remains
+ * available in Settings for anyone testing it.
  *
  * An explicit stored choice still wins in both directions, so anyone who turned
- * it off keeps it off.
+ * it on keeps it on.
+ *
+ * TRACES: UR-003, UR-004 | DR-172
  */
 function load(): boolean {
-  if (typeof localStorage === "undefined") return true;
+  if (typeof localStorage === "undefined") return false;
   try {
-    const stored = localStorage.getItem(STORAGE_KEY);
-    return stored === null ? true : stored === "true";
+    return localStorage.getItem(STORAGE_KEY) === "true";
   } catch {
-    // Private-mode / disabled storage — no stored choice is readable, so this is
-    // the same case as "never chosen".
-    return true;
+    // Private-mode / disabled storage — default to the safe (HTML5) path.
+    return false;
   }
 }
 
@@ -74,8 +80,8 @@ function createExperimentalNativeVideoStore() {
 }
 
 /**
- * User opt-out for the native Android video path. **Defaults to on** since
- * DR-161 — see `load()`. The name still says "experimental" because the flag
+ * User opt-in for the native Android video path. **Defaults to off** again since
+ * DR-172 — see `load()`. The name says "experimental" because the flag
  * remains a suppressor of Rust's backend choice, not a promoter of it.
  */
 export const experimentalNativeVideo = createExperimentalNativeVideoStore();
diff --git a/src/lib/utils/pictureInPicture.ts b/src/lib/utils/pictureInPicture.ts
index 12eeb613..5d3cbb1f 100644
--- a/src/lib/utils/pictureInPicture.ts
+++ b/src/lib/utils/pictureInPicture.ts
@@ -97,9 +97,9 @@ export function setAutoEnterEnabled(enabled: boolean): void {
  * needs for the PiP window's aspect ratio and the play state for its play/pause
  * action.
  *
- * The flag defaults to **on** now (DR-161), so Android normally shrinks the real
- * ExoPlayer surface instead; this remains the path for Linux and for anyone who
- * turned the flag off.
+ * The flag is back to defaulting **off** (DR-172, after native video shipped as
+ * audio with no picture), so this is once again the path Android normally takes —
+ * which is why PiP does not depend on that flag being on.
  *
  * Pass `active: false` when the element goes away, or PiP would be offered over a
  * video that is no longer there.
diff --git a/src/routes/settings/+page.svelte b/src/routes/settings/+page.svelte
index 8a0e25bc..8a4aa8ba 100644
--- a/src/routes/settings/+page.svelte
+++ b/src/routes/settings/+page.svelte
@@ -744,10 +744,10 @@
                 </h3>
                 <p class="text-sm text-gray-400 mt-1">
                   Decode video with the device's hardware decoder instead of the
-                  built-in web player. Better performance and battery life, and
-                  required for picture-in-picture to show the video rather than
-                  the app. Still less tested — turn this off if video fails to
-                  appear or seeking misbehaves.
+                  built-in web player. Better performance and battery life in
+                  principle, but incomplete: on some devices the picture does not
+                  appear at all and only the sound plays. Leave this off unless
+                  you are helping test it.
                 </p>
               </div>
               <button