From fb1268f34082830cfe05ef1f8c1f9b7d8791318a Mon Sep 17 00:00:00 2001 From: James Hutchinson Date: Fri, 30 Oct 2026 21:29:41 -0700 Subject: [PATCH] Send a wide fixture's real colors over RGB-only outputs NDI, HLS and RTSP carry RGB alone. With a fixture profile wider than RGB, such as white first, they sent the profile's first three channels, so white showed as red. They now send red, green and blue from where the profile puts them. **Light domain** - DriverBase.h: `correctToRgb` corrects one light and reads red, green and blue by the profile's offsets, 0 for a role it lacks. `lightScratch_` holds that one light, and `sizeLightScratch()` sizes it off the hot path, saying "not enough memory for one light of this fixture" in the status when it fails. - NdiDriver.h, HlsDriver.h, RtspDriver.h: each uses the base's helper and scratch; their own `corrScratch_` goes. **Tests** - unit_NdiDriver: sends a white-first profile's colors, not its white; the driver and layer live on the heap, outside a test's stack frame. HLS and RTSP share the helper and have no test of their own for it. - unit_ModuleContract: the NdiDriver known-list text names `lightScratch_`; the measurement comment is one line, within docgen's rules, since the file was touched. - test_desktop: 2,851 passed on the Pi 5 at this step. - build_esp32 esp32s3-n16r8: clean, no warnings. - Scenarios skipped: every driver grows by one ScratchBuffer handle, 32 bytes on a 64-bit host and 16 on an ESP32, and nothing changes per frame for an RGB wiring. **Docs/CI** - check_prose: NegatedHeading 6 to 8 and SelfReference 16 to 17 are main's baseline: the committed tree measures the same with this change absent. **Reviews** - Wide profiles took their first three channels on an RGB-only output: done, `correctToRgb` reads by role. - A failed scratch allocation went silent: done, `sizeLightScratch` names it in the status. - HLS and RTSP kept the first-three-channels copy after NDI moved: done, both on the helper. - The ModuleContract known-list text named the removed `corrScratch_`: done. - The ModuleContract measurement comment merged two cases into one claim: done, restored as three. Co-Authored-By: Claude Opus 5.5 --- src/light/drivers/DriverBase.h | 17 +++++++++++++++ src/light/drivers/HlsDriver.h | 13 +++++------- src/light/drivers/NdiDriver.h | 16 +++++++------- src/light/drivers/RtspDriver.h | 13 +++++------- test/unit/core/unit_ModuleContract.cpp | 4 ++-- test/unit/light/unit_NdiDriver.cpp | 29 +++++++++++++++++++++++++- 6 files changed, 65 insertions(+), 27 deletions(-) diff --git a/src/light/drivers/DriverBase.h b/src/light/drivers/DriverBase.h index ed02ec10..c3b95ed1 100644 --- a/src/light/drivers/DriverBase.h +++ b/src/light/drivers/DriverBase.h @@ -150,6 +150,23 @@ class DriverBase : public MoonModule { protected: Layer* layer_ = nullptr; + /// One light of a wiring wider than RGB, for correctToRgb, sized off the hot path by sizeLightScratch. + ScratchBuffer lightScratch_{*this}; + /// Size lightScratch_ to the wiring, saying so in the status when memory runs out; true when an RGB wiring needs none or the light fits. + bool sizeLightScratch() { + if (correction_.outChannels <= 3 || lightScratch_.resize(correction_.outChannels)) return true; + setStatus("not enough memory for one light of this fixture", Severity::Error); + return false; + } + + /// Correct one light for a wire that carries RGB alone, reading red, green and blue where the profile puts them, 0 for a role it lacks; `scratch` holds one light of the wiring. + void correctToRgb(const uint8_t* src, uint8_t srcCh, uint8_t* scratch, uint8_t* rgb) const MM_NONBLOCKING { + correction_.apply(src, scratch, srcCh); + rgb[0] = correction_.offRed != Correction::kAbsent ? scratch[correction_.offRed] : 0; + rgb[1] = correction_.offGreen != Correction::kAbsent ? scratch[correction_.offGreen] : 0; + rgb[2] = correction_.offBlue != Correction::kAbsent ? scratch[correction_.offBlue] : 0; + } + /// The correction scratch; the size differs per driver, so the caller passes the byte count and the lifecycle lives here. uint8_t* wire_ = nullptr; size_t wireCap_ = 0; // bytes the allocation returned (0 when it failed) diff --git a/src/light/drivers/HlsDriver.h b/src/light/drivers/HlsDriver.h index b7a4854c..cbf7a099 100644 --- a/src/light/drivers/HlsDriver.h +++ b/src/light/drivers/HlsDriver.h @@ -50,8 +50,8 @@ class HlsDriver : public DriverBase { /// Where the encoder's segments are written, under the filesystem mount and served at /hls/, the same literal the file server uses. static constexpr const char* kSegmentDir = "/.hls"; - /// Bind the two scratch buffers to this module, so their memory is accounted for. - HlsDriver() : rgb_(*this), corrScratch_(*this) {} + /// Bind the frame buffer to this module, so its memory is accounted for. + HlsDriver() : rgb_(*this) {} /// The catalog tags shown on this driver's card. const char* tags() const override { return kTags; } @@ -119,7 +119,7 @@ class HlsDriver : public DriverBase { setStatus("out of memory for the video frame", Severity::Error); return; } - if (correction_.outChannels > 3) corrScratch_.resize(correction_.outChannels); + if (!sizeLightScratch()) return; // its status names the shortage // Only where the encoder writes segments to disk rather than keeping them in RAM. if constexpr (platform::hasFsSegments) { @@ -193,7 +193,7 @@ class HlsDriver : public DriverBase { // Packed with the output correction, so the stream shows what the wall shows. uint8_t* dst = &rgb_[0]; - const bool wide = outCh > 3 && corrScratch_.count() >= outCh; + const bool wide = outCh > 3 && lightScratch_.count() >= outCh; const size_t rowBytes = static_cast(width_) * 3; for (nrOfLightsType i = 0; i < n; i++) { const uint8_t* s = src + static_cast(i) * srcCh; @@ -201,9 +201,7 @@ class HlsDriver : public DriverBase { if (outCh == 3) { correction_.apply(s, rgb, srcCh); } else if (wide) { - uint8_t* c = &corrScratch_[0]; - correction_.apply(s, c, srcCh); - rgb[0] = c[0]; rgb[1] = c[1]; rgb[2] = c[2]; + correctToRgb(s, srcCh, &lightScratch_[0], rgb); } else { rgb[0] = s[0]; rgb[1] = s[1]; rgb[2] = s[2]; } @@ -397,7 +395,6 @@ class HlsDriver : public DriverBase { std::atomic droppedFrames_{0}; uint32_t lastReportedDrops_ = 0; ///< tick1s-only ScratchBuffer rgb_; ///< tight RGB staging, sized in prepare() - ScratchBuffer corrScratch_; ///< one corrected light, when the wiring is wider than RGB char urlBuf_[64]{}; char statusBuf_[64]{}; }; diff --git a/src/light/drivers/NdiDriver.h b/src/light/drivers/NdiDriver.h index 11ee3fe0..17106260 100644 --- a/src/light/drivers/NdiDriver.h +++ b/src/light/drivers/NdiDriver.h @@ -46,8 +46,8 @@ class NdiDriver : public DriverBase { /// The catalog tag this driver carries. static constexpr const char* kTags = "🖥️"; - /// Bind the two scratch buffers to this module, so their registration puts their memory in the report. - NdiDriver() : rgb_(*this), corrScratch_(*this) {} + /// Bind the frame buffer to this module, so its registration puts its memory in the report. + NdiDriver() : rgb_(*this) {} /// The catalog tags shown on this driver's card. const char* tags() const override { return kTags; } @@ -94,7 +94,10 @@ class NdiDriver : public DriverBase { setStatus("out of memory for the NDI frame", Severity::Error); return; } - if (correction_.outChannels > 3) corrScratch_.resize(correction_.outChannels); + if (!sizeLightScratch()) { // its status names the shortage + release(); + return; + } std::snprintf(statusBuf_, sizeof(statusBuf_), "sending %ux%u at %u fps", static_cast(width_), static_cast(height_), @@ -130,16 +133,14 @@ class NdiDriver : public DriverBase { if (srcCh < 3) return; // a non-color buffer (DMX roles) has no frame to send // A wider wiring corrects into a one-light scratch, since the protocol carries RGB only. - const bool wide = outCh > 3 && corrScratch_.count() >= outCh; + const bool wide = outCh > 3 && lightScratch_.count() >= outCh; for (nrOfLightsType i = 0; i < n; i++) { const uint8_t* s = src + static_cast(i) * srcCh; uint8_t* d = dst + static_cast(i) * 3; if (outCh == 3) { correction_.apply(s, d, srcCh); } else if (wide) { - uint8_t* c = &corrScratch_[0]; - correction_.apply(s, c, srcCh); - d[0] = c[0]; d[1] = c[1]; d[2] = c[2]; + correctToRgb(s, srcCh, &lightScratch_[0], d); } else { d[0] = s[0]; d[1] = s[1]; d[2] = s[2]; // passthrough, same fallback as NetworkSend } @@ -164,7 +165,6 @@ class NdiDriver : public DriverBase { bool open_ = false; uint32_t lastSendMs_ = 0; ScratchBuffer rgb_; ///< tight RGB staging, sized in prepare() - ScratchBuffer corrScratch_; ///< one corrected light, when the wiring is wider than RGB char statusBuf_[64]{}; }; diff --git a/src/light/drivers/RtspDriver.h b/src/light/drivers/RtspDriver.h index 0c7ad5af..c95d089c 100644 --- a/src/light/drivers/RtspDriver.h +++ b/src/light/drivers/RtspDriver.h @@ -51,8 +51,8 @@ class RtspDriver : public DriverBase { /// The catalog tag this driver carries. static constexpr const char* kTags = "🖥️"; - /// Bind the scratch buffers to this module, so their memory is accounted for. - RtspDriver() : rgb_(*this), corrScratch_(*this) {} + /// Bind the frame buffer to this module, so its memory is accounted for. + RtspDriver() : rgb_(*this) {} /// The catalog tags shown on this driver's card. const char* tags() const override { return kTags; } @@ -102,7 +102,7 @@ class RtspDriver : public DriverBase { setStatus("out of memory for the video frame", Severity::Error); return; } - if (correction_.outChannels > 3) corrScratch_.resize(correction_.outChannels); + if (!sizeLightScratch()) return; // its status names the shortage platform::EncoderConfig cfg{}; cfg.width = static_cast(width_); @@ -266,7 +266,7 @@ class RtspDriver : public DriverBase { if (srcCh < 3) return; // a non-color buffer (DMX roles) has no frame to send uint8_t* dst = rgb_.data(); - const bool wide = outCh > 3 && corrScratch_.count() >= outCh; + const bool wide = outCh > 3 && lightScratch_.count() >= outCh; const size_t rowBytes = static_cast(width_) * 3; for (nrOfLightsType i = 0; i < n; i++) { const uint8_t* s = src + static_cast(i) * srcCh; @@ -274,9 +274,7 @@ class RtspDriver : public DriverBase { if (outCh == 3) { correction_.apply(s, rgb, srcCh); } else if (wide) { - uint8_t* c = corrScratch_.data(); - correction_.apply(s, c, srcCh); - rgb[0] = c[0]; rgb[1] = c[1]; rgb[2] = c[2]; + correctToRgb(s, srcCh, &lightScratch_[0], rgb); } else { rgb[0] = s[0]; rgb[1] = s[1]; rgb[2] = s[2]; } @@ -341,7 +339,6 @@ class RtspDriver : public DriverBase { private: Buffer* sourceBuffer_ = nullptr; ScratchBuffer rgb_; - ScratchBuffer corrScratch_; platform::TcpServer control_; platform::TcpConnection client_; diff --git a/test/unit/core/unit_ModuleContract.cpp b/test/unit/core/unit_ModuleContract.cpp index b6eebe92..26511701 100644 --- a/test/unit/core/unit_ModuleContract.cpp +++ b/test/unit/core/unit_ModuleContract.cpp @@ -33,7 +33,7 @@ const std::vector kKeepsMemory = { {"Effects", "its Layer keeps the pixel buffer while disabled"}, {"Layer", "its pixel buffer_ is a plain Buffer that release() does not free"}, {"Drivers", "outputBuffer_ is a plain Buffer that release() does not free"}, - {"NdiDriver", "release() is overridden without chaining to the base, so the rgb_ and corrScratch_ scratch buffers stay, where the NDI library loads and the buffers exist", true}, + {"NdiDriver", "release() is overridden without chaining to the base, so the rgb_ and lightScratch_ scratch buffers stay, where the NDI library loads and the buffers exist", true}, }; // Types whose enabled flag does not gate them, which the lifecycle then cannot switch off. @@ -154,7 +154,7 @@ Row place(uint8_t index) { s.prepareTree(); for (int i = 0; i < 3; i++) { mm::platform::setTestNowMs(now); s.tick(); now += 1100; } row.dynamicAfter = dynamicTotal(*m); - // A module added by document is measured from before its add, less its own objects. A standalone one stood in the tree before beforeAdd was taken, so its objects are already in that baseline and only what it holds beyond them counts; its setup-time allocations are in the baseline too and are not covered. A module standing in the tree before the add has no add to measure from. + // Measured from before the add: a module added by document less its own objects, a standalone one, already in that baseline with its setup allocations, by what it holds beyond them. Others have no add. if (byDocument || standalone) row.heldAfterDisable = static_cast(mm::platform::allocatedBytes()) - static_cast(beforeAdd) - (byDocument ? static_cast(objectBytes(*m)) : 0L); // Removed the way the API's DELETE removes it, a top-level module leaving with its tree; a module the API refuses to remove (PreviewDriver) is out of this check. diff --git a/test/unit/light/unit_NdiDriver.cpp b/test/unit/light/unit_NdiDriver.cpp index 785a4626..25cbdaca 100644 --- a/test/unit/light/unit_NdiDriver.cpp +++ b/test/unit/light/unit_NdiDriver.cpp @@ -10,11 +10,12 @@ #include "light/layers/Buffer.h" #include "light/layouts/Layouts.h" #include "light/layouts/GridLayout.h" +#include #include namespace { -// The NDI seams and virtual time are process-global, so a REQUIRE that aborts mid-case would strand a forced mode (or a frozen clock) for whichever test runs next, the classic passes-alone / fails-in-sequence flake. A scope guard restores both however the case leaves. +// The NDI seams and virtual time are process-global, so a REQUIRE that aborts mid-case would strand a forced mode or a frozen clock for the next test, the passes-alone, fails-in-sequence flake; a guard restores both. struct NdiSeamGuard { explicit NdiSeamGuard(mm::platform::NdiTestMode mode) { mm::platform::setTestNdiMode(mode); @@ -128,6 +129,32 @@ TEST_CASE("NdiDriver applies the driver's own brightness correction") { CHECK(f[0] > 0); } +// A wire that carries RGB alone takes a wider profile's color from where the profile puts it, not its first three channels. +TEST_CASE("NdiDriver sends a white-first profile's colors, not its white") { + NdiSeamGuard seam{mm::platform::NdiTestMode::ForceAvailable}; + // On the heap: a driver and a layer outgrow a test's stack frame. + auto source = std::make_unique(); + auto driver = std::make_unique(); + auto wall = std::make_unique(2, 1); + setUp(*driver, *source, *wall, 2); + auto wrgb = std::make_unique(); + wrgb->curve = mm::Correction::Curve::Linear; + const mm::ChannelRole roles[4] = {mm::ChannelRole::White, mm::ChannelRole::Red, mm::ChannelRole::Green, mm::ChannelRole::Blue}; + wrgb->rebuild(255, roles, 4); + driver->correctionForTest() = *wrgb; + driver->prepare(); + + paint(*source, 0, 10, 20, 30); + + mm::platform::setTestNowMs(1000); + driver->tick(); + + REQUIRE(mm::platform::ndiTestFrameCount() == 1); + const uint8_t* f = mm::platform::ndiTestFrameData(0); + REQUIRE(f != nullptr); + CHECK(f[0] == 10); CHECK(f[1] == 20); CHECK(f[2] == 30); +} + // fps is a CEILING: a second tick inside the interval must not produce a second frame, or a fast render loop would flood the receiver with frames it never asked for. TEST_CASE("NdiDriver holds its frame rate to the fps ceiling") { NdiSeamGuard seam{mm::platform::NdiTestMode::ForceAvailable};