From a8c31892a55591eb8f6263bf003f7edc1c10586d Mon Sep 17 00:00:00 2001 From: Shura Fedorovskyi Date: Tue, 20 Oct 2026 16:49:58 +0400 Subject: [PATCH 1/3] feat: the hardware-free half of USB video capture The UVC payload headers a bulk frame still carries, the mode and rate a device is asked for, and the linear-light area average that fits a frame of any shape onto the picture. No hardware is involved, so the desktop proves all of it: 17 cases over the header lengths, the mode choice, the row stride and the averaging. The picture is now exactly the size it is set to, as the test pattern's already was. A whole scale factor left the right and bottom of the frame unread, so a bright screen edge never reached the lights along it, and the ranges now tile the source with no remainder. The desktop stand-in decodes through that same averaging rather than writing a flat colour of its own, which is what proves the path on the host and retires halveToFit. --- docs/how-to/adding-a-stream-source.md | 2 +- src/platform/desktop/platform_desktop.cpp | 31 +-- src/platform/usbvideo_frames.h | 174 +++++++++++++++++ test/CMakeLists.txt | 1 + test/unit/core/unit_PictureService.cpp | 14 +- test/unit/core/unit_usbvideo_frames.cpp | 224 ++++++++++++++++++++++ 6 files changed, 423 insertions(+), 23 deletions(-) create mode 100644 src/platform/usbvideo_frames.h create mode 100644 test/unit/core/unit_usbvideo_frames.cpp diff --git a/docs/how-to/adding-a-stream-source.md b/docs/how-to/adding-a-stream-source.md index 64caeacf..14af1f1a 100644 --- a/docs/how-to/adding-a-stream-source.md +++ b/docs/how-to/adding-a-stream-source.md @@ -30,7 +30,7 @@ A picture source is an option of the Picture service's `source`, in `src/core/se ```cpp uint8_t* px = beginWrite(); // the back picture, width x height x 3 bytes, top row first -decodeInto(px, width, height); // your source: a decoder writes here directly, nothing is copied +decodeInto(px, width, height); // your source: the pixels land here, uncopied where its decoder can endWrite(); // swap it to the front and bump the generation ``` diff --git a/src/platform/desktop/platform_desktop.cpp b/src/platform/desktop/platform_desktop.cpp index dcc1d6b1..64067db3 100644 --- a/src/platform/desktop/platform_desktop.cpp +++ b/src/platform/desktop/platform_desktop.cpp @@ -133,6 +133,7 @@ /// A host has no second card, so a subfolder of the one real root stands in. `sdMounted_` gates it as a missing card would, and format is a wipe-and-recreate. #include "platform/platform.h" +#include "platform/usbvideo_frames.h" // areaAverage: the shrink the device backend also uses #include "core/util/FirmwareImage.h" // identify/moonBaseRejection: shared image vetting #include "core/util/H264Bitstream.h" // where the encoder's frames begin and end @@ -2878,14 +2879,11 @@ struct TestGrabber { std::atomic decodeFails{false}; std::atomic open{false}; std::atomic decoded{0}, failed{0}; + std::vector frame; ///< the synthesized source, rebuilt only when its size or color moves + uint8_t frameRgb[3] = {}; } g_grabber; // How far `have` is from `want`, so the nearest advertised mode can be picked without . long apart(long have, long want) { return have > want ? have - want : want - have; } -// Halve `w` x `h` until it fits `maxW` x `maxH`, which is the scaling the decode hardware offers; false when even one pixel does not fit. -bool halveToFit(uint16_t& w, uint16_t& h, uint16_t maxW, uint16_t maxH) { - while ((w > maxW || h > maxH) && w > 1 && h > 1) { w = uint16_t(w / 2); h = uint16_t(h / 2); } - return w <= maxW && h <= maxH; -} } // namespace uint32_t videoCaptureFormatGeneration() MM_NONBLOCKING { return g_grabber.generation.load(); } @@ -2921,16 +2919,19 @@ bool videoCaptureDecodeInto(VideoCaptureHandle& h, uint8_t* dst, uint16_t maxWid uint16_t& outWidth, uint16_t& outHeight) { if (!h.impl || !g_grabber.open) return false; if (g_grabber.decodeFails) { g_grabber.failed++; return false; } - uint16_t w = g_grabber.openW, hh = g_grabber.openH; - if (!halveToFit(w, hh, maxWidth, maxHeight)) { g_grabber.failed++; return false; } - const uint8_t r = g_grabber.r, g = g_grabber.g, b = g_grabber.b; - for (size_t i = 0, px = size_t(w) * hh; i < px; i++) { - dst[i * 3 + 0] = r; - dst[i * 3 + 1] = g; - dst[i * 3 + 2] = b; - } - outWidth = w; - outHeight = hh; + if (!maxWidth || !maxHeight) { g_grabber.failed++; return false; } + // A frame at the mode the device opened, averaged onto the whole picture. It runs the same code the device backend does, so the host proves that path rather than a shortcut past it. + const uint16_t w = g_grabber.openW, hh = g_grabber.openH; + const uint8_t rgb[3] = {g_grabber.r, g_grabber.g, g_grabber.b}; + const size_t need = static_cast(w) * hh * 3; + if (g_grabber.frame.size() != need || std::memcmp(g_grabber.frameRgb, rgb, 3) != 0) { + g_grabber.frame.resize(need); + for (size_t i = 0; i < need; i += 3) std::memcpy(g_grabber.frame.data() + i, rgb, 3); + std::memcpy(g_grabber.frameRgb, rgb, 3); + } + usbvideo::areaAverage(g_grabber.frame.data(), static_cast(w) * 3, w, hh, maxWidth, maxHeight, dst); + outWidth = maxWidth; + outHeight = maxHeight; g_grabber.decoded++; return true; } diff --git a/src/platform/usbvideo_frames.h b/src/platform/usbvideo_frames.h new file mode 100644 index 00000000..d9fd526f --- /dev/null +++ b/src/platform/usbvideo_frames.h @@ -0,0 +1,174 @@ +#pragma once + +/// The hardware-free half of USB video capture: the UVC payload headers, the mode choice and the shrink onto a picture. + +#include "core/util/math16.h" // isqrt: the tree's integer root, which the box average needs + +#include +#include +#include + +namespace mm::usbvideo { + +/// One mode a grabber advertises, as its frame list reports it. +struct Mode { + uint16_t width = 0; ///< frame width in pixels + uint16_t height = 0; ///< frame height in pixels + uint8_t fps = 0; ///< frames a second at that size +}; + +// --- UVC bulk payload headers (UVC 1.5 2.4.3.3) --------------------------------------------------- + +constexpr uint8_t kPts = 0x04, kScr = 0x08, kEoh = 0x80, kErr = 0x40, kReserved = 0x10; +constexpr size_t kBulkMps = 512; ///< high-speed bulk packet size, the stride headers sit on +constexpr size_t kConfirmStrides = 3; ///< a short header carries few bits, so one match proves nothing + +/// A header's own length from its flags: 2 bytes, plus 4 for a PTS and 6 for an SCR. +inline size_t headerLenFor(uint8_t flags) { + return 2u + ((flags & kPts) ? 4u : 0u) + ((flags & kScr) ? 6u : 0u); +} + +/// Whether `h` is a payload header of exactly `want` bytes; `pts` pins it to the frame's first one. +inline bool isPayloadHeader(const uint8_t* h, size_t avail, size_t want, const uint8_t* pts) { + if (avail < 2) return false; + const uint8_t len = h[0], flags = h[1]; + if (len != want || len != headerLenFor(flags) || avail < len) return false; + if (!(flags & kEoh) || (flags & (kErr | kReserved))) return false; + // The SCR's top 5 bits are reserved and zero, the strongest single check a header carries. + if ((flags & kScr) && (h[len - 1] & 0xF8)) return false; + return !pts || !(flags & kPts) || std::memcmp(h + 2, pts, 4) == 0; +} + +/// Whether headers of length `hl` really recur after `first`, which is what rules out a chance match. +inline bool confirmedRun(const uint8_t* d, size_t len, size_t first, size_t hl) { + const uint8_t* pts = (d[first + 1] & kPts) ? d + first + 2 : nullptr; + const size_t stride = first + hl; + size_t seen = 0; + for (size_t at = first + stride; at + hl <= len && seen < kConfirmStrides; at += stride, seen++) + if (!isPayloadHeader(d + at, len - at, hl, pts)) return false; + return seen > 0 || first + stride + hl > len; // a frame too short to confirm is taken as it is +} + +/// Drops the payload headers a bulk frame still carries, answering the compacted length. +inline size_t stripPayloadHeaders(uint8_t* d, size_t len) { + static constexpr size_t kLens[] = {12, 8, 6, 2}; // longest first: the more fields, the surer the match + size_t hdr = 0, hl = 0; + for (size_t cand : kLens) { + for (size_t i = kBulkMps - cand; i + cand <= len; i += kBulkMps) { + if (!isPayloadHeader(d + i, len - i, cand, nullptr) || !confirmedRun(d, len, i, cand)) continue; + hdr = i; + hl = cand; + break; + } + if (hdr) break; + } + if (!hdr) return len; + const size_t payload = hdr + hl; + uint8_t pts[4] = {}; // copied: the compaction overwrites the header it came from + const bool hasPts = (d[hdr + 1] & kPts) != 0; + if (hasPts) std::memcpy(pts, d + hdr + 2, sizeof pts); + size_t w = hdr, r = hdr; + while (isPayloadHeader(d + r, len - r, hl, hasPts ? pts : nullptr)) { + r += hl; + const size_t run = payload - hl < len - r ? payload - hl : len - r; + std::memmove(d + w, d + r, run); + w += run; + r += run; + } + std::memmove(d + w, d + r, len - r); // whatever follows a missing header is left as delivered + return w + (len - r); +} + +// --- Choosing a mode ---------------------------------------------------------------------------- + +/// UVC states a rate as a period in 100 ns ticks. Rounded, so 59.94 fps reads as 60 rather than 59. +inline uint8_t fpsFrom(uint32_t interval) { + constexpr uint32_t kTicksPerSecond = 10000000; + return interval ? static_cast((kTicksPerSecond + interval / 2) / interval) : 0; +} + +/// Of two advertised rates, the one to ask for: the fastest at or below `wanted`, else the slowest. +inline uint8_t betterFps(uint8_t have, uint8_t row, uint8_t wanted) { + if (!have) return row; + const bool haveOk = have <= wanted, rowOk = row <= wanted; + if (haveOk != rowOk) return haveOk ? have : row; + if (haveOk) return row > have ? row : have; + return row < have ? row : have; +} + +/// The smallest mode at or above the ceiling, at the rate asked for, so decode and shrink stay cheap. +inline bool pickMode(const Mode* modes, size_t count, uint16_t& width, uint16_t& height, uint8_t& fps) { + uint32_t bestArea = 0; + uint16_t bw = 0, bh = 0; + uint8_t bestFps = 0; + for (size_t i = 0; i < count; i++) { + const Mode& m = modes[i]; + if (m.width < width || m.height < height) continue; // below the ceiling it would be upscaled + const uint32_t area = static_cast(m.width) * m.height; + if (bestArea && area > bestArea) continue; // a bigger picture to decode every frame + // The rates accumulate per resolution, not per area: two shapes can share an area. + if (m.width != bw || m.height != bh) bestFps = 0; + bestArea = area; + bw = m.width; + bh = m.height; + bestFps = betterFps(bestFps, m.fps, fps); + } + if (!bestArea) return false; // nothing listed, or nothing big enough: ask for what was wanted + width = bw; + height = bh; + if (bestFps) fps = bestFps; + return true; +} + +// --- The decoded frame -------------------------------------------------------------------------- + +/// `v` rounded up to a multiple of `to`, the MCU boundary a decoder pads a frame's axis to. +inline uint32_t alignUp(uint32_t v, uint8_t to) { return (v + to - 1u) / to * to; } + +/// The row stride in bytes the decoder wrote, or 0 when `outSize` matches no layout its MCU allows. +inline size_t strideFor(uint16_t w, uint16_t h, uint8_t mcuW, uint8_t mcuH, uint32_t outSize) { + if (!w || !h || !mcuW || !mcuH) return 0; + if (outSize == static_cast(w) * h * 3u) return static_cast(w) * 3u; + const uint32_t padW = alignUp(w, mcuW), padH = alignUp(h, mcuH); + if (outSize == padW * padH * 3u) return static_cast(padW) * 3u; + return 0; +} + +/// The source range an output cell covers, the proportional split the Picture effect's ranges use. +inline void cellRange(uint32_t i, uint32_t out, uint32_t src, uint32_t& lo, uint32_t& hi) { + lo = i * src / out; + hi = (i + 1u) * src / out; + if (hi <= lo) hi = lo + 1; // a source smaller than the output: neighbouring cells share a pixel +} + +/// Taps an axis of one cell on the scattered path, so a cell's sum stays well inside 32 bits. +constexpr uint32_t kTapsPerAxis = 4; + +/// Each output pixel the linear-light mean of its rectangle, sampled at a stride: a scattered read +/// beats a sequential one here, since the arithmetic dominates and a full walk is 2.7x more of it. +inline void areaAverage(const uint8_t* src, size_t stride, uint16_t w, uint16_t h, uint16_t ow, + uint16_t oh, uint8_t* dst) { + for (uint16_t y = 0; y < oh; y++) { + uint32_t ylo = 0, yhi = 0; + cellRange(y, oh, h, ylo, yhi); + const uint32_t sy = (yhi - ylo + kTapsPerAxis - 1) / kTapsPerAxis; + for (uint16_t x = 0; x < ow; x++) { + uint32_t xlo = 0, xhi = 0; + cellRange(x, ow, w, xlo, xhi); + const uint32_t sx = (xhi - xlo + kTapsPerAxis - 1) / kTapsPerAxis; + uint32_t sum[3] = {0, 0, 0}, taps = 0; + for (uint32_t py = ylo; py < yhi; py += sy) { + const uint8_t* row = src + static_cast(py) * stride; + for (uint32_t px = xlo; px < xhi; px += sx) { + const uint8_t* p = row + static_cast(px) * 3; + for (int c = 0; c < 3; c++) sum[c] += static_cast(p[c]) * p[c]; + taps++; + } + } + uint8_t* out = dst + (static_cast(y) * ow + x) * 3; + for (int c = 0; c < 3; c++) out[c] = static_cast(isqrt(sum[c] / taps)); + } + } +} + +} // namespace mm::usbvideo diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 0cc99ba7..7b3fccb5 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -15,6 +15,7 @@ add_executable(mm_tests unit/core/unit_Color.cpp unit/core/unit_math8.cpp unit/core/unit_math16.cpp + unit/core/unit_usbvideo_frames.cpp unit/core/unit_noise.cpp unit/core/unit_fields.cpp unit/core/unit_crc.cpp diff --git a/test/unit/core/unit_PictureService.cpp b/test/unit/core/unit_PictureService.cpp index f3e826e1..f1652219 100644 --- a/test/unit/core/unit_PictureService.cpp +++ b/test/unit/core/unit_PictureService.cpp @@ -179,16 +179,16 @@ bool startCapture(Picture& p, uint32_t& was) { } } // namespace -// A 640 x 480 grabber on a 256 x 256 picture: halved to 160 x 120, every pixel the color it decoded. -TEST_CASE("the usb capture source publishes the grabber's frames, scaled to fit the picture") { +// The whole picture, as the test pattern fills it. A 4:3 frame fills a 2:1 picture rather than leaving it short. +TEST_CASE("the usb capture source publishes the grabber's frames at the size the picture is set to") { Grabber g(640, 480, 30, 10, 200, 90); - Picture p(256, 256); + Picture p(200, 100); uint32_t was = 0; REQUIRE(startCapture(p, was)); const mm::PictureFrame f = current(); - CHECK(f.width == 160); - CHECK(f.height == 120); - const uint8_t* px = pixel(f, 80, 60); + CHECK(f.width == 200); + CHECK(f.height == 100); + const uint8_t* px = pixel(f, 100, 50); CHECK(px[0] == 10); CHECK(px[1] == 200); CHECK(px[2] == 90); @@ -228,7 +228,7 @@ TEST_CASE("the capture status counts the frames that landed and those lost") { uint32_t was = 0; REQUIRE(startCapture(p, was)); const std::string line = captureStatus(p.svc); - CHECK(line.rfind("40 x 30, ", 0) == 0); // 320 x 240 halved three times to fit 64 x 48 + CHECK(line.rfind("64 x 48, ", 0) == 0); // the picture's own size, whatever the grabber streams CHECK(line.find(" frames, 0 lost") != std::string::npos); CHECK(line.find(" 0 frames") == std::string::npos); // the picture that arrived is counted CHECK(line.find("one picture") == std::string::npos); diff --git a/test/unit/core/unit_usbvideo_frames.cpp b/test/unit/core/unit_usbvideo_frames.cpp new file mode 100644 index 00000000..6f0a812e --- /dev/null +++ b/test/unit/core/unit_usbvideo_frames.cpp @@ -0,0 +1,224 @@ +/// The hardware-free half of USB video capture: the payload headers it strips, the mode it asks for, and the shrink onto a picture. + +#include "doctest.h" +#include "platform/usbvideo_frames.h" + +#include +#include +#include + + +using namespace mm::usbvideo; + +namespace { +// A bulk frame carrying `payload`-byte payloads, each opening with a `hl`-byte header the driver left in. +std::vector framed(size_t hl, size_t payload, size_t payloads, uint8_t fill) { + std::vector d; + const uint8_t flags = + static_cast(kEoh | (hl >= 6 ? kPts : 0) | (hl == 12 || hl == 8 ? kScr : 0)); + for (size_t p = 0; p < payloads; p++) { + if (p) { // the driver strips only the first, so every later payload keeps its header + d.push_back(static_cast(hl)); + d.push_back(flags); + for (size_t i = 2; i < hl; i++) d.push_back(i == 2 ? 0x11 : 0x00); // a constant PTS, zero SCR + } + const size_t data = payload - (p ? hl : hl); + for (size_t i = 0; i < data; i++) d.push_back(fill); + } + return d; +} +} // namespace + +// A grabber's payload headers sit at the packet stride, and stripping them leaves only the bitstream. +TEST_CASE("payload headers are stripped at the stride the first one sets") { + const size_t hl = 12, payload = 512; + std::vector d = framed(hl, payload, 4, 0xAB); + const size_t before = d.size(); + const size_t after = stripPayloadHeaders(d.data(), d.size()); + CHECK(after < before); + CHECK(after == before - 3 * hl); // three later payloads, each losing its header + for (size_t i = 0; i < after; i++) CHECK(d[i] == 0xAB); +} + +// A grabber that sends neither a PTS nor an SCR has 2-byte headers, which must still be found. +TEST_CASE("a header without timestamps is found from its flags") { + CHECK(headerLenFor(kEoh) == 2); + CHECK(headerLenFor(kEoh | kPts) == 6); + CHECK(headerLenFor(kEoh | kScr) == 8); + CHECK(headerLenFor(kEoh | kPts | kScr) == 12); +} + +// A frame with no headers left in it is handed back byte for byte, since a false positive corrupts it. +TEST_CASE("a frame the driver already stripped is left alone") { + std::vector d(2048, 0x5A); + const std::vector copy = d; + CHECK(stripPayloadHeaders(d.data(), d.size()) == d.size()); + CHECK(std::memcmp(d.data(), copy.data(), d.size()) == 0); +} + +// UVC states a rate as a period, and 59.94 fps is a real mode that should read as 60. +TEST_CASE("a frame interval becomes the rate it names") { + CHECK(fpsFrom(333333) == 30); // 30 fps + CHECK(166833 ? fpsFrom(166833) == 60 : false); // 59.94 fps, rounded up + CHECK(fpsFrom(0) == 0); +} + +// The fastest rate at or below the one asked for, so a 60 fps row never doubles the work for a 30 fps request. +TEST_CASE("the rate asked for wins over a faster one the device offers") { + CHECK(betterFps(0, 60, 30) == 60); // nothing held yet: take what there is + CHECK(betterFps(60, 30, 30) == 30); // 30 is at the request, 60 above it + CHECK(betterFps(30, 60, 30) == 30); + CHECK(betterFps(15, 30, 30) == 30); // both fit, so the faster + CHECK(betterFps(60, 90, 30) == 60); // neither fits, so the slower +} + +// The smallest mode at or above the picture, at the rate asked for: the decode and the shrink both scale with it. +TEST_CASE("the mode asked for is the smallest that covers the picture") { + const Mode rows[] = {{1920, 1080, 30}, {1280, 720, 60}, {1280, 720, 30}, {640, 480, 60}, {640, 480, 30}}; + uint16_t w = 64, h = 48; + uint8_t fps = 30; + REQUIRE(pickMode(rows, 5, w, h, fps)); + CHECK(w == 640); + CHECK(h == 480); + CHECK(fps == 30); // not the 60 the same resolution also offers +} + +// A grabber offering a small mode saves the decode entirely, so it must be preferred. +TEST_CASE("a smaller advertised mode is preferred over a larger one") { + const Mode rows[] = {{640, 480, 60}, {320, 240, 30}}; + uint16_t w = 64, h = 48; + uint8_t fps = 30; + REQUIRE(pickMode(rows, 2, w, h, fps)); + CHECK(w == 320); + CHECK(h == 240); +} + +// Two shapes can share an area, so the rates must accumulate per resolution or a mode is invented. +TEST_CASE("two modes of equal area do not mix their rates") { + const Mode rows[] = {{1280, 720, 30}, {960, 960, 15}}; // both 921,600 pixels + uint16_t w = 640, h = 480; + uint8_t fps = 30; + REQUIRE(pickMode(rows, 2, w, h, fps)); + // Whichever resolution wins, its own rate comes with it: never 960x960 at the 1280x720 row's 30. + const bool coherent = (w == 1280 && h == 720 && fps == 30) || (w == 960 && h == 960 && fps == 15); + CHECK(coherent); +} + +// Nothing big enough leaves the request alone, so the device picks and the picture is told what arrived. +TEST_CASE("a device with nothing above the picture leaves the request alone") { + const Mode rows[] = {{160, 120, 30}}; + uint16_t w = 320, h = 240; + uint8_t fps = 30; + CHECK_FALSE(pickMode(rows, 1, w, h, fps)); + CHECK(w == 320); + CHECK(h == 240); +} + +// The decoder pads to its MCU, which is 16x16 for YUV420, 16x8 for 422 and 8x8 for 444. +TEST_CASE("the row stride comes from the size the decoder wrote") { + CHECK(strideFor(640, 480, 16, 16, 640u * 480u * 3u) == 640 * 3); // both axes already aligned + CHECK(strideFor(1920, 1080, 16, 16, 1920u * 1088u * 3u) == 1920 * 3); // the height padded, not the width + CHECK(strideFor(1288, 1080, 16, 8, 1296u * 1080u * 3u) == 1296 * 3); // a 422 width padded to 16 + CHECK(strideFor(1288, 1080, 16, 8, 999u) == 0); // neither layout: refuse the frame +} + +// The picture is exactly the size asked for, as the test pattern's is, whatever the source's shape. +TEST_CASE("a frame of any shape fills the whole picture that was asked for") { + // A 4:3 source into a 16:9 picture: squashed rather than letterboxed, which for edge colour is + // what is wanted, and every source pixel still lands in exactly one cell. + const uint16_t w = 640, h = 480, ow = 60, oh = 34; + std::vector src(static_cast(w) * h * 3, 77); + std::vector dst(static_cast(ow) * oh * 3 + 3, 0xEE); + areaAverage(src.data(), static_cast(w) * 3, w, h, ow, oh, dst.data()); + for (size_t i = 0; i < static_cast(ow) * oh * 3; i++) CHECK(dst[i] == 77); + for (size_t i = static_cast(ow) * oh * 3; i < dst.size(); i++) CHECK(dst[i] == 0xEE); +} + +// A picture larger than the source upscales rather than reading past it, so no cell is left empty. +TEST_CASE("a picture larger than the frame stays inside the source") { + const uint16_t w = 4, h = 3, ow = 9, oh = 7; + std::vector src(static_cast(w) * h * 3, 200); + std::vector dst(static_cast(ow) * oh * 3, 0); + areaAverage(src.data(), static_cast(w) * 3, w, h, ow, oh, dst.data()); + for (uint8_t v : dst) CHECK(v == 200); + for (uint32_t i = 0; i < ow; i++) { // no cell may name a column the source does not have + uint32_t lo = 0, hi = 0; + cellRange(i, ow, w, lo, hi); + CHECK(hi <= w); + CHECK(lo < hi); + } +} + +// Every column and row of the source must reach a cell: a dropped remainder would blind the +// right and bottom edges, which is exactly where a backlight reads its colour. +TEST_CASE("the cell ranges tile the whole source with no gap and no overlap") { + for (uint32_t out : {1u, 7u, 34u, 45u, 480u}) { + for (uint32_t src : {480u, 640u, 1080u, 1920u}) { + uint32_t covered = 0, prevHi = 0; + for (uint32_t i = 0; i < out; i++) { + uint32_t lo = 0, hi = 0; + cellRange(i, out, src, lo, hi); + CHECK(lo == prevHi); // no gap: this cell starts where the last ended + CHECK(hi > lo); // and is never empty + covered += hi - lo; + prevHi = hi; + } + CHECK(prevHi == src); // the last cell ends exactly at the source's edge + CHECK(covered == src); + } + } +} + +// The regression for the dropped remainder. An integer factor of 14 read only columns 0..629, so +// a bright screen edge in 630..639 was invisible to the lights that sit along that edge. A cell +// samples at a stride, so a single lit column can still fall between taps; a real edge cannot. +TEST_CASE("a bright screen edge the integer factor never read now reaches the edge cell") { + const uint16_t w = 640, h = 480, ow = 45, oh = 34; + std::vector src(static_cast(w) * h * 3, 0); + for (uint16_t y = 0; y < h; y++) + for (uint16_t x = 630; x < w; x++) { // exactly the columns an n=14 box dropped + uint8_t* p = src.data() + (static_cast(y) * w + x) * 3; + p[0] = p[1] = p[2] = 255; + } + std::vector dst(static_cast(ow) * oh * 3, 0); + areaAverage(src.data(), static_cast(w) * 3, w, h, ow, oh, dst.data()); + const uint8_t* last = dst.data() + (static_cast(oh / 2) * ow + (ow - 1)) * 3; + CHECK(last[0] > 0); + // And the cell's own range really does end at the source's last column. + uint32_t lo = 0, hi = 0; + cellRange(ow - 1, ow, w, lo, hi); + CHECK(hi == w); + CHECK(lo < 630); +} + +// A uniform region keeps its value exactly, whatever the ratio between source and picture. +TEST_CASE("a uniform frame averages to itself") { + const uint16_t w = 97, h = 61; // deliberately prime-ish, so no whole factor exists + std::vector src(static_cast(w) * h * 3, 123); + std::vector dst(45u * 34u * 3u, 0); + areaAverage(src.data(), static_cast(w) * 3, w, h, 45, 34, dst.data()); + for (uint8_t v : dst) CHECK(v == 123); +} + +// Averaging in linear light rather than on the bytes, so a shrink does not darken the picture. +TEST_CASE("a half-black half-white frame averages above the arithmetic mean") { + const uint16_t w = 2, h = 2; + std::vector src{0, 0, 0, 255, 255, 255, 0, 0, 0, 255, 255, 255}; + uint8_t dst[3] = {}; + areaAverage(src.data(), static_cast(w) * 3, w, h, 1, 1, dst); + CHECK(dst[0] > 128); // the arithmetic mean would be 127 + CHECK(dst[0] == 180); // sqrt((255*255*2)/4) = 180 +} + +// The taps are bounded, which is what keeps a 1080p frame off the PSRAM budget and inside a 32-bit sum. +TEST_CASE("a cell samples at a stride rather than reading every pixel it covers") { + uint32_t lo = 0, hi = 0; + cellRange(0, 1, 1920, lo, hi); // the worst case: one cell over a whole 1080p row + const uint32_t stride = (hi - lo + kTapsPerAxis - 1) / kTapsPerAxis; + uint32_t taps = 0; + for (uint32_t p = lo; p < hi; p += stride) taps++; + CHECK(taps <= kTapsPerAxis); + // At most 16 taps a cell, so the largest sum is far inside what a uint32 holds. + CHECK(static_cast(kTapsPerAxis) * kTapsPerAxis * 255u * 255u < 0xFFFFFFFFull); +} + From febc97dbb30abce24deeb2633b6e20740cad919a Mon Sep 17 00:00:00 2001 From: Shura Fedorovskyi Date: Wed, 21 Oct 2026 13:44:51 +0400 Subject: [PATCH 2/3] refactor: one home for the picture shrink, and the review's smaller items cellSpan and areaMean move to core/util/AreaSample.h, beside PictureFrame.h, and both the Picture effect and the capture path call them. The effect's own isqrt goes with them, so a fix to the ranges or the mean reaches both. The UVC headers and the mode choice stay with the transport. Every header length headerLenFor names is now stripped in a test, not only 12. That found the test helper setting a PTS flag on an 8-byte header, which made it declare one size and occupy another; the production code was right to refuse it. The rest of the review: the shrink's tests follow it into their own file, a test name that says what holds, the tautology in fpsFrom's check and in framed(), and American spelling. --- src/core/util/AreaSample.h | 47 +++++++++++ src/light/effects/PictureEffect.h | 30 ++----- src/platform/usbvideo_frames.h | 36 ++------ test/CMakeLists.txt | 1 + test/unit/core/unit_AreaSample.cpp | 93 +++++++++++++++++++++ test/unit/core/unit_usbvideo_frames.cpp | 104 +++++------------------- 6 files changed, 174 insertions(+), 137 deletions(-) create mode 100644 src/core/util/AreaSample.h create mode 100644 test/unit/core/unit_AreaSample.cpp diff --git a/src/core/util/AreaSample.h b/src/core/util/AreaSample.h new file mode 100644 index 00000000..ba3ad0a9 --- /dev/null +++ b/src/core/util/AreaSample.h @@ -0,0 +1,47 @@ +#pragma once + +/// Sampling a picture's rectangle onto one output cell: the proportional span and its mean in linear light. + +#include "core/util/color.h" // RGB +#include "core/util/math16.h" // isqrt: the root the linear-light mean undoes its squares with + +#include +#include + +namespace mm { + +/// The source pixels one output cell covers, `hi` one past the last. +struct Span { + uint16_t lo = 0; + uint16_t hi = 0; +}; + +/// Taps an axis of one cell, so a cell's sum stays well inside 32 bits whatever the source size. +constexpr uint32_t kAreaTapsPerAxis = 4; + +/// The source span output cell `i` of `out` covers, the proportional split every sampler shares. +inline Span cellSpan(uint32_t i, uint32_t out, uint32_t src) { + uint32_t lo = i * src / out, hi = (i + 1u) * src / out; + if (hi <= lo) hi = lo + 1; // a source smaller than the output: neighboring cells share a pixel + return Span{static_cast(lo), static_cast(hi)}; +} + +/// The rectangle's mean in linear light (square, average, root: gamma 2 for the display curve), from at most four reads per axis. +inline RGB areaMean(const uint8_t* rgb, size_t stride, Span cx, Span ry) { + const uint32_t sx = (cx.hi - cx.lo + kAreaTapsPerAxis - 1u) / kAreaTapsPerAxis; + const uint32_t sy = (ry.hi - ry.lo + kAreaTapsPerAxis - 1u) / kAreaTapsPerAxis; + uint32_t sum[3] = {0, 0, 0}, taps = 0; + for (uint32_t y = ry.lo; y < ry.hi; y += sy) { + const uint8_t* row = rgb + static_cast(y) * stride; + for (uint32_t x = cx.lo; x < cx.hi; x += sx) { + const uint8_t* p = row + static_cast(x) * 3u; + for (int c = 0; c < 3; c++) sum[c] += static_cast(p[c]) * p[c]; + taps++; + } + } + if (!taps) return RGB{0, 0, 0}; + return RGB{static_cast(isqrt(sum[0] / taps)), static_cast(isqrt(sum[1] / taps)), + static_cast(isqrt(sum[2] / taps))}; +} + +} // namespace mm diff --git a/src/light/effects/PictureEffect.h b/src/light/effects/PictureEffect.h index 5dcec752..dff11c1d 100644 --- a/src/light/effects/PictureEffect.h +++ b/src/light/effects/PictureEffect.h @@ -2,6 +2,7 @@ #include "light/effects/EffectBase.h" #include "core/services/PictureService.h" // the picture it samples, as audio effects include AudioService +#include "core/util/AreaSample.h" // cellSpan, areaMean: the shrink every sampler shares namespace mm { @@ -105,7 +106,7 @@ class PictureEffect : public EffectBase { private: /// One axis of a cell's source rectangle, [lo, hi) in picture pixels; empty outside a fitted picture. - struct Range { uint16_t lo, hi; }; + using Range = Span; /// An edge cell's range at `deep` picture lines in: the first reaches inward from 0, the last back from the end. static void reachEdge(int32_t k, int32_t used, uint32_t pic, uint32_t deep, uint32_t& lo, uint32_t& hi) { @@ -126,9 +127,10 @@ class PictureEffect : public EffectBase { for (uint32_t i = 0; i < n; i++) { const int32_t k = static_cast(i) - offset; if (k < 0 || k >= used) { out[i] = {0, 0}; continue; } - uint32_t lo = static_cast(k) * pic / static_cast(used), hi = (static_cast(k) + 1) * pic / static_cast(used); + const Span base = cellSpan(static_cast(k), static_cast(used), pic); + uint32_t lo = base.lo, hi = base.hi; if (deep) reachEdge(k, used, pic, deep, lo, hi); - if (hi <= lo) hi = lo + 1; // a picture smaller than the grid: neighboring cells share a pixel + if (hi <= lo) hi = lo + 1; // an edge reaching inward can land on its own start out[i] = {static_cast(lo), static_cast(hi)}; } } @@ -201,27 +203,7 @@ class PictureEffect : public EffectBase { /// The rectangle's mean in linear light (square, average, root: gamma 2 for the display curve), from at most four reads per axis. static RGB average(const PictureFrame& f, Range c, Range r) { - const uint32_t sx = (c.hi - c.lo + 3u) / 4u, sy = (r.hi - r.lo + 3u) / 4u; - uint32_t sum[3] = {0, 0, 0}, n = 0; - for (uint32_t y = r.lo; y < r.hi; y += sy) - for (uint32_t x = c.lo; x < c.hi; x += sx) { - const RGB p = at(f, x, y); - sum[0] += static_cast(p.r) * p.r; - sum[1] += static_cast(p.g) * p.g; - sum[2] += static_cast(p.b) * p.b; - n++; - } - return RGB{isqrt(sum[0] / n), isqrt(sum[1] / n), isqrt(sum[2] / n)}; - } - - /// The integer square root of a value below 65,536, the bitwise method. - static uint8_t isqrt(uint32_t v) { - uint32_t root = 0; - for (uint32_t bit = 1u << 14; bit; bit >>= 2) { - if (v >= root + bit) { v -= root + bit; root = (root >> 1) + bit; } - else root >>= 1; - } - return static_cast(root); + return areaMean(f.rgb, static_cast(f.width) * 3u, c, r); } ScratchBuffer state_{*this}; ///< 8.8 per channel per cell, only while smoothing diff --git a/src/platform/usbvideo_frames.h b/src/platform/usbvideo_frames.h index d9fd526f..40a57473 100644 --- a/src/platform/usbvideo_frames.h +++ b/src/platform/usbvideo_frames.h @@ -2,7 +2,7 @@ /// The hardware-free half of USB video capture: the UVC payload headers, the mode choice and the shrink onto a picture. -#include "core/util/math16.h" // isqrt: the tree's integer root, which the box average needs +#include "core/util/AreaSample.h" // cellSpan, areaMean: the shrink every sampler shares #include #include @@ -134,39 +134,17 @@ inline size_t strideFor(uint16_t w, uint16_t h, uint8_t mcuW, uint8_t mcuH, uint return 0; } -/// The source range an output cell covers, the proportional split the Picture effect's ranges use. -inline void cellRange(uint32_t i, uint32_t out, uint32_t src, uint32_t& lo, uint32_t& hi) { - lo = i * src / out; - hi = (i + 1u) * src / out; - if (hi <= lo) hi = lo + 1; // a source smaller than the output: neighbouring cells share a pixel -} - -/// Taps an axis of one cell on the scattered path, so a cell's sum stays well inside 32 bits. -constexpr uint32_t kTapsPerAxis = 4; - -/// Each output pixel the linear-light mean of its rectangle, sampled at a stride: a scattered read -/// beats a sequential one here, since the arithmetic dominates and a full walk is 2.7x more of it. +/// Each output pixel the mean of its rectangle, through the shared sampler so one fix reaches both paths. inline void areaAverage(const uint8_t* src, size_t stride, uint16_t w, uint16_t h, uint16_t ow, uint16_t oh, uint8_t* dst) { for (uint16_t y = 0; y < oh; y++) { - uint32_t ylo = 0, yhi = 0; - cellRange(y, oh, h, ylo, yhi); - const uint32_t sy = (yhi - ylo + kTapsPerAxis - 1) / kTapsPerAxis; + const Span ry = cellSpan(y, oh, h); for (uint16_t x = 0; x < ow; x++) { - uint32_t xlo = 0, xhi = 0; - cellRange(x, ow, w, xlo, xhi); - const uint32_t sx = (xhi - xlo + kTapsPerAxis - 1) / kTapsPerAxis; - uint32_t sum[3] = {0, 0, 0}, taps = 0; - for (uint32_t py = ylo; py < yhi; py += sy) { - const uint8_t* row = src + static_cast(py) * stride; - for (uint32_t px = xlo; px < xhi; px += sx) { - const uint8_t* p = row + static_cast(px) * 3; - for (int c = 0; c < 3; c++) sum[c] += static_cast(p[c]) * p[c]; - taps++; - } - } + const RGB mean = areaMean(src, stride, cellSpan(x, ow, w), ry); uint8_t* out = dst + (static_cast(y) * ow + x) * 3; - for (int c = 0; c < 3; c++) out[c] = static_cast(isqrt(sum[c] / taps)); + out[0] = mean.r; + out[1] = mean.g; + out[2] = mean.b; } } } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 403ef1c0..d600973c 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -17,6 +17,7 @@ add_executable(mm_tests unit/core/unit_Color.cpp unit/core/unit_math8.cpp unit/core/unit_math16.cpp + unit/core/unit_AreaSample.cpp unit/core/unit_usbvideo_frames.cpp unit/core/unit_noise.cpp unit/core/unit_fields.cpp diff --git a/test/unit/core/unit_AreaSample.cpp b/test/unit/core/unit_AreaSample.cpp new file mode 100644 index 00000000..3e63c8d6 --- /dev/null +++ b/test/unit/core/unit_AreaSample.cpp @@ -0,0 +1,93 @@ +/// The sampling every picture path shares: the proportional span of one output cell, and its mean in linear light. + +#include "doctest.h" +#include "core/util/AreaSample.h" + +#include +#include + +using namespace mm; + +namespace { +// Every output pixel the mean of its own span, as the capture path and the Picture effect both do. +void shrink(const uint8_t* src, uint16_t w, uint16_t h, uint16_t ow, uint16_t oh, uint8_t* dst) { + for (uint16_t y = 0; y < oh; y++) { + const Span ry = cellSpan(y, oh, h); + for (uint16_t x = 0; x < ow; x++) { + const RGB mean = areaMean(src, static_cast(w) * 3u, cellSpan(x, ow, w), ry); + uint8_t* out = dst + (static_cast(y) * ow + x) * 3; + out[0] = mean.r; + out[1] = mean.g; + out[2] = mean.b; + } + } +} +} // namespace + +// Every column and row of the source must reach a cell: a dropped remainder would blind the right and bottom edges, which is exactly where a backlight reads its color. +TEST_CASE("the cell ranges tile the whole source with no gap and no overlap") { + for (uint32_t out : {1u, 7u, 34u, 45u, 480u}) { + for (uint32_t src : {480u, 640u, 1080u, 1920u}) { + uint32_t covered = 0, prevHi = 0; + for (uint32_t i = 0; i < out; i++) { + const Span s = cellSpan(i, out, src); + CHECK(s.lo == prevHi); // no gap: this cell starts where the last ended + CHECK(s.hi > s.lo); // and is never empty + covered += s.hi - s.lo; + prevHi = s.hi; + } + CHECK(prevHi == src); // the last cell ends exactly at the source's edge + CHECK(covered == src); + } + } +} + +// A cell's span reaches the source's last column, so a bright screen edge lights the lights along it. A cell samples at a stride, so one lit column can fall between taps; a real edge cannot. +TEST_CASE("a bright screen edge reaches the edge cell") { + const uint16_t w = 640, h = 480, ow = 45, oh = 34; + std::vector src(static_cast(w) * h * 3, 0); + for (uint16_t y = 0; y < h; y++) + for (uint16_t x = 630; x < w; x++) { // the rightmost columns, which an integer factor would drop + uint8_t* p = src.data() + (static_cast(y) * w + x) * 3; + p[0] = p[1] = p[2] = 255; + } + std::vector dst(static_cast(ow) * oh * 3, 0); + shrink(src.data(), w, h, ow, oh, dst.data()); + const uint8_t* last = dst.data() + (static_cast(oh / 2) * ow + (ow - 1)) * 3; + CHECK(last[0] > 0); + // And the cell's own range really does end at the source's last column. + const Span edge = cellSpan(ow - 1, ow, w); + CHECK(edge.hi == w); + CHECK(edge.lo < 630); +} + +// A uniform region keeps its value exactly, whatever the ratio between source and picture. +TEST_CASE("a uniform frame averages to itself") { + const uint16_t w = 97, h = 61; // deliberately prime-ish, so no whole factor exists + std::vector src(static_cast(w) * h * 3, 123); + std::vector dst(45u * 34u * 3u, 0); + shrink(src.data(), w, h, 45, 34, dst.data()); + for (uint8_t v : dst) CHECK(v == 123); +} + +// Averaging in linear light rather than on the bytes, so a shrink does not darken the picture. +TEST_CASE("a half-black half-white frame averages above the arithmetic mean") { + const uint16_t w = 2, h = 2; + std::vector src{0, 0, 0, 255, 255, 255, 0, 0, 0, 255, 255, 255}; + uint8_t dst[3] = {}; + shrink(src.data(), w, h, 1, 1, dst); + CHECK(dst[0] > 128); // the arithmetic mean would be 127 + CHECK(dst[0] == 180); // sqrt((255*255*2)/4) = 180 +} + +// The taps are bounded, which is what keeps a 1080p frame off the PSRAM budget and inside a 32-bit sum. +TEST_CASE("a cell samples at a stride rather than reading every pixel it covers") { + const Span whole = cellSpan(0, 1, 1920); // the worst case: one cell over a whole 1080p row + const uint32_t stride = (whole.hi - whole.lo + kAreaTapsPerAxis - 1) / kAreaTapsPerAxis; + uint32_t taps = 0; + for (uint32_t p = whole.lo; p < whole.hi; p += stride) taps++; + CHECK(taps <= kAreaTapsPerAxis); + // At most 16 taps a cell, so the largest sum is far inside what a uint32 holds. + CHECK(static_cast(kAreaTapsPerAxis) * kAreaTapsPerAxis * 255u * 255u < 0xFFFFFFFFull); +} + diff --git a/test/unit/core/unit_usbvideo_frames.cpp b/test/unit/core/unit_usbvideo_frames.cpp index 6f0a812e..76d5200e 100644 --- a/test/unit/core/unit_usbvideo_frames.cpp +++ b/test/unit/core/unit_usbvideo_frames.cpp @@ -14,15 +14,16 @@ namespace { // A bulk frame carrying `payload`-byte payloads, each opening with a `hl`-byte header the driver left in. std::vector framed(size_t hl, size_t payload, size_t payloads, uint8_t fill) { std::vector d; + // The flags must agree with the length, or a header declares one size and occupies another. const uint8_t flags = - static_cast(kEoh | (hl >= 6 ? kPts : 0) | (hl == 12 || hl == 8 ? kScr : 0)); + static_cast(kEoh | (hl == 6 || hl == 12 ? kPts : 0) | (hl == 8 || hl == 12 ? kScr : 0)); for (size_t p = 0; p < payloads; p++) { if (p) { // the driver strips only the first, so every later payload keeps its header d.push_back(static_cast(hl)); d.push_back(flags); for (size_t i = 2; i < hl; i++) d.push_back(i == 2 ? 0x11 : 0x00); // a constant PTS, zero SCR } - const size_t data = payload - (p ? hl : hl); + const size_t data = payload - hl; for (size_t i = 0; i < data; i++) d.push_back(fill); } return d; @@ -41,13 +42,24 @@ TEST_CASE("payload headers are stripped at the stride the first one sets") { } // A grabber that sends neither a PTS nor an SCR has 2-byte headers, which must still be found. -TEST_CASE("a header without timestamps is found from its flags") { +TEST_CASE("a header's length comes from its own flags") { CHECK(headerLenFor(kEoh) == 2); CHECK(headerLenFor(kEoh | kPts) == 6); CHECK(headerLenFor(kEoh | kScr) == 8); CHECK(headerLenFor(kEoh | kPts | kScr) == 12); } +// Each length headerLenFor names must be found and stripped, since a grabber picks one and keeps it. +TEST_CASE("every header length is stripped, not only the longest") { + for (size_t hl : {size_t(2), size_t(6), size_t(8), size_t(12)}) { + std::vector d = framed(hl, 512, 4, 0x7E); + const size_t before = d.size(); + const size_t after = stripPayloadHeaders(d.data(), d.size()); + CHECK(after == before - 3 * hl); // three later payloads, each losing its header + for (size_t i = 0; i < after; i++) CHECK(d[i] == 0x7E); + } +} + // A frame with no headers left in it is handed back byte for byte, since a false positive corrupts it. TEST_CASE("a frame the driver already stripped is left alone") { std::vector d(2048, 0x5A); @@ -59,7 +71,7 @@ TEST_CASE("a frame the driver already stripped is left alone") { // UVC states a rate as a period, and 59.94 fps is a real mode that should read as 60. TEST_CASE("a frame interval becomes the rate it names") { CHECK(fpsFrom(333333) == 30); // 30 fps - CHECK(166833 ? fpsFrom(166833) == 60 : false); // 59.94 fps, rounded up + CHECK(fpsFrom(166833) == 60); // 59.94 fps, rounded up CHECK(fpsFrom(0) == 0); } @@ -124,8 +136,7 @@ TEST_CASE("the row stride comes from the size the decoder wrote") { // The picture is exactly the size asked for, as the test pattern's is, whatever the source's shape. TEST_CASE("a frame of any shape fills the whole picture that was asked for") { - // A 4:3 source into a 16:9 picture: squashed rather than letterboxed, which for edge colour is - // what is wanted, and every source pixel still lands in exactly one cell. + // A 4:3 source into a 16:9 picture: squashed rather than letterboxed, which for edge color is what is wanted, and every source pixel still lands in exactly one cell. const uint16_t w = 640, h = 480, ow = 60, oh = 34; std::vector src(static_cast(w) * h * 3, 77); std::vector dst(static_cast(ow) * oh * 3 + 3, 0xEE); @@ -142,83 +153,8 @@ TEST_CASE("a picture larger than the frame stays inside the source") { areaAverage(src.data(), static_cast(w) * 3, w, h, ow, oh, dst.data()); for (uint8_t v : dst) CHECK(v == 200); for (uint32_t i = 0; i < ow; i++) { // no cell may name a column the source does not have - uint32_t lo = 0, hi = 0; - cellRange(i, ow, w, lo, hi); - CHECK(hi <= w); - CHECK(lo < hi); - } -} - -// Every column and row of the source must reach a cell: a dropped remainder would blind the -// right and bottom edges, which is exactly where a backlight reads its colour. -TEST_CASE("the cell ranges tile the whole source with no gap and no overlap") { - for (uint32_t out : {1u, 7u, 34u, 45u, 480u}) { - for (uint32_t src : {480u, 640u, 1080u, 1920u}) { - uint32_t covered = 0, prevHi = 0; - for (uint32_t i = 0; i < out; i++) { - uint32_t lo = 0, hi = 0; - cellRange(i, out, src, lo, hi); - CHECK(lo == prevHi); // no gap: this cell starts where the last ended - CHECK(hi > lo); // and is never empty - covered += hi - lo; - prevHi = hi; - } - CHECK(prevHi == src); // the last cell ends exactly at the source's edge - CHECK(covered == src); - } + const mm::Span s = mm::cellSpan(i, ow, w); + CHECK(s.hi <= w); + CHECK(s.lo < s.hi); } } - -// The regression for the dropped remainder. An integer factor of 14 read only columns 0..629, so -// a bright screen edge in 630..639 was invisible to the lights that sit along that edge. A cell -// samples at a stride, so a single lit column can still fall between taps; a real edge cannot. -TEST_CASE("a bright screen edge the integer factor never read now reaches the edge cell") { - const uint16_t w = 640, h = 480, ow = 45, oh = 34; - std::vector src(static_cast(w) * h * 3, 0); - for (uint16_t y = 0; y < h; y++) - for (uint16_t x = 630; x < w; x++) { // exactly the columns an n=14 box dropped - uint8_t* p = src.data() + (static_cast(y) * w + x) * 3; - p[0] = p[1] = p[2] = 255; - } - std::vector dst(static_cast(ow) * oh * 3, 0); - areaAverage(src.data(), static_cast(w) * 3, w, h, ow, oh, dst.data()); - const uint8_t* last = dst.data() + (static_cast(oh / 2) * ow + (ow - 1)) * 3; - CHECK(last[0] > 0); - // And the cell's own range really does end at the source's last column. - uint32_t lo = 0, hi = 0; - cellRange(ow - 1, ow, w, lo, hi); - CHECK(hi == w); - CHECK(lo < 630); -} - -// A uniform region keeps its value exactly, whatever the ratio between source and picture. -TEST_CASE("a uniform frame averages to itself") { - const uint16_t w = 97, h = 61; // deliberately prime-ish, so no whole factor exists - std::vector src(static_cast(w) * h * 3, 123); - std::vector dst(45u * 34u * 3u, 0); - areaAverage(src.data(), static_cast(w) * 3, w, h, 45, 34, dst.data()); - for (uint8_t v : dst) CHECK(v == 123); -} - -// Averaging in linear light rather than on the bytes, so a shrink does not darken the picture. -TEST_CASE("a half-black half-white frame averages above the arithmetic mean") { - const uint16_t w = 2, h = 2; - std::vector src{0, 0, 0, 255, 255, 255, 0, 0, 0, 255, 255, 255}; - uint8_t dst[3] = {}; - areaAverage(src.data(), static_cast(w) * 3, w, h, 1, 1, dst); - CHECK(dst[0] > 128); // the arithmetic mean would be 127 - CHECK(dst[0] == 180); // sqrt((255*255*2)/4) = 180 -} - -// The taps are bounded, which is what keeps a 1080p frame off the PSRAM budget and inside a 32-bit sum. -TEST_CASE("a cell samples at a stride rather than reading every pixel it covers") { - uint32_t lo = 0, hi = 0; - cellRange(0, 1, 1920, lo, hi); // the worst case: one cell over a whole 1080p row - const uint32_t stride = (hi - lo + kTapsPerAxis - 1) / kTapsPerAxis; - uint32_t taps = 0; - for (uint32_t p = lo; p < hi; p += stride) taps++; - CHECK(taps <= kTapsPerAxis); - // At most 16 taps a cell, so the largest sum is far inside what a uint32 holds. - CHECK(static_cast(kTapsPerAxis) * kTapsPerAxis * 255u * 255u < 0xFFFFFFFFull); -} - From fa8a0ea431605b9ba75d660c9626e1a3e6aff00a Mon Sep 17 00:00:00 2001 From: Shura Fedorovskyi Date: Wed, 21 Oct 2026 13:53:13 +0400 Subject: [PATCH 3/3] fix: the three checks the review run failed check_docgen: Span's lo and hi carry their own line, as a public member must. check_code: the header parse and the mode choice were each over lizard's complexity bound. The field tests become named predicates, the scan and the compaction become their own steps, and a mode's fit becomes one question, so every function is back under it with the same behaviour. check_prose: three weasel words out of the comments. --- src/core/util/AreaSample.h | 4 +- src/platform/usbvideo_frames.h | 62 ++++++++++++++++--------- test/unit/core/unit_AreaSample.cpp | 2 +- test/unit/core/unit_usbvideo_frames.cpp | 2 +- 4 files changed, 45 insertions(+), 25 deletions(-) diff --git a/src/core/util/AreaSample.h b/src/core/util/AreaSample.h index ba3ad0a9..96d483ad 100644 --- a/src/core/util/AreaSample.h +++ b/src/core/util/AreaSample.h @@ -12,8 +12,8 @@ namespace mm { /// The source pixels one output cell covers, `hi` one past the last. struct Span { - uint16_t lo = 0; - uint16_t hi = 0; + uint16_t lo = 0; ///< the first source pixel the cell covers + uint16_t hi = 0; ///< one past the last, so hi - lo is the count }; /// Taps an axis of one cell, so a cell's sum stays well inside 32 bits whatever the source size. diff --git a/src/platform/usbvideo_frames.h b/src/platform/usbvideo_frames.h index 40a57473..2f3302a4 100644 --- a/src/platform/usbvideo_frames.h +++ b/src/platform/usbvideo_frames.h @@ -28,18 +28,28 @@ inline size_t headerLenFor(uint8_t flags) { return 2u + ((flags & kPts) ? 4u : 0u) + ((flags & kScr) ? 6u : 0u); } +/// Whether the flags are a header's: end of header set, no error, no reserved bit. +inline bool headerFlagsOk(uint8_t flags) { return (flags & kEoh) && !(flags & (kErr | kReserved)); } + +/// Whether the SCR's reserved top 5 bits are zero, the strongest single check a header carries. +inline bool headerScrOk(const uint8_t* h, uint8_t flags, uint8_t len) { + return !(flags & kScr) || !(h[len - 1] & 0xF8); +} + +/// Whether the PTS matches the frame's first header, which is constant across one frame's payloads. +inline bool headerPtsOk(const uint8_t* h, uint8_t flags, const uint8_t* pts) { + return !pts || !(flags & kPts) || std::memcmp(h + 2, pts, 4) == 0; +} + /// Whether `h` is a payload header of exactly `want` bytes; `pts` pins it to the frame's first one. inline bool isPayloadHeader(const uint8_t* h, size_t avail, size_t want, const uint8_t* pts) { if (avail < 2) return false; const uint8_t len = h[0], flags = h[1]; if (len != want || len != headerLenFor(flags) || avail < len) return false; - if (!(flags & kEoh) || (flags & (kErr | kReserved))) return false; - // The SCR's top 5 bits are reserved and zero, the strongest single check a header carries. - if ((flags & kScr) && (h[len - 1] & 0xF8)) return false; - return !pts || !(flags & kPts) || std::memcmp(h + 2, pts, 4) == 0; + return headerFlagsOk(flags) && headerScrOk(h, flags, len) && headerPtsOk(h, flags, pts); } -/// Whether headers of length `hl` really recur after `first`, which is what rules out a chance match. +/// Whether headers of length `hl` recur after `first`, which is what rules out a chance match. inline bool confirmedRun(const uint8_t* d, size_t len, size_t first, size_t hl) { const uint8_t* pts = (d[first + 1] & kPts) ? d + first + 2 : nullptr; const size_t stride = first + hl; @@ -49,20 +59,15 @@ inline bool confirmedRun(const uint8_t* d, size_t len, size_t first, size_t hl) return seen > 0 || first + stride + hl > len; // a frame too short to confirm is taken as it is } -/// Drops the payload headers a bulk frame still carries, answering the compacted length. -inline size_t stripPayloadHeaders(uint8_t* d, size_t len) { - static constexpr size_t kLens[] = {12, 8, 6, 2}; // longest first: the more fields, the surer the match - size_t hdr = 0, hl = 0; - for (size_t cand : kLens) { - for (size_t i = kBulkMps - cand; i + cand <= len; i += kBulkMps) { - if (!isPayloadHeader(d + i, len - i, cand, nullptr) || !confirmedRun(d, len, i, cand)) continue; - hdr = i; - hl = cand; - break; - } - if (hdr) break; - } - if (!hdr) return len; +/// The offset of the first confirmed header of length `hl`, or 0 when the frame carries none. +inline size_t findHeader(const uint8_t* d, size_t len, size_t hl) { + for (size_t i = kBulkMps - hl; i + hl <= len; i += kBulkMps) + if (isPayloadHeader(d + i, len - i, hl, nullptr) && confirmedRun(d, len, i, hl)) return i; + return 0; +} + +/// Close the gaps the headers from `hdr` on leave, answering the compacted length. +inline size_t compactFrom(uint8_t* d, size_t len, size_t hdr, size_t hl) { const size_t payload = hdr + hl; uint8_t pts[4] = {}; // copied: the compaction overwrites the header it came from const bool hasPts = (d[hdr + 1] & kPts) != 0; @@ -79,6 +84,16 @@ inline size_t stripPayloadHeaders(uint8_t* d, size_t len) { return w + (len - r); } +/// Drops the payload headers a bulk frame still carries, answering the compacted length. +inline size_t stripPayloadHeaders(uint8_t* d, size_t len) { + static constexpr size_t kLens[] = {12, 8, 6, 2}; // longest first: the more fields, the surer the match + for (size_t hl : kLens) { + const size_t hdr = findHeader(d, len, hl); + if (hdr) return compactFrom(d, len, hdr, hl); + } + return len; +} + // --- Choosing a mode ---------------------------------------------------------------------------- /// UVC states a rate as a period in 100 ns ticks. Rounded, so 59.94 fps reads as 60 rather than 59. @@ -96,6 +111,12 @@ inline uint8_t betterFps(uint8_t have, uint8_t row, uint8_t wanted) { return row < have ? row : have; } +/// Whether a mode covers the ceiling and is no bigger to decode than the best so far. +inline bool modeFits(const Mode& m, uint16_t width, uint16_t height, uint32_t area, uint32_t bestArea) { + if (m.width < width || m.height < height) return false; // below the ceiling it would be upscaled + return !bestArea || area <= bestArea; // a bigger picture to decode every frame +} + /// The smallest mode at or above the ceiling, at the rate asked for, so decode and shrink stay cheap. inline bool pickMode(const Mode* modes, size_t count, uint16_t& width, uint16_t& height, uint8_t& fps) { uint32_t bestArea = 0; @@ -103,9 +124,8 @@ inline bool pickMode(const Mode* modes, size_t count, uint16_t& width, uint16_t& uint8_t bestFps = 0; for (size_t i = 0; i < count; i++) { const Mode& m = modes[i]; - if (m.width < width || m.height < height) continue; // below the ceiling it would be upscaled const uint32_t area = static_cast(m.width) * m.height; - if (bestArea && area > bestArea) continue; // a bigger picture to decode every frame + if (!modeFits(m, width, height, area, bestArea)) continue; // The rates accumulate per resolution, not per area: two shapes can share an area. if (m.width != bw || m.height != bh) bestFps = 0; bestArea = area; diff --git a/test/unit/core/unit_AreaSample.cpp b/test/unit/core/unit_AreaSample.cpp index 3e63c8d6..8617aa1f 100644 --- a/test/unit/core/unit_AreaSample.cpp +++ b/test/unit/core/unit_AreaSample.cpp @@ -55,7 +55,7 @@ TEST_CASE("a bright screen edge reaches the edge cell") { shrink(src.data(), w, h, ow, oh, dst.data()); const uint8_t* last = dst.data() + (static_cast(oh / 2) * ow + (ow - 1)) * 3; CHECK(last[0] > 0); - // And the cell's own range really does end at the source's last column. + // And the cell's own span ends at the source's last column. const Span edge = cellSpan(ow - 1, ow, w); CHECK(edge.hi == w); CHECK(edge.lo < 630); diff --git a/test/unit/core/unit_usbvideo_frames.cpp b/test/unit/core/unit_usbvideo_frames.cpp index 76d5200e..f770f6eb 100644 --- a/test/unit/core/unit_usbvideo_frames.cpp +++ b/test/unit/core/unit_usbvideo_frames.cpp @@ -95,7 +95,7 @@ TEST_CASE("the mode asked for is the smallest that covers the picture") { CHECK(fps == 30); // not the 60 the same resolution also offers } -// A grabber offering a small mode saves the decode entirely, so it must be preferred. +// A grabber offering a small mode saves the decode, so it must be preferred. TEST_CASE("a smaller advertised mode is preferred over a larger one") { const Mode rows[] = {{640, 480, 60}, {320, 240, 30}}; uint16_t w = 64, h = 48;