diff --git a/clients/android/app/src/main/kotlin/io/unom/punktfunk/App.kt b/clients/android/app/src/main/kotlin/io/unom/punktfunk/App.kt index 63253bac..64006b7a 100644 --- a/clients/android/app/src/main/kotlin/io/unom/punktfunk/App.kt +++ b/clients/android/app/src/main/kotlin/io/unom/punktfunk/App.kt @@ -54,6 +54,7 @@ import io.unom.punktfunk.kit.link.DeepLinkResult import io.unom.punktfunk.kit.link.DeepLinks import io.unom.punktfunk.kit.link.HostResolution import io.unom.punktfunk.kit.SessionEndReason +import io.unom.punktfunk.kit.security.KnownHost import io.unom.punktfunk.kit.security.KnownHostStore import io.unom.punktfunk.models.ActiveSession import io.unom.punktfunk.models.LibraryReturn @@ -76,6 +77,11 @@ fun App(forceGamepadUi: Boolean = false) { // navigation state does not outlive the stream. Cleared once the shell has consumed it, so a // later manual Back out of the library is not undone by a stale value. var reopenLibrary by remember { mutableStateOf(null) } + // Which host's game library the TOUCH shell is showing, and which of its pinned cards opened it + // (null = the host's own card). Held here beside `tab` rather than inside the tab content: it is + // a PUSH over the whole shell, and a `remember` down in ConnectScreen would not survive the + // stream that a launch off the shelf starts — which is exactly what `reopenLibrary` restores. + var touchLibrary by remember { mutableStateOf?>(null) } // Console (gamepad) mode mirrors the Apple client: the setting AND (its mode says Always OR a // pad is attached OR this is a TV OR the dev force flag). Flips live as controllers @@ -116,6 +122,21 @@ fun App(forceGamepadUi: Boolean = false) { } } + // The touch shell's half of "come back to the library this game was launched from" — the console + // shell consumes the same intent on its way in (see GamepadShell). Gated on which shell is up so + // exactly one of the two ever claims it, and cleared here so a later manual Back stays backed + // out. A host forgotten while the game ran simply leaves us on the host grid. + LaunchedEffect(reopenLibrary, gamepadUi) { + if (gamepadUi) return@LaunchedEffect + val (id, pinId) = reopenLibrary ?: return@LaunchedEffect + KnownHostStore(context).all().firstOrNull { it.id == id }?.let { kh -> + // A pin unpinned while the game was running is no longer a shelf: fall back to the + // host's own, rather than a card that no longer exists. + touchLibrary = kh to pinId?.takeIf { it in kh.pinnedProfileIds } + } + reopenLibrary = null + } + // The console backdrop's colour family, published once from the live settings rather than // threaded through every screen that draws a backdrop. Because it is read from the SAME // `settings` state the gamepad settings screen writes, stepping the Background row recolours @@ -158,6 +179,21 @@ fun App(forceGamepadUi: Boolean = false) { reopenLibrary = reopenLibrary, onReopenLibraryHandled = { reopenLibrary = null }, ) + } else if (touchLibrary != null) { + // The touch shell's library is a PUSHED screen, not a tab: it belongs to one host, and a + // third permanent tab for something you reach from a card would be a nav item that is + // meaningless until you pick one. So it takes the whole window (bar included, like the + // console shell's does) and Back — the arrow or the system gesture — returns to the grid. + // Read once: `touchLibrary` is a `var`, so it does not smart-cast through the branch. + val (host, pinId) = touchLibrary!! + LibraryScreen( + host = host, + settings = settings, + onLaunched = { session = it }, + onBack = { touchLibrary = null }, + pinnedProfileId = pinId, + console = false, + ) } else { // Adaptive nav: a bottom bar on phones; on tablets / large windows a side NavigationRail // with its items centred vertically (the common Android tablet idiom, mirroring iPad's @@ -193,6 +229,9 @@ fun App(forceGamepadUi: Boolean = false) { onSettingsChange = { settings = it; settingsStore.save(it) }, deepLink = pendingLink, onDeepLinkHandled = { activity?.pendingDeepLink = null }, + // "Browse library…" in a card's overflow — the touch route to the shelf + // the console shell reaches with Y. + onOpenLibrary = { kh, pinId -> touchLibrary = kh to pinId }, ) Tab.Settings -> SettingsScreen( initial = settings, diff --git a/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectGrid.kt b/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectGrid.kt index ed8155df..bf628d85 100644 --- a/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectGrid.kt +++ b/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectGrid.kt @@ -82,6 +82,14 @@ internal fun ConnectGrid( onSpeedTest: (KnownHost) -> Unit, onCopyLink: (KnownHost, StreamProfile?) -> Unit, onTogglePin: (KnownHost, StreamProfile) -> Unit, + /** The experimental game-library toggle — off hides "Browse library…" everywhere. */ + libraryEnabled: Boolean, + /** + * Open this card's game library. The second argument is the shelf's pinned profile id, exactly + * as [onConnect] takes the card's one-off: browsing IS this card's connect with a title picked + * first, so a pinned card's shelf launches with that card's profile. + */ + onBrowseLibrary: (KnownHost, StreamProfile?) -> Unit, onRescan: () -> Unit, onAddHost: () -> Unit, ) { @@ -90,6 +98,13 @@ internal fun ConnectGrid( // "Connect with" is a ONE-OFF on every card: it never rebinds the host, which is why rebinding // lives in the Edit sheet instead. fun hostMenu(kh: KnownHost, pin: StreamProfile?): List = buildList { + // Browsing IS a connect-shaped action — this card's connect with a title picked first — so + // a PINNED card offers it too, and its shelf launches with that card's profile. Without it + // the touch home had no route to the library at all: the console shell reaches it with Y + // from a tile, and a finger has no Y. + if (libraryEnabled) { + add(HostMenuItem("Browse library…") { onBrowseLibrary(kh, pin) }) + } if (pin == null) { add(HostMenuItem("Network speed test") { onSpeedTest(kh) }) } diff --git a/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectScreen.kt b/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectScreen.kt index 56b6fe0c..5a388948 100644 --- a/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectScreen.kt +++ b/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectScreen.kt @@ -1,8 +1,6 @@ package io.unom.punktfunk import android.Manifest -import android.content.ClipData -import android.content.ClipboardManager import android.content.Context import android.content.Intent import android.content.pm.PackageManager @@ -96,12 +94,13 @@ fun ConnectScreen( // screen that can land in the defaults layer (design/client-settings-profiles.md §5.3). onSettingsChange: (Settings) -> Unit = {}, // Console (gamepad) mode: render the host carousel instead of the touch grid, sharing all of this - // screen's connect/trust/discovery logic. [onOpenSettings]/[onOpenLibrary] are the X/Y actions the - // gamepad shell owns (the touch UI reaches Settings via the bottom bar and has no library button). + // screen's connect/trust/discovery logic. [onOpenSettings] is the console's X action (the touch + // UI reaches Settings via the bottom bar). gamepadUi: Boolean = false, onOpenSettings: () -> Unit = {}, // (host, pinned profile id) — a pinned host+profile card opens ITS shelf, and the id is the // one-off every launch off that shelf runs with (design §5.2a). Null = the host's own tile. + // BOTH homes raise it: Y on a console tile, and "Browse library…" in a touch card's overflow. onOpenLibrary: (KnownHost, String?) -> Unit = { _, _ -> }, navGate: Boolean = true, // false while the console home is cross-fading out // A `punktfunk://` URL to route (design/client-deep-links.md §3). This screen owns it because @@ -635,15 +634,8 @@ fun ConnectScreen( // host's binding, exactly like a tap on it does. fun copyLink(kh: KnownHost, pin: StreamProfile?) { val url = DeepLinks.forHost(kh, profile = pin?.id).toUrl() - val clipboard = context.getSystemService(Context.CLIPBOARD_SERVICE) as? ClipboardManager - val copied = clipboard != null && runCatching { - clipboard.setPrimaryClip(ClipData.newPlainText("Punktfunk link", url)) - }.isSuccess - // Android 13 draws its own clipboard confirmation, and stacking a second one on top of it is - // the platform's own documented anti-pattern. Below it nothing visible happens at all unless - // we say so — a silent menu item reads as a broken one. - if (copied && Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) return - val message = if (copied) "Link copied." else "Couldn't copy the link to the clipboard." + val copied = putLinkOnClipboard(context, url) + val message = linkCopyMessage(copied) ?: return // The console home renders neither the notice nor the status banner, so there it has to be a // toast; the touch grid has both, and a success dressed as an error banner is a small lie. when { @@ -823,6 +815,8 @@ fun ConnectScreen( onSpeedTest = { kh -> startSpeedTest(HostCardEntry(kh, null)) }, onCopyLink = { kh, pin -> copyLink(kh, pin) }, onTogglePin = { kh, p -> togglePin(kh, p) }, + libraryEnabled = settings.libraryEnabled, + onBrowseLibrary = { kh, pin -> onOpenLibrary(kh, pin?.id) }, onRescan = { discovery.restart() }, onAddHost = { showManualSheet = true }, ) diff --git a/clients/android/app/src/main/kotlin/io/unom/punktfunk/LibraryScreen.kt b/clients/android/app/src/main/kotlin/io/unom/punktfunk/LibraryScreen.kt index eb2c4956..672592e3 100644 --- a/clients/android/app/src/main/kotlin/io/unom/punktfunk/LibraryScreen.kt +++ b/clients/android/app/src/main/kotlin/io/unom/punktfunk/LibraryScreen.kt @@ -2,25 +2,39 @@ package io.unom.punktfunk import android.widget.Toast import androidx.activity.compose.BackHandler +import androidx.compose.foundation.ExperimentalFoundationApi import androidx.compose.foundation.background import androidx.compose.foundation.border import androidx.compose.foundation.clickable +import androidx.compose.foundation.combinedClickable import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.BoxWithConstraints import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.PaddingValues +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.aspectRatio import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.width +import androidx.compose.foundation.lazy.grid.GridCells +import androidx.compose.foundation.lazy.grid.GridItemSpan +import androidx.compose.foundation.lazy.grid.LazyVerticalGrid +import androidx.compose.foundation.lazy.grid.items import androidx.compose.foundation.pager.HorizontalPager import androidx.compose.foundation.pager.PageSize import androidx.compose.foundation.pager.rememberPagerState +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.automirrored.filled.ArrowBack +import androidx.compose.material.icons.filled.Refresh import androidx.compose.material3.CircularProgressIndicator +import androidx.compose.material3.DropdownMenu +import androidx.compose.material3.DropdownMenuItem import androidx.compose.material3.Icon +import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text import androidx.compose.runtime.Composable @@ -59,6 +73,7 @@ import coil.ImageLoader import coil.compose.AsyncImage import coil.request.ImageRequest import io.unom.punktfunk.components.launcherIcon +import io.unom.punktfunk.kit.link.DeepLinks import io.unom.punktfunk.kit.library.GameEntry import io.unom.punktfunk.kit.library.LibraryClient import io.unom.punktfunk.kit.library.LibraryResult @@ -75,9 +90,13 @@ import kotlin.math.sign import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext -// The host game-library browser — the Android mirror of the Apple client's LibraryCoverflowView: -// a gamepad-driven poster coverflow (centered cover flat + prominent, neighbours receding on a 3D -// Y-tilt) fetched from the host's management API over mTLS. Reached with Y from a saved host. +// The host game-library browser — the Android mirror of the Apple client's LibraryView: ONE screen +// with two presentations of the same shelf, chosen the same way every other screen here chooses. +// The console (gamepad) one is a poster coverflow (centered cover flat + prominent, neighbours +// receding on a 3D Y-tilt), reached with Y from a saved host; the touch one is the poster GRID the +// Apple, GTK and Windows shells draw, reached from a host card's "Browse library…". Both fetch from +// the host's management API over mTLS, and everything data-shaped — the fetch, the states, the +// launch — is shared: only the arrangement differs, because only the input device does. private sealed class LibState { object Loading : LibState() @@ -100,13 +119,20 @@ fun LibraryScreen( * [ProfileStore.resolveFor] applies to every other connect. */ pinnedProfileId: String? = null, + /** + * Which presentation to draw: the console coverflow (default — this screen's original and only + * form) or the touch poster grid. The CALLER decides rather than this screen reading the + * gamepad setting itself, because the two shells reach it by different routes and each already + * knows which one it is; a screen that guessed could disagree with the shell that pushed it. + */ + console: Boolean = true, ) { - val ink = LocalGamepadInk.current BackHandler(onBack = onBack) val context = LocalContext.current val scope = rememberCoroutineScope() - val hazeState = remember { HazeState() } - val landscape = LocalConfiguration.current.orientation == Configuration.ORIENTATION_LANDSCAPE + // Bumped by the touch header's Reload — re-keys the fetch effect below without the screen + // having to be popped and re-pushed to try a flaky host again. + var reloadKey by remember { mutableIntStateOf(0) } var state by remember { mutableStateOf(LibState.Loading) } // A launch (connect) in flight: shows an overlay + gates the pad so a second press can't dial twice. var launching by remember { mutableStateOf(false) } @@ -121,7 +147,7 @@ fun LibraryScreen( // Keyed on the mgmt port too: a discovery tick can learn it after this screen is composed, and // the fetch must redo itself against the real port rather than stay on a stale 47990 failure. - LaunchedEffect(host.address, host.port, host.fpHex, host.effectiveMgmtPort) { + LaunchedEffect(host.address, host.port, host.fpHex, host.effectiveMgmtPort, reloadKey) { state = LibState.Loading state = withContext(Dispatchers.IO) { val id = runCatching { obtainIdentity(IdentityStore(context)) }.getOrNull() @@ -145,58 +171,126 @@ fun LibraryScreen( } } + // A pinned card's shelf says so, in the card's own `host · profile` shape: what a launch here + // will use is a property of the shelf, not something to remember from the tile two screens back. + val title = if (pinnedProfileId != null && profile != null) { + "${host.name} · ${profile.name} — Library" + } else { + "${host.name} — Library" + } + + // Dial the host over the same pinned mTLS trust, booting straight into this title (the host + // resolves `launch` = its library id). Shared by both presentations: a tap on a grid tile and A + // on a centred cover are the same act, and a launch that behaved differently between them would + // be a bug nobody could see until they switched input device. + fun launch(identity: ClientIdentity, game: GameEntry) { + if (launching) return + launching = true + scope.launch { + val handle = connectToHost( + context, streamSettings, identity, + host.address, host.port, host.fpHex, launch = game.id, + ) + launching = false + if (handle != 0L) { + onLaunched( + ActiveSession( + handle, + streamSettings, + host.clipboardSync, + profileName = profile?.name, + hostId = host.id, + // Where to come back to when this game exits — this shelf, pin and all, + // not the host's default one. + launchedFromLibrary = true, + libraryProfileId = pinnedProfileId, + ), + ) + } else { + Toast.makeText( + context, + "Launch failed — check the host and try again.", + Toast.LENGTH_LONG, + ).show() + } + } + } + + // "Copy link" for one TITLE — the self-emitted form a host card already hands out (design/ + // client-deep-links.md §4/§5), plus this game's `launch=` id, so pasting the URL into Playnite + // or a Stream Deck macro boots straight into it. A shelf opened from a PINNED card copies that + // card's profile with it, because that combination is the thing being copied. + fun copyLink(game: GameEntry) { + val url = DeepLinks.forHost(host, launch = game.id, profile = pinnedProfileId).toUrl() + // A toast either way here: this screen renders neither the touch home's notice banner nor + // the console's status line, and both of its presentations are full-bleed over artwork. + linkCopyMessage(putLinkOnClipboard(context, url))?.let { + Toast.makeText(context, it, Toast.LENGTH_SHORT).show() + } + } + + // Lambdas, NOT `::launch` / `::copyLink`: two callable references to the same local function + // compare EQUAL however different the frame they captured, so a skipped recomposition would + // leave the child calling a closure over stale settings. SettingsScreen documents the same + // trap at `scopeProfile()`, having been bitten by it. + if (console) { + ConsoleLibrary( + title = title, + state = state, + launching = launching, + navActive = navActive, + onBack = onBack, + onLaunch = { identity, game -> launch(identity, game) }, + onCopyLink = { game -> copyLink(game) }, + ) + } else { + TouchLibrary( + title = title, + state = state, + launching = launching, + onBack = onBack, + onReload = { reloadKey++ }, + onLaunch = { identity, game -> launch(identity, game) }, + onCopyLink = { game -> copyLink(game) }, + ) + } +} + +/** The console (gamepad) shelf: aurora, console header, coverflow, floating legend. */ +@Composable +private fun ConsoleLibrary( + title: String, + state: LibState, + launching: Boolean, + navActive: Boolean, + onBack: () -> Unit, + onLaunch: (ClientIdentity, GameEntry) -> Unit, + onCopyLink: (GameEntry) -> Unit, +) { + val ink = LocalGamepadInk.current + val hazeState = remember { HazeState() } + val landscape = LocalConfiguration.current.orientation == Configuration.ORIENTATION_LANDSCAPE + // The cover the legend's X acts on — the coverflow reports it as the cursor settles, so the + // hint and the press agree about which title they mean. + var focused by remember { mutableStateOf(null) } + Box(Modifier.fillMaxSize()) { Box(Modifier.fillMaxSize().hazeSource(hazeState)) { GamepadAuroraBackground(Modifier.fillMaxSize()) Column(Modifier.fillMaxSize().consoleSafeArea()) { - // A pinned card's shelf says so, in the card's own `host · profile` shape: what a - // launch here will use is a property of the shelf, not something to remember from - // the tile two screens back. - ConsoleHeader( - if (pinnedProfileId != null && profile != null) { - "${host.name} · ${profile.name} — Library" - } else { - "${host.name} — Library" - }, - ) + ConsoleHeader(title) Box(Modifier.weight(1f).fillMaxWidth(), contentAlignment = Alignment.Center) { - when (val s = state) { + when (state) { is LibState.Loading -> LoadingState() - is LibState.Message -> MessageState(s.text) - is LibState.Ready -> Coverflow(s.games, s.loader, navActive && !launching) { game -> - if (!launching) { - launching = true - scope.launch { - // Dial the host over the same pinned mTLS trust, booting straight - // into this title (the host resolves `launch` = its library id). - val handle = connectToHost( - context, streamSettings, s.identity, - host.address, host.port, host.fpHex, launch = game.id, - ) - launching = false - if (handle != 0L) { - onLaunched( - ActiveSession( - handle, - streamSettings, - host.clipboardSync, - profileName = profile?.name, - hostId = host.id, - // Where to come back to when this game exits — - // this shelf, pin and all, not the host's default one. - launchedFromLibrary = true, - libraryProfileId = pinnedProfileId, - ), - ) - } - else Toast.makeText( - context, - "Launch failed — check the host and try again.", - Toast.LENGTH_LONG, - ).show() - } - } - } + is LibState.Message -> MessageState(state.text) + is LibState.Ready -> Coverflow( + games = state.games, + loader = state.loader, + navActive = navActive && !launching, + onFocus = { focused = it }, + onCopyLink = onCopyLink, + onLaunch = { game -> onLaunch(state.identity, game) }, + ) } } } @@ -225,7 +319,16 @@ fun LibraryScreen( ) { GamepadHintBar( buildList { - if (state is LibState.Ready) add(PadGlyph.hint('A', "Launch")) + if (state is LibState.Ready) { + add(PadGlyph.hint('A', "Launch")) + // A controller has no right-click, so the grid's context menu becomes a + // face button and a legend entry — the one per-game action there is. + add( + PadGlyph.hint('X', "Copy link") { + focused?.let(onCopyLink) + }, + ) + } add(PadGlyph.hint('B', "Close", onClick = onBack)) }, hazeState = hazeState, @@ -234,6 +337,270 @@ fun LibraryScreen( } } +/** + * The touch shelf: a Material poster grid under a back/reload header — the same page the Apple, + * GTK and Windows shells draw, and the reason a finger-driven user can reach the library at all. + * + * Deliberately NOT the coverflow with the pad legend hidden: a coverflow is a one-at-a-time strip + * built for a D-pad, and on a phone it turns a 400-title library into 400 swipes. The grid is what + * every other touch surface in this app (and every other client's) already shows. + */ +@Composable +private fun TouchLibrary( + title: String, + state: LibState, + launching: Boolean, + onBack: () -> Unit, + onReload: () -> Unit, + onLaunch: (ClientIdentity, GameEntry) -> Unit, + onCopyLink: (GameEntry) -> Unit, +) { + Box(Modifier.fillMaxSize()) { + Column(Modifier.fillMaxSize().consoleSafeArea()) { + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = Modifier.fillMaxWidth().padding(start = 4.dp, end = 4.dp, top = 8.dp), + ) { + IconButton(onClick = onBack) { + Icon(Icons.AutoMirrored.Filled.ArrowBack, contentDescription = "Back") + } + Text( + title, + style = MaterialTheme.typography.titleLarge, + maxLines = 2, + overflow = TextOverflow.Ellipsis, + modifier = Modifier.weight(1f), + ) + // A shelf that failed to load is otherwise a dead end you have to back out of and + // re-enter; every other client's library page has had this from the start. + IconButton(onClick = onReload, enabled = state !is LibState.Loading) { + Icon(Icons.Filled.Refresh, contentDescription = "Reload") + } + } + when (state) { + is LibState.Loading -> Box( + Modifier.weight(1f).fillMaxWidth(), + contentAlignment = Alignment.Center, + ) { + Column( + horizontalAlignment = Alignment.CenterHorizontally, + verticalArrangement = Arrangement.spacedBy(14.dp), + ) { + CircularProgressIndicator() + Text("Loading library…", style = MaterialTheme.typography.bodyLarge) + } + } + is LibState.Message -> Box( + Modifier.weight(1f).fillMaxWidth(), + contentAlignment = Alignment.Center, + ) { + Text( + state.text, + style = MaterialTheme.typography.bodyLarge, + color = MaterialTheme.colorScheme.onSurfaceVariant, + textAlign = TextAlign.Center, + modifier = Modifier.padding(horizontal = 24.dp), + ) + } + is LibState.Ready -> TouchGrid( + games = state.games, + loader = state.loader, + onLaunch = { game -> onLaunch(state.identity, game) }, + onCopyLink = onCopyLink, + modifier = Modifier.weight(1f), + ) + } + } + if (launching) { + Box( + Modifier.fillMaxSize().background(Color.Black.copy(alpha = 0.55f)), + contentAlignment = Alignment.Center, + ) { + Column( + horizontalAlignment = Alignment.CenterHorizontally, + verticalArrangement = Arrangement.spacedBy(14.dp), + ) { + CircularProgressIndicator(color = Color.White) + Text("Launching…", color = Color.White, style = MaterialTheme.typography.bodyLarge) + } + } + } + } +} + +/** + * The poster grid. Design D4: launcher entries get their own shelf above the titles, never + * interleaved, and the headings only appear when both groups exist — so a library without + * launchers looks exactly like a plain grid. + * + * Internal (not private) for the same reason as [Coverflow]: the screenshot harness composes the + * real grid with a mock shelf, because the screen around it takes its state off the network. + */ +@Composable +internal fun TouchGrid( + games: List, + loader: ImageLoader, + onLaunch: (GameEntry) -> Unit, + onCopyLink: (GameEntry) -> Unit, + modifier: Modifier = Modifier, +) { + val launchers = games.filter { it.isLauncher } + val titles = games.filter { !it.isLauncher } + val both = launchers.isNotEmpty() && titles.isNotEmpty() + LazyVerticalGrid( + columns = GridCells.Adaptive(minSize = 130.dp), + modifier = modifier.fillMaxWidth(), + contentPadding = PaddingValues(16.dp), + horizontalArrangement = Arrangement.spacedBy(12.dp), + verticalArrangement = Arrangement.spacedBy(16.dp), + ) { + if (launchers.isNotEmpty()) { + if (both) item(span = { GridItemSpan(maxLineSpan) }) { TouchGroupHeading("Launchers") } + items(launchers, key = { "launcher-${it.id}" }) { + TouchPoster(it, loader, onLaunch, onCopyLink) + } + } + if (titles.isNotEmpty()) { + if (both) item(span = { GridItemSpan(maxLineSpan) }) { TouchGroupHeading("Games") } + items(titles, key = { "game-${it.id}" }) { + TouchPoster(it, loader, onLaunch, onCopyLink) + } + } + } +} + +@Composable +private fun TouchGroupHeading(text: String) { + Text( + text.uppercase(), + style = MaterialTheme.typography.labelSmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + letterSpacing = 1.4.sp, + ) +} + +/** + * One touch tile: 2:3 poster, store badge, title. Tap launches; a LONG PRESS opens this title's own + * actions — the finger's context menu, and the same gesture the host cards' overflow answers to. + */ +@OptIn(ExperimentalFoundationApi::class) +@Composable +private fun TouchPoster( + game: GameEntry, + loader: ImageLoader, + onLaunch: (GameEntry) -> Unit, + onCopyLink: (GameEntry) -> Unit, +) { + var menu by remember { mutableStateOf(false) } + val shape = MaterialTheme.shapes.medium + Box { + Column( + Modifier.combinedClickable( + onClickLabel = "Launch ${game.title}", + onLongClickLabel = "More options for ${game.title}", + onClick = { onLaunch(game) }, + onLongClick = { menu = true }, + ), + ) { + Box( + Modifier + .fillMaxWidth() + .aspectRatio(2f / 3f) + .clip(shape) + .background(MaterialTheme.colorScheme.surfaceVariant), + contentAlignment = Alignment.Center, + ) { + TouchPosterArt(game, loader) + Box(Modifier.fillMaxSize().padding(6.dp), contentAlignment = Alignment.TopStart) { + Text( + game.storeLabel, + style = MaterialTheme.typography.labelSmall, + // A launcher's badge is brand-filled (design D4); a game's sits on a dark + // wash over its own art, where the theme's own ink would be a coin toss. + color = if (game.isLauncher) { + MaterialTheme.colorScheme.onPrimary + } else { + Color.White + }, + modifier = Modifier + .semantics { + contentDescription = if (game.isLauncher) { + "Opens ${game.storeLabel}" + } else { + "From ${game.storeLabel}" + } + } + .clip(ConsoleShape.Pill) + .background( + if (game.isLauncher) { + MaterialTheme.colorScheme.primary + } else { + Color.Black.copy(alpha = 0.5f) + }, + ) + .padding(horizontal = 8.dp, vertical = 3.dp), + ) + } + } + Text( + game.title, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + maxLines = 2, + overflow = TextOverflow.Ellipsis, + modifier = Modifier.padding(top = 6.dp), + ) + } + DropdownMenu(expanded = menu, onDismissRequest = { menu = false }) { + DropdownMenuItem( + text = { Text("Copy link") }, + onClick = { + menu = false + onCopyLink(game) + }, + ) + } + } +} + +/** The tile's artwork: the candidates in order (portrait → header → hero), then a placeholder. */ +@Composable +private fun TouchPosterArt(game: GameEntry, loader: ImageLoader) { + val candidates = game.art.posterCandidates + var idx by remember(game.id) { mutableIntStateOf(0) } + if (idx < candidates.size) { + AsyncImage( + model = ImageRequest.Builder(LocalContext.current).data(candidates[idx]).build(), + imageLoader = loader, + contentDescription = game.title, + contentScale = ContentScale.Crop, + modifier = Modifier.fillMaxSize(), + onError = { idx++ }, // this candidate failed — try the next, or fall to the placeholder + ) + return + } + // A launcher ships no poster by design, so its brand mark IS the poster; falling back to the + // launcher's NAME says "opens Steam", where a title would read as "a cover that failed to load". + val mark = launcherIcon(game.iconToken) + if (mark != null) { + Icon( + imageVector = mark, + contentDescription = game.title, + tint = MaterialTheme.colorScheme.onSurfaceVariant, + modifier = Modifier.fillMaxSize(0.45f), + ) + } else { + Text( + if (game.isLauncher) game.storeLabel else game.title, + style = MaterialTheme.typography.titleSmall, + fontWeight = FontWeight.SemiBold, + color = MaterialTheme.colorScheme.onSurfaceVariant, + textAlign = TextAlign.Center, + modifier = Modifier.padding(12.dp), + ) + } +} + @Composable private fun LoadingState() { val ink = LocalGamepadInk.current @@ -262,6 +629,10 @@ internal fun Coverflow( games: List, loader: ImageLoader, navActive: Boolean, + /** Reports the CENTRED title as the cursor settles, so the screen's legend acts on it too. */ + onFocus: (GameEntry?) -> Unit = {}, + /** X — copy the centred title's link. Defaulted so the screenshot harness needs no wiring. */ + onCopyLink: (GameEntry) -> Unit = {}, onLaunch: (GameEntry) -> Unit, ) { val ink = LocalGamepadInk.current @@ -275,9 +646,13 @@ internal fun Coverflow( var navTarget by remember { mutableIntStateOf(0) } LaunchedEffect(pagerState.settledPage) { navTarget = pagerState.settledPage } val current = games.getOrNull(navTarget) + // Publish the centred title outward. Keyed on the ENTRY, not the index, so a library + // refresh that shortens the strip can't leave the legend pointing at a title that moved. + LaunchedEffect(current) { onFocus(current) } // Controller nav: the pad drives the coverflow. Left/right steps a coalesced target the pager - // chases; A launches the centred title; B closes via the screen's BackHandler. + // chases; A launches the centred title; X copies its link; B closes via the screen's + // BackHandler. GamepadNavEffect( active = navActive && games.isNotEmpty(), onMove = { dir -> @@ -285,6 +660,9 @@ internal fun Coverflow( if (t != navTarget) { navTarget = t; scope.launch { pagerState.animateScrollToPage(t) } } }, onActivate = { games.getOrNull(navTarget)?.let(onLaunch) }, + // Read at press time rather than closed over: `navTarget` moves under this callback, + // and the link must be the one the cover under the cursor now points at. + onTertiary = { games.getOrNull(navTarget)?.let(onCopyLink) }, ) // Design D4: the launcher entries lead the strip (the client groups them at parse time). diff --git a/clients/android/app/src/main/kotlin/io/unom/punktfunk/Links.kt b/clients/android/app/src/main/kotlin/io/unom/punktfunk/Links.kt new file mode 100644 index 00000000..c2671cc0 --- /dev/null +++ b/clients/android/app/src/main/kotlin/io/unom/punktfunk/Links.kt @@ -0,0 +1,34 @@ +package io.unom.punktfunk + +import android.content.ClipData +import android.content.ClipboardManager +import android.content.Context +import android.os.Build + +// The clipboard half of "Copy link" (design/client-deep-links.md §4/§5), shared by every surface +// that hands a `punktfunk://` URL out: a host card, a pinned card, and a library title. The URL +// each one builds is its own business; whether the platform TOOK it, and what to say about that, +// is the same answer three times over — and getting it wrong in one place is how a menu item ends +// up silently doing nothing on exactly one screen. + +/** Put a `punktfunk://` URL on the clipboard. False = no clipboard service, or it refused. */ +internal fun putLinkOnClipboard(context: Context, url: String): Boolean { + val clipboard = context.getSystemService(Context.CLIPBOARD_SERVICE) as? ClipboardManager + ?: return false + return runCatching { + clipboard.setPrimaryClip(ClipData.newPlainText("Punktfunk link", url)) + }.isSuccess +} + +/** + * What to tell the user about a copy, or null for "say nothing". + * + * Android 13 draws its own clipboard confirmation, and stacking a second one on top of it is the + * platform's own documented anti-pattern. Below it nothing visible happens at all unless we say + * so — a silent menu item reads as a broken one. + */ +internal fun linkCopyMessage(copied: Boolean): String? = when { + copied && Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU -> null + copied -> "Link copied." + else -> "Couldn't copy the link to the clipboard." +} diff --git a/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ScreenshotTest.kt b/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ScreenshotTest.kt index 7bc47ccf..17b1aa91 100644 --- a/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ScreenshotTest.kt +++ b/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ScreenshotTest.kt @@ -210,6 +210,14 @@ class ScreenshotTest { @Config(sdk = [36], qualifiers = "w800dp-h360dp-xxhdpi") fun library() = shootRoot("library", statusBar = false) { LibraryScene() } + /** + * The same shelf as the TOUCH grid — the presentation a finger gets from a host card's + * "Browse library…". Portrait (the default qualifiers), because that is the orientation a + * phone browses a poster wall in, and the one whose column count the layout has to get right. + */ + @Test + fun libraryTouch() = shootRoot("library-touch") { TouchLibraryScene() } + @Test fun consoleControllersLight() = shootRoot("console-controllers-light", statusBar = false) { ConsoleControllersScene(paletteId = "holo") } diff --git a/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ShotScenes.kt b/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ShotScenes.kt index c2a5b6eb..fc2064ef 100644 --- a/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ShotScenes.kt +++ b/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ShotScenes.kt @@ -26,10 +26,13 @@ import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.automirrored.filled.ArrowBack import androidx.compose.material.icons.filled.BatteryFull +import androidx.compose.material.icons.filled.Refresh import androidx.compose.material.icons.filled.SignalCellular4Bar import androidx.compose.material.icons.filled.Wifi import androidx.compose.material3.Icon +import androidx.compose.material3.IconButton import androidx.compose.foundation.lazy.grid.GridCells import androidx.compose.foundation.lazy.grid.GridItemSpan import androidx.compose.foundation.lazy.grid.LazyVerticalGrid @@ -67,6 +70,7 @@ import io.unom.punktfunk.ConsoleLegendInset import io.unom.punktfunk.ConsoleLicensesScreen import io.unom.punktfunk.ControllersScreen import io.unom.punktfunk.Coverflow +import io.unom.punktfunk.TouchGrid import io.unom.punktfunk.GamepadAuroraBackground import io.unom.punktfunk.GamepadHintBar import io.unom.punktfunk.PadGlyph @@ -705,6 +709,39 @@ internal fun LibraryScene(paletteId: String = "violet") = ConsolePalette(palette } } +/** + * The TOUCH library — the poster grid a finger reaches through a host card's "Browse library…", + * with the same mock shelf the coverflow scene uses. Same construction as [LibraryScene]: the real + * [TouchGrid] under a rebuilt header, because the screen around it takes its state off the network. + */ +@Composable +internal fun TouchLibraryScene() { + val context = LocalContext.current + val loader = remember { shotLibraryLoader(context) } + val games = remember { shotGames() } + Surface(color = MaterialTheme.colorScheme.background) { + Column(Modifier.fillMaxSize()) { + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = Modifier.fillMaxWidth().padding(start = 4.dp, end = 4.dp, top = 8.dp), + ) { + IconButton(onClick = {}) { + Icon(Icons.AutoMirrored.Filled.ArrowBack, contentDescription = "Back") + } + Text( + "Living Room PC — Library", + style = MaterialTheme.typography.titleLarge, + modifier = Modifier.weight(1f), + ) + IconButton(onClick = {}) { + Icon(Icons.Filled.Refresh, contentDescription = "Reload") + } + } + TouchGrid(games, loader, onLaunch = {}, onCopyLink = {}, modifier = Modifier.weight(1f)) + } + } +} + /** A believable shelf: four titles with art plus the Steam launcher entry (brand-mark tile). */ private fun shotGames() = listOf( GameEntry("custom:aurora", "custom", "Aurora Drift", Artwork("shot://art/aurora", null, null)), diff --git a/clients/apple/Sources/PunktfunkClient/Home/LibraryCoverflowView.swift b/clients/apple/Sources/PunktfunkClient/Home/LibraryCoverflowView.swift index 7b9b362b..538e4a81 100644 --- a/clients/apple/Sources/PunktfunkClient/Home/LibraryCoverflowView.swift +++ b/clients/apple/Sources/PunktfunkClient/Home/LibraryCoverflowView.swift @@ -29,6 +29,11 @@ struct LibraryCoverflowView: View { /// Button B (back) — dismisses the library screen. No touch equivalent needed here (the toolbar /// Close button already covers that); this is what makes gamepad-only exit possible. var onDismiss: (() -> Void)? + /// Button X — copy the centered title's `punktfunk://` link. The coverflow's answer to the + /// touch grid's context menu: a controller has no right-click, so the one per-game action + /// there is gets a face button and a legend entry rather than a menu holding a single row. + /// nil where the platform has no clipboard (tvOS), which also drops the hint. + var onCopyLink: ((GameEntry) -> Void)? /// Whether the carousel owns the controller — the in-place shell gates it (mid-transition, /// and under the connect takeover after A launches a title, where this coverflow used to /// keep polling underneath). Cover/sheet presentations keep the default. @@ -44,6 +49,11 @@ struct LibraryCoverflowView: View { private let compact = false // no size classes on macOS #endif @State private var selection: String? + /// The copy hint's acknowledgement. There is no toast on this surface, so the legend entry + /// says it itself — the same answer `GamepadHostOptionsView` gives, in the place the user is + /// already looking. Transient here (the screen stays up, unlike that menu), and cleared the + /// moment the strip moves, since "Copied" was about the cover that WAS centred. + @State private var copied = false /// How many covers have settled (art loaded, or every candidate exhausted). @State private var artSettled = 0 /// The backstop below has fired: play the entrance regardless of what the art is doing. @@ -79,6 +89,14 @@ struct LibraryCoverflowView: View { try? await Task.sleep(for: .milliseconds(700)) artWaitOver = true } + // "Copied" is an acknowledgement, not a state — it goes away on its own, and at once if + // the strip moves off the cover it was about. + .task(id: copied) { + guard copied else { return } + try? await Task.sleep(for: .milliseconds(1600)) + withAnimation(.smooth(duration: 0.2)) { copied = false } + } + .onChange(of: selection) { _, _ in copied = false } } @ViewBuilder private func content(for size: CGSize) -> some View { @@ -109,6 +127,7 @@ struct LibraryCoverflowView: View { itemWidth: coverWidth, spacing: 34, onActivate: { onLaunch?($0.id) }, + onTertiary: onCopyLink.map { copy in { copyCentered(copy) } }, onBack: { onDismiss?() }, shoulderJump: 5, isActive: controllerActive, @@ -160,6 +179,14 @@ struct LibraryCoverflowView: View { } } + /// Hand the CENTERED title to the copy action. Read at press time, not when the legend or + /// the carousel was built (the same rule A's hint follows), and inert with nothing centred. + private func copyCentered(_ copy: (GameEntry) -> Void) { + guard let game = games.first(where: { $0.id == selection }) else { return } + copy(game) + withAnimation(.smooth(duration: 0.2)) { copied = true } + } + /// Does this library have both groups? Only then does the heading earn its row — a /// launcher-less library gets exactly the layout it had before design D4. private var showsGroupHeading: Bool { @@ -217,6 +244,12 @@ struct LibraryCoverflowView: View { // what A does. action: { if let id = selection { onLaunch(id) } })) } + if let onCopyLink { + hints.append(.init( + glyph: buttonGlyph(\.buttonX, fallback: "x.circle"), + text: copied ? "Copied" : "Copy link", + action: { copyCentered(onCopyLink) })) + } hints.append(.init( glyph: buttonGlyph(\.buttonB, fallback: "b.circle"), text: "Close", action: { onDismiss?() })) diff --git a/clients/apple/Sources/PunktfunkClient/Home/LibraryView.swift b/clients/apple/Sources/PunktfunkClient/Home/LibraryView.swift index 2cd5de6c..84114c4e 100644 --- a/clients/apple/Sources/PunktfunkClient/Home/LibraryView.swift +++ b/clients/apple/Sources/PunktfunkClient/Home/LibraryView.swift @@ -165,6 +165,9 @@ struct LibraryView: View { LibraryCoverflowView( games: games, artLoader: artLoader, onLaunch: onLaunch, onDismiss: { (onClose ?? { dismiss() })() }, + // Nil where there is nothing to copy into (tvOS), which is what drops the + // hint from the legend rather than leaving a button that does nothing. + onCopyLink: LinkClipboard.isAvailable ? { copyLink($0) } : nil, controllerActive: controllerActive) } else { grid @@ -270,20 +273,44 @@ struct LibraryView: View { private func tiles(_ entries: [GameEntry]) -> some View { LazyVGrid(columns: columns, spacing: 18) { ForEach(entries) { game in - if let onLaunch { - Button { onLaunch(game.id) } label: { + Group { + if let onLaunch { + Button { onLaunch(game.id) } label: { + GameCard(game: game, artLoader: artLoader, selected: isKeyCursor(game)) + } + .buttonStyle(.plain) + } else { GameCard(game: game, artLoader: artLoader, selected: isKeyCursor(game)) } - .buttonStyle(.plain) - .id(game.id) - } else { - GameCard(game: game, artLoader: artLoader, selected: isKeyCursor(game)) - .id(game.id) } + .id(game.id) + // Right-click / long-press a poster for that TITLE's own actions — the same + // gesture a host card answers to, one level down. `.contextMenu` doesn't exist + // on tvOS, which is also the one platform with no clipboard to copy into. + #if !os(tvOS) + .contextMenu { + if LinkClipboard.isAvailable { + Button("Copy Link") { copyLink(game) } + } + } + #endif } } } + /// Put this title's self-emitted `punktfunk://` link on the clipboard: the shelf's host, + /// the pinned card's profile when a pin opened it, and the game's own `launch=` id — so + /// the URL boots straight into the title, the way a host card's link opens the desktop + /// (design/client-deep-links.md §5). + /// + /// Addressed to the STORE's current record rather than the one the shelf was opened with, + /// so a host re-addressed while browsing hands out the address it actually has now. + private func copyLink(_ game: GameEntry) { + let current = store.hosts.first { $0.id == host.id } ?? host + LinkClipboard.copy( + DeepLink.forHost(current, launch: game.id, profile: target.pinnedProfileID).urlString) + } + /// Whether the keyboard cursor is on this tile (always false where there is no keyboard /// navigation to have moved it). private func isKeyCursor(_ game: GameEntry) -> Bool { diff --git a/clients/apple/Sources/PunktfunkClient/Home/LibraryWidgets.swift b/clients/apple/Sources/PunktfunkClient/Home/LibraryWidgets.swift index 475b240b..f5dae289 100644 --- a/clients/apple/Sources/PunktfunkClient/Home/LibraryWidgets.swift +++ b/clients/apple/Sources/PunktfunkClient/Home/LibraryWidgets.swift @@ -63,9 +63,10 @@ private extension Image { /// Sequentially tries cover-art URLs over `loader` (so a paired client can reach the host's own /// art proxy, not just public CDNs — see `LibraryArtLoader`), advancing past any that fail to /// load, then a placeholder. The loaded image is hard-clipped to fill the card's actual frame -/// regardless of its own aspect ratio: a portrait capsule fills it as intended, but a fallback -/// banner (wide hero/header art, used when a title has no portrait capsule) would otherwise report -/// a much wider intrinsic size than the card and overflow into neighboring cards. Not `private` — +/// regardless of its own aspect ratio: a portrait capsule fills it as intended, and a fallback +/// banner (wide hero/header art, used when a title has no portrait capsule) is cropped to the same +/// tile rather than allowed to size it — see the `Color.clear` in `body` for why that takes more +/// than a `.frame(maxWidth:)` and a `.clipped()`. Not `private` — /// the gamepad coverflow (`LibraryCoverflowView`) reuses it directly rather than re-fetching art. struct PosterImage: View { let candidates: [URL] @@ -84,9 +85,20 @@ struct PosterImage: View { var body: some View { Group { if let image { - Image(platformImage: image) - .resizable() - .scaledToFill() + // `Color.clear` is what takes the proposed size; the art rides along as its + // overlay, where it can be DRAWN but never MEASURED. Handing the image the sizing + // role instead is what let a fallback banner escape the tile: `scaledToFill` + // reports a size that covers the proposal, and the flexible frame below clamps it + // to `.infinity` — i.e. not at all. Measured offscreen, a 460×215 `header.jpg` in a + // 170pt grid column resolved the tile to 545×255 and overran its neighbours, while + // a 300×450 cover in the same chain came out correct — which is why this only ever + // showed on the titles whose cover was missing. + Color.clear + .overlay { + Image(platformImage: image) + .resizable() + .scaledToFill() + } .transition(.opacity) } else if index < candidates.count { ZStack { placeholder; ProgressView() } diff --git a/clients/linux/src/app.rs b/clients/linux/src/app.rs index 78c8963f..a213e900 100644 --- a/clients/linux/src/app.rs +++ b/clients/linux/src/app.rs @@ -61,6 +61,13 @@ const CSS: &str = " .pf-poster { border-radius: 10px; background: alpha(currentColor, 0.08); } .pf-poster-monogram { font-size: 2.4em; font-weight: bold; color: alpha(currentColor, 0.45); } .pf-store-badge { color: white; background: rgba(0, 0, 0, 0.55); } +/* The poster's own overflow menu button. It sits ON artwork, so it needs the badge's dark + scrim to read at all — a flat button inherits the page's foreground and vanishes into a + pale cover. Small and round so it balances the store badge opposite it rather than + competing with the art. (No quotes in here — see the top of this string.) */ +.pf-poster-menu { color: white; background: rgba(0, 0, 0, 0.55); border-radius: 999px; + min-width: 24px; min-height: 24px; padding: 0; margin: 6px; } +.pf-poster-menu:hover { background: rgba(0, 0, 0, 0.75); } /* Launcher entries (design D4) open the launcher itself. They rarely have poster art, so an art-less one must not read as a game whose cover failed to load: accent face, the launcher named instead of a title monogram, and an accent badge. */ diff --git a/clients/linux/src/main.rs b/clients/linux/src/main.rs index 7a90ff0c..dd7b5496 100644 --- a/clients/linux/src/main.rs +++ b/clients/linux/src/main.rs @@ -22,6 +22,9 @@ mod cli; mod shortcuts; #[cfg(target_os = "linux")] mod spawn; +// The guarded FlowBox `child-activated → activate` bridge every card grid needs. +#[cfg(target_os = "linux")] +mod ui_flow; #[cfg(target_os = "linux")] mod ui_hosts; #[cfg(target_os = "linux")] diff --git a/clients/linux/src/ui_flow.rs b/clients/linux/src/ui_flow.rs new file mode 100644 index 00000000..a8626b34 --- /dev/null +++ b/clients/linux/src/ui_flow.rs @@ -0,0 +1,70 @@ +//! One shared fix for a GTK4 footgun every card grid in this shell walks into. +//! +//! A pointer click (and keyboard activate) on a [`gtk::FlowBoxChild`] emits +//! `child-activated` on the *FlowBox*, never the child's own `activate` signal — so a +//! grid whose per-card handler hangs off `child.connect_activate()` has to bridge the +//! one to the other. The naive bridge is a stack overflow: `FlowBoxChild`'s default +//! `activate` handler re-emits `child-activated` on its parent, which calls the bridge, +//! which activates the child again, forever. +//! +//! [`bridge_child_activation`] is that bridge with the re-entrancy guard that breaks the +//! cycle. Use it for every `child-activated → child.activate()` hop; do not hand-roll it, +//! since a bare `flow.connect_child_activated(|_, c| c.activate())` aborts the process on +//! the first click and looks perfectly reasonable in review. + +use gtk::prelude::*; + +/// Bridge a FlowBox's `child-activated` to the activated child's own `activate` signal, +/// exactly once per click. The re-entrant emission the child's default handler bounces +/// back is swallowed rather than recursed into. +pub(crate) fn bridge_child_activation(flow: >k::FlowBox) { + let activating = std::cell::Cell::new(false); + flow.connect_child_activated(move |_, child| { + if activating.replace(true) { + return; + } + child.activate(); + activating.set(false); + }); +} + +#[cfg(test)] +mod tests { + use super::bridge_child_activation; + use gtk::prelude::*; + use std::cell::Cell; + use std::rc::Rc; + + // Reproduces the exact FlowBox/FlowBoxChild wiring the card grids use: the bridge + // calls `child.activate()`, whose own default handler re-emits `child-activated` — + // that ping-pong recursed forever (a real stack overflow on every card click/Enter, + // reported on the hosts page and then again on the library page) until the + // re-entrancy guard landed here, where both pages share it. + #[test] + #[ignore = "needs a Wayland/X display"] + fn flow_box_activation_bridge_does_not_recurse() { + assert!(gtk::init().is_ok(), "no display"); + + let flow = gtk::FlowBox::builder() + .selection_mode(gtk::SelectionMode::None) + .activate_on_single_click(true) + .build(); + bridge_child_activation(&flow); + + let child = gtk::FlowBoxChild::new(); + flow.insert(&child, -1); + let fired = Rc::new(Cell::new(0u32)); + { + let fired = fired.clone(); + child.connect_activate(move |_| fired.set(fired.get() + 1)); + } + + flow.emit_by_name::<()>("child-activated", &[&child]); + + assert_eq!( + fired.get(), + 1, + "the per-card handler should fire exactly once" + ); + } +} diff --git a/clients/linux/src/ui_hosts.rs b/clients/linux/src/ui_hosts.rs index 76e12576..28484bc0 100644 --- a/clients/linux/src/ui_hosts.rs +++ b/clients/linux/src/ui_hosts.rs @@ -781,18 +781,11 @@ impl SimpleComponent for HostsPage { // A pointer click (and keyboard activate) emits `child-activated` on the // *FlowBox*, never the child's own `activate` signal — bridge it back to the - // child, where each card wires its connect handler. The re-entrancy flag breaks - // the child-activated ↔ activate ping-pong that otherwise recurses forever - // (a real stack overflow on every card click; see the ignored display test). + // child, where each card wires its connect handler. The guard inside the bridge + // breaks the child-activated ↔ activate ping-pong that otherwise recurses forever + // (a real stack overflow on every card click; see `ui_flow`'s display test). for flow in [saved.widget(), discovered.widget()] { - let activating = std::cell::Cell::new(false); - flow.connect_child_activated(move |_, child| { - if activating.replace(true) { - return; - } - child.activate(); - activating.set(false); - }); + crate::ui_flow::bridge_child_activation(flow); } // Shown under the discovered heading while no (unsaved) advert is live yet. @@ -1466,49 +1459,3 @@ impl HostsPage { dialog.present(Some(&self.widgets.stack)); } } - -#[cfg(test)] -mod tests { - use adw::prelude::*; - use std::cell::Cell; - use std::rc::Rc; - - // Reproduces the exact FlowBox/FlowBoxChild wiring from `init()`: `child-activated` - // bridges to `child.activate()`, whose own default handler re-emits - // `child-activated` — that ping-pong recursed forever (stack overflow on every - // host-card click/Enter) until the re-entrancy guard was added. - #[test] - #[ignore = "needs a Wayland/X display"] - fn flow_box_activation_bridge_does_not_recurse() { - assert!(gtk::init().is_ok(), "no display"); - - let flow = gtk::FlowBox::builder() - .selection_mode(gtk::SelectionMode::None) - .activate_on_single_click(true) - .build(); - let activating = Cell::new(false); - flow.connect_child_activated(move |_, child| { - if activating.replace(true) { - return; - } - child.activate(); - activating.set(false); - }); - - let child = gtk::FlowBoxChild::new(); - flow.insert(&child, -1); - let fired = Rc::new(Cell::new(0u32)); - { - let fired = fired.clone(); - child.connect_activate(move |_| fired.set(fired.get() + 1)); - } - - flow.emit_by_name::<()>("child-activated", &[&child]); - - assert_eq!( - fired.get(), - 1, - "the per-card handler should fire exactly once" - ); - } -} diff --git a/clients/linux/src/ui_library.rs b/clients/linux/src/ui_library.rs index 7ec5d6d3..2bd7995e 100644 --- a/clients/linux/src/ui_library.rs +++ b/clients/linux/src/ui_library.rs @@ -10,7 +10,7 @@ use crate::library::{self, GameEntry}; use crate::trust; use crate::ui_hosts::ConnectRequest; use adw::prelude::*; -use gtk::{gdk, glib}; +use gtk::{gdk, gio, glib}; use relm4::prelude::*; use std::cell::{Cell, RefCell}; use std::collections::{HashMap, VecDeque}; @@ -64,6 +64,32 @@ fn page_host_label(req: &ConnectRequest) -> String { ) } +/// One title's self-emitted `punktfunk://` link (design/client-deep-links.md §5): this +/// page's host with the game's own `launch=` id attached, so the URL boots straight into +/// that title instead of the desktop. Built from the STORE, like every other "Copy link" +/// in this shell, because the stable id and the pin live there rather than on the request. +/// +/// A shelf opened from a PINNED card carries that card's one-off profile into the link: +/// what you copy off that shelf is what pressing the card and picking the title does. +/// `None` only when the host has left the store while the page was open. +fn game_link(req: &ConnectRequest, game_id: &str) -> Option { + let known = pf_client_core::trust::KnownHosts::load(); + let host = req + .fp_hex + .as_deref() + .filter(|fp| !fp.is_empty()) + .and_then(|fp| known.find_by_fp(fp)) + .or_else(|| known.find_by_addr(&req.addr, req.port))?; + Some( + pf_client_core::deeplink::DeepLink::for_host( + host, + Some(game_id), + req.profile.as_deref().filter(|p| !p.is_empty()), + ) + .to_url(), + ) +} + /// Open the library page for a saved host and start the fetch. `mgmt_port` comes from /// the live mDNS `mgmt` TXT when the host is advertising (the hosts page resolves it). pub fn open( @@ -116,10 +142,9 @@ fn build( .valign(gtk::Align::Start) .build(); // Click/keyboard activation fires `child-activated` on the FlowBox, not the child's own - // `activate` — bridge it so each poster's connect handler (below) runs on click. - flow.connect_child_activated(|_, child| { - child.activate(); - }); + // `activate` — bridge it so each poster's connect handler (below) runs on click. The + // bridge must be the guarded one: bare, it recurses until the stack overflows. + crate::ui_flow::bridge_child_activation(&flow); // The launcher shelf: same tile geometry as the games grid, its own FlowBox so the two // groups never interleave and each wraps on its own. let launcher_flow = gtk::FlowBox::builder() @@ -132,9 +157,7 @@ fn build( .row_spacing(18) .valign(gtk::Align::Start) .build(); - launcher_flow.connect_child_activated(|_, child| { - child.activate(); - }); + crate::ui_flow::bridge_child_activation(&launcher_flow); let launchers_heading = gtk::Label::new(Some("Launchers")); launchers_heading.add_css_class("pf-group-heading"); launchers_heading.set_halign(gtk::Align::Start); @@ -379,10 +402,44 @@ fn game_card(state: &Rc, game: &GameEntry) -> gtk::FlowBoxChild { badge.set_margin_start(6); badge.set_margin_top(6); + // The tile's own actions. Today that is one — "Copy link", the per-GAME half of the + // pairing the host cards already offer (design/client-deep-links.md §5 names the + // library game context menu as an attach point) — hung off a menu rather than a bare + // button so the next one lands next to it instead of growing a second affordance. + let actions = gio::SimpleActionGroup::new(); + { + let (sender, req, id) = (state.sender.clone(), state.req.clone(), game.id.clone()); + let a = gio::SimpleAction::new("copy-link", None); + a.connect_activate(move |_, _| match game_link(&req, &id) { + Some(url) => { + if let Some(display) = gdk::Display::default() { + display.clipboard().set_text(&url); + } + sender.input(AppMsg::Toast("Link copied".into())); + } + // Only reachable if the host was forgotten while this page was open. + None => sender.input(AppMsg::Toast("This host isn't saved any more".into())), + }); + actions.add_action(&a); + } + let menu = gio::Menu::new(); + menu.append(Some("Copy link"), Some("game.copy-link")); + let menu_btn = gtk::MenuButton::builder() + .icon_name("view-more-symbolic") + .menu_model(&menu) + .halign(gtk::Align::End) + .valign(gtk::Align::Start) + .build(); + menu_btn.add_css_class("flat"); + menu_btn.add_css_class("pf-poster-menu"); + menu_btn.set_tooltip_text(Some("More options")); + let poster = gtk::Overlay::new(); poster.set_child(Some(&placeholder)); poster.add_overlay(&pic); poster.add_overlay(&badge); + poster.add_overlay(&menu_btn); + poster.insert_action_group("game", Some(&actions)); poster.add_css_class("pf-poster"); if launcher { poster.add_css_class("pf-launcher"); @@ -403,6 +460,14 @@ fn game_card(state: &Rc, game: &GameEntry) -> gtk::FlowBoxChild { let child = gtk::FlowBoxChild::new(); child.set_child(Some(&card)); + // Right-click anywhere on the tile is the same menu — the desktop gesture for "this + // item's actions", and what the host cards already answer to. + let right_click = gtk::GestureClick::builder().button(3).build(); + { + let menu_btn = menu_btn.clone(); + right_click.connect_pressed(move |_, _, _, _| menu_btn.popup()); + } + child.add_controller(right_click); let sender = state.sender.clone(); let mut req = state.req.clone(); req.launch = Some((game.id.clone(), game.title.clone())); diff --git a/clients/windows/src/app/library.rs b/clients/windows/src/app/library.rs index 367c5c74..52e12104 100644 --- a/clients/windows/src/app/library.rs +++ b/clients/windows/src/app/library.rs @@ -228,6 +228,37 @@ fn initials(title: &str) -> String { .collect() } +/// The tile overflow's only entry today — the per-GAME half of the pairing the host tiles +/// already offer (design/client-deep-links.md §5 names the library game context menu as an +/// attach point for exactly this). +const MENU_COPY_LINK: &str = "Copy link"; + +/// One title's self-emitted `punktfunk://` link: this page's host with the game's own +/// `launch=` id attached, so the URL boots straight into that title rather than the desktop. +/// Built from the STORE — the stable id and the fingerprint live there, not on the target — +/// which is what makes a link taken here identical to one taken off the host tile. +/// +/// A library opened from a "Connect with" one-off carries that profile into the link, so a +/// copied URL streams the way the shelf it came from does. `None` only when the host has +/// left the store while the page was open. +fn game_link(target: &super::Target, game_id: &str) -> Option { + let known = crate::trust::KnownHosts::load(); + let host = target + .fp_hex + .as_deref() + .filter(|fp| !fp.is_empty()) + .and_then(|fp| known.find_by_fp(fp)) + .or_else(|| known.find_by_addr(&target.addr, target.port))?; + Some( + pf_client_core::deeplink::DeepLink::for_host( + host, + Some(game_id), + target.profile.as_deref().filter(|p| !p.is_empty()), + ) + .to_url(), + ) +} + /// A small group label above a tile grid ("Launchers" / "Games"). Only drawn when the page shows /// both groups — a single unlabelled grid is what every launcher-less library looked like before. fn group_heading(text: &str) -> Element { @@ -246,6 +277,7 @@ fn poster_tile( art_uri: Option<&str>, poster_h: f64, on_tap: Box, + on_copy_link: Box, ) -> Element { let poster: Element = match art_uri { Some(uri) => Image::new_with_uri(uri) @@ -311,7 +343,7 @@ fn poster_tile( }) .border_thickness(uniform(1.0)); - border( + let tappable = border( vstack(( framed, text_block(&game.title) @@ -322,7 +354,33 @@ fn poster_tile( .spacing(0.0), ) .background(hit_test_backstop()) - .on_tapped(on_tap) + .on_tapped(on_tap); + + // This entry's own actions, opposite the store badge. A button rather than a right-click + // context flyout because the reactor hangs `menu_flyout` off buttons only — and a menu a + // mouse user cannot see is one they never find. + // + // A SIBLING of the tappable area, not a child of it: `host_tile` on the hosts page splits + // the two exactly this way, and that split is what keeps a click on the overflow from also + // launching the title underneath it. + grid(vec![ + tappable.into(), + button("") + .icon(Symbol::More) + .subtle() + .tooltip("More options") + .automation_name(format!("More options for {}", game.title)) + .menu_flyout(vec![menu_item(MENU_COPY_LINK)]) + .on_item_clicked(move |item: String| { + if item == MENU_COPY_LINK { + on_copy_link(); + } + }) + .horizontal_alignment(HorizontalAlignment::Right) + .vertical_alignment(VerticalAlignment::Top) + .margin(edges(0.0, 4.0, 4.0, 0.0)) + .into(), + ]) .into() } @@ -394,11 +452,19 @@ pub(crate) fn library_page(props: &LibraryProps, cx: &mut RenderCx) -> Element { let tile = |g: &Game| -> Element { let (ctx2, ss, st) = (ctx.clone(), ss.clone(), st.clone()); let (target, id) = (target.clone(), g.id.clone()); + let (link_target, link_id) = (target.clone(), id.clone()); poster_tile( g, props.state.art.get(&g.id).map(String::as_str), poster_h, Box::new(move || initiate_launch(&ctx2, target.clone(), id.clone(), &ss, &st)), + // Silent on success, exactly like the host tile's "Copy link" on the + // hosts page — this shell has no toast, and the two must not disagree + // about what copying a link looks like. + Box::new(move || match game_link(&link_target, &link_id) { + Some(url) => pf_client_core::clipboard::set_text(&url), + None => tracing::warn!(id = %link_id, "no saved host to build a link from"), + }), ) }; // Design D4: launcher entries get their own shelf above the titles, never diff --git a/crates/pf-bitstream/src/av1.rs b/crates/pf-bitstream/src/av1.rs index 2a59d881..7dee0185 100644 --- a/crates/pf-bitstream/src/av1.rs +++ b/crates/pf-bitstream/src/av1.rs @@ -277,6 +277,19 @@ pub struct PicturePlan { /// Colour signalling, per picture and never latched — the same rule the other two /// planners follow, because a host can switch an HDR desktop to PQ/BT.2020 in band. pub colour: ColourDescription, + /// Every picture this frame predicts from was itself decoded from a fully-available + /// reference chain — so a host claim that this frame is a clean re-anchor + /// (`USER_FLAG_RECOVERY_ANCHOR`) can be corroborated rather than taken on trust. + /// `true` for a key or intra-only frame (nothing to predict from) and for any + /// frame whose whole reference chain is clean; `false` from the moment this frame + /// — or anything it descends from — needed concealment. + /// + /// On a `show_existing_frame` this describes the picture being DISPLAYED, which is + /// the only thing such a frame puts on the screen: it decodes nothing of its own. + /// + /// Purely additive observation. See [`crate::clean`] for why it propagates and why + /// every rule errs toward `false`. + pub references_clean: bool, } /// One planned access unit. @@ -329,6 +342,35 @@ pub enum PlanWarning { TruncatedAu { offset: usize }, } +impl PlanWarning { + /// Does this warning mean the PICTURE is damaged? The AV1 twin of + /// [`crate::h264::PlanWarning::is_integrity`], and + /// `pf_vkdecode::is_integrity_warning_av1` delegates here. + /// + /// Every variant AV1 has IS damage, and that is a fact about the codec rather than + /// an oversight: AV1 puts nothing in this channel resembling h265's + /// `NonZeroReorder` or h264's `Mmco5Rebase`. It has no reorder envelope to report + /// (no bumping process, no `max_num_reorder_pics`) and no MMCO to rebase — the + /// frame header states the whole reference update outright — so the only things + /// left to warn about are pictures that went missing and an OBU walk that stopped + /// early. + /// + /// `MissingShowExisting` is the one that could be argued, and it is damage: a + /// `show_existing_frame` naming an empty slot means the picture the STREAM chose + /// to display was lost upstream. Nothing is displayed for that frame, so the + /// screen keeps the previous one — exactly the "silently stale picture" state a + /// re-anchor exists to end. + /// + /// Exhaustive with no wildcard, for the reason the H.264 twin spells out. + pub fn is_integrity(&self) -> bool { + match self { + PlanWarning::MissingReference { .. } + | PlanWarning::MissingShowExisting { .. } + | PlanWarning::TruncatedAu { .. } => true, + } + } +} + /// Why an access unit cannot be planned at all. #[derive(Debug, Clone, PartialEq, Eq)] pub enum PlanError { @@ -366,6 +408,10 @@ pub struct Av1Planner { slots: [Option; NUM_REF_SLOTS], next_id: PicId, sequence: Option>, + /// Which resident pictures came off a BROKEN reference chain — the fact behind + /// [`PicturePlan::references_clean`]. Empty on a healthy stream; see + /// [`crate::clean::CleanLedger`] for the propagation rules. + clean: crate::clean::CleanLedger, } impl Default for Av1Planner { @@ -381,6 +427,7 @@ impl Av1Planner { slots: [None; NUM_REF_SLOTS], next_id: 1, sequence: None, + clean: Default::default(), } } @@ -549,7 +596,22 @@ impl Av1Planner { } else { Vec::new() }; - let picture = picture_plan(&header, &sequence); + // A `show_existing_frame` decodes nothing, so the only picture it puts on + // the screen is the one it displays: report THAT picture's cleanliness. A + // slot that held nothing already warned above and shows nothing at all, + // which is damage in its own right — reporting it unclean keeps the two + // statements consistent. + let references_clean = match shown { + Some(pic) => self.clean.references_clean([pic.id]), + None => false, + }; + let picture = picture_plan(&header, &sequence, references_clean); + // A key-frame `show_existing_frame` rewrote every slot with the shown + // picture (7.20), so the ledger has to follow that aliasing: the refreshed + // slots all hold `pic.id`, whose mark already stands. Nothing new is + // stored, so there is no verdict to fold — only residency to re-bound. + self.clean + .retain_live(self.slots.iter().flatten().map(|p| p.id)); return Ok(AuPlan { picture, tiles, @@ -597,12 +659,34 @@ impl Av1Planner { } let removed = self.refresh_slots(header.refresh_frame_flags, id, RefState::of(&header)); - let picture = picture_plan(&header, &sequence); + // Was every picture this frame predicts from decoded off an intact chain? + // Over the resolved names only: a `None` hole is a lost reference, which has + // already pushed `MissingReference` and therefore condemns this frame through + // `concealed` below. A key or intra-only frame names nothing, so this is + // vacuously true for it (`CleanLedger::references_clean`). + let references_clean = self + .clean + .references_clean(refs.iter().flatten().map(|r| r.id)); + + let picture = picture_plan(&header, &sequence, references_clean); let outputs = if header.show_frame { vec![id] } else { Vec::new() }; + // Fold this frame's verdict, then bound the ledger to slot residency. After + // `refresh_slots`, so the live set reflects the writes this frame performed. + // `concealed` mirrors what a consumer conceals on, via the ONE classification + // (`PlanWarning::is_integrity`), so the ledger and the consumer can never + // disagree about whether this frame was damaged. + self.clean.note_stored( + id, + references_clean, + warnings.iter().any(PlanWarning::is_integrity), + ); + self.clean + .retain_live(self.slots.iter().flatten().map(|p| p.id)); + Ok(AuPlan { picture, tiles, @@ -655,7 +739,11 @@ impl Av1Planner { } } -fn picture_plan(header: &FrameHeaderObu, sequence: &SequenceHeaderObu) -> PicturePlan { +fn picture_plan( + header: &FrameHeaderObu, + sequence: &SequenceHeaderObu, + references_clean: bool, +) -> PicturePlan { let color = &sequence.color_config; let bit_depth = if color.high_bitdepth { if color.twelve_bit { @@ -698,6 +786,7 @@ fn picture_plan(header: &FrameHeaderObu, sequence: &SequenceHeaderObu) -> Pictur matrix_coefficients: color.matrix_coefficients as u8, video_full_range: color.color_range, }, + references_clean, } } #[cfg(test)] diff --git a/crates/pf-bitstream/src/clean.rs b/crates/pf-bitstream/src/clean.rs new file mode 100644 index 00000000..f0ffb64b --- /dev/null +++ b/crates/pf-bitstream/src/clean.rs @@ -0,0 +1,245 @@ +//! Which decoded pictures came off a fully-available reference chain — the fact a +//! client needs to CORROBORATE a host's claim that a frame is a clean re-anchor. +//! +//! # Why this exists +//! +//! After a loss the client freezes on its last good picture and lifts only on a proven +//! re-anchor. One of those proofs — `USER_FLAG_RECOVERY_ANCHOR`, the host's LTR-RFI +//! recovery frame — lifts the freeze on its FIRST occurrence, exactly like a real IDR, +//! because the host says the frame was coded against a known-good reference. +//! +//! The host's "known-good" is an inference from what the client RECEIVED. The client's +//! own DPB is the only place that knows what it actually DECODED, and it did not +//! previously record it: a picture the planner concealed (a reference the DPB could not +//! resolve, an AU that stopped early) entered the DPB looking exactly like a clean one. +//! So an anchor naming that picture lifted the freeze onto a gray plate, and every +//! frame after it chained off the corruption — the freeze gone, nothing left to +//! re-arm it, and the picture stayed broken until an unrelated signal forced an IDR. +//! +//! This ledger is the missing fact, and it is deliberately the SMALLEST one that +//! answers the question: a set of picture ids that are NOT clean. Membership is +//! per-picture, so it costs one `u64` per damaged picture and nothing at all on a +//! healthy stream — the overwhelmingly common case, where the set stays empty for the +//! life of the session. +//! +//! # Damage propagates; that is the whole point +//! +//! A picture is unclean when the AU that produced it needed concealment, OR when +//! ANYTHING it predicted from was unclean. Without the second half the ledger would be +//! useless: the concealed picture itself is rarely the one an anchor names — it is the +//! chain of ordinary P-frames DESCENDING from it, each of which planned perfectly and +//! raised no warning of its own, that carries the corruption forward. +//! +//! # It errs toward "unclean", never toward "clean" +//! +//! Every rule here is one-way. An id the ledger has forgotten (evicted from the DPB, +//! dropped at a flush) reads as clean, which is correct — a picture no longer in the +//! DPB cannot be referenced. An id it holds stays unclean until the picture leaves the +//! DPB. There is no path that clears the mark on a picture that is still resident, so +//! the ledger can only ever make a consumer MORE conservative: hold the freeze longer +//! and take an IDR it might not have needed. The opposite mistake — reporting a damaged +//! chain as clean — is the failure this exists to end, so the asymmetry is deliberate. + +use std::collections::BTreeSet; + +/// Per-picture "this came off a broken chain" marks for one planner. +/// +/// Keyed by the planner's own `PicId` (a `u64` in all three codecs), so this type is +/// codec-agnostic and the H.264, H.265 and AV1 planners share ONE implementation rather +/// than three hand-copies that can drift apart. +#[derive(Debug, Clone, Default)] +pub struct CleanLedger { + /// Ids of resident pictures that are NOT clean. Empty on a healthy stream — the + /// set only ever gains an entry when a plan needed concealment. + unclean: BTreeSet, +} + +impl CleanLedger { + pub fn new() -> Self { + Self::default() + } + + /// Is every id in `references` clean? — i.e. may a picture predicted from exactly + /// these be trusted? + /// + /// Vacuously true for an empty list, which is what makes an IRAP/IDR clean by + /// construction: it predicts from nothing, so there is nothing to distrust. + pub fn references_clean(&self, references: I) -> bool + where + I: IntoIterator, + { + // Short-circuits on the first unclean reference, and — because the set is + // empty on a healthy stream — degenerates to one `is_empty`-cheap lookup per + // reference in the case that matters for throughput. + self.unclean.is_empty() || !references.into_iter().any(|id| self.unclean.contains(&id)) + } + + /// Record the verdict for the picture this AU stored. + /// + /// `references_clean` is what [`Self::references_clean`] answered for this AU's + /// reference lists; `concealed` is whether the AU's own plan carried an integrity + /// warning. Either one being bad makes the stored picture unclean, and its + /// descendants inherit that through their own `references_clean` call. + pub fn note_stored(&mut self, id: u64, references_clean: bool, concealed: bool) { + if references_clean && !concealed { + // The common path. Nothing is inserted, so a healthy stream never allocates + // — and `remove` still runs below because an id can be REUSED after the + // planner recycles it, and a stale mark would then condemn a fresh picture. + self.unclean.remove(&id); + } else { + self.unclean.insert(id); + } + } + + /// Drop the marks of pictures that have left the DPB. + /// + /// Called with the ids still live after each plan. Bounding the set to DPB + /// residency is what keeps it from growing without limit across a long lossy + /// session, and it is safe precisely because a picture outside the DPB can never + /// appear in a later reference list. + pub fn retain_live(&mut self, live: I) + where + I: IntoIterator, + { + if self.unclean.is_empty() { + return; + } + let live: BTreeSet = live.into_iter().collect(); + self.unclean.retain(|id| live.contains(id)); + } + + /// Forget everything — the DPB was drained (a flush, a stream discontinuity), so no + /// mark describes a resident picture any more. + pub fn clear(&mut self) { + self.unclean.clear(); + } + + /// Is this picture known to have come off a broken chain? (Diagnostics and tests; + /// the plan path uses [`Self::references_clean`].) + pub fn is_unclean(&self, id: u64) -> bool { + self.unclean.contains(&id) + } + + /// How many resident pictures are marked unclean (diagnostics and tests). + pub fn unclean_count(&self) -> usize { + self.unclean.len() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The headline: damage propagates down the prediction chain. The concealed + /// picture is rarely the one an anchor names — it is the ordinary P-frames + /// descending from it, each of which planned perfectly and warned about nothing. + #[test] + fn damage_propagates_to_every_descendant() { + let mut led = CleanLedger::new(); + // An IDR: no references, no concealment. + assert!(led.references_clean([])); + led.note_stored(0, true, false); + assert!(!led.is_unclean(0)); + + // A clean P off it. + assert!(led.references_clean([0])); + led.note_stored(1, true, false); + + // Picture 2's plan needed concealment. + let refs_clean = led.references_clean([1]); + assert!(refs_clean, "its reference was still fine"); + led.note_stored(2, refs_clean, true); + assert!(led.is_unclean(2)); + + // …and picture 3 predicts from it, raising NO warning of its own. + let refs_clean = led.references_clean([2]); + assert!(!refs_clean, "the chain is broken from here down"); + led.note_stored(3, refs_clean, false); + assert!(led.is_unclean(3), "3 inherited 2's damage"); + + // The rot keeps travelling, arbitrarily far from the original loss. + let refs_clean = led.references_clean([3]); + assert!(!refs_clean); + led.note_stored(4, refs_clean, false); + assert!(led.is_unclean(4)); + } + + /// A picture that references BOTH a clean and an unclean predecessor is unclean — + /// one broken reference is enough to make the reconstruction wrong. + #[test] + fn one_unclean_reference_is_enough() { + let mut led = CleanLedger::new(); + led.note_stored(0, true, false); + led.note_stored(1, true, true); // damaged + assert!(!led.references_clean([0, 1])); + assert!(!led.references_clean([1, 0]), "order does not matter"); + assert!(led.references_clean([0])); + } + + /// An IDR predicts from nothing, so it is clean however broken the stream was + /// before it. This is the property that lets a real keyframe end a damaged run. + #[test] + fn a_picture_with_no_references_is_clean_however_bad_the_stream_was() { + let mut led = CleanLedger::new(); + led.note_stored(0, true, true); + led.note_stored(1, false, false); + assert_eq!(led.unclean_count(), 2); + // The IDR: an empty reference list is vacuously clean. + assert!(led.references_clean([])); + led.note_stored(2, true, false); + assert!(!led.is_unclean(2)); + } + + /// Marks are bounded by DPB residency: a picture that left the DPB can never be + /// referenced again, so keeping its mark would only grow the set forever. + #[test] + fn marks_are_dropped_when_their_picture_leaves_the_dpb() { + let mut led = CleanLedger::new(); + led.note_stored(7, true, true); + led.note_stored(8, false, false); + assert_eq!(led.unclean_count(), 2); + led.retain_live([8, 9]); + assert!(!led.is_unclean(7), "7 was evicted"); + assert!(led.is_unclean(8), "8 is still resident and still damaged"); + assert_eq!(led.unclean_count(), 1); + } + + /// A flush drains the whole DPB, so no mark describes anything resident. + #[test] + fn a_flush_forgets_every_mark() { + let mut led = CleanLedger::new(); + led.note_stored(1, true, true); + led.note_stored(2, false, false); + led.clear(); + assert_eq!(led.unclean_count(), 0); + assert!(led.references_clean([1, 2])); + } + + /// Planners hand out ids from a counter the flush path can rewind, so an id CAN be + /// reused. A stale mark must not condemn the fresh picture that inherits the id. + #[test] + fn a_reused_id_is_not_condemned_by_its_predecessors_mark() { + let mut led = CleanLedger::new(); + led.note_stored(5, true, true); + assert!(led.is_unclean(5)); + // The same id, planned cleanly this time. + led.note_stored(5, true, false); + assert!(!led.is_unclean(5)); + assert!(led.references_clean([5])); + } + + /// A healthy stream never marks anything, forever — the property that makes this + /// free to carry on every session that is working correctly. + #[test] + fn a_stream_without_loss_never_marks_a_picture() { + let mut led = CleanLedger::new(); + for id in 0..512u64 { + let refs = if id == 0 { vec![] } else { vec![id - 1] }; + let clean = led.references_clean(refs.iter().copied()); + assert!(clean, "picture {id} must read clean"); + led.note_stored(id, clean, false); + led.retain_live(id.saturating_sub(3)..=id); + } + assert_eq!(led.unclean_count(), 0); + } +} diff --git a/crates/pf-bitstream/src/h264.rs b/crates/pf-bitstream/src/h264.rs index 146555c9..646865de 100644 --- a/crates/pf-bitstream/src/h264.rs +++ b/crates/pf-bitstream/src/h264.rs @@ -149,6 +149,18 @@ pub struct PicturePlan { /// DPB size in frames per A.3.1 — backends size their slot pool from this. pub max_dpb_frames: usize, pub recovery_point: Option, + /// Every picture this AU predicts from was itself decoded from a fully-available + /// reference chain — so a host claim that this AU is a clean re-anchor + /// (`USER_FLAG_RECOVERY_ANCHOR`) can be corroborated rather than taken on trust. + /// `true` for an IDR (nothing to predict from) and for any picture whose whole + /// reference chain is clean; `false` from the moment this AU — or anything it + /// descends from — needed concealment. + /// + /// Purely additive observation: nothing in the plan, the warnings or the DPB + /// changes because of it, and on a stream that never loses a reference it is + /// `true` on every picture forever. See [`crate::clean`] for why it propagates and + /// why every rule errs toward `false`. + pub references_clean: bool, } /// The region of the coded picture that is actually displayed. @@ -252,6 +264,41 @@ pub enum PlanWarning { }, } +impl PlanWarning { + /// Does this warning mean the PICTURE is damaged — the plan was completed with a + /// SUBSTITUTE in place of something that was lost — rather than reporting a + /// spec-legal fact about the stream's envelope? + /// + /// The distinction decides two things that must never disagree: whether a consumer + /// releases the AU's output unshown and asks for a re-anchor, and whether the + /// picture enters [`crate::clean::CleanLedger`] as unclean. It lives HERE, on the + /// enum, because those two consumers sit in different crates and a second copy of + /// the list would let one of them conceal damage the other reports — the exact + /// shape of the invisible-corruption failure the native-decode program exists to + /// end. `pf_vkdecode::is_integrity_warning` delegates to this. + /// + /// `Mmco5Rebase` is not damage: the AU carried an MMCO 5 and this planner planned + /// it in full (the plan holds the pre-rebase 8.2.1 values; later AUs reference the + /// rebased ones). `LevelDerivedDpb` is not either: the picture is intact and fully + /// planned — it reports that the SPS never declared its DPB depth, so the plan had + /// to size from A.3.1's level ceiling, a property of the STREAM's signalling which + /// a backend answers by failing to open a session, not by showing a damaged frame. + /// + /// Written as an EXHAUSTIVE match with no wildcard, deliberately. A `matches!` (or + /// a `_ => false`) makes "damage" the opt-in and silence the default, so a variant + /// added later — by definition one nobody here has classified — would be reported + /// as clean and its picture shown. The compiler is the only reviewer guaranteed to + /// be present when that variant is written, so it gets the decision. + pub fn is_integrity(&self) -> bool { + match self { + PlanWarning::FrameNumGap { .. } + | PlanWarning::MissingReference { .. } + | PlanWarning::TruncatedAu { .. } => true, + PlanWarning::Mmco5Rebase | PlanWarning::LevelDerivedDpb { .. } => false, + } + } +} + /// The AU cannot be planned at all. #[derive(Debug, Clone, PartialEq, Eq)] pub enum PlanError { @@ -482,6 +529,10 @@ pub struct H264Planner { reported_live: BTreeSet, /// Set by [`Self::flush`]: planning resumes only at an IDR (upstream: `Reset`). awaiting_idr: bool, + /// Which resident pictures came off a BROKEN reference chain — the fact behind + /// [`PicturePlan::references_clean`]. Empty on a healthy stream; see + /// [`crate::clean::CleanLedger`] for the propagation rules. + clean: crate::clean::CleanLedger, } impl H264Planner { @@ -600,9 +651,20 @@ impl H264Planner { let cur = current .ok_or_else(|| PlanError::Parse("access unit contains no coded picture".into()))?; + // Was every picture this AU predicts from decoded off an intact chain? Asked + // over the SLICE reference lists rather than the DPB snapshot, because those + // are what this picture actually predicts from — a resident-but-unreferenced + // damaged picture says nothing about this one. An IDR references nothing, so + // this is vacuously true for it (`CleanLedger::references_clean`). + let references_clean = self.clean.references_clean( + slices + .iter() + .flat_map(|s: &SlicePlan| s.ref_list0.iter().chain(&s.ref_list1)) + .map(|r| r.id), + ); // Captured before finish_picture: MMCO5 rewrites the stored POC afterwards, but // backends submit the picture with its 8.2.1 values. - let picture = Self::picture_plan(&cur, recovery_point); + let picture = Self::picture_plan(&cur, recovery_point, references_clean); // The activated parameter sets ride out with the plan (AuPlan field docs); // cloned before finish_picture consumes `cur`. let pps = Rc::clone(&cur.first_slice_pps); @@ -619,6 +681,20 @@ impl H264Planner { let removed = previously_live.difference(&live_after).copied().collect(); self.reported_live = live_after; + // Fold this picture's verdict, then bound the ledger to DPB residency. Both + // AFTER `finish_picture`, so `stored` is the id the picture really got and + // `live_after` reflects the marking this AU performed — a mark written against + // a pre-marking view could survive an eviction it should have died with. + // `concealed` mirrors what a consumer conceals on, via the ONE classification + // (`PlanWarning::is_integrity`), so the ledger and the consumer can never + // disagree about whether this AU was damaged. + self.clean.note_stored( + stored, + references_clean, + warnings.iter().any(PlanWarning::is_integrity), + ); + self.clean.retain_live(self.reported_live.iter().copied()); + Ok(AuPlan { picture, slices, @@ -650,6 +726,9 @@ impl H264Planner { self.max_long_term_frame_idx = Default::default(); self.negotiation_info = Default::default(); self.awaiting_idr = true; + // The DPB is drained, so no mark describes a resident picture any more — and + // planning resumes at an IDR, which is clean by construction. + self.clean.clear(); DpbUpdate { stored: None, @@ -1747,7 +1826,11 @@ impl H264Planner { Ok(id) } - fn picture_plan(cur: &CurrentPicState, recovery_point: Option) -> PicturePlan { + fn picture_plan( + cur: &CurrentPicState, + recovery_point: Option, + references_clean: bool, + ) -> PicturePlan { let pic = &cur.pic; // The first slice's PPS defines the picture's parameters (upstream's // start_picture semantics); `cur.pps` may have drifted to a later slice's. @@ -1791,6 +1874,7 @@ impl H264Planner { chroma_format_idc: sps.chroma_format_idc, max_dpb_frames: dpb_limit(sps), recovery_point, + references_clean, } } } @@ -2343,6 +2427,110 @@ mod tests { assert!(missing_seen); } + /// The clean bit, end to end through the real planner: a `frame_num` gap + /// concealed one picture, and EVERY picture descending from it reports + /// `references_clean == false` even though their own plans are spotless. That + /// propagation is the whole point — the concealed picture is rarely the one a host + /// recovery anchor names; the ordinary P-frames after it are. + #[test] + fn a_concealed_picture_makes_every_descendant_report_unclean_references() { + let (sps, pps) = authored_sps_pps(); + let mut au0 = param_set_au(&sps, &pps); + au0.extend(write_idr_slice()); + + let mut planner = H264Planner::new(); + let p0 = planner.plan_au(&au0).unwrap(); + assert!( + p0.picture.references_clean, + "an IDR references nothing, so it is clean by construction" + ); + + // A healthy P off the IDR: still clean. + let p1 = planner.plan_au(&write_p_slice(1, 2, 1, 1, None)).unwrap(); + assert!(picture_warnings(&p1).is_empty()); + assert!(p1.picture.references_clean); + + // frame_num 2 never arrives — 8.2.5.2 fabricates a placeholder and the plan + // conceals. THIS picture's references were still intact; the damage is its own. + let p3 = planner.plan_au(&write_p_slice(3, 6, 1, 3, None)).unwrap(); + assert!(p3 + .warnings + .iter() + .any(|w| matches!(w, PlanWarning::FrameNumGap { .. }))); + + // …and every picture after it inherits the damage with a clean plan of its own. + let p4 = planner.plan_au(&write_p_slice(4, 8, 1, 1, None)).unwrap(); + assert!( + picture_warnings(&p4).is_empty(), + "p4's own plan raises nothing — which is exactly why the bit is needed" + ); + assert!( + !p4.picture.references_clean, + "p4 predicts from the concealed chain, so it must not read as clean" + ); + + let p5 = planner.plan_au(&write_p_slice(5, 10, 1, 1, None)).unwrap(); + assert!(picture_warnings(&p5).is_empty()); + assert!(!p5.picture.references_clean, "the rot keeps travelling"); + } + + /// An IDR ends a damaged run: it predicts from nothing, so it reads clean however + /// broken the stream was before it. Without this a session could never recover a + /// trustworthy anchor. + #[test] + fn an_idr_reports_clean_references_however_damaged_the_run_before_it() { + let (sps, pps) = authored_sps_pps(); + let mut au0 = param_set_au(&sps, &pps); + au0.extend(write_idr_slice()); + + let mut planner = H264Planner::new(); + planner.plan_au(&au0).unwrap(); + planner.plan_au(&write_p_slice(1, 2, 1, 1, None)).unwrap(); + // Gap: frame_num 2 lost. + let p3 = planner.plan_au(&write_p_slice(3, 6, 1, 3, None)).unwrap(); + assert!(p3 + .warnings + .iter() + .any(|w| matches!(w, PlanWarning::FrameNumGap { .. }))); + let p4 = planner.plan_au(&write_p_slice(4, 8, 1, 1, None)).unwrap(); + assert!(!p4.picture.references_clean); + + // A fresh IDR re-anchors, and the pictures after it are clean again. + let mut idr = param_set_au(&sps, &pps); + idr.extend(write_idr_slice()); + let p5 = planner.plan_au(&idr).unwrap(); + assert!(p5.picture.references_clean, "an IDR is always clean"); + let p6 = planner.plan_au(&write_p_slice(1, 2, 1, 1, None)).unwrap(); + assert!( + p6.picture.references_clean, + "the damaged chain died with the IDR's DPB flush" + ); + } + + /// A stream that never loses a reference reports `references_clean` on every + /// picture, forever — the property that makes this free to carry in production. + #[test] + fn a_healthy_stream_reports_clean_references_on_every_picture() { + let (sps, pps) = authored_sps_pps(); + let mut au0 = param_set_au(&sps, &pps); + au0.extend(write_idr_slice()); + + let mut planner = H264Planner::new(); + assert!(planner.plan_au(&au0).unwrap().picture.references_clean); + // log2_max_frame_num_minus4 = 0 and pic_order_cnt_lsb is u(4): both wrap at 16. + for n in 1..16u32 { + let plan = planner + .plan_au(&write_p_slice(n, (n * 2) % 16, 1, 1, None)) + .unwrap(); + assert!( + picture_warnings(&plan).is_empty(), + "frame {n} should plan cleanly: {:?}", + picture_warnings(&plan) + ); + assert!(plan.picture.references_clean, "frame {n} must read clean"); + } + } + #[test] fn a_gap_placeholder_inside_a_ref_list_is_substituted_in_place_not_compacted() { let (sps, pps) = authored_sps_pps(); diff --git a/crates/pf-bitstream/src/h265.rs b/crates/pf-bitstream/src/h265.rs index 49774d13..68da10ca 100644 --- a/crates/pf-bitstream/src/h265.rs +++ b/crates/pf-bitstream/src/h265.rs @@ -166,6 +166,18 @@ pub struct PicturePlan { /// came from the SPS by index) — Vulkan's `NumBitsForSTRefPicSetInSlice`. pub short_term_ref_pic_set_size_bits: u32, pub recovery_point: Option, + /// Every picture this AU predicts from was itself decoded from a fully-available + /// reference chain — so a host claim that this AU is a clean re-anchor + /// (`USER_FLAG_RECOVERY_ANCHOR`) can be corroborated rather than taken on trust. + /// `true` for an IRAP (nothing to predict from) and for any picture whose whole + /// reference chain is clean; `false` from the moment this AU — or anything it + /// descends from — needed concealment. + /// + /// Purely additive observation: nothing in the plan, the warnings or the DPB + /// changes because of it, and on a stream that never loses a reference it is + /// `true` on every picture forever. See [`crate::clean`] for why it propagates and + /// why every rule errs toward `false`. + pub references_clean: bool, } /// A reference list / RPS entry: the minimum every backend picparams format needs. @@ -232,6 +244,28 @@ pub enum PlanWarning { NonZeroReorder { max_num_reorder_pics: u8 }, } +impl PlanWarning { + /// Does this warning mean the PICTURE is damaged? The H.265 twin of + /// [`crate::h264::PlanWarning::is_integrity`] — the same one-list argument applies, + /// and `pf_vkdecode::is_integrity_warning_h265` delegates here. + /// + /// `NonZeroReorder` is NOT damage, and excluding it matters more here than the + /// H.264 exclusions do: it fires on the AU that ACTIVATES an SPS — the opening + /// IRAP, and the fresh IRAP at every ABR resolution change — so treating it as + /// concealment would cost a released-unshown frame plus a keyframe round trip at + /// every renegotiation, on a stream the planner says it planned correctly. It + /// would also poison the [`crate::clean::CleanLedger`] at exactly those IRAPs, + /// marking the one picture that is clean by construction as broken. + /// + /// Exhaustive with no wildcard, for the reason the H.264 twin spells out. + pub fn is_integrity(&self) -> bool { + match self { + PlanWarning::MissingReference { .. } | PlanWarning::TruncatedAu { .. } => true, + PlanWarning::NonZeroReorder { .. } => false, + } + } +} + /// The AU cannot be planned at all. #[derive(Debug, Clone, PartialEq, Eq)] pub enum PlanError { @@ -452,6 +486,10 @@ pub struct H265Planner { reported_live: BTreeSet, /// Set by [`Self::flush`]: planning resumes only at an IRAP (upstream: `Reset`). awaiting_idr: bool, + /// Which resident pictures came off a BROKEN reference chain — the fact behind + /// [`PicturePlan::references_clean`]. Empty on a healthy stream; see + /// [`crate::clean::CleanLedger`] for the propagation rules. + clean: crate::clean::CleanLedger, } impl Default for H265Planner { @@ -471,6 +509,7 @@ impl Default for H265Planner { pending_outputs: Vec::new(), reported_live: BTreeSet::new(), awaiting_idr: false, + clean: Default::default(), } } } @@ -690,7 +729,20 @@ impl H265Planner { let cur = current .ok_or_else(|| PlanError::Parse("access unit contains no coded picture".into()))?; - let picture = Self::picture_plan(&cur, recovery_point); + // Was every picture this AU predicts from decoded off an intact chain? Asked + // over the SLICE reference lists rather than the RPS or the DPB snapshot, + // because those are what this picture actually predicts from: 8.3.2 RETAINS + // pictures in the RPS that the current picture does not use + // (`used_by_curr_pic` clear), and a damaged one among those says nothing about + // this picture. An IRAP's lists are empty, so this is vacuously true for it + // (`CleanLedger::references_clean`). + let references_clean = self.clean.references_clean( + slices + .iter() + .flat_map(|s: &SlicePlan| s.ref_list0.iter().chain(&s.ref_list1)) + .map(|r| r.id), + ); + let picture = Self::picture_plan(&cur, recovery_point, references_clean); let rps = cur.rps_plan.clone(); let dpb_refs = cur.dpb_refs.clone(); // The activated parameter sets ride out with the plan (AuPlan field docs); @@ -708,6 +760,20 @@ impl H265Planner { let removed = previously_live.difference(&live_after).copied().collect(); self.reported_live = live_after; + // Fold this picture's verdict, then bound the ledger to DPB residency. Both + // AFTER `finish_picture`, so `stored` is the id the picture really got and + // `live_after` reflects the C.3.4/8.3.2 marking this AU performed — a mark + // written against a pre-marking view could survive an eviction it should have + // died with. `concealed` mirrors what a consumer conceals on, via the ONE + // classification (`PlanWarning::is_integrity`), so the ledger and the consumer + // can never disagree about whether this AU was damaged. + self.clean.note_stored( + stored, + references_clean, + warnings.iter().any(PlanWarning::is_integrity), + ); + self.clean.retain_live(self.reported_live.iter().copied()); + Ok(AuPlan { picture, rps, @@ -745,6 +811,9 @@ impl H265Planner { // re-entry sound. self.first_picture_after_eos = true; self.awaiting_idr = true; + // The DPB is drained, so no mark describes a resident picture any more — and + // planning resumes at an IRAP, which is clean by construction. + self.clean.clear(); DpbUpdate { stored: None, @@ -1452,6 +1521,7 @@ impl H265Planner { fn picture_plan( cur: &CurrentPicState, recovery_point: Option, + references_clean: bool, ) -> PicturePlan { let pic = &cur.pic; // The first slice's PPS defines the picture's parameters; `cur.pps` may have @@ -1498,6 +1568,7 @@ impl H265Planner { max_dpb_frames: dpb_limit(sps), short_term_ref_pic_set_size_bits: pic.short_term_ref_pic_set_size_bits, recovery_point, + references_clean, } } } @@ -1550,11 +1621,15 @@ mod tests { /// `NonZeroReorder` is excluded: the vendored conformance clips are general /// (reordering) encodes, and the planner deliberately plans them while flagging /// the envelope fact. + /// + /// Delegates rather than restating the list. This harness exists to prove the + /// planner conceals exactly where production conceals, so a second copy here + /// could drift and quietly prove the wrong thing — and a `matches!` in + /// particular reads any FUTURE variant as clean, which is the one answer a + /// damage predicate must never default to. [`PlanWarning::is_integrity`] is an + /// exhaustive match, so a new variant stops the compiler there instead. fn is_integrity_warning(w: &PlanWarning) -> bool { - matches!( - w, - PlanWarning::MissingReference { .. } | PlanWarning::TruncatedAu { .. } - ) + w.is_integrity() } /// Plan a whole vendored clip and assert the global invariants: every AU plans, @@ -1658,6 +1733,30 @@ mod tests { assert!(!bbb.is_empty()); } + /// The false-positive guard for [`PicturePlan::references_clean`], on REAL + /// bitstreams rather than authored ones: two conformance clips that lose nothing + /// must report every single picture clean. A regression that let the ledger mark a + /// healthy stream would refuse every host recovery anchor and force an IDR on + /// every loss — the cheap re-anchor path gone, silently. + /// + /// These clips carry B-slices and real reordering, so they also exercise the + /// "reference lists, not the RPS" reading: 8.3.2 retains pictures the current + /// picture does not use, and folding those in would condemn pictures at random. + #[test] + fn a_lossless_conformance_clip_reports_clean_references_on_every_picture() { + for (name, clip) in [("bear", TEST_BEAR), ("bbb", TEST_BBB)] { + let (_, plans) = plan_whole_clip(clip); + assert!(!plans.is_empty(), "{name} produced no plans"); + for (i, plan) in plans.iter().enumerate() { + assert!( + plan.picture.references_clean, + "{name} picture {i} (poc {}) must read clean on a lossless clip", + plan.picture.pic_order_cnt + ); + } + } + } + #[test] fn b_slices_get_a_future_led_list1_distinct_from_list0() { let aus = split_into_aus(TEST_64X64_I_P_B_P); diff --git a/crates/pf-bitstream/src/lib.rs b/crates/pf-bitstream/src/lib.rs index ee6936c3..12f37b52 100644 --- a/crates/pf-bitstream/src/lib.rs +++ b/crates/pf-bitstream/src/lib.rs @@ -21,6 +21,7 @@ #![forbid(unsafe_code)] pub mod av1; +pub mod clean; pub mod h264; pub mod h265; pub mod sei; diff --git a/crates/pf-client-core/src/pad_audio.rs b/crates/pf-client-core/src/pad_audio.rs index d1b08d02..bb175797 100644 --- a/crates/pf-client-core/src/pad_audio.rs +++ b/crates/pf-client-core/src/pad_audio.rs @@ -365,14 +365,10 @@ pub(crate) fn pick_pad_sink(sinks: &[SinkNode], cards: &[CardDevice]) -> Option< /// Read a sink node's facts out of a proplist. Split out because it has to run against the /// node's INFO props, not the registry's — see [`walk_graph`]. /// -/// Linux ONLY, unlike its neighbours in this module. They carry `cfg(any(target_os = "linux", -/// test))` so their pure-logic halves stay testable on any dev box, but this one takes a -/// `pipewire::` type in its signature and `pipewire` is a Linux-only dependency — so a bare -/// `test` arm compiles it in test configuration on EVERY platform and fails to resolve on -/// Windows. It builds green in release (the fn is not reachable there) and only breaks under -/// `clippy --all-targets`, which is why it reached main: the Windows client's release build -/// passes and its clippy lane is the one that goes red. Its sole caller, [`walk_graph`], is -/// already Linux-only, and no test calls it. +/// Linux-only, unlike its pure-logic neighbours: `DictRef` comes from `pipewire`, which is a +/// `cfg(target_os = "linux")` dependency. Widening this to `any(…, test)` the way the testable +/// helpers around it do puts the item into the Windows `lib test` target, where the crate does +/// not exist — E0433, visible only under `--all-targets`, and so only on the Windows CI leg. #[cfg(target_os = "linux")] pub(crate) fn sink_from_props(props: &pipewire::spa::utils::dict::DictRef) -> Option { // Both spellings: PipeWire's own objects use the `device.`-prefixed keys, the pulse-facing diff --git a/crates/pf-client-core/src/session.rs b/crates/pf-client-core/src/session.rs index 24f758f8..cd86bacc 100644 --- a/crates/pf-client-core/src/session.rs +++ b/crates/pf-client-core/src/session.rs @@ -1430,9 +1430,38 @@ fn pump( // `image.is_keyframe()` as the decoder's own IDR belt, applies the two-mark // rule + the mark-patience backstop, clears the no-output streak, and returns // whether to present this frame or withhold it as a post-loss concealment. - let present = - gate.on_decoded(frame.flags, image.is_keyframe(), Instant::now()) - == GateVerdict::Present; + // + // CORROBORATED (the grey-frame fix): the wire's RECOVERY_ANCHOR is the host + // asserting something about THIS decoder — "the picture I coded this + // P-frame against is one you still hold, intact" — and it lifts the freeze + // on the FIRST occurrence, no two-mark wait. The host derives that from + // bookkeeping that tracks what the client RECEIVED, not what it managed to + // DECODE, and when those diverge the anchor lifts the freeze onto a + // concealed picture and LEAVES it lifted: grey with motion painted on it + // until some later signal re-arms and the 500 ms backstop extracts a real + // IDR. A rung that planned the AU itself knows better, so it says so here. + // + // What a refusal costs is exactly one thing: the freeze keeps holding the + // last good picture until the backstop fires on its ORIGINAL deadline and + // forces the IDR the anchor failed to be. That is strictly the better half + // of the trade — the alternative is presenting a picture this client can + // prove is damaged — and it is the same direction every rule in the gate + // errs in. Every non-native lane reports `Unavailable` and is untouched. + let evidence = image.anchor_evidence(); + if evidence == punktfunk_core::reanchor::AnchorEvidence::ReferencesDamaged + && frame.flags & punktfunk_core::packet::USER_FLAG_RECOVERY_ANCHOR != 0 + { + tracing::debug!( + "refused a host recovery anchor: this AU predicts from a picture \ + this decoder had to conceal — holding for a real IDR" + ); + } + let present = gate.on_decoded_corroborated( + frame.flags, + image.is_keyframe(), + evidence, + Instant::now(), + ) == GateVerdict::Present; total_frames += 1; // ⚠ The `stats:` decode-path tag is a machine interface — // additive only. M10 removed the rungs whose tags were `vaapi`, diff --git a/crates/pf-client-core/src/video.rs b/crates/pf-client-core/src/video.rs index b95dc96b..9a4016b2 100644 --- a/crates/pf-client-core/src/video.rs +++ b/crates/pf-client-core/src/video.rs @@ -493,6 +493,17 @@ pub struct NativeVkFrame { /// the host, and it cannot be lost separately from the picture. Fed to /// [`ReanchorGate::on_local_recovery`](punktfunk_core::reanchor::ReanchorGate::on_local_recovery). pub recovery: punktfunk_core::reanchor::LocalRecovery, + /// Every picture this AU predicts from was itself decoded from a fully-available + /// reference chain (pf-vkdecode's `DecodedVkFrame::references_clean`). + /// + /// The corroboration for the host's `USER_FLAG_RECOVERY_ANCHOR`, which is a claim + /// about THIS decoder that only this decoder can check. The host derives its + /// anchor from slot bookkeeping that tracks what the client RECEIVED; this tracks + /// what the client managed to DECODE. When they disagree the anchor lifts the + /// post-loss freeze onto a concealed picture and leaves it lifted, which is the + /// grey-with-motion field report. `true` on every ordinary frame of a healthy + /// stream, so the flag is only ever load-bearing on the AU that carries an anchor. + pub references_clean: bool, /// This picture's position in DECODE order (pf-vkdecode's strictly increasing /// per-session ordinal). Delivery order is not decode order: after a failed AU /// the H.265 decoder flushes its DPB, handing back every buffered picture at @@ -559,6 +570,40 @@ impl DecodedImage { } } + /// What this lane can say about the host's re-anchor claim on this frame — the + /// corroboration for `USER_FLAG_RECOVERY_ANCHOR`, fed to + /// [`ReanchorGate::on_decoded_corroborated`](punktfunk_core::reanchor::ReanchorGate::on_decoded_corroborated). + /// + /// An anchor is the host asserting a fact about THIS decoder — *the picture I + /// coded this P-frame against is one you still hold, intact* — and the gate lifts + /// its post-loss freeze on the first one, no two-mark wait. Only a rung that + /// planned the AU itself knows which pictures it predicts from and whether each of + /// those decoded cleanly, so only such a rung can catch the host being wrong. + /// + /// The native Vulkan rung answers; everyone else reports + /// [`AnchorEvidence::Unavailable`](punktfunk_core::reanchor::AnchorEvidence::Unavailable) + /// and the gate treats them exactly as it did before this existed — silence is not + /// refutation, so no lane becomes stricter by accident. + /// + /// ⚠ The CPU rung's H.264 leg plans every AU with the same `H264Planner` and so + /// COULD answer; it does not yet, because its frame type carries no equivalent of + /// [`NativeVkFrame::references_clean`]. Reporting `Unavailable` there is the + /// conservative reading (today's behaviour), not a claim that its references are + /// fine. + pub fn anchor_evidence(&self) -> punktfunk_core::reanchor::AnchorEvidence { + use punktfunk_core::reanchor::AnchorEvidence; + match self { + DecodedImage::NativeVk(f) => { + if f.references_clean { + AnchorEvidence::ReferencesClean + } else { + AnchorEvidence::ReferencesDamaged + } + } + _ => AnchorEvidence::Unavailable, + } + } + /// This frame's position in DECODE order, where the lane knows one — see /// [`NativeVkFrame::decode_order`]. `None` everywhere else, which is what the /// pump reads as "this lane reports no local recovery either, so there is diff --git a/crates/pf-client-core/src/video_vaapi_native.rs b/crates/pf-client-core/src/video_vaapi_native.rs index 51130618..0d876cd9 100644 --- a/crates/pf-client-core/src/video_vaapi_native.rs +++ b/crates/pf-client-core/src/video_vaapi_native.rs @@ -3060,6 +3060,11 @@ mod tests { picture: pf_vaadec::PicturePlanAv1 { frame_type: pf_vaadec::FrameTypeAv1::KeyFrame, is_key: true, + // Vacuously true for a key frame: it predicts from nothing. This fixture + // exists to exercise the SIZING path (sequence max vs coded vs render), so + // the clean bit is incidental here — but it must state the honest value, + // because `false` is the answer that withholds a re-anchor. + references_clean: true, show_frame: true, showable_frame: false, order_hint: 0, diff --git a/crates/pf-client-core/src/video_vk_native.rs b/crates/pf-client-core/src/video_vk_native.rs index d6a5598d..5ab54bce 100644 --- a/crates/pf-client-core/src/video_vk_native.rs +++ b/crates/pf-client-core/src/video_vk_native.rs @@ -741,6 +741,12 @@ fn project_frame(frame: &DecodedVkFrame, guard: NativeReleaseGuard) -> NativeVkF sei_here: frame.recovery.sei_here, is_recovery_point: frame.recovery.is_recovery_point, }, + // Whether this picture's own references decoded cleanly — the corroboration + // the shared gate weighs a host `USER_FLAG_RECOVERY_ANCHOR` against. The + // planner already knows it (it is the one party that resolved this AU's + // reference lists), and it is the only thing that can catch the host + // asserting a re-anchor over a picture THIS decoder had to conceal. + references_clean: frame.references_clean, // Which side of a loss this picture was DECODED on. Carried beside the // recovery mark because the mark is worthless without it: a post-failure // DPB flush delivers pre-loss pictures after the loss, and their marks @@ -1690,6 +1696,11 @@ mod tests { sei_here: true, is_recovery_point: true, }, + // SET, per this fixture's no-boolean-is-false rule — and it earns the + // rule: a projection that dropped this would default it to `false`, + // which reads as "this picture's references were concealed" and would + // make the gate refuse EVERY host recovery anchor on a healthy stream. + references_clean: true, // Distinct from every other number here for the same reason: a // projection that dropped the decode ordinal would make every frame // look pre-loss (0) and silently disable the local-recovery path. @@ -1792,6 +1803,7 @@ mod tests { keyframe, poc, recovery, + references_clean, decode_order, guard: _, } = p; @@ -1838,6 +1850,12 @@ mod tests { "the recovery point SEI's verdict reaches the gate — it is the ONLY \ clean point an intra-refresh session has" ); + assert!( + references_clean, + "the reference-cleanliness verdict rides along — without it the gate \ + cannot refute a host recovery anchor that names a picture this decoder \ + had to conceal, which is the grey-with-motion field report" + ); assert_eq!( decode_order, 17, "the decode ordinal rides along — without it the pump cannot tell a \ @@ -2150,6 +2168,7 @@ mod tests { keyframe: true, poc: 0, recovery: punktfunk_core::reanchor::LocalRecovery::NONE, + references_clean: true, decode_order: 1, guard: NativeReleaseGuard::new( tx, diff --git a/crates/pf-console-ui/src/screens.rs b/crates/pf-console-ui/src/screens.rs index ab744cfa..331a0be3 100644 --- a/crates/pf-console-ui/src/screens.rs +++ b/crates/pf-console-ui/src/screens.rs @@ -103,23 +103,34 @@ impl Outbox { } } -/// This row's `punktfunk://` link, built from the STORE so it carries the fingerprint and -/// stable id a row doesn't hold — the same builder the desktop shells' "Copy link" uses, -/// so a link is identical whichever surface hands it to you. `None` if the host has left +/// A saved host's `punktfunk://` link, built from the STORE so it carries the fingerprint +/// and stable id a screen doesn't hold — the same builder the desktop shells' "Copy link" +/// uses, so a link is identical whichever surface hands it to you. `launch` attaches a +/// library id, which is what makes a game's link a game's link. `None` if the host has left /// the store since the menu was opened. -pub(crate) fn host_link(row: &HostRow) -> Option { +pub(crate) fn saved_host_link( + fp_hex: &str, + addr: &str, + port: u16, + profile: Option<&str>, + launch: Option<&str>, +) -> Option { let known = trust::KnownHosts::load(); - let host = (!row.fp_hex.is_empty()) - .then(|| known.find_by_fp(&row.fp_hex)) + let host = (!fp_hex.is_empty()) + .then(|| known.find_by_fp(fp_hex)) .flatten() - .or_else(|| known.find_by_addr(&row.addr, row.port))?; - Some( - pf_client_core::deeplink::DeepLink::for_host( - host, - None, - row.pin.as_ref().map(|p| p.id.as_str()), - ) - .to_url(), + .or_else(|| known.find_by_addr(addr, port))?; + Some(pf_client_core::deeplink::DeepLink::for_host(host, launch, profile).to_url()) +} + +/// This row's link — the host itself, with a pinned card's profile when the row is one. +pub(crate) fn host_link(row: &HostRow) -> Option { + saved_host_link( + &row.fp_hex, + &row.addr, + row.port, + row.pin.as_ref().map(|p| p.id.as_str()), + None, ) } diff --git a/crates/pf-console-ui/src/screens/library.rs b/crates/pf-console-ui/src/screens/library.rs index 91f005b0..16dfc5dc 100644 --- a/crates/pf-console-ui/src/screens/library.rs +++ b/crates/pf-console-ui/src/screens/library.rs @@ -75,6 +75,18 @@ impl LibraryScreen { } } + /// One title's self-emitted link: this shelf's host, this shelf's pinned profile (so a + /// link taken off a pinned card's shelf streams the way that card does), and the game. + fn game_link(&self, id: &str) -> Option { + crate::screens::saved_host_link( + &self.fp_hex, + &self.addr, + self.port, + self.pin.as_ref().map(|p| p.id.as_str()), + Some(id), + ) + } + fn fetch_cmd(&self) -> ConsoleCmd { ConsoleCmd::FetchLibrary { addr: self.addr.clone(), @@ -149,11 +161,29 @@ impl LibraryScreen { }); Some(MenuPulse::Confirm) } + // X copies the focused title's own `punktfunk://` link — the same + // self-emitted URL a host tile's "Copy link" hands out, plus this game's + // `launch=` id, so pasting it into Playnite or a Stream Deck macro boots + // straight into the title. Direct rather than behind an options screen: + // it is the only per-game action there is, and a menu holding one row is + // a press the user pays for nothing. + MenuEvent::Tertiary => { + let g = self.games.get(self.cursor as usize)?; + match self.game_link(&g.id) { + Some(url) => { + fx.copy = Some(url); + fx.toast = Some("Link copied".into()); + } + // Only if the host left the store while the shelf was open. + None => fx.toast = Some("This host isn't saved any more".into()), + } + Some(MenuPulse::Confirm) + } MenuEvent::Back => { fx.pop(); None } - MenuEvent::Move(_) | MenuEvent::Secondary | MenuEvent::Tertiary => None, + MenuEvent::Move(_) | MenuEvent::Secondary => None, }, LibraryPhase::Error { can_retry, .. } => match ev { MenuEvent::Confirm if *can_retry => { @@ -254,6 +284,7 @@ impl LibraryScreen { "Play" }, ), + Hint::new(HintKey::Tertiary, "Copy link"), Hint::new(HintKey::Shoulders, "Jump"), Hint::new(HintKey::Back, "Back"), ], diff --git a/crates/pf-encode/src/enc/codec.rs b/crates/pf-encode/src/enc/codec.rs index 61188a3f..5ee3fbc2 100644 --- a/crates/pf-encode/src/enc/codec.rs +++ b/crates/pf-encode/src/enc/codec.rs @@ -368,6 +368,32 @@ pub trait Encoder: Send { fn invalidate_ref_frames(&mut self, _first_frame: i64, _last_frame: i64) -> bool { false } + /// Mark every resident reference UNTRUSTED FOR RFI ANCHORING — the answer to "the client told + /// us it has damage and we did NOT repair it". + /// + /// Why this exists at all. The slot-family RFI trust domain is the WIRE index each reference + /// holds, which answers *did the client receive this frame*; what an anchor pick actually needs + /// is *did the client DECODE it intact*. [`super::rfi`]'s taint sweep bridges that gap, but it + /// only runs inside [`invalidate_ref_frames`] — reachable from exactly ONE of the client's five + /// damage signals (the frame-index gap, which carries a loss RANGE). The other four report + /// through [`request_keyframe`](Self::request_keyframe), which carries no range and so cannot + /// sweep anything. That is self-healing while the IDR is actually emitted — an IDR flushes the + /// DPB and rebuilds trust from scratch — but the host coalesces those requests (a keyframe + /// storm is a 20-40× spike that deepens the very loss it recovers), and a coalesced request + /// leaves the client's damage unrepaired AND unrecorded. Those references stay anchor + /// candidates, and the next loss is answered with one of them tagged `recovery_anchor` — the + /// client's *definitive* clean re-anchor signal, which lifts its post-loss freeze on the first + /// occurrence. Grey frames, presented, with the freeze lifted. + /// + /// Distrust is deliberately NOT "unusable": ordinary prediction runs off the backend's own slot + /// INDEX, never the wire domain, so this costs nothing but the next anchor pick — which + /// declines and falls through to the (still coalesced, so still non-storming) keyframe path. + /// It is also self-correcting on all three backends: a slot re-marked with a fresh frame, or an + /// IDR flushing the DPB, restores trust within a few frames. So this can suppress RFI briefly, + /// never permanently. + /// + /// Default: no-op — the backends with no reference bookkeeping have no trust to withdraw. + fn distrust_references(&mut self) {} /// Escalate into a pipelined (two-thread) retrieve mode under sustained GPU contention — the /// encoder analog of the capturer depth escalation: AUs ride ~one loop tick behind (`poll` /// may return `None` while an encode is in flight) in exchange for capture/submit no longer diff --git a/crates/pf-encode/src/enc/linux/vulkan_video.rs b/crates/pf-encode/src/enc/linux/vulkan_video.rs index 36d48ad2..732eb5fe 100644 --- a/crates/pf-encode/src/enc/linux/vulkan_video.rs +++ b/crates/pf-encode/src/enc/linux/vulkan_video.rs @@ -3991,6 +3991,35 @@ impl Encoder for VulkanVideoEncoder { } } + /// Withdraw anchor trust from every resident reference (trait docs carry the why). + /// + /// The mechanism is this backend's half of the split `enc::rfi` documents: blank `slot_wire` + /// ONLY. `slot_poc` MUST keep naming every physically-resident DPB picture — it is what + /// [`build_h265_rps_s0`] retains the RPS from, and an RPS that stops naming a resident lets a + /// conforming decoder mark it "unused for reference" and reclaim it (8.3.2), so a later anchor + /// would reference a picture the client has already dropped. That is its own grey-screen bug, + /// documented on `build_h265_rps_s0`, and it is the exact failure this method exists to + /// prevent — so getting the two domains the wrong way round here would trade one for the other. + /// `slot_wire` is the RFI/loss domain; `slot_poc` is the reference-delta domain. + /// + /// `pending_loss` is deliberately left armed, matching this backend's decline arm: a stale arm + /// is re-resolved at frame-build, where the re-pick now finds nothing trusted and forces the + /// IDR that heals the stream. Clearing it here would ship an untagged plain P instead. + /// + /// Ordinary prediction is untouched — it runs off `prev_slot`, an index, not a wire. + fn distrust_references(&mut self) { + let trusted = self.slot_wire.iter().filter(|&&w| w >= 0).count(); + if trusted == 0 { + return; // already fully distrusted — nothing to log or clear + } + self.slot_wire.iter_mut().for_each(|w| *w = -1); + tracing::debug!( + trusted, + "vulkan-encode: client reported unrepaired damage — withdrawing RFI anchor trust from \ + every resident reference (prediction and the RPS are unaffected)" + ); + } + fn poll(&mut self) -> Result> { // Backpressure-drained frames (already read, oldest) come out first, then the oldest slot // still in flight — both in submission order. BLOCKING, per the depth-1 pump contract diff --git a/crates/pf-encode/src/enc/rfi.rs b/crates/pf-encode/src/enc/rfi.rs index 214a3543..048d24ee 100644 --- a/crates/pf-encode/src/enc/rfi.rs +++ b/crates/pf-encode/src/enc/rfi.rs @@ -175,4 +175,56 @@ mod tests { apply(&mut all, plan.tainted); assert_eq!(pick_anchor(&view(&all), 5), None); } + + /// `Encoder::distrust_references` — the OTHER way trust is withdrawn, and the one that needs no + /// loss range. The host calls it when the client reports damage the host did not repair (a + /// coalesced keyframe request, or an RFI anchor the client kept asking past): the sweep cannot + /// run there because a keyframe request carries no range, so every resident reference is + /// withdrawn wholesale instead. All three backends persist that through their own marker; what + /// the shared policy must guarantee is the consequence — the next pick finds nothing and + /// declines, so the caller keyframes instead of serving an anchor over unrepaired damage. + #[test] + fn distrusting_every_reference_makes_the_next_anchor_pick_decline() { + // A table with plenty of pre-loss candidates: without the withdrawal, wire 7 anchors. + let mut wires = [4i64, 5, 6, 7, -1, -1, -1, -1]; + assert_eq!( + pick_anchor(&view(&wires), 9), + Some((3, 7)), + "precondition: this table would happily anchor" + ); + + // The Vulkan mechanism (blank the wire) stands in for all three: AMF clears its mirror and + // QSV raises `ltr_tainted`, but each is filtered out of the trusted view identically — + // which is exactly what makes one pure policy serve three persistence schemes. + apply(&mut wires, u32::MAX); + assert_eq!( + pick_anchor(&view(&wires), 9), + None, + "every reference withdrawn → no anchor, caller falls through to its keyframe path" + ); + // And it holds for ANY later loss, not just this one — the point of persisting distrust. + assert_eq!(pick_anchor(&view(&wires), 100), None); + } + + /// The withdrawal must be temporary, or one coalesced keyframe request would cost a session its + /// RFI recovery for good and every later loss would ride the 20-40× IDR path. Each backend + /// restores trust the same way it always did — a slot re-marked with a fresh frame (and an IDR + /// flush, which empties the table first) — so a refilled slot anchors again. + #[test] + fn a_re_marked_slot_restores_anchor_trust_after_a_full_withdrawal() { + let mut wires = [4i64, 5, 6, 7, -1, -1, -1, -1]; + apply(&mut wires, u32::MAX); + assert_eq!(pick_anchor(&view(&wires), 20), None); + + // Encoding continues; the ring refills two slots with post-withdrawal frames. Those really + // are clean — the client's damage was repaired by the IDR the withdrawal forced — so they + // are legitimate anchors and the sweep must not keep rejecting them. + wires[0] = 14; + wires[1] = 15; + assert_eq!( + pick_anchor(&view(&wires), 20), + Some((1, 15)), + "a re-marked slot is trusted again — the suppression is a few frames, not the session" + ); + } } diff --git a/crates/pf-encode/src/enc/windows/amf.rs b/crates/pf-encode/src/enc/windows/amf.rs index a11e2c56..879451c8 100644 --- a/crates/pf-encode/src/enc/windows/amf.rs +++ b/crates/pf-encode/src/enc/windows/amf.rs @@ -2010,6 +2010,30 @@ impl Encoder for AmfEncoder { } } + /// Withdraw anchor trust from every live LTR (trait docs carry the why). + /// + /// This backend's mechanism, unchanged from the sweep's: distrust = clear the mirror slot. + /// Dropped slots stay dropped and the marking cadence re-marks a clean frame within ~1/4 s, so + /// the suppression is brief by construction. + /// + /// `pending_force` is cleared with them, matching the decline arm above: an un-consumed force + /// would otherwise point at a slot this call just distrusted, and the next submit would + /// force-reference it anyway — shipping the corruption tagged `recovery_anchor`, which is the + /// whole failure being closed. + fn distrust_references(&mut self) { + let live = self.ltr_slots.iter().filter(|m| m.is_some()).count(); + if live == 0 && self.pending_force.is_none() { + return; + } + self.ltr_slots = [None; NUM_LTR_SLOTS]; + self.pending_force = None; + tracing::debug!( + live, + "AMF LTR-RFI: client reported unrepaired damage — withdrawing anchor trust from every \ + live LTR (the marking cadence re-marks a clean frame within ~1/4 s)" + ); + } + fn caps(&self) -> EncoderCaps { EncoderCaps { // As Windows NVENC: the capturer composites; this backend never reads `frame.cursor`. diff --git a/crates/pf-encode/src/enc/windows/qsv.rs b/crates/pf-encode/src/enc/windows/qsv.rs index d2448dfe..19e9f719 100644 --- a/crates/pf-encode/src/enc/windows/qsv.rs +++ b/crates/pf-encode/src/enc/windows/qsv.rs @@ -1467,6 +1467,39 @@ impl Encoder for QsvEncoder { } } + /// Withdraw anchor trust from every live LTR (trait docs carry the why). + /// + /// This backend's mechanism, unchanged from the sweep's: distrust is the SEPARATE + /// `ltr_tainted` flag, never a cleared mirror slot. `ltr_slots` mirrors the HARDWARE DPB and + /// nulling an entry issues no VPL call, so the frame stays marked long-term in the encoder — + /// and the RejectedRefList built at submit only names `Some` slots, so a cleared mirror would + /// silently SKIP the very entry being distrusted and the recovery frame could still predict + /// from it. Taint keeps the mirror intact and the rejection reachable. + /// + /// The taint lifts itself: an IDR flush and a re-mark both clear it, so this suppresses RFI + /// for a few frames, never for the session. + /// + /// `pending_force` is cleared for the same reason as the decline arm above — an un-consumed + /// force would point at a slot this call just distrusted. + fn distrust_references(&mut self) { + let live = self + .ltr_slots + .iter() + .enumerate() + .filter(|&(slot, m)| m.is_some() && !self.ltr_tainted[slot]) + .count(); + if live == 0 && self.pending_force.is_none() { + return; + } + self.ltr_tainted = [true; NUM_LTR_SLOTS]; + self.pending_force = None; + tracing::debug!( + live, + "QSV LTR-RFI: client reported unrepaired damage — withdrawing anchor trust from every \ + live LTR (cleared by the next re-mark or IDR flush)" + ); + } + fn caps(&self) -> EncoderCaps { EncoderCaps { // As Windows NVENC: the capturer composites; this backend never reads `frame.cursor`. diff --git a/crates/pf-encode/src/lib.rs b/crates/pf-encode/src/lib.rs index 918aefb8..5e6d89eb 100644 --- a/crates/pf-encode/src/lib.rs +++ b/crates/pf-encode/src/lib.rs @@ -274,6 +274,14 @@ impl Encoder for TrackedEncoder { fn invalidate_ref_frames(&mut self, first_frame: i64, last_frame: i64) -> bool { self.inner.invalidate_ref_frames(first_frame, last_frame) } + // Same trap class as `set_wire_chunking`, and the one where it would hurt most: unforwarded, + // the default no-op would leave every session serving RFI anchors over damage the client + // reported and the host never repaired — the failure this method exists to close, silently + // reintroduced by the wrapper. (The `every_encoder_method_is_forwarded` guard below catches + // it, which is exactly why that guard is there.) + fn distrust_references(&mut self) { + self.inner.distrust_references() + } // Forwarded for the same reason as `set_wire_chunking` below — the unforwarded default // (`false` = "backend can't pipeline, stop asking") silently killed the §7 LN3 contention // escalation for every session, since the host loop only ever holds the wrapped box. diff --git a/crates/pf-vkdecode/src/decoder.rs b/crates/pf-vkdecode/src/decoder.rs index bf0854b8..462947ae 100644 --- a/crates/pf-vkdecode/src/decoder.rs +++ b/crates/pf-vkdecode/src/decoder.rs @@ -185,6 +185,24 @@ pub struct DecodedVkFrame { /// the loss, and lift a freeze on a wave that completed before it. Comparing /// this ordinal against the one current at the arm is what tells them apart. pub decode_order: u64, + /// Every picture this one was predicted from came off a fully-available reference + /// chain ([`pf_bitstream::h264::PicturePlan::references_clean`] and its two twins). + /// `true` for an IDR/IRAP/key frame and for any picture whose whole chain is clean; + /// `false` from the moment this AU — or anything it descends from — needed + /// concealment. + /// + /// It exists to let a consumer CORROBORATE a host's claim that a frame is a clean + /// re-anchor. `USER_FLAG_RECOVERY_ANCHOR` — the host's LTR-RFI recovery frame — + /// lifts a post-loss freeze on its FIRST occurrence, exactly like a real IDR, + /// because the host says the frame was coded against a known-good reference. The + /// host's "known-good" is an inference from what the client RECEIVED; this is what + /// the client actually DECODED. Where they disagree the freeze used to lift onto a + /// gray plate and every frame after it chained off the corruption, with nothing + /// left to re-arm the gate. + /// + /// A consumer with no such claim to check can ignore it: it is an observation + /// about the stream, and nothing in this crate's own behaviour reads it. + pub references_clean: bool, /// The decode op's slot in the status query pool. pub query_slot: u32, /// The decode op's submission ordinal (validates the query slot has not been @@ -281,12 +299,24 @@ pub enum VkDecodeError { /// correct consumer can never hit this). The AU was planned but NOT decoded; /// release frames and request a keyframe. NoFreeSlot, - /// A DPB slot this AU references holds no bound image. H.265 only, and fatal - /// rather than skippable: `StdVideoDecodeH265PictureInfo`'s RPS arrays are - /// INDICES into `pReferenceSlots`, so dropping one entry would silently - /// re-point every later index at the wrong picture — the exact class of - /// plausible-looking corruption this crate refuses to produce. (H.264 carries - /// no such index arrays and only traces the case.) + /// A DPB slot this AU references holds no bound image. Fatal on all three codecs + /// rather than skippable. + /// + /// H.265 and AV1 have a structural argument: their picture info names DPB slots by + /// INDEX (the H.265 RPS arrays, AV1's name-indexed `refs`), so dropping one entry + /// silently re-points a later index at the wrong picture. H.264 has no such index + /// arrays, and used to skip the case with a `trace!` on exactly that reasoning — + /// but the reasoning was about the STRUCTURE, not the output. The hardware still + /// decodes a P-picture against a reference that was never bound, and on the + /// DPB-and-output-COINCIDE path that is a gray plate with the new frame's motion + /// painted over it. Nothing warned: the planner's DPB genuinely holds the picture + /// (the breakage is this crate's slot→image ledger), so the frame was shipped, + /// presented, and cleared the consumer's demotion streak on the way past — + /// invisible damage, which is the one outcome this crate exists to make impossible. + /// + /// Failing closed is only safe because it is paired with recovery: the latch + /// ([`crate::decoder_h265::RecoveryLatch`]) flushes to the next IRAP/IDR rather + /// than leaving the stream wedged on a slot nothing can honour. UnboundReferenceSlot { slot: u8 }, /// The frame belongs to a generation whose retired pool is already gone /// (double release, or a frame outliving its graveyard entry). @@ -615,6 +645,11 @@ pub(crate) struct PendingPic { pub(crate) recovery: crate::recovery::RecoveryMark, /// See [`DecodedVkFrame::decode_order`]. pub(crate) decode_order: u64, + /// Read off the plan at DECODE time and carried here for the same reason + /// [`Self::recovery`] is: display order is not decode order, and this describes + /// the picture rather than the moment it is delivered. See + /// [`DecodedVkFrame::references_clean`]. + pub(crate) references_clean: bool, } /// A retired generation's picture pool: images the presenter still holds live @@ -678,7 +713,15 @@ pub struct VkH264Decoder { /// The outstanding recovery point SEI, if any — see [`crate::recovery`]. /// Survives session rebuilds on purpose: it is a fact about the STREAM's /// prediction structure, not about this decoder's Vulkan objects. + /// + /// Named apart from [`Self::recovery`], which is this decoder's DPB-recovery + /// latch: the two are unrelated (one is a fact about the stream's prediction + /// structure, the other about this decoder's own wedged state). recovery_watch: crate::recovery::RecoveryWatch, + /// Post-failure DPB recovery owed — see + /// [`crate::decoder_h265::RecoveryLatch`], whose docs carry the whole + /// fail-closed/recover argument for all three codecs. + recovery: crate::decoder_h265::RecoveryLatch, /// Pictures planned so far — stamped onto each one as /// [`DecodedVkFrame::decode_order`]. Survives session rebuilds for the same /// reason the watch does. @@ -724,6 +767,7 @@ impl VkH264Decoder { graveyard: Vec::new(), last_warnings: Vec::new(), recovery_watch: crate::recovery::RecoveryWatch::new(), + recovery: Default::default(), decoded: 0, generation: 0, device_lost: false, @@ -748,6 +792,12 @@ impl VkH264Decoder { } fn decode_inner(&mut self, au: &[u8]) -> Result, VkDecodeError> { + // A previous AU failed after its planning had advanced: clear the stale + // DPB residency BEFORE planning this one, or every AU referencing the + // stranded picture fails forever ([`RecoveryLatch`] docs). + if self.recovery.take() { + self.recover_dpb(); + } // `take_warnings` promises "cleared by the next decode", and this IS a // decode: clear BEFORE planning, so an AU that fails to plan at all cannot // leave the previous AU's warnings behind to be re-read as damage on the @@ -784,7 +834,38 @@ impl VkH264Decoder { ); } - self.ensure_state(&plan)?; + // From here the PLANNER has already advanced past this AU — its DPB holds + // the picture whatever happens next — so any failure below leaves the + // planner's DPB and this decoder's slot/image ledgers able to disagree. + // Latch the recovery for the next decode rather than returning into a + // permanently wedged state. (Deliberately wider than the paths that mutate + // the SlotMap: a failure BEFORE `plan_to_vk` mutates it — an `ensure_state` + // refusal, a `NoFreeSlot` — strands the picture the other way round, + // planner-resident with no slot at all, and wedges just as hard. One flush + // cures both.) The H.265 twin, for the same reason: `decoder_h265`'s + // `RecoveryLatch` docs carry the whole argument. + let result = self.decode_planned(&plan, au, recovery, decode_order); + if result.is_err() { + self.recovery.latch(); + } + result + } + + /// The submission half of one decode, from the point the planner has already + /// advanced. Split out so [`Self::decode_inner`] can latch recovery on ANY + /// failure past that line without threading a flag through every exit. + /// `au` is the same buffer `plan`'s slice ranges index into; `recovery` is the + /// recovery-point verdict already folded for this AU and `decode_order` its + /// decode-order ordinal (both advance in decode order, so neither can be + /// derived here — this path is not reached for every planned AU). + fn decode_planned( + &mut self, + plan: &AuPlan, + au: &[u8], + recovery: crate::recovery::RecoveryMark, + decode_order: u64, + ) -> Result, VkDecodeError> { + self.ensure_state(plan)?; let sps_id = plan.sps.seq_parameter_set_id; // Convert, with ONE rebuild retry on CapacityMismatch — the designed @@ -810,7 +891,7 @@ impl VkH264Decoder { // satisfies ensure_parameters' Recreate contract, and Current/Add // touch nothing a submitted decode reads. unsafe { state.session.ensure_parameters(&plan.sps, &plan.pps)? }; - match plan_to_vk(&plan, &mut state.slots, sps_id) { + match plan_to_vk(plan, &mut state.slots, sps_id) { Ok(converted) => { vk_plan = Some(converted); break; @@ -820,7 +901,7 @@ impl VkH264Decoder { required, capacity, "DPB depth renegotiated — rebuilding session" ); - self.rebuild_state(&plan)?; + self.rebuild_state(plan)?; } Err(e) => return Err(VkDecodeError::Convert(e)), } @@ -1002,6 +1083,7 @@ impl VkH264Decoder { is_idr: plan.picture.is_idr, recovery, decode_order, + references_clean: plan.picture.references_clean, }, ); Ok(()) @@ -1358,6 +1440,40 @@ impl VkH264Decoder { } } + /// Clear the DPB state a failed AU left behind, so planning resumes at the + /// next IDR instead of erroring on residency nothing can honour. + /// + /// Three ledgers have to agree and, after a post-planning failure, do not: + /// the PLANNER's DPB, this decoder's [`SlotMap`], and the slot→image + /// bindings. [`Self::flush`] settles the first (and hands back any picture + /// that did reach output — those frames are real and are still delivered), + /// then [`crate::decoder_h265::reset_slot_bindings`] empties the other two. + /// Pool images the stale bindings pinned go back on the free list; images a + /// consumer still HOLDS stay pinned by their own `held` counts, exactly as + /// they would across a session rebuild. + /// + /// Deliberately not a session rebuild: the session, pools and ring are all + /// still valid — only the DPB bookkeeping is stale — and a rebuild would + /// churn every image allocation for a condition an IDR fixes anyway. + /// + /// The H.265 twin (`decoder_h265::recover_dpb`) is the same function one codec + /// over; the two share `reset_slot_bindings` rather than the whole body because + /// each has to call its OWN `flush`, which settles its own planner's DPB. + fn recover_dpb(&mut self) { + debug!("recovering from a failed AU — flushing the H.264 DPB to the next IDR"); + self.flush(); + if let Some(state) = &mut self.state { + let unbound = crate::decoder_h265::reset_slot_bindings( + &mut state.slots, + &mut state.slot_image, + &mut state.slot_refs, + ); + for picture in unbound { + state.pool.pictures[picture].bound = false; + } + } + } + /// Session/caps for THIS plan exist and match its extent + profile, and the /// stream sits inside the device's level ceiling. DPB-depth mismatches /// surface later as `plan_to_vk`'s `CapacityMismatch` (the designed trigger) @@ -1657,6 +1773,7 @@ pub(crate) fn build_frame( is_idr: entry.is_idr, recovery: entry.recovery, decode_order: entry.decode_order, + references_clean: entry.references_clean, query_slot: entry.query_slot, submission: entry.submission, picture: entry.image as u32, @@ -1848,37 +1965,9 @@ unsafe fn record_and_submit( } // ---- bound-slot staging ---- - // Scope list: this AU's references first, then every other still-held slot - // (their resources must stay bound for their associations to persist), then - // the setup slot as the ACTIVATION entry (slot index -1 binds its resource - // without a current association; the decode op's setup slot then claims it). - let mut scope: Vec<(i32, vk::ImageView, hh::StdVideoDecodeH264ReferenceInfo)> = Vec::new(); - for r in &vk_plan.refs { - match slot_view(state, r.slot) { - Some(view) => scope.push((i32::from(r.slot), view, r.std)), - None => trace!(slot = r.slot, "referenced slot without a bound image"), - } - } - for (slot, _id) in state.slots.held() { - if slot == vk_plan.setup_slot - || scope - .iter() - .any(|&(index, _, _)| index >= 0 && index as u8 == slot) - { - continue; - } - match (state.slot_refs[usize::from(slot)], slot_view(state, slot)) { - (Some(std), Some(view)) => scope.push((i32::from(slot), view, std)), - // Unreachable in practice: every held slot was a setup slot once. - _ => trace!( - slot, - "held slot without reference info/binding — left unbound" - ), - } - } - let reference_count = vk_plan.refs.len().min(scope.len()); // The setup/dst resource: the fresh pool image (coincide) or the DPB layer - // (distinct — the pool image is the separate decode output). + // (distinct — the pool image is the separate decode output). Resolved before the + // scope is built, because it is the scope's last entry. let setup_view = if coincide { state.pool.pictures[dst].view } else { @@ -1888,31 +1977,48 @@ unsafe fn record_and_submit( .expect("distinct mode") .dpb_view(vk_plan.setup_slot) }; - scope.push((-1, setup_view, vk_plan.setup_ref)); + // Scope list: this AU's references first, then every other still-held slot + // (their resources must stay bound for their associations to persist), then + // the setup slot as the ACTIVATION entry (slot index -1 binds its resource + // without a current association; the decode op's setup slot then claims it). + // + // Shared with H.265 (`decoder_h265::build_scope`): the two codecs' layout, + // fail-closed rule and reference-count derivation are the same algorithm over a + // different `StdVideo*` type, and this function's whole job is refusing to guess — + // the property least tolerant of two copies drifting apart. + let held: Vec = state.slots.held().map(|(slot, _id)| slot).collect(); + let (scope, reference_count) = crate::decoder_h265::build_scope( + &vk_plan.refs, + held.into_iter(), + vk_plan.setup_slot, + setup_view, + vk_plan.setup_ref, + &state.slot_refs, + |slot| slot_view(state, slot), + )?; // Staged arrays: resources → std infos → codec slot infos → slot infos. Each // vector is fully built before the next borrows it, so nothing reallocates // under a stored pointer. let resources: Vec> = scope .iter() - .map(|&(_, view, _)| { + .map(|e| { vk::VideoPictureResourceInfoKHR::default() .coded_extent(coded_extent) .base_array_layer(0) - .image_view_binding(view) + .image_view_binding(e.view) }) .collect(); - let std_refs: Vec = - scope.iter().map(|&(_, _, std)| std).collect(); + let std_refs: Vec = scope.iter().map(|e| e.std).collect(); let mut dpb_infos: Vec> = std_refs .iter() .map(|std| vk::VideoDecodeH264DpbSlotInfoKHR::default().std_reference_info(std)) .collect(); let mut begin_slots: Vec> = Vec::with_capacity(scope.len()); - for (index, &(slot_index, _, _)) in scope.iter().enumerate() { + for (index, entry) in scope.iter().enumerate() { begin_slots.push( vk::VideoReferenceSlotInfoKHR::default() - .slot_index(slot_index) + .slot_index(entry.slot_index) .picture_resource(&resources[index]), ); } @@ -2038,8 +2144,96 @@ unsafe fn record_and_submit( #[cfg(test)] mod tests { + use ash::vk::Handle as _; + use super::*; + /// A fake, never-dereferenced view handle keyed by slot, so a scope's bindings + /// can be checked without a device (the H.265 tests' idiom, one codec over). + fn fake_view(slot: u8) -> vk::ImageView { + vk::ImageView::from_raw(u64::from(slot) + 1) + } + + /// A reference-info value carrying just the field the assertions read. + fn h264_std_ref(frame_num: u16) -> hh::StdVideoDecodeH264ReferenceInfo { + // SAFETY: StdVideoDecodeH264ReferenceInfo is a plain-C bindgen struct of a + // bitfield word and integers; all-zero is valid for every field. + let mut std: hh::StdVideoDecodeH264ReferenceInfo = unsafe { std::mem::zeroed() }; + std.FrameNum = frame_num; + std + } + + fn h264_ref(slot: u8, frame_num: u16) -> crate::pic::VkRef { + crate::pic::VkRef { + slot, + std: h264_std_ref(frame_num), + id: u64::from(slot), + } + } + + /// The H.264 leg of the fail-closed rule. It used to trace-and-continue here, on + /// the grounds that H.264 carries no RPS index arrays — but the hardware still + /// decoded the picture against a reference that was never bound, which is a gray + /// plate with motion on it, shipped with no warning attached. Fail closed. + #[test] + fn an_h264_reference_slot_without_a_bound_image_fails_the_whole_op() { + let refs = vec![h264_ref(1, 10), h264_ref(3, 20)]; + let slot_refs = vec![Some(h264_std_ref(0)); 8]; + let err = crate::decoder_h265::build_scope( + &refs, + [1u8, 3].into_iter(), + 0, + fake_view(0), + h264_std_ref(30), + &slot_refs, + |slot| (slot != 3).then(|| fake_view(slot)), + ) + .unwrap_err(); + assert!( + matches!(err, VkDecodeError::UnboundReferenceSlot { slot: 3 }), + "{err}" + ); + } + + /// `reference_count` must be the number of THIS AU's references and nothing else. + /// The old H.264 form (`refs.len().min(scope.len())`) was taken AFTER the + /// held-slot pass appended to the same vector, so a short refs list let the decode + /// op's reference array run past the references into unrelated held slots — a + /// picture predicted from something the stream never named. + #[test] + fn the_h264_reference_count_covers_the_references_and_never_a_held_slot() { + // Two references (slots 1, 3); slots 5 and 6 are held but NOT referenced. + let refs = vec![h264_ref(1, 10), h264_ref(3, 20)]; + let slot_refs = vec![Some(h264_std_ref(77)); 8]; + let (scope, reference_count) = crate::decoder_h265::build_scope( + &refs, + [1u8, 3, 5, 6].into_iter(), + 0, + fake_view(0), + h264_std_ref(30), + &slot_refs, + |slot| Some(fake_view(slot)), + ) + .unwrap(); + + assert_eq!(reference_count, 2, "exactly this AU's references"); + assert_eq!( + scope[..reference_count] + .iter() + .map(|e| e.slot_index) + .collect::>(), + vec![1, 3], + "the decode op's reference prefix is the references, in order" + ); + // The rest of the scope keeps the other slots bound (so their associations + // survive) and ends on the setup activation entry — but none of that is a + // reference of this AU. + assert_eq!( + scope.iter().map(|e| e.slot_index).collect::>(), + vec![1, 3, 5, 6, -1] + ); + } + #[test] fn settle_dpb_readies_outputs_in_order_and_returns_never_output_removals() { let mut pending: BTreeMap = BTreeMap::new(); diff --git a/crates/pf-vkdecode/src/decoder_av1.rs b/crates/pf-vkdecode/src/decoder_av1.rs index b106917b..88add75a 100644 --- a/crates/pf-vkdecode/src/decoder_av1.rs +++ b/crates/pf-vkdecode/src/decoder_av1.rs @@ -1114,6 +1114,7 @@ impl VkAv1Decoder { is_idr: plan.picture.is_key, recovery: crate::recovery::RecoveryMark::NONE, decode_order, + references_clean: plan.picture.references_clean, }, ); diff --git a/crates/pf-vkdecode/src/decoder_h265.rs b/crates/pf-vkdecode/src/decoder_h265.rs index a3713085..b1f338f2 100644 --- a/crates/pf-vkdecode/src/decoder_h265.rs +++ b/crates/pf-vkdecode/src/decoder_h265.rs @@ -139,13 +139,20 @@ struct SessionStateH265 { /// /// This decoder FAILS CLOSED, and that stays: when an AU cannot be carried /// through to a submitted decode, it returns an error rather than substituting a -/// reference or decoding against a slot whose image is gone. H.264's -/// soft-degrade (trace the missing binding, drop that reference, decode anyway) -/// is not available here because `StdVideoDecodeH265PictureInfo`'s +/// reference or decoding against a slot whose image is gone. The structural argument +/// is that `StdVideoDecodeH265PictureInfo`'s /// `RefPicSetStCurrBefore`/`StCurrAfter`/`LtCurr` arrays hold INDICES into the /// decode op's reference array — dropping one entry re-points every later index /// at the wrong picture, which is the corruption-hiding class this crate refuses. /// +/// ⚠ H.264 used to soft-degrade here (trace the missing binding, drop that reference, +/// decode anyway) on the grounds that it carries no such index arrays. It now fails +/// closed and carries this same latch: the arrays were never the point, the OUTPUT +/// was. A P-picture decoded against a reference that was never bound is a gray plate +/// with motion painted over it, and because the planner raises no warning for it, that +/// frame reached the screen and cleared the consumer's demotion streak. Both codecs +/// now fail closed, and both recover through this latch rather than wedging. +/// /// But failing closed once must not wedge the stream FOREVER, and without this /// latch it did: by the time an AU reaches a failure exit, `plan_to_vk_h265` has /// already mutated the [`SlotMap`] (releases + the setup assignment) and the @@ -636,6 +643,7 @@ impl VkH265Decoder { is_idr: plan.picture.is_idr, recovery, decode_order, + references_clean: plan.picture.references_clean, }, ); @@ -1247,10 +1255,15 @@ fn profile_key_for(plan: &AuPlan) -> Result { /// let [`build_scope`] bind a slot the planner no longer knows about, which is the /// same "plausible-looking picture in the wrong place" the unbound-reference /// refusal exists to prevent. -fn reset_slot_bindings( +/// +/// Generic over the cached reference-info type so H.264's recovery uses this exact +/// code rather than a twin: the three ledgers and the "empty them together" rule are +/// codec-independent (`SlotMap` is already shared), and only the `StdVideo*` type in +/// `slot_refs` differs. +pub(crate) fn reset_slot_bindings( slots: &mut SlotMap, slot_image: &mut [Option], - slot_refs: &mut [Option], + slot_refs: &mut [Option], ) -> Vec { // `release` is the only way a slot is freed (SlotMap docs); the collect is // because `held` borrows the map the releases mutate. @@ -1279,10 +1292,55 @@ fn slot_view(state: &SessionStateH265, slot: u8) -> Option { /// (No derived equality: `StdVideoDecodeH265ReferenceInfo` is a plain-C bindgen /// struct without it. Assertions compare the fields that carry meaning.) #[derive(Debug, Clone, Copy)] -struct ScopeEntry { - slot_index: i32, - view: vk::ImageView, - std: hh::StdVideoDecodeH265ReferenceInfo, +pub(crate) struct ScopeEntry { + pub(crate) slot_index: i32, + pub(crate) view: vk::ImageView, + pub(crate) std: S, +} + +/// One of this AU's references, as [`build_scope`] needs to see it: a DPB slot and +/// the codec reference info to bind with it. +/// +/// It exists so H.264 and H.265 share ONE scope builder instead of two hand-copies of +/// a function whose whole job is refusing to guess — the property most in need of a +/// single implementation. Their `VkRef`/`VkRefH265` differ only in the `StdVideo*` +/// type they carry, so the shape generalises exactly. +/// +/// ⚠ AV1 deliberately keeps its own ([`crate::decoder_av1`]'s `build_scope_av1`): its +/// reference array is indexed by reference NAME and may hold HOLES, so its walk is a +/// different algorithm rather than the same one over a different Std type. Folding it +/// in here would mean a builder with a mode flag, which is how the two would drift. +pub(crate) trait ScopeRef { + /// The codec's `StdVideoDecode*ReferenceInfo`. + type Std: Copy; + /// The DPB slot this reference is bound in. + fn slot(&self) -> u8; + fn std(&self) -> Self::Std; +} + +/// [`build_scope`]'s answer: the bound-slot list, and how many of its LEADING entries +/// are this AU's own references (the prefix the decode op takes as its reference +/// array — see the ordering note in `build_scope`'s docs). +pub(crate) type Scope = (Vec::Std>>, usize); + +impl ScopeRef for crate::pic_h265::VkRefH265 { + type Std = hh::StdVideoDecodeH265ReferenceInfo; + fn slot(&self) -> u8 { + self.slot + } + fn std(&self) -> Self::Std { + self.std + } +} + +impl ScopeRef for crate::pic::VkRef { + type Std = ash::vk::native::StdVideoDecodeH264ReferenceInfo; + fn slot(&self) -> u8 { + self.slot + } + fn std(&self) -> Self::Std { + self.std + } } /// Build the coding scope's bound-slot list and say how many leading entries are @@ -1295,36 +1353,51 @@ struct ScopeEntry { /// resources must stay bound even when this AU does not reference them); /// 3. the setup slot as the activation entry, slot index `-1`. /// -/// A reference whose slot binds no image is a hard error, never a skip: -/// `StdVideoDecodeH265PictureInfo`'s `RefPicSetStCurrBefore`/`StCurrAfter`/ -/// `LtCurr` arrays name DPB slots, and every slot they name is one of `refs`' -/// ([`crate::pic_h265`]) — so dropping an entry leaves the hardware with a named -/// slot this op never bound, which it can only answer by guessing or failing. -/// Output that looks plausible and is wrong is the outcome this refusal exists to -/// prevent. -fn build_scope( - refs: &[crate::pic_h265::VkRefH265], +/// A reference whose slot binds no image is a hard error, never a skip. For H.265 the +/// argument is `StdVideoDecodeH265PictureInfo`'s `RefPicSetStCurrBefore`/`StCurrAfter`/ +/// `LtCurr` arrays: they name DPB slots, every slot they name is one of `refs`' +/// ([`crate::pic_h265`]), so dropping an entry leaves the hardware with a named slot +/// this op never bound — which it can only answer by guessing or failing. +/// +/// H.264 has no such index arrays, and it used to skip the case with a `trace!` on +/// exactly that reasoning. The reasoning was wrong about the OUTPUT: the hardware +/// still decodes a P-picture against a reference that was never bound, which on the +/// DPB-and-output-COINCIDE path is a gray plate with the new frame's motion painted +/// over it — and because the planner raised no warning (its DPB genuinely holds the +/// picture; the breakage is in this ledger), the frame was shipped, presented, and +/// cleared the consumer's demotion streak on its way past. Both codecs fail closed +/// here now; the recovery latch is what keeps failing closed from wedging the stream. +/// +/// `reference_count` is captured the instant the `refs` loop ends, BEFORE the +/// held-slot pass appends anything. That ordering is load-bearing: the decode op takes +/// `scope[..reference_count]` as its reference list, so a count computed after the +/// second pass could hand it a still-held slot that this AU does not reference, in +/// place of one that failed to resolve. (Fail-closed above makes that unreachable — +/// but the construction must be correct on its own, not by depending on a check +/// somewhere else.) +pub(crate) fn build_scope( + refs: &[R], held_slots: impl Iterator, setup_slot: u8, setup_view: vk::ImageView, - setup_ref: hh::StdVideoDecodeH265ReferenceInfo, - slot_refs: &[Option], + setup_ref: R::Std, + slot_refs: &[Option], view_of: impl Fn(u8) -> Option, -) -> Result<(Vec, usize), VkDecodeError> { - let mut scope: Vec = Vec::with_capacity(refs.len() + slot_refs.len() + 1); +) -> Result, VkDecodeError> { + let mut scope: Vec> = Vec::with_capacity(refs.len() + slot_refs.len() + 1); for r in refs { - match view_of(r.slot) { + match view_of(r.slot()) { Some(view) => scope.push(ScopeEntry { - slot_index: i32::from(r.slot), + slot_index: i32::from(r.slot()), view, - std: r.std, + std: r.std(), }), - None => return Err(VkDecodeError::UnboundReferenceSlot { slot: r.slot }), + None => return Err(VkDecodeError::UnboundReferenceSlot { slot: r.slot() }), } } let reference_count = scope.len(); for slot in held_slots { - if slot == setup_slot || refs.iter().any(|r| r.slot == slot) { + if slot == setup_slot || refs.iter().any(|r| r.slot() == slot) { continue; } match ( @@ -1862,8 +1935,11 @@ mod tests { let setup_slot = slots.assign(400).unwrap(); assert_eq!(setup_slot, 0, "the freed slots are assignable again"); slot_image[usize::from(setup_slot)] = Some(9); + // The empty slice needs its element type named now that `build_scope` is + // generic over the two codecs' reference types. + let no_refs: [VkRefH265; 0] = []; let (scope, reference_count) = build_scope( - &[], + &no_refs, slots.held().map(|(slot, _id)| slot), setup_slot, fake_view(setup_slot), diff --git a/crates/pf-vkdecode/src/integrity.rs b/crates/pf-vkdecode/src/integrity.rs index a5f493a3..979669c9 100644 --- a/crates/pf-vkdecode/src/integrity.rs +++ b/crates/pf-vkdecode/src/integrity.rs @@ -38,19 +38,20 @@ use crate::{Av1PlanWarning, H265PlanWarning, PlanWarning}; /// property of the STREAM's signalling, which the decoder answers by failing to open /// a session, not by showing a damaged frame. /// -/// Written as an EXHAUSTIVE match with no wildcard, deliberately. A `matches!` (or -/// a `_ => false`) makes "damage" the opt-in and silence the default, so a -/// `PlanWarning` added later — by definition one nobody here has classified — -/// would be reported as clean and its picture shown. Invisible damage is the bug -/// this whole program exists to end; the compiler is the only reviewer guaranteed -/// to be present when that variant is written, so it gets the decision. +/// ⚠ The classification itself now lives on the warning enum, in pf-bitstream +/// ([`PlanWarning::is_integrity`]), and this function delegates. It moved there when +/// the planners gained the per-picture clean bit +/// ([`pf_bitstream::h264::PicturePlan::references_clean`]): that ledger has to mark a +/// picture damaged on exactly the warnings a consumer conceals on, and it lives one +/// crate DOWN from here. A copy of the list in each crate would let the two disagree — +/// the planner recording a picture as clean while the client concealed it, or the +/// reverse — which is the same invisible-damage failure the single-list rule below was +/// written to prevent, one layer lower. One list, in the crate that owns the enum. +/// +/// This function stays as the crate's public spelling of the question (the fault +/// harness, the client and the tests all name it) and keeps its exact semantics. pub fn is_integrity_warning(w: &PlanWarning) -> bool { - match w { - PlanWarning::FrameNumGap { .. } - | PlanWarning::MissingReference { .. } - | PlanWarning::TruncatedAu { .. } => true, - PlanWarning::Mmco5Rebase | PlanWarning::LevelDerivedDpb { .. } => false, - } + w.is_integrity() } /// The H.265 twin — the same set pf-bitstream's own `h265` conformance harness @@ -59,10 +60,7 @@ pub fn is_integrity_warning(w: &PlanWarning) -> bool { /// Exhaustive for the same reason as [`is_integrity_warning`]: a new H.265 warning /// must not be able to mean "damaged" and read as clean. pub fn is_integrity_warning_h265(w: &H265PlanWarning) -> bool { - match w { - H265PlanWarning::MissingReference { .. } | H265PlanWarning::TruncatedAu { .. } => true, - H265PlanWarning::NonZeroReorder { .. } => false, - } + w.is_integrity() } /// The AV1 twin (M7). Every variant the AV1 planner has today IS damage, and that @@ -93,11 +91,7 @@ pub fn is_integrity_warning_h265(w: &H265PlanWarning) -> bool { /// Exhaustive for the same reason as [`is_integrity_warning`]: a new AV1 warning /// must not be able to mean "damaged" and read as clean. pub fn is_integrity_warning_av1(w: &Av1PlanWarning) -> bool { - match w { - Av1PlanWarning::MissingReference { .. } - | Av1PlanWarning::MissingShowExisting { .. } - | Av1PlanWarning::TruncatedAu { .. } => true, - } + w.is_integrity() } #[cfg(test)] diff --git a/crates/pf-vkdecode/src/pic_h265.rs b/crates/pf-vkdecode/src/pic_h265.rs index d6fe2c1e..1a4b485b 100644 --- a/crates/pf-vkdecode/src/pic_h265.rs +++ b/crates/pf-vkdecode/src/pic_h265.rs @@ -899,6 +899,9 @@ mod tests { max_dpb_frames, short_term_ref_pic_set_size_bits: 0, recovery_point: None, + // These fixtures model a healthy stream; the clean bit is the planner's + // observation and nothing in this conversion layer reads it. + references_clean: true, } } diff --git a/crates/punktfunk-core/src/abi.rs b/crates/punktfunk-core/src/abi.rs index d04cb597..1f4fea20 100644 --- a/crates/punktfunk-core/src/abi.rs +++ b/crates/punktfunk-core/src/abi.rs @@ -5730,6 +5730,15 @@ pub unsafe extern "C" fn punktfunk_reanchor_gate_arm_expecting_drops( /// `USER_FLAG_RECOVERY_POINT`. Pass `decoder_keyframe = false` where the platform decoder doesn't flag /// IDRs (VideoToolbox/MediaCodec) — the wire `FLAG_SOF` covers it. /// +/// This is the uncorroborated entry point and deliberately stays that way. Rust embedders whose +/// decoder parses the bitstream call [`ReanchorGate::on_decoded_corroborated`] to let their own +/// parser refute a `USER_FLAG_RECOVERY_ANCHOR` that names a picture they had to conceal; every +/// client reachable through THIS surface (Apple VideoToolbox, Android MediaCodec) uses a platform +/// decoder that surfaces no such fact, so it would have nothing to pass but +/// `AnchorEvidence::Unavailable` — which is exactly what this wrapper already means. Growing a +/// second export for a corroboration no C caller can supply would spend an ABI version bump on +/// dead surface. +/// /// # Safety /// `g` is a valid gate handle; `out_present` is writable or NULL. #[unsafe(no_mangle)] diff --git a/crates/punktfunk-core/src/reanchor.rs b/crates/punktfunk-core/src/reanchor.rs index b5977d1f..f6b27f11 100644 --- a/crates/punktfunk-core/src/reanchor.rs +++ b/crates/punktfunk-core/src/reanchor.rs @@ -20,6 +20,30 @@ //! VideoToolbox, every FFmpeg rung, which exposes no SEI) simply never calls it and every wire //! behaviour above is bit-for-bit unchanged. //! +//! # The one claim a client can REFUTE +//! +//! Of the three lifts, two are self-evident to the client and one is pure hearsay. An IDR predicts +//! from nothing, so "this re-anchors decode" is a property of the picture itself. A recovery mark is +//! only *half* a re-anchor and the gate says so by requiring two. But +//! [`USER_FLAG_RECOVERY_ANCHOR`] is the HOST asserting a fact about the CLIENT's decoder — *the +//! picture I coded this P-frame against is one you still hold, intact* — and until +//! [`AnchorEvidence`] existed the client took it on faith, on the first occurrence, with no +//! scrutiny at all. +//! +//! When that assertion is wrong the failure is the worst-shaped one in this module: the anchor lifts +//! the freeze onto a picture predicted from a reference the client had to conceal, so the gray plate +//! reaches the screen AND the gate stops holding, which means it keeps reaching the screen until +//! some later signal re-arms. A re-anchor claim the client can refute is therefore worse than no +//! claim at all — no claim merely holds the last good frame until the backstop. +//! +//! So a client whose decoder parses the bitstream corroborates it: it already knows which pictures +//! this AU predicts from and whether each of those decoded from a complete reference chain, and +//! [`on_decoded_corroborated`](ReanchorGate::on_decoded_corroborated) refuses an anchor whose +//! references it can prove were damaged. Refusing can only ever make the gate hold LONGER — the +//! freeze stays up, the backstop fires on its ORIGINAL deadline, and the client escalates to a real +//! IDR — which is the direction every other rule here errs in, deliberately. Lanes that cannot +//! answer pass [`AnchorEvidence::Unavailable`] and behave exactly as they always have. +//! //! [`USER_FLAG_RECOVERY_POINT`]: crate::packet::USER_FLAG_RECOVERY_POINT //! [`USER_FLAG_RECOVERY_ANCHOR`]: crate::packet::USER_FLAG_RECOVERY_ANCHOR @@ -97,8 +121,9 @@ pub fn index_gap(expected: u32, got: u32) -> Option { /// Fold one decoded frame into the re-anchor state and decide whether it lifts the post-loss freeze. /// /// `is_keyframe` — a real IDR (always a clean re-anchor). `has_anchor` — this AU carried -/// [`USER_FLAG_RECOVERY_ANCHOR`](crate::packet::USER_FLAG_RECOVERY_ANCHOR), the host's definitive -/// single-frame re-anchor from an LTR-RFI recovery (a clean P-frame coded against a known-good +/// [`USER_FLAG_RECOVERY_ANCHOR`](crate::packet::USER_FLAG_RECOVERY_ANCHOR) **and the caller did not +/// refute it** ([`AnchorEvidence`]), the host's definitive single-frame re-anchor from an LTR-RFI +/// recovery (a clean P-frame coded against a known-good /// reference), so it lifts on the FIRST occurrence exactly like an IDR — no two-mark wait. `has_mark` — /// this AU carried [`USER_FLAG_RECOVERY_POINT`](crate::packet::USER_FLAG_RECOVERY_POINT), a /// host-signalled intra-refresh wave boundary (only *half* a re-anchor). `marks` — recovery marks seen @@ -158,6 +183,44 @@ impl LocalRecovery { }; } +/// What a client's OWN parser can say about the host's re-anchor claim on one decoded frame — the +/// corroboration for [`USER_FLAG_RECOVERY_ANCHOR`](crate::packet::USER_FLAG_RECOVERY_ANCHOR). +/// +/// An anchor is the host asserting something about the CLIENT's decoder: *this P-frame is coded +/// against a picture you still hold, intact, so decoding it re-anchors you*. The host derives that +/// from its own slot bookkeeping — which tracks whether the client RECEIVED a frame, not whether it +/// DECODED that frame from a complete reference chain. Those two differ exactly when the client had +/// to conceal, and the gap between them is what puts a gray plate on screen with the freeze lifted. +/// +/// Three states rather than a bool, for the same reason [`LocalRecovery`] is two facts: a lane that +/// *cannot* answer must be able to say so instead of being folded into "nothing wrong here". Only +/// [`Self::ReferencesDamaged`] changes any behaviour; the other two are indistinguishable to the +/// gate and differ only in what they claim at the call site. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum AnchorEvidence { + /// This lane has no local bitstream parser, so it cannot corroborate or refute anything — the + /// host's claim stands, exactly as it always has. Android MediaCodec, Apple VideoToolbox and + /// every lane reached over the C ABI pass this, and their behaviour is bit-for-bit unchanged. + #[default] + Unavailable, + /// Corroborated: every picture this AU predicts from was itself decoded from a fully-available + /// reference chain, so the host's claim is consistent with what this decoder actually holds. + ReferencesClean, + /// Refuted: this AU predicts from a picture that needed concealment. Whatever the host believes, + /// decoding this frame cannot re-anchor a decoder whose reference for it is already damaged, so + /// the anchor does not lift the freeze. + ReferencesDamaged, +} + +impl AnchorEvidence { + /// May an [`USER_FLAG_RECOVERY_ANCHOR`](crate::packet::USER_FLAG_RECOVERY_ANCHOR) on this frame + /// be honoured? Only an outright refutation withholds it — silence is not refutation, so a lane + /// that cannot corroborate never becomes *stricter* than it was. + fn honours_anchor(self) -> bool { + !matches!(self, AnchorEvidence::ReferencesDamaged) + } +} + /// Whether a decoded frame should be shown or withheld while the gate is (or isn't) frozen. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum GateVerdict { @@ -333,6 +396,11 @@ impl ReanchorGate { /// A decoded frame always clears the no-output streak. When frozen, a live mark stream pushes the /// backstop out ([`RECOVERY_MARK_PATIENCE`]) so a healing wave isn't pre-empted by a mid-heal IDR. /// + /// This is the whole-hearsay entry point: it believes an anchor on sight. A client whose decoder + /// parses the bitstream should call + /// [`on_decoded_corroborated`](Self::on_decoded_corroborated) instead and let its own parser + /// check the host's claim. + /// /// [`USER_FLAG_RECOVERY_ANCHOR`]: crate::packet::USER_FLAG_RECOVERY_ANCHOR /// [`USER_FLAG_RECOVERY_POINT`]: crate::packet::USER_FLAG_RECOVERY_POINT pub fn on_decoded( @@ -340,10 +408,49 @@ impl ReanchorGate { wire_flags: u32, decoder_keyframe: bool, now: Instant, + ) -> GateVerdict { + self.on_decoded_corroborated( + wire_flags, + decoder_keyframe, + AnchorEvidence::Unavailable, + now, + ) + } + + /// [`on_decoded`](Self::on_decoded) for a client that can CHECK the host's re-anchor claim + /// against its own decoder — the native-decode lanes, which parse every AU themselves and so + /// know both which pictures this one predicts from and whether each of those decoded cleanly. + /// + /// `evidence` is consulted for exactly one thing: whether a + /// [`USER_FLAG_RECOVERY_ANCHOR`](crate::packet::USER_FLAG_RECOVERY_ANCHOR) on THIS frame may + /// lift the freeze. [`AnchorEvidence::ReferencesDamaged`] withholds that lift and nothing else, + /// and the two exclusions are as deliberate as the rule itself: + /// + /// * **A real IDR still lifts.** It predicts from nothing, so no evidence about its references + /// can bear on it — and the IDR is precisely the escalation a refused anchor is trying to + /// provoke. Refusing it would turn the fix into the permanent freeze it exists to avoid. + /// * **The two-mark [`USER_FLAG_RECOVERY_POINT`](crate::packet::USER_FLAG_RECOVERY_POINT) rule + /// is untouched**, including its [`RECOVERY_MARK_PATIENCE`] deadline push. An intra-refresh + /// wave heals by overwriting stripes rather than by predicting from one named picture, so + /// "this frame's references were damaged" says nothing about whether the wave completed. + /// + /// A refused anchor also leaves the backstop deadline exactly where the arm put it. That is the + /// point rather than an omission: the freeze becomes overdue on its ORIGINAL schedule, [`poll`](Self::poll) + /// re-asks, and the client escalates to a real IDR — the recovery the host's anchor failed to + /// deliver. Pushing the deadline out on a refusal would reward a host whose anchors do not work + /// with a longer wait. + pub fn on_decoded_corroborated( + &mut self, + wire_flags: u32, + decoder_keyframe: bool, + evidence: AnchorEvidence, + now: Instant, ) -> GateVerdict { self.no_output_streak = 0; let is_keyframe = decoder_keyframe || (wire_flags & FLAG_SOF as u32 != 0); - let has_anchor = wire_flags & USER_FLAG_RECOVERY_ANCHOR != 0; + // An anchor the client's own parser refutes is not an anchor. Folded in HERE rather than + // inside `reanchor_after_frame` so that function stays a pure statement of the wire rules. + let has_anchor = wire_flags & USER_FLAG_RECOVERY_ANCHOR != 0 && evidence.honours_anchor(); let has_mark = wire_flags & USER_FLAG_RECOVERY_POINT != 0; if has_mark && self.awaiting { self.deadline = Some(now + RECOVERY_MARK_PATIENCE); @@ -888,4 +995,188 @@ mod tests { assert!(!g.poll(0, t + Duration::from_millis(1))); assert!(g.is_holding()); } + + // ---- the corroborated-anchor path (AnchorEvidence) ---- + + use AnchorEvidence::{ReferencesClean, ReferencesDamaged, Unavailable}; + + /// The headline. The host says "this P-frame re-anchors you"; the client's own parser says the + /// picture it predicts from is one IT had to conceal. Both cannot be true, and the client's + /// statement is about its OWN decoder — so the anchor does not lift and the gray plate the + /// anchor would have presented never reaches the screen. + #[test] + fn an_anchor_whose_references_the_decoder_concealed_does_not_lift() { + let mut g = ReanchorGate::new(0); + let now = t0(); + g.arm(now); + assert_eq!( + g.on_decoded_corroborated(ANCHOR, false, ReferencesDamaged, now), + GateVerdict::Hold, + "a refuted anchor is not a re-anchor" + ); + assert!(g.is_holding(), "and the freeze stays up"); + // Repeating it changes nothing — a host that keeps sending anchors it cannot honour never + // talks its way past the gate. + assert_eq!( + g.on_decoded_corroborated(ANCHOR, false, ReferencesDamaged, now), + GateVerdict::Hold + ); + assert!(g.is_holding()); + } + + /// The escalation a refusal exists to provoke must still work. An IDR predicts from nothing, so + /// no evidence about damaged references can bear on it — refusing it too would convert this fix + /// into the permanent freeze it is meant to avoid. + #[test] + fn a_real_idr_lifts_even_while_the_evidence_refutes_anchors() { + // The decoder's own keyframe flag... + let mut g = ReanchorGate::new(0); + let now = t0(); + g.arm(now); + assert_eq!( + g.on_decoded_corroborated(ANCHOR, false, ReferencesDamaged, now), + GateVerdict::Hold + ); + assert_eq!( + g.on_decoded_corroborated(0, true, ReferencesDamaged, now), + GateVerdict::Present, + "the IDR re-anchors regardless of what the anchor evidence says" + ); + assert!(!g.is_holding()); + + // ...and the wire's FLAG_SOF, for the lanes whose decoder does not flag IDRs. + let mut g = ReanchorGate::new(0); + g.arm(now); + assert_eq!( + g.on_decoded_corroborated(SOF, false, ReferencesDamaged, now), + GateVerdict::Present + ); + assert!(!g.is_holding()); + } + + /// A corroborated anchor is still an anchor: the whole point is to refuse the ones the client + /// can disprove, not to stop honouring the mechanism. + #[test] + fn a_corroborated_anchor_lifts_on_the_first_occurrence() { + let mut g = ReanchorGate::new(0); + let now = t0(); + g.arm(now); + assert_eq!( + g.on_decoded_corroborated(0, false, ReferencesClean, now), + GateVerdict::Hold, + "an ordinary frame is still withheld" + ); + assert_eq!( + g.on_decoded_corroborated(ANCHOR, false, ReferencesClean, now), + GateVerdict::Present + ); + assert!(!g.is_holding()); + } + + /// `Unavailable` is the promise made to every lane without a local parser: silence is not + /// refutation. This walks the same sequences the wire-path tests above assert and requires the + /// identical verdicts through the corroborated entry point. + #[test] + fn an_uncorroborated_lane_behaves_exactly_as_it_always_has() { + // The anchor lift, byte for byte the `a_gap_lifts_on_the_first_rfi_anchor` contract. + let mut g = ReanchorGate::new(0); + let now = t0(); + g.arm(now); + assert_eq!( + g.on_decoded_corroborated(0, false, Unavailable, now), + GateVerdict::Hold + ); + assert_eq!( + g.on_decoded_corroborated(ANCHOR, false, Unavailable, now), + GateVerdict::Present + ); + assert!(!g.is_holding()); + + // And `on_decoded` — which every such lane actually calls — must agree with it exactly. + let mut wire = ReanchorGate::new(0); + let mut corroborated = ReanchorGate::new(0); + wire.arm(now); + corroborated.arm(now); + for flags in [0, POINT, 0, ANCHOR, SOF, 0] { + assert_eq!( + wire.on_decoded(flags, false, now), + corroborated.on_decoded_corroborated(flags, false, Unavailable, now), + "flags {flags:#x} diverged between the two entry points" + ); + assert_eq!(wire.is_holding(), corroborated.is_holding()); + } + } + + /// A refused anchor must not buy the host time. The freeze becomes overdue on the deadline the + /// ARM set — not one pushed out by the refusal — so the client escalates to the real IDR that + /// the failed anchor did not deliver. + #[test] + fn a_refused_anchor_leaves_the_backstop_on_its_original_deadline() { + let mut g = ReanchorGate::new(0); + let start = t0(); + g.arm(start); + // Anchors keep arriving and keep being refused, right up to the deadline. + for ms in [10, 100, 300, 490] { + assert_eq!( + g.on_decoded_corroborated( + ANCHOR, + false, + ReferencesDamaged, + start + Duration::from_millis(ms) + ), + GateVerdict::Hold + ); + assert!(!g.poll(0, start + Duration::from_millis(ms)), "not yet due"); + } + let overdue = start + REANCHOR_FREEZE_MAX + Duration::from_millis(1); + assert!( + g.poll(0, overdue), + "the backstop fires on the arm's own deadline — the refusals did not extend it" + ); + assert!( + g.is_holding(), + "and it keeps holding, never resuming to gray" + ); + } + + /// Refuting an anchor says nothing about an intra-refresh wave: a wave heals by overwriting + /// stripes rather than by predicting from one named picture, so the two-mark rule and its + /// patience deadline must be untouched by the evidence. + #[test] + fn refuted_anchors_do_not_disturb_the_two_mark_rule() { + let mut g = ReanchorGate::new(0); + let now = t0(); + g.arm(now); + assert_eq!( + g.on_decoded_corroborated(POINT, false, ReferencesDamaged, now), + GateVerdict::Hold, + "mark #1 is still only half a re-anchor" + ); + // An anchor in between is refused and must not consume or reset the mark count. + assert_eq!( + g.on_decoded_corroborated(ANCHOR, false, ReferencesDamaged, now), + GateVerdict::Hold + ); + assert_eq!( + g.on_decoded_corroborated(POINT, false, ReferencesDamaged, now), + GateVerdict::Present, + "mark #2 lifts exactly as it does on the wire path" + ); + assert!(!g.is_holding()); + } + + /// The evidence is consulted only while an anchor flag is actually present — a refutation on an + /// ordinary frame must not become a second, sticky reason to hold. + #[test] + fn damaged_evidence_alone_neither_holds_nor_arms_an_unfrozen_gate() { + let mut g = ReanchorGate::new(0); + let now = t0(); + assert_eq!( + g.on_decoded_corroborated(0, false, ReferencesDamaged, now), + GateVerdict::Present, + "an unfrozen gate presents; the evidence is about anchors, not about frames" + ); + assert!(!g.is_holding()); + assert!(!g.poll(0, now)); + } } diff --git a/crates/punktfunk-host/src/native/stream.rs b/crates/punktfunk-host/src/native/stream.rs index 2bade60a..98a1a62a 100644 --- a/crates/punktfunk-host/src/native/stream.rs +++ b/crates/punktfunk-host/src/native/stream.rs @@ -2893,15 +2893,46 @@ pub(super) fn virtual_stream(ctx: SessionContext, prepared: Option 0; + tracing::debug!(rfi_unhealed, "forcing keyframe (client decode recovery)"); + if rfi_unhealed { + enc.distrust_references(); + } enc.request_keyframe(); last_forced_idr = Some(now); rfi_echo_swallowed = 0; // the IDR resets the episode — echoes of IT coalesce via the cooldown diff --git a/include/punktfunk_core.h b/include/punktfunk_core.h index b2785c7c..16e90303 100644 --- a/include/punktfunk_core.h +++ b/include/punktfunk_core.h @@ -4163,6 +4163,15 @@ void punktfunk_reanchor_gate_arm_expecting_drops(ReanchorGate *g, uint64_t expec // `USER_FLAG_RECOVERY_POINT`. Pass `decoder_keyframe = false` where the platform decoder doesn't flag // IDRs (VideoToolbox/MediaCodec) — the wire `FLAG_SOF` covers it. // +// This is the uncorroborated entry point and deliberately stays that way. Rust embedders whose +// decoder parses the bitstream call [`ReanchorGate::on_decoded_corroborated`] to let their own +// parser refute a `USER_FLAG_RECOVERY_ANCHOR` that names a picture they had to conceal; every +// client reachable through THIS surface (Apple VideoToolbox, Android MediaCodec) uses a platform +// decoder that surfaces no such fact, so it would have nothing to pass but +// `AnchorEvidence::Unavailable` — which is exactly what this wrapper already means. Growing a +// second export for a corroboration no C caller can supply would spend an ABI version bump on +// dead surface. +// // # Safety // `g` is a valid gate handle; `out_present` is writable or NULL. PunktfunkStatus punktfunk_reanchor_gate_on_decoded(ReanchorGate *g, diff --git a/plugin-kit/package.json b/plugin-kit/package.json index eb8fa5d3..689e5a27 100644 --- a/plugin-kit/package.json +++ b/plugin-kit/package.json @@ -1,6 +1,6 @@ { "name": "@punktfunk/plugin-kit", - "version": "0.4.1", + "version": "0.4.2", "description": "Effect-based framework for punktfunk plugins: lifecycle runtime, config/state, sync engine, UI serving, CLI scaffold, and browser helpers.", "type": "module", "license": "MIT OR Apache-2.0", diff --git a/plugin-kit/src/library/parsers/art.ts b/plugin-kit/src/library/parsers/art.ts index 90cbdc88..35cdba03 100644 --- a/plugin-kit/src/library/parsers/art.ts +++ b/plugin-kit/src/library/parsers/art.ts @@ -54,21 +54,44 @@ export const steamCdnUrl = ( return `https://cdn.cloudflare.steamstatic.com/steam/apps/${appid}/${file}`; }; -/** Filenames Steam's local `librarycache` uses per kind, in preference order (2x is sharper). */ +/** + * Filenames Steam's local `librarycache` uses per kind, in preference order (2x is sharper). + * + * Two spellings per kind, because Steam renamed these assets and one cache holds both eras side by + * side — on a 779-app cache, 594 appids carry `header.jpg` and 122 carry `library_header.jpg`, and + * NO appid carries both. Same story for the cover: 46 appids have only `library_capsule.jpg`. The + * renamed files are the same assets, byte-for-byte the same shapes (cover 300×450, header 460×215), + * so which name wins is cosmetic — but knowing only one name loses the art outright. + * + * Missing the cover is the one that shows: the fallback is then the flat CDN URL, which 404s for + * anything Valve has re-hashed, so the client walks on to the header and draws a BANNER in a 2:3 + * poster slot (Forza Horizon 6 / appid 2483190 is the reference case). + */ const localFilenames = (kind: ArtKind): string[] => kind === "portrait" - ? ["library_600x900_2x.jpg", "library_600x900.jpg"] + ? ["library_600x900_2x.jpg", "library_600x900.jpg", "library_capsule.jpg"] : kind === "hero" ? ["library_hero.jpg"] : kind === "logo" ? ["logo.png"] : // Steam's local cache names the header asset differently from the store CDN's - // `header.jpg` — this trips everyone once. - ["library_header.jpg"]; + // `header.jpg` — this trips everyone once. Newer entries use the CDN's name, so + // both belong here. + ["library_header.jpg", "header.jpg"]; /** - * This kind's file under one Steam root's `appcache/librarycache///`, or `undefined`. - * Steam reuses one hash dir per asset version, so there is normally exactly one candidate. + * This kind's file under one Steam root's `appcache/librarycache/`, or `undefined`. + * + * Three layouts, all of them live in the same cache at the same time — a title's art is in exactly + * one of them, so all three have to be checked or its cover is simply not found: + * + * 1. `//` — per-asset-version hash dir. Steam reuses one hash dir per version, + * so there is normally exactly one candidate. Checked first: where a title has been re-fetched + * into this layout, this is the copy Steam itself is displaying. + * 2. `/` — straight in the appid dir, and the MAJORITY case (623 of 779 appids on the + * reference cache). A hash-dir-only walk misses every one of them, which stayed invisible only + * because the flat CDN URL those titles fall back to still resolves for older appids. + * 3. `_` flat in `librarycache/` — the oldest layout. */ export const findLocalArtFile = ( root: string, @@ -82,6 +105,11 @@ export const findLocalArtFile = ( if (isFile(p)) return p; } } + // Layout 2: no hash dir, the asset sits directly in the appid dir. + for (const name of localFilenames(kind)) { + const p = path.join(base, name); + if (isFile(p)) return p; + } // Older Steam wrote the files directly under `librarycache/` with the appid in the name. for (const name of localFilenames(kind)) { const flat = path.join( diff --git a/plugin-kit/test/library-parsers.test.ts b/plugin-kit/test/library-parsers.test.ts index 55ead4bf..b4b3e983 100644 --- a/plugin-kit/test/library-parsers.test.ts +++ b/plugin-kit/test/library-parsers.test.ts @@ -280,6 +280,62 @@ describe("art locations", () => { fs.rmSync(dir, { recursive: true, force: true }); }); + test("finds a cover cached under Steam's newer `library_capsule` name", () => { + // The bug this pins: appid 2483190 (Forza Horizon 6) caches its 300×450 cover as + // `library_capsule.jpg`, the flat CDN URL for its `library_600x900.jpg` 404s, and the client + // therefore fell through to the header and drew a banner in a 2:3 poster slot. + const dir = tmp("art-capsule"); + const hashDir = path.join( + dir, + "appcache", + "librarycache", + "2483190", + "711e", + ); + fs.mkdirSync(hashDir, { recursive: true }); + fs.writeFileSync(path.join(hashDir, "library_capsule.jpg"), "x"); + expect(findLocalArtFile(dir, 2483190, "portrait")).toBe( + path.join(hashDir, "library_capsule.jpg"), + ); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + test("finds art stored straight in the appid dir, with no hash dir", () => { + // The majority layout — 623 of 779 appids on the reference cache. A hash-dir-only walk finds + // none of it and silently falls back to a CDN URL that 404s for anything re-hashed. + const dir = tmp("art-flat"); + const appDir = path.join(dir, "appcache", "librarycache", "813230"); + fs.mkdirSync(appDir, { recursive: true }); + fs.writeFileSync(path.join(appDir, "library_600x900.jpg"), "x"); + fs.writeFileSync(path.join(appDir, "header.jpg"), "x"); + expect(findLocalArtFile(dir, 813230, "portrait")).toBe( + path.join(appDir, "library_600x900.jpg"), + ); + // `header.jpg` is the CDN's name, but the local cache uses it too for newer entries — the + // two spellings are the same 460×215 asset and never appear together for one appid. + expect(findLocalArtFile(dir, 813230, "header")).toBe( + path.join(appDir, "header.jpg"), + ); + expect(findLocalArtFile(dir, 813230, "hero")).toBeUndefined(); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + test("a hash dir wins over a loose file of the same kind", () => { + // No appid on the reference cache carries both, so this is only about which copy is the + // current one if Steam ever leaves the old layout behind: the hash dir is what it re-fetches + // into, so that is the copy it is itself displaying. + const dir = tmp("art-both"); + const appDir = path.join(dir, "appcache", "librarycache", "570"); + const hashDir = path.join(appDir, "abc123"); + fs.mkdirSync(hashDir, { recursive: true }); + fs.writeFileSync(path.join(appDir, "library_600x900.jpg"), "x"); + fs.writeFileSync(path.join(hashDir, "library_600x900.jpg"), "x"); + expect(findLocalArtFile(dir, 570, "portrait")).toBe( + path.join(hashDir, "library_600x900.jpg"), + ); + fs.rmSync(dir, { recursive: true, force: true }); + }); + test("fileUrl produces the host's local-art contract shape", () => { const u = fileUrl(path.join(path.sep, "home", "u", "My Games", "c.jpg")); expect(u.startsWith("file:///")).toBe(true);