diff --git a/android/app/src/main/java/com/remotedisplay/player/telemetry/PlayerCapabilities.kt b/android/app/src/main/java/com/remotedisplay/player/telemetry/PlayerCapabilities.kt index 90b2a0e..c044158 100644 --- a/android/app/src/main/java/com/remotedisplay/player/telemetry/PlayerCapabilities.kt +++ b/android/app/src/main/java/com/remotedisplay/player/telemetry/PlayerCapabilities.kt @@ -52,6 +52,27 @@ object PlayerCapabilities { // Native view rotation: the ExoPlayer surface sits inside the rotated view, so video // turns with the graphics. No hardware-plane problem here. "display.rotation", + // Per-window overlay dim (WindowManager.LayoutParams.screenBrightness) — Tier 0, no + // permission, works at any privilege level. Distinct from "system.brightness" below, + // which writes the system-wide setting and DOES need WRITE_SETTINGS or owner. + // Declared here because the android BASELINE already grants it: without this line an + // updated panel replaces the baseline with a declared set that lacks it, and LOSES + // the dim slider it had before it updated. + "display.brightness", + // Capture, at ANY privilege level. captureScreen() is a three-rung fallback — + // MediaProjection (system-wide, needs consent), then the accessibility screenshot + // API, then ScreenshotCapture.captureView, which is a plain view draw with no + // permission check of any kind. The last rung is narrower than the others (the + // player's own window, foreground only) but on a kiosk panel that window IS the + // content, so the operator gets a picture rather than a refusal. + // + // Declared unconditionally for the same reason display.brightness is: the android + // BASELINE grants both, and a declared set replaces the baseline rather than + // merging with it. Gating on accessibility meant a panel LOST live view and + // screenshots by updating, while the fallback that still served them kept working. + // It also made granting MediaProjection invisible — consent was given, capture + // genuinely started, and the server went on refusing because nothing re-declared. + "remote.screenshot", "remote.stream", // Input is plain view dispatch and works regardless of privilege. "remote.input", // The player restarts itself; the OTA checker updates the APK. @@ -67,10 +88,6 @@ object PlayerCapabilities { // ---- conditional on runtime state ------------------------------------------------------- - // Full-screen capture needs the accessibility service; without it capture falls back to - // the app's own view. Declared only for the real thing, per the capability contract. - if (accessibility) caps += listOf("remote.screenshot", "remote.stream") - // Display power is asymmetric and only honest when BOTH halves exist. screen_off needs // owner, device-admin FORCE_LOCK, or accessibility; screen_on now works anywhere via a // wake lock (WAKE_LOCK is a normal permission). So the binding constraint is the OFF @@ -86,6 +103,13 @@ object PlayerCapabilities { // confirmation — unusable on a panel with no input, so not claimed. if (isOwner) caps += "system.kiosk" + // The privilege itself, declared as a capability. Every #161 Tier-2 command + // (lock_now / power_menu / status_bar / block_uninstall / unblock_uninstall) gates on + // this name. No player declared it, so the server accepts "system.kiosk" as a stand-in — + // exact, because kiosk is itself owner-only, but a stand-in nonetheless. Declaring the + // canonical name makes those refusals say what they mean and lets the stand-in retire. + if (isOwner) caps += "system.device_owner" + // Owner-only clock control. if (isOwner) caps += "system.time" diff --git a/docs/player-parity.md b/docs/player-parity.md index 8cfe4f8..2d2655e 100644 --- a/docs/player-parity.md +++ b/docs/player-parity.md @@ -192,16 +192,25 @@ written and tested starts being true. ## Real gaps worth closing -Prioritised by how visible the failure is to an operator. **None of these are implemented here** — -other agents own the player files. +Prioritised by how visible the failure is to an operator. + +**Gaps 1, 2 and 5 are closed** (gap 1 shipped in 1.9.31; 2 and 5 are on +`fix/player-parity-small-gaps` and unreleased). ⚠️ **The baselines below have deliberately NOT been +moved** — per the rule in this document a baseline entry moves when the fix *reaches displays*, +which is the release AFTER the one carrying it. Moving them together would grant a capability to +every panel still running the old build. + +⚠️ **Gaps 3 and 4 were implemented, audited, and REVERTED.** Both are still open, and both are now +known to be considerably more expensive than "small". Their rows record what the audit found, so the +next attempt starts from the traps rather than rediscovering them. | # | gap | difficulty | why it matters | |---|---|---|---| -| 1 | **`set_volume` payload mismatch** (`server/player/index.html`, `tizen/js/app.js`) — accept `level` (0..1) alongside `value` (0..100). | **trivial** — one line each | The volume slider is dead on 3 of 4 players. Highest visibility, lowest cost in the list. | -| 2 | **`PlayerCapabilities.kt` under-declares.** Add `display.brightness` (Tier 0, `setWindowBrightness`, always available) and `system.device_owner` under `if (isOwner)`. | **trivial** | Updating an Android panel currently *loses* it the per-window dim slider, and keeps the Tier-2 stand-in in `player-capabilities.js` necessary. | -| 3 | **BrightSign "Force update" is a dead button.** `index.html` has no `update` branch; the host self-updates on its own poll. Either handle `update` by posting a bridge message that triggers `CheckPackageUpdate` now, or stop declaring `system.self_update`. | **small** (a bridge message + a BrightScript branch) | The button is rendered on every host-bridged BrightSign and does nothing. | -| 4 | **`declaredCapabilities()` should defer to `BS.capabilities()` on BrightSign.** | **small** | Turns 199 lines of already-written, already-passing tests from decoration into enforcement, and fixes six wrong declarations at once — including `offline.cache`, the one that shipped a lie to a real customer panel. | -| 5 | **The capture-bootstrap button is hidden where it is needed** (`device-detail.js`, `can('remote.screenshot')`). Render it whenever the device is Android and lacks `remote.screenshot`. | **small** | Server-side gating is fixed; the UI half is not. | +| 1 | ✅ **`set_volume` payload mismatch** (`server/player/index.html`, `tizen/js/app.js`) — accept `level` (0..1) alongside `value` (0..100). | **trivial** — one line each | The volume slider is dead on 3 of 4 players. Highest visibility, lowest cost in the list. **Fixed and released in 1.9.31** (`volumeLevelFromCommand()`). | +| 2 | ✅ **`PlayerCapabilities.kt` under-declares.** Add `display.brightness` (Tier 0, `setWindowBrightness`, always available) and `system.device_owner` under `if (isOwner)`. | **trivial** | Updating an Android panel currently *loses* it the per-window dim slider, and keeps the Tier-2 stand-in in `player-capabilities.js` necessary. **Fixed** — both declared; the `system.kiosk` stand-in can retire one release after this ships. **The gap was wider than written**: `remote.screenshot` and `remote.stream` were gated on the accessibility service while `captureScreen()` falls through to `ScreenshotCapture.captureView`, a plain view draw with no permission check — so a Tier-0 panel *lost live view and screenshots by updating*, and a granted MediaProjection never became a capability at all (nothing re-declares on consent, so the operator granted it, capture started, and the server went on refusing). Both are now unconditional, matching the baseline's own reasoning. `display.power` remains conditional on purpose — see the DELIBERATE list in `player-parity-baselines.test.js`. | +| 3 | ❌ **REVERTED — BrightSign "Force update" is a dead button.** `index.html` has no `update` branch; the host self-updates on its own poll. | **NOT small — needs host work first** | Wiring the button to `CheckPackageUpdate` was tried and withdrawn. The update path is **synchronous and unbounded** (no `SetTimeout` on either transfer), so a slow failing download blocks the message loop past `WATCHDOG_MS` (120s), fabricating a crash event and rebuilding the widget. Worse, `MAX_ATTEMPTS_PER_VERSION` is 3 and the counter carries **no version binding**, so three presses on a bad link refuse that panel *every future version* until someone clears the registry by hand. `cfg.self_update` is not in the probe payload either, so an opted-out fleet shows a button guaranteed to do nothing while the dashboard toasts success — and `update` is allowed as a **group broadcast**, so one click can start N synchronous downloads. Prerequisites: a transfer timeout under the watchdog, a version-bound (or manual-exempt) attempt counter, a result message so the toast can tell the truth, and `self_update` in the probe. Until then, **withdrawing the claim is the cheaper honest fix.** | +| 4 | ❌ **REVERTED — `declaredCapabilities()` should defer to `BS.capabilities()` on BrightSign.** | **NOT small — the bridge's list is not a superset** | Deferring wholesale was tried and withdrawn: the two lists disagree in **both** directions. The bridge gates `remote.screenshot`/`remote.stream` on `storage_present` alongside `system.self_update`, but that reasoning is stale — the player captures via `@brightsign/screenshot` into **RAM** (`/tmp`) and falls back to canvas, so both work with no disk and deferring *removes working controls*. Only `system.self_update` is genuinely storage-gated. The bridge also declares `playback.transitions` unconditionally, where the page checks `transitionRuntimeReady()` — an over-declare on the one platform where the UMD/`nodejs_enabled` collision silently kills transitions. And `offline.cache` gets *looser*, not stricter: the bridge omits the page's `swRegistrationFailed` check. Compounding all of it, `probeHost`'s 3s timeout sets `answered = true`, so a late `probe-result` is discarded **for the page's lifetime** — and `autorun.brs` runs a blocking update check *before* entering the message loop, making a >3s answer plausible. A correct fix is a per-capability MERGE, not a wholesale hand-off, plus fixing the probe timeout. | +| 5 | ✅ **The capture-bootstrap button is hidden where it is needed** (`device-detail.js`, `can('remote.screenshot')`). | **small** | Server-side gating is fixed; the UI half is not. **Fixed** via `isAndroidDevice()`, mirroring `platformFamily()` in `server/lib/player-capabilities.js` — all four signals in the same order, since a Tizen TV registers `android_version: 'Tizen 6.5'` and an Android-test-only helper classifies every Samsung panel as Android. ⚠️ The gate is Android-and-nothing-else, **not** "Android that lacks `remote.screenshot`": `/api/devices/:id` ships `capabilitiesFor()`, which flattens declared and baseline into one array, and the android baseline *contains* `remote.screenshot` — so that condition hides the button from all ~440 undeclared panels. The dashboard cannot currently distinguish "declared" from "baseline-filled" at all; if a future gate needs that, the API must expose the raw declaration. | | 6 | **Tizen `remote.screenshot` is images-only.** Video and YouTube return a status card. AVPlay has no readable surface for a canvas. | **hard**, possibly impossible | Honest today, but an operator checking a video panel gets a card instead of a picture. | | 7 | **BrightSign transitions/PiP over video.** DOM composited over a hwz hardware plane may be invisible. The likely fix is `roVideoMode.SetGraphicsZOrder("front")`, deliberately not applied blind. | **medium**, ❓ **needs hardware** | Changing z-order blind risks hiding video entirely on a player that currently works. | | 8 | **BrightSign offline caching is unproven either way.** See below. | ❓ **needs hardware** | | diff --git a/frontend/js/views/device-detail.js b/frontend/js/views/device-detail.js index d8371da..4f8b268 100644 --- a/frontend/js/views/device-detail.js +++ b/frontend/js/views/device-detail.js @@ -177,6 +177,39 @@ function isBrightSignDevice(device) { return String(device.platform || '').toLowerCase().includes('brightsign'); } +// Mirrors platformFamily() in server/lib/player-capabilities.js — SAME FOUR SIGNALS, SAME ORDER, +// so the UI and the server never disagree about what a device is. +// +// The precedence is the whole point and is easy to get wrong. An earlier version of this helper +// kept only the last test, and a Tizen TV registers `android_version: 'Tizen 6.5'` (see +// tizen/js/app.js) — non-empty, not "Web/..." — so every Samsung panel in the fleet classified as +// Android. It was invisible only because Tizen happens to declare remote.screenshot today; the +// moment that changes, a MediaProjection button appears on a TV that has no such API. +// +// Gates the MediaProjection capture bootstrap below, and that gate is deliberately Android-and- +// nothing-else — NOT "Android that cannot already capture". +// +// The tempting extra condition is to hide it once a panel declares remote.screenshot. Two reasons +// not to. First, the dashboard cannot tell "this device declared it" from "the server filled in a +// baseline": /api/devices/:id ships capabilitiesFor(), which resolves both into one array (see +// server/routes/devices.js), and the android baseline CONTAINS remote.screenshot — so that +// condition hides the button from every one of the ~440 undeclared panels in the field, which is +// exactly backwards. Second, even where capture already works it is the accessibility path; +// MediaProjection is the better one (WebSocketService tries it FIRST), so offering the upgrade to +// a panel that has the weaker path is a feature, not redundancy. +function isAndroidDevice(device) { + if (!device) return false; + const platform = String(device.platform || '').toLowerCase(); + if (platform.includes('brightsign')) return false; + if (platform.includes('tizen')) return false; + // Second, independent signal for a Tizen TV: the .wgt player sends client_type 'wgt'. `platform` + // is the primary key, but it lives in a column an older client's register could overwrite. + if (device.client_type === 'wgt') return false; + if (device.client_type === 'apk') return true; + const av = String(device.android_version || ''); + return av !== '' && !av.startsWith('Web/'); +} + export function render(container, deviceId) { container.innerHTML = `
@@ -799,7 +832,7 @@ async function loadDevice(deviceId, activeTab = null) { ${device.tier === 2 ? ` ${t('device.remote.system_view_owner')} ` : ` - ${can('remote.screenshot') ? ` + ${isAndroidDevice(device) ? ` diff --git a/server/test/device-controls-hidden.test.js b/server/test/device-controls-hidden.test.js index 549e6d0..47e79e8 100644 --- a/server/test/device-controls-hidden.test.js +++ b/server/test/device-controls-hidden.test.js @@ -53,6 +53,20 @@ function render(device) { renderDeviceClock: () => '', renderPlaylist: () => '', isBrightSignDevice: (d) => String(d.platform || '').toLowerCase().includes('brightsign'), + // Same four signals, same order, as the real helper in device-detail.js and platformFamily() + // in server/lib/player-capabilities.js. Kept as a stub rather than imported because this file + // renders the template in a bare VM context — but if the real rule changes, change it here too. + // The brightsign/tizen/wgt short-circuits come FIRST and are load-bearing: a Tizen TV registers + // android_version 'Tizen 6.5', which satisfies the Android test below. + isAndroidDevice: (d) => { + if (!d) return false; + const p = String(d.platform || '').toLowerCase(); + if (p.includes('brightsign') || p.includes('tizen')) return false; + if (d.client_type === 'wgt') return false; + if (d.client_type === 'apk') return true; + const av = String(d.android_version || ''); + return av !== '' && !av.startsWith('Web/'); + }, TERMINAL_PRESETS: [], localStorage: { getItem: () => null, setItem: () => {} }, Math, Date, JSON, String, Array, Object, @@ -71,8 +85,12 @@ const WEB = { capabilities: ['playback.video', 'audio.volume', 'remote.screenshot', 'remote.stream', 'remote.input', 'system.restart_player'], }; +// Exactly what tizen/js/app.js registers, including the android_version field — which reads +// 'Tizen 6.5' and NOT anything Android-shaped. An earlier version of this fixture omitted it, so +// every "not offered to Tizen" assertion below passed without ever exercising the case that +// actually matters. const TIZEN = { - platform: 'Tizen 6.5', + platform: 'Tizen 6.5', client_type: 'wgt', android_version: 'Tizen 6.5', capabilities: ['playback.video', 'audio.volume', 'display.rotation', 'remote.input', 'system.restart_player'], }; @@ -188,3 +206,103 @@ test('every gated control still renders balanced markup', () => { assert.equal(bopen, bclose, 'unbalanced