From 296569232e3e300483955f29d36f69d0647506d4 Mon Sep 17 00:00:00 2001 From: Hermes Date: Tue, 3 Nov 2026 22:49:36 +0100 Subject: [PATCH] fix: hide cursor natively, keep OSD on top, pin codec extension contract (#3) --- internal/display/x11.go | 104 ++++++++++++++++++------ internal/display/x11_test.go | 42 ++++++---- internal/osd/contract_test.go | 144 ++++++++++++++++++++++++++++++++++ internal/osd/osd.go | 18 +++++ 4 files changed, 271 insertions(+), 37 deletions(-) diff --git a/internal/display/x11.go b/internal/display/x11.go index cec2a61..7156d35 100644 --- a/internal/display/x11.go +++ b/internal/display/x11.go @@ -117,21 +117,27 @@ func stackModeForVideoAppear(role LayerRole) uint32 { return xproto.StackModeBelow } -// stackReassertTarget reports which video window the OSD overlay must sit -// above after an external restack, and whether one exists. Only the OSD -// overlay re-asserts its place: window managers promote fullscreened video -// to the top of their stacking order after the video-appear restack, which -// would bury the overlay — and its text — for the rest of the flight. The -// fallback canvas keeps its event-driven lifecycle, and the video window is -// never moved. -func stackReassertTarget(role LayerRole, videoActive bool, topVideo xproto.Window, videoWindows map[xproto.Window]struct{}) (xproto.Window, bool) { - if role != RoleAbove || !videoActive || topVideo == 0 { - return 0, false +// stackReassert decides the OSD overlay's stacking correction on its +// refresh: above the tracked video window while video is live, and raised +// to the top while video is absent — the fallback canvas raises itself to +// the top when video ends and would otherwise bury the overlay's text under +// the fullscreen presentation indefinitely. Only the OSD overlay ever +// re-asserts; the canvas and video windows are never moved by this. +func stackReassert(role LayerRole, videoActive bool, topVideo xproto.Window, videoWindows map[xproto.Window]struct{}) (sibling xproto.Window, mode uint32, ok bool) { + if role != RoleAbove { + return 0, 0, false + } + if !videoActive { + // No sibling: raise the overlay to the top of the stack. + return 0, xproto.StackModeAbove, true + } + if topVideo == 0 { + return 0, 0, false } if _, tracked := videoWindows[topVideo]; !tracked { - return 0, false + return 0, 0, false } - return topVideo, true + return topVideo, xproto.StackModeAbove, true } // raisesOnVideoEnd reports whether a layer raises itself when video ends: @@ -180,6 +186,43 @@ func osdFallbackFontMetrics() (ascent, descent, cell int16) { return 11, 3, 6 } +// createWindowValues builds the CreateWindow value mask and values; the +// order must follow the mask bit order (back pixel, event mask, cursor) so +// the server reads each value correctly. The blank cursor hides the pointer +// over the layer from its first frame, independently of video, wfb-ng or +// any cursor-hiding helper process. +func createWindowValues(backPixel uint32, eventMask uint32, cursor xproto.Cursor) (uint32, []uint32) { + values := []uint32{backPixel, eventMask, uint32(cursor)} + return xproto.CwBackPixel | xproto.CwEventMask | xproto.CwCursor, values +} + +// newBlankCursor builds the invisible cursor from zeroed 1x1 source and +// mask pixmaps: an all-zero mask displays nothing regardless of the source +// contents, so the pointer disappears over the layer without any shell +// polling or helper process. The pixmaps and graphics context are freed +// immediately; the cursor itself lives until the session closes. +func newBlankCursor(conn *xgb.Conn, screen *xproto.ScreenInfo) xproto.Cursor { + pixmap, _ := xproto.NewPixmapId(conn) + xproto.CreatePixmap(conn, screen.RootDepth, pixmap, + xproto.Drawable(screen.Root), 1, 1) + mask, _ := xproto.NewPixmapId(conn) + xproto.CreatePixmap(conn, screen.RootDepth, mask, + xproto.Drawable(screen.Root), 1, 1) + gc, _ := xproto.NewGcontextId(conn) + xproto.CreateGC(conn, gc, xproto.Drawable(pixmap), 0, nil) + unit := []xproto.Rectangle{{Width: 1, Height: 1}} + xproto.PolyFillRectangle(conn, xproto.Drawable(pixmap), gc, unit) + xproto.PolyFillRectangle(conn, xproto.Drawable(mask), gc, unit) + xproto.FreeGC(conn, gc) + cursor, _ := xproto.NewCursorId(conn) + // Zeroed colors and hotspot: with the all-zero mask nothing is ever + // displayed, so the fore/back colors never reach the screen. + xproto.CreateCursor(conn, cursor, pixmap, mask, 0, 0, 0, 0, 0, 0, 0, 0) + xproto.FreePixmap(conn, pixmap) + xproto.FreePixmap(conn, mask) + return cursor +} + // osdTextRect is the output region covering one drawn text: its glyph cell // around the baseline, widened by the shape padding. func osdTextRect(x, y, units int, cell, ascent, descent int16) xproto.Rectangle { @@ -302,11 +345,15 @@ func (o X11Opener) Open(ctx context.Context) (Session, error) { if o.Role == RoleAbove { eventMask = osdEventMask } - values := uint32(xproto.CwBackPixel | xproto.CwEventMask) + // The blank cursor hides the pointer over this layer from its first + // frame and stays on the layer across no-video/video transitions; the + // server reclaims it when the session's connection closes. + cursor := newBlankCursor(conn, screen) + valueMask, values := createWindowValues(screen.BlackPixel, eventMask, cursor) xproto.CreateWindow(conn, screen.RootDepth, window, screen.Root, 0, 0, screen.WidthInPixels, screen.HeightInPixels, 0, - xproto.WindowClassInputOutput, screen.RootVisual, values, - []uint32{screen.BlackPixel, eventMask}) + xproto.WindowClassInputOutput, screen.RootVisual, valueMask, + values) // Tag the window as a Ground Station layer and announce its EWMH role // before mapping, so no observer or window manager ever sees an // untagged frame of our own layers. @@ -442,6 +489,7 @@ func (o X11Opener) Open(ctx context.Context) (Session, error) { markerAtom: markerAtom, role: o.Role, unicodeFont: unicodeFont, + cursor: cursor, fontAscent: ascent, fontDescent: descent, fontCell: cell, @@ -711,6 +759,9 @@ type x11Session struct { markerAtom xproto.Atom role LayerRole unicodeFont bool + // cursor is the blank cursor applied to the layer window at creation; + // it is freed when the session closes. + cursor xproto.Cursor // fontAscent, fontDescent and fontCell describe the drawn font so the // OSD overlay can punch its output shape around each text's glyph cell. fontAscent int16 @@ -849,17 +900,22 @@ func (s *x11Session) WaitEvent(ctx context.Context) (bool, error) { } } -// reassertStacking re-claims the overlay's place above the presented video -// window; a no-op for the canvas and whenever video is absent. +// reassertStacking re-claims the overlay's stacking place on the OSD's +// slower refresh cadence: above the live video window, or on top of the +// stack while video is absent so the fallback canvas can never bury it. A +// no-op for the canvas layer. func (s *x11Session) reassertStacking() { - if s.video == nil || !s.video.Active() { - return - } - target, ok := stackReassertTarget(s.role, s.video.Active(), s.topVideo, s.videoWindows) + videoActive := s.video != nil && s.video.Active() + sibling, mode, ok := stackReassert(s.role, videoActive, s.topVideo, s.videoWindows) if !ok { return } - stackWindowRelative(s.conn, s.window, target, xproto.StackModeAbove) + if sibling != 0 { + stackWindowRelative(s.conn, s.window, sibling, mode) + return + } + xproto.ConfigureWindow(s.conn, s.window, + xproto.ConfigWindowStackMode, []uint32{mode}) } // applyVideoChange folds one root structure event into the shared video @@ -983,7 +1039,9 @@ func foldToASCII(message string) string { } func (s *x11Session) Close() error { - // Closing the connection makes the X server destroy the kiosk window. + // Free the blank cursor explicitly; closing the connection then makes + // the X server reclaim everything else, including the kiosk window. + xproto.FreeCursor(s.conn, s.cursor) s.conn.Close() return nil } diff --git a/internal/display/x11_test.go b/internal/display/x11_test.go index dbcc0e6..fca180d 100644 --- a/internal/display/x11_test.go +++ b/internal/display/x11_test.go @@ -286,27 +286,41 @@ func TestOSDEventMaskNeverSelectsDeviceInput(t *testing.T) { } } -func TestStackReassertTargetOnlyAboveLiveTrackedVideo(t *testing.T) { - // Window managers promote fullscreened video to the top of their - // stacking order after the video-appear restack, burying the OSD - // overlay; the overlay's refresh must re-assert its place above the - // tracked video window — and only the OSD overlay may do so. +func TestStackReassertKeepsTheOSDOnTop(t *testing.T) { + // While video is live the overlay must sit above the tracked video + // window; while video is absent the overlay must rise to the top, + // because the fallback canvas raises itself to the top when video ends + // and would otherwise bury the overlay's text. Only the OSD overlay + // re-asserts; the canvas and video windows are never moved by this. windows := map[xproto.Window]struct{}{0x20: {}} - if target, ok := stackReassertTarget(RoleAbove, true, 0x20, windows); !ok || target != 0x20 { - t.Fatalf("target=%v ok=%v want the tracked video window", target, ok) + if sibling, mode, ok := stackReassert(RoleAbove, true, 0x20, windows); !ok || sibling != 0x20 || mode != xproto.StackModeAbove { + t.Fatalf("sibling=%v mode=%v ok=%v want above the tracked video window", sibling, mode, ok) } - if _, ok := stackReassertTarget(RoleBelow, true, 0x20, windows); ok { + if sibling, mode, ok := stackReassert(RoleAbove, false, 0, nil); !ok || sibling != 0 || mode != xproto.StackModeAbove { + t.Fatalf("sibling=%v mode=%v ok=%v want a top raise while video is absent", sibling, mode, ok) + } + if _, _, ok := stackReassert(RoleBelow, true, 0x20, windows); ok { t.Fatal("the fallback canvas must never re-assert stacking") } - if _, ok := stackReassertTarget(RoleAbove, false, 0x20, windows); ok { - t.Fatal("no re-assert while video is absent") - } - if _, ok := stackReassertTarget(RoleAbove, true, 0x30, windows); ok { + if _, _, ok := stackReassert(RoleAbove, true, 0x30, windows); ok { t.Fatal("a stale top video window must not be restacked against") } - if _, ok := stackReassertTarget(RoleAbove, true, 0, windows); ok { - t.Fatal("no tracked video window means nothing to re-assert") + if _, _, ok := stackReassert(RoleAbove, true, 0, windows); ok { + t.Fatal("no tracked video window means no video-relative restack") + } +} + +func TestCreateWindowValuesOrderMatchesTheValueMask(t *testing.T) { + // The CreateWindow values must follow the mask bit order (back pixel, + // event mask, cursor) so the server reads each value correctly; the + // blank cursor hides the pointer over the layer from its first frame. + mask, values := createWindowValues(0x100, 0x200, 0xc0de) + if mask != xproto.CwBackPixel|xproto.CwEventMask|xproto.CwCursor { + t.Fatalf("mask=%b want back pixel, event mask and cursor bits", mask) + } + if len(values) != 3 || values[0] != 0x100 || values[1] != 0x200 || values[2] != 0xc0de { + t.Fatalf("values=%v want [backPixel eventMask cursor]", values) } } diff --git a/internal/osd/contract_test.go b/internal/osd/contract_test.go index c24e36d..a21bd00 100644 --- a/internal/osd/contract_test.go +++ b/internal/osd/contract_test.go @@ -1,7 +1,12 @@ package osd import ( + "os" + "path/filepath" "testing" + "time" + + "git.piksar.eu/piotr/groundstation/internal/protocol" ) func TestTableSelectionRequiresAnIdentifier(t *testing.T) { @@ -129,3 +134,142 @@ func TestVTXLifecycleStatesAreDistinct(t *testing.T) { t.Fatal("pending, applied and error must be distinct lifecycle states") } } + +func TestSnapshotDomainsAreEncodedInDisjointByteRanges(t *testing.T) { + // The four contract domains — operator configuration (table identity + // and ceiling), the requested policy level, the applied acknowledgment + // level and the observed link — must occupy disjoint byte ranges: a + // change to one domain may never alias another domain's bytes, so + // future extensions land behind a version bump instead of colliding. + base := Snapshot{ + Sequence: 1, + TimestampMS: 2, + Channels: [protocol.Channels]int16{}, + Armed: false, + Video: false, + OpConfig: OperatorConfig{ + Table: Table{Gen: 1, Hash: 1}, + MaxPower: 1, + }, + Policy: GSPolicy{Level: 1}, + Ack: VTXAck{ + Table: Table{Gen: 1}, + Level: 1, + Lifecycle: VTXLifecycleApplied, + }, + Link: GSObservedLink{Known: false}, + } + + deltas := map[string]struct { + rangeStart, rangeEnd int + mutate func(*Snapshot) + }{ + "operator-config": {opMaxPowerOffset, policyLevelOffset, func(s *Snapshot) { s.OpConfig.MaxPower = 2 }}, + "policy-level": {policyLevelOffset, ackTableGenOffset, func(s *Snapshot) { s.Policy.Level = 2 }}, + "ack-level": {ackTableGenOffset, linkAgeOffset, func(s *Snapshot) { s.Ack.Level = 2 }}, + "observed-link": {linkAgeOffset, crcOffset, func(s *Snapshot) { s.Link.AgeMS = 2 }}, + } + reference := Encode(base) + for name, delta := range deltas { + modified := base + delta.mutate(&modified) + encoded := Encode(modified) + if len(encoded) != len(reference) { + t.Fatalf("%s: encoded length changed", name) + } + for i := range encoded { + if encoded[i] == reference[i] { + continue + } + // The CRC covers the whole record and necessarily changes with + // any domain change; every payload byte must stay inside the + // domain's own range. + if i >= crcOffset { + continue + } + if i < delta.rangeStart || i >= delta.rangeEnd { + t.Fatalf("%s: byte %d changed outside the domain range [%d,%d)", + name, i, delta.rangeStart, delta.rangeEnd) + } + } + if _, err := Decode(encoded); err != nil { + t.Fatalf("%s: modified snapshot must still decode: %v", name, err) + } + } +} + +func TestDecodeRejectsCorruptionAtomically(t *testing.T) { + // Malformed input must never yield a partially decoded snapshot that + // could clear or corrupt one domain while keeping others: any flipped + // byte anywhere in the record rejects the whole snapshot, and the + // publisher keeps its last valid state until a new valid one arrives. + encoded := Encode(Snapshot{ + OpConfig: OperatorConfig{Table: Table{Gen: 5, Hash: 5}, MaxPower: 5}, + Policy: GSPolicy{Level: 5}, + Ack: VTXAck{Level: 5, Lifecycle: VTXLifecycleApplied}, + Link: GSObservedLink{Known: true, AgeMS: 5}, + }) + // Sample corruption positions across the whole record — magic, body + // regions and CRC — derived from the actual encoded length, so the + // atomicity contract holds across future version bumps and layout + // extensions without coupling to today's offsets. + positions := []int{ + 0, + len(encoded) / 4, + len(encoded) / 2, + 3 * len(encoded) / 4, + len(encoded) - 1, + } + for _, offset := range positions { + corrupt := append([]byte(nil), encoded...) + corrupt[offset] ^= 0xff + got, err := Decode(corrupt) + if err == nil { + t.Fatalf("flipped byte at %d must reject the record", offset) + } + if got != (Snapshot{}) { + t.Fatalf("flipped byte at %d must not emit a partial snapshot", offset) + } + } +} + +func TestFileCorruptionDoesNotClearLastValidSnapshot(t *testing.T) { + // A corrupted state file reports no snapshot for the moment, but the + // publisher retains its last valid snapshot and the next heartbeat + // rewrite restores exactly those values: corruption never clears + // last valid data, and nothing is fabricated in between. + path := filepath.Join(t.TempDir(), "osd-state.bin") + now := time.UnixMilli(10_000_000) + source := NewFileSource(path) + source.now = func() time.Time { return now } + + want := testSnapshot() + want.PublishedMS = now.UnixMilli() + if err := os.WriteFile(path, Encode(want), 0644); err != nil { + t.Fatal(err) + } + if _, state := source.Latest(); state != SnapshotFresh { + t.Fatalf("state=%v want fresh before corruption", state) + } + + corrupt := append([]byte(nil), Encode(want)...) + corrupt[len(corrupt)/2] ^= 0xff + if err := os.WriteFile(path, corrupt, 0644); err != nil { + t.Fatal(err) + } + if _, state := source.Latest(); state != SnapshotNone { + t.Fatalf("state=%v want none while the file is corrupt", state) + } + + // The publisher's heartbeat rewrites its retained last valid snapshot. + if err := os.WriteFile(path, Encode(want), 0644); err != nil { + t.Fatal(err) + } + got, state := source.Latest() + if state != SnapshotFresh { + t.Fatalf("state=%v want fresh after the heartbeat restore", state) + } + if got != want { + t.Fatalf("restored snapshot=%+v want the untouched last valid %+v", got, want) + } +} diff --git a/internal/osd/osd.go b/internal/osd/osd.go index 3ee79f4..b190cbb 100644 --- a/internal/osd/osd.go +++ b/internal/osd/osd.go @@ -16,6 +16,24 @@ // intent only: it constrains what may be requested and is never proof of // applied VTX power, and static configuration is never rendered or serialized // as applied VTX state. +// +// Extension contract for future domains: the snapshot is an explicit, +// extensible foundation of independent domains — operator table identity, +// requested level, applied level, observed link, and flight telemetry +// (versioned WFB-derived battery/GPS/arm/flight-mode/attitude with source +// and freshness, future work). Telemetry domains land as new typed nested +// structs with their own explicit pending/unknown states, appended at fixed +// offsets before the CRC behind a version bump; the typed fields plus the +// versioned all-or-nothing codec are the extension seam, and no runtime +// placeholder fields exist before a domain is actually modeled. Domains are +// independent by construction: each occupies a disjoint byte range (pinned +// by contract tests), decoding is atomic — malformed or damaged records are +// rejected whole and can never clear one domain while keeping others — and +// the publisher retains its last valid snapshot, republishing it until a +// new valid snapshot replaces it. Flight telemetry stays independent of +// video and policy; battery, GPS and similar aircraft state must never be +// inferred from RC channels, and the only RC-derived flight state remains +// the already-defined CH5/AUX1 arm indication. package osd import (