alexei.dolgolyov
9cfe628cc5
Stability: Add outer try/except/finally with _running=False cleanup to all 6
processing loop methods (live, color_strip, effect, audio, composite, mapped).
Add exponential backoff on consecutive capture errors in live_stream. Move
audio stream.stop() outside lock scope.
Performance: Replace per-pixel Python loop with np.array().tobytes() in
ddp_client. Vectorize pixelate filter with cv2.resize down+up. Vectorize
gradient rendering with np.searchsorted.
Frontend: Add lockBody/unlockBody re-entrancy counter. Add {once:true} to
fetchWithAuth abort listener. Null ws.onclose before ws.close() in LED preview.
Backend: Remove auth token prefix from log messages. Add atomic_write_json
helper (tempfile + os.replace) and update all 10 stores. Add name uniqueness
checks to all update methods. Fix DELETE status codes to 204 in audio_sources
and value_sources. Fix get_source() silent bug in color_strip_sources.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
55 lines
5.8 KiB
Markdown
55 lines
5.8 KiB
Markdown
# Codebase Review — 2026-02-26
|
||
|
||
Findings from full codebase review. Items ordered by priority within each category.
|
||
|
||
## Stability (Critical)
|
||
|
||
- [x] **Fatal loop exception leaks resources** — Added outer `try/except/finally` with `self._running = False` to all 10 processing loop methods across `live_stream.py`, `color_strip_stream.py`, `effect_stream.py`, `audio_stream.py`, `composite_stream.py`, `mapped_stream.py`. Also added per-iteration `try/except` where missing.
|
||
- [x] **`_is_running` flag cleanup** — Fixed via `finally: self._running = False` in all loop methods. *(Race condition via `threading.Event` deferred — current pattern sufficient with the finally block.)*
|
||
- [x] **`ColorStripStreamManager` thread safety** — **FALSE POSITIVE**: All access is from the async event loop; methods are synchronous with no `await` points, so no preemption is possible.
|
||
- [x] **Audio `stream.stop()` called under lock** — Moved `stream.stop()` outside lock scope in both `release()` and `release_all()` in `audio_capture.py`.
|
||
- [x] **WS accept-before-validate** — **FALSE POSITIVE**: All WS endpoints validate auth and resolve configs BEFORE calling `websocket.accept()`.
|
||
- [x] **Capture error no-backoff** — Added consecutive error counter with exponential backoff (`min(1.0, 0.1 * (errors - 5))`) in `ScreenCaptureLiveStream._capture_loop()`.
|
||
- [ ] **WGC session close not detected** — Deferred (Windows-specific edge case, low priority).
|
||
- [x] **`LiveStreamManager.acquire()` not thread-safe** — **FALSE POSITIVE**: Same as ColorStripStreamManager — all access from async event loop, no await in methods.
|
||
|
||
## Performance (High Impact)
|
||
|
||
- [x] **Per-pixel Python loop in `send_pixels()`** — Replaced per-pixel Python loop with `np.array().tobytes()` in `ddp_client.py`. Hot path already uses `send_pixels_numpy()`.
|
||
- [ ] **WGC 6MB frame allocation per callback** — Deferred (Windows-specific, requires WGC API changes).
|
||
- [x] **Gradient rendering O(LEDs×Stops) Python loop** — Vectorized with NumPy: `np.searchsorted` for stop lookup + vectorized interpolation in `_compute_gradient_colors()`.
|
||
- [x] **`PixelateFilter` nested Python loop** — Replaced with `cv2.resize` down (INTER_AREA) + up (INTER_NEAREST) — pure C++ backend.
|
||
- [x] **`DownscalerFilter` double allocation** — **FALSE POSITIVE**: Already uses single `cv2.resize()` call (vectorized C++).
|
||
- [x] **`SaturationFilter` ~25MB temp arrays** — **FALSE POSITIVE**: Already uses pre-allocated scratch buffer and vectorized in-place numpy.
|
||
- [x] **`FrameInterpolationFilter` copies full image** — **FALSE POSITIVE**: Already uses vectorized numpy integer blending with image pool.
|
||
- [x] **`datetime.utcnow()` per frame** — **LOW IMPACT**: ~1-2μs per call, negligible at 60fps. Deprecation tracked under Backend Quality.
|
||
- [x] **Unbounded diagnostic lists** — **FALSE POSITIVE**: Lists are cleared every 5 seconds (~300 entries max at 60fps). Trivial memory.
|
||
|
||
## Frontend Quality
|
||
|
||
- [x] **`lockBody()`/`unlockBody()` not re-entrant** — Added `_lockCount` reference counter and `_savedScrollY` in `ui.js`. First lock saves scroll, last unlock restores.
|
||
- [x] **XSS via unescaped engine config keys** — **FALSE POSITIVE**: Both capture template and audio template card renderers already use `escapeHtml()` on keys and values.
|
||
- [x] **LED preview WS `onclose` not nulled** — Added `ws.onclose = null` before `ws.close()` in `disconnectLedPreviewWS()` in `targets.js`.
|
||
- [x] **`fetchWithAuth` retry adds duplicate listeners** — Added `{ once: true }` to abort signal listener in `api.js`.
|
||
- [x] **Audio `requestAnimationFrame` loop continues after WS close** — **FALSE POSITIVE**: Loop already checks `testAudioModal.isOpen` before scheduling next frame, and `_cleanupTest()` cancels the animation frame.
|
||
|
||
## Backend Quality
|
||
|
||
- [ ] **No thread-safety in `JsonStore`** — Deferred (low risk — all stores are accessed from async event loop).
|
||
- [x] **Auth token prefix logged** — Removed token prefix from log message in `auth.py`. Now logs only "Invalid API key attempt".
|
||
- [ ] **Duplicate capture/test code** — Deferred (code duplication, not a bug — refactoring would reduce LOC but doesn't fix a defect).
|
||
- [x] **Update methods allow duplicate names** — Added name uniqueness checks to `update_template` in `template_store.py`, `postprocessing_template_store.py`, `audio_template_store.py`, `pattern_template_store.py`, and `update_profile` in `profile_store.py`. Also added missing check to `create_profile`.
|
||
- [ ] **Routes access `manager._private` attrs** — Deferred (stylistic, not a bug — would require adding public accessor methods).
|
||
- [x] **Non-atomic file writes** — Created `utils/file_ops.py` with `atomic_write_json()` helper (tempfile + `os.replace`). Updated all 10 store files.
|
||
- [ ] **444 f-string logger calls** — Deferred (performance impact negligible — Python evaluates f-strings very fast; lazy `%s` formatting only matters at very high call rates).
|
||
- [x] **`get_source()` silent bug** — Fixed: `color_strip_sources.py:_resolve_display_index()` called `picture_source_store.get_source()` which doesn't exist (should be `get_stream()`). Was silently returning `0` for display index.
|
||
- [ ] **`get_config()` race** — Deferred (low risk — config changes are infrequent user-initiated operations).
|
||
- [ ] **`datetime.utcnow()` deprecated** — Deferred (functional, deprecation warning only appears in Python 3.12+).
|
||
- [x] **Inconsistent DELETE status codes** — Changed `audio_sources.py` and `value_sources.py` DELETE endpoints from 200 to 204 (matching all other DELETE endpoints).
|
||
|
||
## Architecture (Observations, no action needed)
|
||
|
||
**Strengths**: Clean layered design, plugin registries, reference-counted stream sharing, consistent API patterns.
|
||
|
||
**Weaknesses**: No backpressure (slow consumers buffer frames), thread count grows linearly, config global singleton, reference counting races.
|