Initial commit: Greenfield architectural analysis (14 files, ~80 pages, 10 parallel research agents)

11-pitfalls.md

@@ -0,0 +1,532 @@
+# 11 — Pitfalls, Anti-patterns, and What to Avoid
+
+This is the prominent pitfalls chapter. Greenfield's known weaknesses,
+anti-patterns, and design pitfalls are catalogued here so the user can
+avoid making the same mistakes in their own Wayland-to-HTML compositor
+(Odoo 20 + JS/Python). Each pitfall comes with: **what Greenfield does**
+(with file/directory reference), **why it's a problem** (technical
+reason), and **what the user should do instead** (concrete recommendation
+for their JS/Python + Odoo 20 stack).
+
+## Headline numbers
+
+- **299** `TODO|FIXME|HACK|XXX|@deprecated|workaround` hits across non-vendored source (after excluding `examples/sdk/*` upstreams and `xtsb/protocol/xproto.xml`).
+- **13 packages** all pinned to `"version": "1.0.0-rc1"` — no `CHANGELOG.md`, no `SECURITY.md`, no `CONTRIBUTING.md`, no issue/PR templates.
+- **1 commit** visible on `master` (`6c578f4`, "Fix typo in code snippet (#161)", 2025-10-15) — implying the public repo is a release snapshot, not the live dev branch.
+- **3 native Node `.node` addons**, all Linux-x64 only.
+- **0** uses of `OffscreenCanvas` or `transferControlToOffscreen` anywhere in the compositor.
+- **2** Web Workers used — and only for the WASM H.264 *fallback* decoder path. The primary WebCodecs path uses none.
+
+## Catalogued pitfalls (A–U)
+
+### Critical (will bite you on day 1)
+
+#### A. Native Node addons
+
+**What Greenfield does**: Three Node-API addons:
+- `wayland-server-addon.node` (1231 lines C, wraps patched libwayland-server)
+- `proxy-encoding-addon.node` (290 lines C, wraps GStreamer)
+- `proxy-poll-addon.node` (127 lines C, wraps libuv `uv_poll_t`)
+
+All Linux-x64 only, built with CMake + Ninja. ABI-locked to Node major
+versions.
+
+**Why it's a problem**: 
+- Linux-x64 only — no ARM64 support
+- ABI breaks on Node major upgrades
+- Hard to ship — must rebuild for each target platform
+- Hard to debug — C N-API debugging requires core dumps
+- GStreamer integration is via C, not Python
+- No Python story
+
+**What you should do instead**: Use `pywayland` (cffi bindings to
+libwayland-server), `python-xkbcommon`, and GStreamer's Python bindings
+(`gi.repository.Gst`). One Python process can replace the entire
+`compositor-proxy-cli`. See `10-porting-to-odoo-js-python.md` for details.
+
+#### B. rc1 with no policy
+
+**What Greenfield does**: All 13 packages at `1.0.0-rc1`. No CHANGELOG,
+no SECURITY.md, no CONTRIBUTING.md.
+
+**Why it's a problem**: Breaking changes can land silently. The
+maintainer's roadmap is unclear.
+
+**What you should do instead**: Pin to a specific commit hash if you fork
+it. Track upstream commits manually. Plan for the eventuality that
+1.0.0 final may break APIs.
+
+#### C. Encoding rigidity
+
+**What Greenfield does**: Three H.264 GStreamer pipelines (x264/nvh264/
+vaapih264) with hand-tuned magic constants:
+- `bitrate=12000 vbv-buf-capacity=160 bframes=0 rc-lookahead=0`
+- VA-API is "experimental"
+- AMD (`vah264enc`) absent
+- PNG fallback at hardcoded ≤256×256 threshold
+- Browser-side hardware decode is explicitly disabled in `Session.ts:93`
+  with a FIXME "hardware decoding frame offset is wrong"
+
+**Why it's a problem**: 
+- No adaptive bitrate — video plays at 12 Mbps regardless of network
+  conditions
+- No AMD support — limits hardware encoder options
+- Browser decode is software-only — wastes GPU
+
+**What you should do instead**: 
+- Build an adaptive multi-codec encoding router (PNG for ≤64×64 or static,
+  VP8/H.264 for mid-size, H.264/AV1 for video)
+- Drive the router by the `EncoderFeedback` measurement Greenfield already
+  produces but doesn't consume
+- Use `pykcp` or similar for congestion-aware bitrate adaptation
+- Enable browser hardware decode (`prefer-hardware`) — fix the "frame
+  offset is wrong" bug
+
+#### D. Wire protocol has no schema version field
+
+**What Greenfield does**: No version prefix on any binary message:
+- `EncodedFrame` (FRAME channel)
+- `RemoteOutOfBandChannel` (PROTOCOL channel OOB)
+- The FD-channel in `RemoteAppLauncher.ts:612`
+- Undocumented magic close codes (`4200`, `4201`, `4403`, `4001`, `1005`)
+
+All break-lockstep — if you change the wire format, both sides must
+upgrade simultaneously or messages will be misinterpreted.
+
+**Why it's a problem**: 
+- Cannot evolve the protocol without breaking all clients
+- Cannot detect version mismatches
+- Hard to debug — close codes are magic numbers
+
+**What you should do instead**: 
+- Prefix every binary message with `u16 protocolVersion`
+- Document every close code in a comment
+- Use JSON `{"v":1, "type":"..."}` for the control plane
+- Build a feature-flag system so old clients can negotiate capabilities
+
+#### E. Main-thread decode + render
+
+**What Greenfield does**: 
+- `VideoDecoder` runs on main thread
+- WebGL context bound to `HTMLCanvasElement`
+- No `OffscreenCanvas`
+- No `Worker` in render path
+- 0 uses of `OffscreenCanvas` or `transferControlToOffscreen` in the
+  compositor
+- Only 2 Web Workers used (FFmpeg Wasm fallback), shared across surfaces
+
+**Why it's a problem**: 
+- Jank on 1080p surfaces — main thread blocks on decode
+- No way to recover if main thread stalls (input also blocked)
+- Can't use `requestAnimationFrame` for compositing if main thread is busy
+
+**What you should do instead**: 
+- Render in a Web Worker with `OffscreenCanvas` from day one
+- Ship `EncodedVideoChunk` / `ArrayBuffer` transfers (zero-copy) to the
+  worker
+- Use `VideoFrame` directly as a WebGL texture source — no `copyTo`
+- Use multiple workers (one per output) for parallel rendering
+
+### High severity
+
+#### F. xkbcommon Wasm is multi-MB
+
+**What Greenfield does**: `libs/compositor-wasm/xkbcommon/build.sh`
+clones libxkbcommon 1.5.0, embeds the entire xkeyboard-config database
+via `--embed-file`. Result: multi-MB WASM module loaded on every page.
+
+**Why it's a problem**: 
+- Slow initial load
+- Layout list parsed via fragile string matching
+- Hard to update keyboard layouts without rebuilding WASM
+
+**What you should do instead**: 
+- Use the browser's `navigator.keyboard.getLayoutMap()` API for basic
+  layout detection
+- For full xkbcommon support, lazy-load the WASM only when needed
+- Consider server-side keymap compilation (Python `python-xkbcommon`)
+  and ship only the result string to the browser
+
+#### G. Clipboard edge cases
+
+**What Greenfield does**: 
+- Hardcoded MIME list `['text/plain', 'text/html', 'image/png']`
+- No primary selection (`wp_primary_selection_v1` not implemented)
+- Change detection by blob size (false negatives on same-size different
+  content)
+- Focus check at `DataOffer.ts:73` is a FIXME
+- Browser-side clipboard polling (no `clipboardchange` event)
+
+**Why it's a problem**: 
+- Middle-click paste doesn't work (Linux users expect it)
+- Large clipboard content (e.g. copied image) is poorly handled
+- MIME negotiation races
+
+**What you should do instead**: 
+- Implement `zwp_primary_selection_device_manager_v1` from day one
+- Use exact MIME matching, not substring
+- For large clipboard content, use chunked transfer
+- Watch for the `clipboardchange` event in modern browsers (currently
+  behind flags in Chrome)
+
+#### H. XWayland/xtsb complexity
+
+**What Greenfield does**: 
+- A full XCB reimplementation in TypeScript with its own 5800-line
+  `xproto.xml`
+- The XWM is a hand-port of wlroots' `xwm.c` with FIXMEs pointing at
+  specific wlroots commits
+- `XWindowManager.ts:746` literally says "this makes xterm close somehow"
+- Title bars painted pixel-by-pixel in `XWindowFrame.ts`
+
+**Why it's a problem**: 
+- 5800 lines of X11 protocol XML to maintain
+- Hand-ported XWM logic is fragile and out of sync with upstream wlroots
+- Magic workarounds in code
+
+**What you should do instead**: 
+- Use `python-xlib` on the Python proxy side instead of porting xtsb
+- Consider running XWayland with a minimal window manager (e.g. mutter
+  in kiosk mode) instead of implementing XWM yourself
+- For XWayland apps only (no full X11 desktop needed), the XWM can be
+  much simpler than wlroots'
+
+#### I. Adaptive bitrate feedback loop is wired but not consumed
+
+**What Greenfield does**: 
+- `EncoderFeedback.ts` measures client decode duration at 1 Hz
+- 4-byte feedback messages sent from browser to proxy
+- Proxy receives them but does NOT use them to adapt bitrate
+- `Channel.ts:181` has a TODO confirming the gap
+
+**Why it's a problem**: 
+- Video plays at fixed 12 Mbps regardless of network conditions
+- No way to recover from congestion
+- Wasted bandwidth on slow networks
+
+**What you should do instead**: 
+- Wire `EncoderFeedback` to a bitrate adaptation controller
+- Use `GstElement.set_property("bitrate", ...)` to adjust x264enc/nvh264enc
+- Implement `requestKeyUnit` plumbing (Greenfield has it end-to-end but
+  the browser never calls it)
+
+#### J. Single-seat, single-session model
+
+**What Greenfield does**: 
+- One seat per browser session
+- One Node process per browser session (per `compositorSessionId`)
+- No multi-seat support (e.g. multiple keyboards/mice for collaborative
+  editing)
+- No multi-region isolation
+
+**Why it's a problem**: 
+- Can't support multi-user scenarios (e.g. conference room with multiple
+  keyboards)
+- No way to isolate apps between tenants
+
+**What you should do instead**: 
+- For multi-tenant isolation: separate Python proxy processes per tenant,
+  containerized (Docker + bubblewrap)
+- For multi-seat: implement `wl_seat` global per seat, route input
+  events by seat
+
+#### K. Custom web-bitmapbuf protocol — apps must be Greenfield-aware
+
+**What Greenfield does**: 
+- `protocol/web-bitmapbuf-unstable-v1.xml` is Greenfield's custom
+  extension
+- Lets browser JS apps hand a GPU-backed `ImageBitmap` directly to the
+  compositor
+- Standard Wayland apps (even browser-resident ones) fall to slow H.264
+  path
+
+**Why it's a problem**: 
+- Apps must be rebuilt against `@gfld/client-protocol` to use the fast
+  path
+- Standard Wayland apps can't take advantage of zero-copy GPU textures
+- Browser-resident WASM apps are slow unless rebuilt
+
+**What you should do instead**: 
+- Document clearly that the fast path requires Greenfield-aware apps
+- For native Linux apps (your main use case), this isn't an issue — they
+  go through the H.264 path regardless
+- For browser-resident WASM apps, encourage users to build against
+  `@gfld/client-protocol`
+
+#### L. Connection loss handling
+
+**What Greenfield does**: 
+- 10-minute zombie timeout on disconnect (`NativeAppContext.ts:152-160`)
+- No state resumption on reconnect
+- `eagerBufferContents` cache in `BufferStream.ts:31` is unbounded (TODO
+  at `:87` confirms the leak)
+- App kill timers: SIGHUP@10min, SIGKILL@+10s
+- Close code 1001 → immediate SIGHUP; 4001 → immediate SIGTERM
+
+**Why it's a problem**: 
+- User loses all running apps on browser refresh (unless they reconnect
+  within 10 min)
+- Memory leak from unbounded cache
+- No way to survive proxy restart
+
+**What you should do instead**: 
+- Bound the `eagerBufferContents` cache (e.g. max 100 frames)
+- Persist app state on browser side (localStorage) so users can resume
+- For proxy restart, checkpoint app state to disk (json file)
+- Implement graceful reconnection with state sync (the KCP plumbing
+  exists)
+
+#### M. No sandboxing
+
+**What Greenfield does**: 
+- `spawn(executable, args, { env })` with no bubblewrap/seccomp/namespaces
+- Basic Auth is a single replayable credential compared with `!==`
+- `compositorSessionId` is client-generated
+
+**Why it's a problem**: 
+- Native apps have full access to the host filesystem
+- One compromised app can read all other apps' data
+- Basic Auth credentials can be replayed
+- Client-generated session IDs can be spoofed
+
+**What you should do instead**: 
+- Sandbox every spawned app with bubblewrap + seccomp + per-session Linux
+  user namespace
+- Replace Basic Auth with short-lived signed JWT bound to Odoo's session
+  model
+- Server-generated session IDs (use `secrets.token_urlsafe(32)`)
+
+#### N. Heavy toolchain
+
+**What Greenfield does**: 
+- Yarn 4 + CMake + Ninja + Meson + pkg-config
+- Two different Emscripten versions: 3.1.46 for FFmpeg, 3.1.60 for SDK
+- SDK builds 30+ C libraries from source
+- gtk4 WASM is currently broken
+- Native build needs `autoconf libtool automake meson ninja-build bison
+  cmake build-essential gperf`
+
+**Why it's a problem**: 
+- Complex build environment setup
+- Long build times
+- Emscripten version skew between libs and SDK
+- gtk4 WASM broken means no GTK4 browser apps
+
+**What you should do instead**: 
+- Stick with a single Emscripten version
+- Pre-build WASM modules and ship them as artifacts (don't rebuild every
+  time)
+- For Python, use `pip install` for all dependencies (simpler than CMake
+  + Ninja)
+- Skip the WASM app SDK entirely if you only care about native Linux apps
+
+### Medium / low (but worth fixing)
+
+#### O. WebCodecs usage
+
+**What Greenfield does**: 
+- WebCodecs IS used (correcting the brief in some agents' notes)
+- BUT HW-decode is disabled
+- `VideoEncoder` is unused
+
+**Why it's a problem**: 
+- Wastes GPU on software decode
+- Could encode in browser (for upload to other peers) but doesn't
+
+**What you should do instead**: 
+- Enable HW-decode by fixing the "frame offset is wrong" FIXME
+- Consider WebCodecs `VideoEncoder` for any peer-to-peer video paths
+
+#### P. WebCodecs hardware decode disabled
+
+**What Greenfield does**: 
+- `Session.ts:93` has `// FIXME hardware decoding frame offset is wrong`
+- `hardwareAcceleration: 'prefer-software'` is hardcoded
+
+**Why it's a problem**: 
+- Software decode at 1080p60 is ~30% CPU on a modern laptop
+- Battery life impact on mobile
+- Wastes GPU
+
+**What you should do instead**: 
+- Investigate and fix the frame offset bug
+- Enable `prefer-hardware` for capable browsers
+- Provide a runtime toggle
+
+#### Q. Unbounded `eagerBufferContents` cache
+
+**What Greenfield does**: 
+- `BufferStream.ts:31` has unbounded cache
+- TODO at `:87` confirms the leak
+
+**Why it's a problem**: 
+- Memory leak on long-running sessions
+- Can OOM the browser tab
+
+**What you should do instead**: 
+- Bound the cache to a maximum number of frames (e.g. 100)
+- Use LRU eviction
+- Periodic cleanup of orphaned entries
+
+#### R. `H264NALDecoder.worker.ts` is minimal
+
+**What Greenfield does**: 
+- 22 lines of code
+- `@ts-ignore`'d
+- No error boundary
+
+**Why it's a problem**: 
+- Worker crashes silently
+- No retry on transient errors
+- No metrics on decode failures
+
+**What you should do instead**: 
+- Add error boundary with retry logic
+- Log decode failures to the proxy (for diagnostics)
+- Pool workers for resilience
+
+#### S. Magic undocumented WebSocket close codes
+
+**What Greenfield does**: 
+- Close codes `4200`, `4201`, `4403`, `4001`, `1005` are used without
+  documentation
+- Only codes 4000-4999 (application-level) tear down KCP
+
+**Why it's a problem**: 
+- Hard to debug connection issues
+- Close codes are magic numbers
+
+**What you should do instead**: 
+- Document every close code in code comments
+- Use named constants (e.g. `CLOSE_APP_TERMINATED = 4001`)
+- Build a close-code → human-readable mapping
+
+#### T. Generated protocol files committed
+
+**What Greenfield does**: 
+- `packages/compositor-proxy/src/protocol/*.js` is generated but
+  committed to the repo
+- Can drift from XML source
+
+**Why it's a problem**: 
+- Easy to forget to regenerate
+- PRs may include stale generated code
+
+**What you should do instead**: 
+- Don't commit generated files
+- Use `prebuild` script to generate them
+- Add a CI check that regenerated files match committed ones
+
+#### U. Confirmed copy-paste bug in `webcodec-buffer-decoder.ts`
+
+**What Greenfield does**: 
+- `webcodec-buffer-decoder.ts:142-144, 156-157` has:
+  `this.opaqueDecoder = undefined` twice
+- Should be `alphaDecoder` the second time
+- Leaks the alpha decoder after errors
+
+**Why it's a problem**: 
+- Memory leak
+- Wasm decoder context not cleaned up properly
+- Can cause decoder pool exhaustion
+
+**What you should do instead**: 
+- Audit all `cleanup` paths in decoder code
+- Add unit tests for error scenarios
+
+## Design wins to copy
+
+Not all of Greenfield's choices are bad. Here are 12 design wins worth
+copying verbatim:
+
+1. **Generator-driven protocol stubs** (`libs/compositor-generator/`,
+   `client-generator/`, `compositor-proxy-generator/`) — single XML source
+   for both sides. Eliminates wire-format drift.
+2. **Dual-buffer commit semantics** (`Surface.ts`: `state` vs
+   `pendingState`, atomic merge with pixman damage regions). Classic
+   Wayland correctness.
+3. **UserShellApi decoupling** — shell sees only `{events, actions}`,
+   never Wayland resources. This is THE integration seam you'll use.
+4. **Asynchronous browser compositor** — per-client channels, never blocks
+   on a slow client. Essential for multi-app scenarios.
+5. **KCP-over-WebSocket** for UDP-like latency over TCP (`kcp.ts`, 1170
+   lines). Better than plain TCP for real-time video.
+6. **WebCodecs primary + WASM fallback + capability probe** (`Session.ts`).
+   Modern browsers get the fast path, older ones still work.
+7. **Zero-copy dmabuf → encoder** via standard `linux-dmabuf-unstable-v1`
+   (`westfield-dmabuf.c`). Pixels stay on GPU.
+8. **Per-surface encoder feedback channel** (`EncoderFeedback.ts`)
+   measuring client decode duration — even though adaptation isn't wired
+   up yet, the telemetry is right.
+9. **`ReconnectingWebSocket`** with exponential backoff + message queue.
+   Survives transient network drops.
+10. **Minimal shell** — 562 lines of TypeScript total in `compositor-shell`;
+    kiosk variant is 40 lines. Demonstrates that the shell can be tiny.
+11. **Honest docs** — `docs/pages/design/` openly admits WebSocket/TCP is
+    wrong for real-time and waits on WebTransport. Useful as a reference
+    for what trade-offs were considered.
+12. **AGPL-3.0** — deliberate copyleft, but be aware before integrating.
+    If you want to keep your modifications proprietary, fork the MIT
+    libs and reimplement the AGPL parts.
+
+## Top-5 recommendations for the user's Odoo 20 + JS/Python compositor
+
+1. **Skip native Node addons entirely.** Use `pywayland` (cffi),
+   `python-xkbcommon`, and GStreamer's `gi.repository.Gst` Python
+   bindings. One Python process can replace the entire
+   `compositor-proxy-cli`. See `10-porting-to-odoo-js-python.md` for
+   details.
+
+2. **Sandbox every spawned app** with bubblewrap + seccomp + per-session
+   Linux user namespace; replace Basic Auth with short-lived signed JWT
+   bound to Odoo's session model.
+
+3. **Render in a Web Worker with `OffscreenCanvas`** from day one; ship
+   `EncodedVideoChunk` / `ArrayBuffer` transfers (zero-copy) to the
+   worker; use `VideoFrame` directly as a WebGL texture source — no
+   `copyTo`.
+
+4. **Build an adaptive multi-codec encoding router** (PNG for ≤64×64 or
+   static, VP8/H.264 for mid-size, H.264/AV1 for video), driven by the
+   `EncoderFeedback` measurement Greenfield already produces but doesn't
+   consume.
+
+5. **Version your wire protocol from commit #1** — prefix every binary
+   message with `u16 protocolVersion`; document every close code; use
+   JSON `{"v":1, "type":"..."}` for the control plane.
+
+## License implications of AGPL-3.0
+
+The four `packages/*` (compositor, compositor-proxy, compositor-proxy-cli,
+compositor-shell) are **AGPL-3.0-or-later**. Implications:
+
+- If you host Greenfield's `compositor-proxy` as a network service and
+  modify it, you MUST release your modifications under AGPL-3.0+.
+- The browser-side `@gfld/compositor` is also AGPL. If you modify it
+  (e.g. to integrate with Odoo), your modifications must be AGPL.
+- The lower-level `libs/*` (common, client-protocol, compositor-protocol,
+  the generators, compositor-wasm, compositor-ffmpeg-h264, xtsb) are MIT
+  or Apache-2.0 — these are safe to use without copyleft obligations.
+- **Strategy to avoid AGPL**: use the MIT libs as-is, reimplement the
+  AGPL parts from scratch (browser compositor + native proxy). The
+  Python port described in `10-porting-to-odoo-js-python.md` is a clean
+  way to do this — if you write your Python proxy from scratch, you can
+  license it however you want.
+
+## What Greenfield's codebase reveals about its engineering process
+
+The codebase reveals several engineering process insights:
+
+1. **One-person show** — most commits are by Erik De Rijcke. The codebase
+   has the strengths (consistent vision, tight integration) and weaknesses
+   (single point of failure, no code review diversity) of a one-person
+   project.
+2. **Long-running project** — code references back to 2017-era commits
+   (WebRTC was a previous transport; the `kcp.ts` MTU comment says
+   "webrtc datachannel MTU"). The architecture has evolved through several
+   major pivots.
+3. **rc1 is meaningful** — the maintainer has been working toward 1.0 for
+   years and rc1 represents a deliberate freeze. Don't expect 1.0 final
+   any time soon, but also don't expect major architecture changes.
+4. **Honest about limitations** — docs/pages/design/ explicitly admits
+   what's wrong and what's planned. Use this to inform your own roadmap.