From ac9dc02b5182521515ce1ec649cdad705264bda0 Mon Sep 17 00:00:00 2001 From: Florian Egger Date: Mon, 7 Sep 2026 10:17:59 +0200 Subject: [PATCH] feat(capture): give CaptureFactory a proper error channel Replace the nullable unique_ptr returned by CaptureFactory::create() with CaptureResult> using the new CaptureError/CaptureResult pattern, mirroring codec/error.h. The stub now reports 'not implemented yet' as an error instead of returning nullptr. Update handoff memory with the review outcome and forward-looking notes for phases 3, 5, and 7. --- .agents/MEMORY.md | 50 ++++++++++++++++++++++------ include/screencast/capture/capture.h | 4 ++- include/screencast/capture/error.h | 36 ++++++++++++++++++++ src/capture/pipewire_capture.cpp | 6 ++-- 4 files changed, 81 insertions(+), 15 deletions(-) create mode 100644 include/screencast/capture/error.h diff --git a/.agents/MEMORY.md b/.agents/MEMORY.md index f155786..bb9a025 100644 --- a/.agents/MEMORY.md +++ b/.agents/MEMORY.md @@ -1,19 +1,31 @@ # Project Memory — screen_cast -Last updated: initial scaffold. +Last updated: Phase 2 codec review fixes applied. ## Project state -- Phase 2 follow-up review items addressed: encoder rate control is now - bitrate-only (removed CRF), encoder/decoder cache their SwsContext, configs - own their strings, bitstream helper is internal to the encoder with a NAL - length guard, FFmpeg open errors are reported, and the round-trip test - verifies Annex-B prefix and keyframes. -- `.clang-format` added at repo root and clang-format enforcement added to - `AGENTS.md` and `cpp-meson-build/SKILL.md`. -- Phase 3 capture now has a linkable stub (`src/capture/pipewire_capture.cpp`) - so the API can be consumed without an unresolved symbol. -- Remaining implementation: capture, transport, rendering, and CLI/pipeline glue. +- Phase 2 codec implementation reviewed with valgrind; all confirmed defects + fixed on top of the capture-stub commit: + - decoder leaked every packet payload (`av_malloc` + direct `packet->data` + assignment bypassed the packet's owning `AVBufferRef`); payloads are now + allocated with `av_new_packet`. + - decoder extradata lacked `AV_INPUT_BUFFER_PADDING_SIZE`; FFmpeg's + extradata parser over-read the buffer (valgrind invalid reads). Now + allocated padded and zeroed. + - oversized encoded frames are rejected before the int cast. + - EAGAIN-retry loops in encoder/decoder now handle unexpected EOF instead + of retrying forever. + - fixed "RTP packet" → "AVPacket" error message in the encoder. +- `CaptureFactory::create()` now returns + `CaptureResult>` (variant with + `CaptureError`) instead of a nullable unique_ptr; the stub reports + "PipeWire capture is not implemented yet" as an error. New pattern lives in + `include/screencast/capture/error.h`, mirroring `codec/error.h`. +- Validation: `meson test` 2/2 OK; valgrind on `test_codec_roundtrip` is now + clean (0 definite losses, 0 invalid reads); clang-format clean. See + `docs/RUNBOOK.md` for the reusable checks. +- Phase 3 capture stub remains in place; remaining implementation: capture, + transport, rendering, and CLI/pipeline glue. ## Decisions @@ -28,6 +40,8 @@ Last updated: initial scaffold. - Discovery: mDNS/Avahi. - Rendering: SDL2 or SDL3 + OpenGL. - Namespace: `sc`. +- Module error results use per-module `std::variant` types + (`CodecResult`, `CaptureResult`) since C++20 has no `std::expected`. ## Active blockers @@ -38,3 +52,17 @@ None. - GUI framework (Qt6 vs. none / CLI only) — deferred to later phase. - Hardware acceleration strategy (VAAPI / Vulkan Video / NVENC) — evaluate after software encode path works. + +## Forward-looking review notes (for later phases) + +- Encoder rejects non-packed strides in `make_input_frame`; PipeWire/portal + frames usually have alignment-padded strides — Phase 3 must pass the real + stride through to swscale instead of rejecting it. +- `AV_CODEC_FLAG_GLOBAL_HEADER` suppresses in-band SPS/PPS; a receiver cannot + join mid-stream or recover after PLI without parameter sets. Phase 5 must + prepend SPS/PPS to keyframes or negotiate them in signaling. +- Encoder sets no VBV (`maxrate`/`buffer_size`) — ABR only; add for smoother + UDP streaming in Phase 7. +- No negative-path tests yet (bad config, bad stride, undersized buffer). +- `to_annex_b_h264` sniffs AVCC vs Annex-B by content; if an AVCC-emitting + encoder is ever added, prefer an explicit config flag over the heuristic. \ No newline at end of file diff --git a/include/screencast/capture/capture.h b/include/screencast/capture/capture.h index 2b5ac94..c5f3660 100644 --- a/include/screencast/capture/capture.h +++ b/include/screencast/capture/capture.h @@ -1,5 +1,7 @@ #pragma once +#include "screencast/capture/error.h" + #include #include #include @@ -52,7 +54,7 @@ class CaptureSession { // Factory for the PipeWire / xdg-desktop-portal capture backend. class CaptureFactory { public: - static std::unique_ptr create(CaptureTarget target); + static CaptureResult> create(CaptureTarget target); }; } // namespace sc diff --git a/include/screencast/capture/error.h b/include/screencast/capture/error.h new file mode 100644 index 0000000..ac62f6a --- /dev/null +++ b/include/screencast/capture/error.h @@ -0,0 +1,36 @@ +#pragma once + +#include +#include + +namespace sc { + +struct CaptureError { + std::string message; +}; + +// C++20 does not provide std::expected. Use a variant-based result type so +// fallible capture operations do not rely on exceptions. +template using CaptureResult = std::variant; + +template constexpr bool is_capture_error(const CaptureResult& result) noexcept { + return std::holds_alternative(result); +} + +template T& capture_value(CaptureResult& result) { + return std::get(result); +} + +template const T& capture_value(const CaptureResult& result) { + return std::get(result); +} + +template CaptureError& capture_error(CaptureResult& result) { + return std::get(result); +} + +template const CaptureError& capture_error(const CaptureResult& result) { + return std::get(result); +} + +} // namespace sc diff --git a/src/capture/pipewire_capture.cpp b/src/capture/pipewire_capture.cpp index 9ff5b49..4e489d3 100644 --- a/src/capture/pipewire_capture.cpp +++ b/src/capture/pipewire_capture.cpp @@ -4,10 +4,10 @@ namespace sc { -std::unique_ptr CaptureFactory::create(CaptureTarget /*target*/) { +CaptureResult> CaptureFactory::create(CaptureTarget /*target*/) { // Phase 3 placeholder. The PipeWire / xdg-desktop-portal capture backend - // will be implemented here once the codec round-trip is solidified. - return nullptr; + // will be implemented here once the portal integration lands. + return CaptureError{"PipeWire capture is not implemented yet"}; } } // namespace sc