From ee789775b4e425b9085c06829970a15e61216181 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Sat, 3 Oct 2026 13:48:27 -0400 Subject: [PATCH] perf(install): build rpi-rgb-led-matrix with a faster SetImage (#736) first_time_install.sh applies patches/rpi-rgb-led-matrix/0001-bulk-setimage.patch just before building the Python binding and reverts it afterwards (and from the EXIT trap), so the submodule stays at its pinned commit. The patch copies each image row with one bulk FrameCanvas::SetPixels call and writes the bit planes branch-free: on a 512x64 Pi 4 the frame copy went from 6.57 ms to 2.21 ms. A patch that no longer applies is reported and skipped. Existing installs get it on a rebuild (RPI_RGB_FORCE_REBUILD=1). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 24 +++ first_time_install.sh | 57 +++++- .../0001-bulk-setimage.patch | 174 ++++++++++++++++++ test/test_install_rgb_checkout.py | 96 ++++++++++ 4 files changed, 347 insertions(+), 4 deletions(-) create mode 100644 patches/rpi-rgb-led-matrix/0001-bulk-setimage.patch diff --git a/CHANGELOG.md b/CHANGELOG.md index fb660b04..f7ce8eb4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,30 @@ accepts both, but the store flags the old spelling as deprecated ## Unreleased +### Faster frame copy into the panel (library patch, applied at build time) + +- Copying each frame into the panel buffer (`SetImage`) was the biggest CPU + cost LEDMatrix owns on large panels: 6-7.5 ms per frame on a 512x64 Pi 4 at + ~85 fps, about 60% of a core. The library's binding walked the image column + by column and set one pixel at a time, and each pixel rewrote a word in + every PWM bit plane, 2KB apart, so nearly every write missed the cache. + `patches/rpi-rgb-led-matrix/0001-bulk-setimage.patch` copies row by row in + one bulk call per row, with the colour lookup done once and branch-free + bit-plane writes. The panel buffer is byte-identical to before (882 checks + across image types, offsets, PWM bits, brightness, inverse colours and a + pixel mapper). +- Measured on hdpi (Pi 4, 4x128x64): frame copy 6.57 -> 2.21 ms, the display + process 139% -> 103% of a core, late frames 7.8 -> 5.4 per 1,000. +- `first_time_install.sh` applies the patch to `rpi-rgb-led-matrix-master` + just before building the binding and takes it back out straight after (and + on any exit), so the submodule stays at its pinned commit with no local + changes. A patch that no longer applies after a submodule bump is reported + and skipped; the unpatched library still builds. +- Existing installs keep the library they have until it is rebuilt: + `sudo RPI_RGB_FORCE_REBUILD=1 ./first_time_install.sh`. + `scripts/build_rgbmatrix_nogil.sh` builds from an unpatched copy and is + unchanged. + ### Install - Raspberry Pi OS **Bookworm** (Debian 12, Python 3.11) is supported, diff --git a/first_time_install.sh b/first_time_install.sh index a149fd8c..65cd66fc 100755 --- a/first_time_install.sh +++ b/first_time_install.sh @@ -232,6 +232,51 @@ _sync_rgb_submodule() { fi return 0 } + +# LEDMatrix's own changes to the library live in patches/rpi-rgb-led-matrix/ and +# are applied only for the build: _apply_rgb_patches before it, _revert_rgb_patches +# after it, success or not. The checkout is left exactly as it was, so `git pull` +# and _sync_rgb_submodule never meet local modifications in the submodule. +# A patch that no longer applies (a submodule bump, a hand-edited checkout) is +# reported and skipped -- the unpatched library still builds and works, so it is +# never fatal. One that is already applied is left alone and not reverted. +_RGB_APPLIED_PATCHES=() + +_apply_rgb_patches() { + local sub="$PROJECT_ROOT_DIR/rpi-rgb-led-matrix-master" + local dir="$PROJECT_ROOT_DIR/patches/rpi-rgb-led-matrix" patch name + _RGB_APPLIED_PATCHES=() + [ -d "$dir" ] || return 0 + for patch in "$dir"/*.patch; do + [ -f "$patch" ] || continue + name=$(basename "$patch") + if _git_as_repo_owner -C "$sub" apply --check "$patch" >/dev/null 2>&1; then + if _git_as_repo_owner -C "$sub" apply "$patch"; then + _RGB_APPLIED_PATCHES+=("$patch") + echo "Applied library patch $name" + else + echo "⚠ Could not apply library patch $name; building without it" + fi + elif _git_as_repo_owner -C "$sub" apply --reverse --check "$patch" >/dev/null 2>&1; then + echo "Library patch $name is already applied" + else + echo "⚠ Library patch $name does not apply to this checkout; building without it" + fi + done + return 0 +} + +_revert_rgb_patches() { + local sub="$PROJECT_ROOT_DIR/rpi-rgb-led-matrix-master" i + # Last applied first, in case two patches touch the same file. + for ((i = ${#_RGB_APPLIED_PATCHES[@]} - 1; i >= 0; i--)); do + if ! _git_as_repo_owner -C "$sub" apply --reverse "${_RGB_APPLIED_PATCHES[i]}"; then + echo "⚠ Could not revert $(basename "${_RGB_APPLIED_PATCHES[i]}"); restore the checkout with: git -C $sub checkout -- ." + fi + done + _RGB_APPLIED_PATCHES=() + return 0 +} # --- end rpi-rgb-led-matrix checkout helpers --------------------------------- # Determine the Project Root Directory (where this script is located) @@ -342,10 +387,12 @@ else lm_remove_build_swap() { return 0; } fi -# Remove the temporary build swapfile no matter how the script ends. Step 6 -# tears it down itself; this is the backstop for the error path, since -# on_error ends in `exit` and EXIT traps still run. -trap 'lm_remove_build_swap' EXIT +# Remove the temporary build swapfile, and take any library patches back out +# of the submodule, no matter how the script ends. Step 6 does both itself; +# this is the backstop for the error path (on_error ends in `exit` and EXIT +# traps still run) and for an interrupted build. _revert_rgb_patches only +# touches patches it applied, so running it twice is harmless. +trap 'lm_remove_build_swap; _revert_rgb_patches' EXIT # Helpers retry() { @@ -1289,9 +1336,11 @@ else fi BUILD_OUTPUT=$(mktemp) BUILD_SUCCESS=false + _apply_rgb_patches if run_rgbmatrix_build "$BUILD_JOBS" "$BUILD_OUTPUT"; then BUILD_SUCCESS=true fi + _revert_rgb_patches cat "$BUILD_OUTPUT" >> "$LOG_FILE" if [ "$BUILD_SUCCESS" != true ]; then print_rgbmatrix_build_failure "$BUILD_OUTPUT" diff --git a/patches/rpi-rgb-led-matrix/0001-bulk-setimage.patch b/patches/rpi-rgb-led-matrix/0001-bulk-setimage.patch new file mode 100644 index 00000000..c378c2b9 --- /dev/null +++ b/patches/rpi-rgb-led-matrix/0001-bulk-setimage.patch @@ -0,0 +1,174 @@ +Faster SetImage for rpi-rgb-led-matrix (applied by first_time_install.sh at build +time; the submodule itself stays at its pinned commit). + +Copying a frame into the panel buffer was the biggest CPU cost LEDMatrix owns on +large panels: the binding's SetPixelsPillow walked the image column by column +and called SetPixel per pixel, and each SetPixel read-modify-writes one word per +PWM bit plane, 2KB apart, so consecutive pixels were a whole double-row apart and +almost every write missed the cache. This patch: + + * FrameCanvas gets its own SetPixelsPillow: row by row, one bulk SetPixels call + per row; + * Framebuffer::SetPixels clips once, looks colours up once per pixel, walks each + row's designators in order and writes the bit planes branch-free; + * the base Canvas.SetPixelsPillow loop (RGBMatrix.SetImage) is row-major. + +The bit-plane buffer is byte-identical to the old code's (882 memcmp checks over +noise/gradient/solid/low-value/sparse images, clipped offsets, pwm 7/8/11, +brightness 1/50/90/100, inverse colours, luminance correction off and a pixel +mapper). Measured on a Pi 4 at 512x64: 6.0-6.3 ms -> 1.8 ms per frame through +the Python binding; on hdpi (Pi 4, 4x128x64) frame copy 6.57 -> 2.21 ms and the +display process 139% -> 103% of a core. + +LEDMatrix always draws into the canvas that is not on screen and swaps it in +(DisplayManager.update_display), so the write order cannot show as tearing. + +Against hzeller/rpi-rgb-led-matrix 1ee4f76. + +diff --git a/bindings/python/rgbmatrix/core.pyx b/bindings/python/rgbmatrix/core.pyx +index 230d87f..babc3bb 100644 +--- a/bindings/python/rgbmatrix/core.pyx ++++ b/bindings/python/rgbmatrix/core.pyx +@@ -2,6 +2,7 @@ + + from libcpp cimport bool + from libc.stdint cimport uint8_t, uint32_t, uintptr_t ++from libc.stdlib cimport malloc, free + import cython + + cdef extern from "Python.h": +@@ -59,8 +60,9 @@ cdef class Canvas: + + buffer = get_pillow_buffer(image_capsule) + +- for col in range(max(0, -xstart), min(width, frame_width - xstart)): +- for row in range(max(0, -ystart), min(height, frame_height - ystart)): ++ # Row-major: walks both the image and the bitplane buffer sequentially. ++ for row in range(max(0, -ystart), min(height, frame_height - ystart)): ++ for col in range(max(0, -xstart), min(width, frame_width - xstart)): + pixel = buffer[row][col] + r = (pixel ) & 0xFF + g = (pixel >> 8) & 0xFF +@@ -86,6 +88,41 @@ cdef class FrameCanvas(Canvas): + def SetPixel(self, int x, int y, uint8_t red, uint8_t green, uint8_t blue): + (self._getCanvas()).SetPixel(x, y, red, green, blue) + ++ @cython.boundscheck(False) ++ @cython.wraparound(False) ++ def SetPixelsPillow(self, int xstart, int ystart, int width, int height, object image_capsule): ++ # Same result as Canvas.SetPixelsPillow(), but hands each image row ++ # to the C++ bulk FrameCanvas::SetPixels() instead of calling the ++ # virtual SetPixel() once per pixel. ++ cdef cppinc.FrameCanvas* my_canvas = self._getCanvas() ++ cdef int col_start = max(0, -xstart) ++ cdef int col_end = min(width, my_canvas.width() - xstart) ++ cdef int row_start = max(0, -ystart) ++ cdef int row_end = min(height, my_canvas.height() - ystart) ++ cdef int row, col, pixel ++ cdef int *src ++ cdef cppinc.Color *line ++ cdef int **buffer ++ ++ if col_end <= col_start or row_end <= row_start: ++ return ++ buffer = get_pillow_buffer(image_capsule) ++ line = malloc((col_end - col_start) * sizeof(cppinc.Color)) ++ if line == NULL: ++ raise MemoryError() ++ try: ++ for row in range(row_start, row_end): ++ src = buffer[row] ++ for col in range(col_start, col_end): ++ pixel = src[col] ++ line[col - col_start].r = pixel & 0xFF ++ line[col - col_start].g = (pixel >> 8) & 0xFF ++ line[col - col_start].b = (pixel >> 16) & 0xFF ++ my_canvas.SetPixels(xstart + col_start, ystart + row, ++ col_end - col_start, 1, line) ++ finally: ++ free(line) ++ + + property width: + def __get__(self): return (self._getCanvas()).width() +diff --git a/bindings/python/rgbmatrix/cppinc.pxd b/bindings/python/rgbmatrix/cppinc.pxd +index 8bec241..314332d 100644 +--- a/bindings/python/rgbmatrix/cppinc.pxd ++++ b/bindings/python/rgbmatrix/cppinc.pxd +@@ -25,6 +25,7 @@ cdef extern from "led-matrix.h" namespace "rgb_matrix": + FrameCanvas *SwapOnVSync(FrameCanvas*, uint8_t) + + cdef cppclass FrameCanvas(Canvas): ++ void SetPixels(int, int, int, int, Color*) nogil + bool SetPWMBits(uint8_t) + uint8_t pwmbits() + void SetBrightness(uint8_t) +diff --git a/lib/framebuffer.cc b/lib/framebuffer.cc +index 36d138b..aee62ca 100644 +--- a/lib/framebuffer.cc ++++ b/lib/framebuffer.cc +@@ -807,11 +807,60 @@ void Framebuffer::SetPixel(int x, int y, uint8_t r, uint8_t g, uint8_t b) { + } + } + ++// Bulk version of SetPixel(); produces exactly the same bitplane content. ++// Faster because it hoists the per-pixel work out of the loop: the color ++// mapping becomes one 256-entry table built per call (each channel maps ++// independently through the same function), the pixel designators of a row ++// are contiguous in the PixelDesignatorMap, and the bit-plane loop is ++// branchless (the color bits are effectively random, so the branches in ++// SetPixel() mispredict a lot). + void Framebuffer::SetPixels(int x, int y, int width, int height, Color *colors) { +- for (int iy = 0; iy < height; ++iy) { +- for (int ix = 0; ix < width; ++ix) { +- SetPixel(x + ix, y + iy, colors->r, colors->g, colors->b); +- ++colors; ++ PixelDesignatorMap *const mapper = *shared_mapper_; ++ const int ix_start = std::max(0, -x); ++ const int ix_end = std::min(width, mapper->width() - x); ++ const int iy_start = std::max(0, -y); ++ const int iy_end = std::min(height, mapper->height() - y); ++ if (ix_start >= ix_end || iy_start >= iy_end) return; ++ ++ // Common case (luminance correction, no inversion): use the precomputed ++ // table directly; otherwise build one. Cheap enough to do per call, which ++ // matters for callers that send one row at a time. ++ uint16_t local_map[256]; ++ const uint16_t *color_map; ++ if (do_luminance_correct_ && !inverse_color_) { ++ color_map = ColorLookupTable::GetLookup(brightness_).color; ++ } else { ++ for (int c = 0; c < 256; ++c) { ++ uint16_t unused1, unused2; ++ MapColors(c, 0, 0, &local_map[c], &unused1, &unused2); ++ } ++ color_map = local_map; ++ } ++ ++ const int min_bit_plane = kBitPlanes - pwm_bits_; ++ gpio_bits_t *const plane_start = bitplane_buffer_ + columns_ * min_bit_plane; ++ for (int iy = iy_start; iy < iy_end; ++iy) { ++ const Color *c = colors + iy * width + ix_start; ++ const PixelDesignator *designator = mapper->get(x + ix_start, y + iy); ++ for (int ix = ix_start; ix < ix_end; ++ix, ++c, ++designator) { ++ const long pos = designator->gpio_word; ++ if (pos < 0) continue; // non-used pixel marker. ++ const uint16_t red = color_map[c->r]; ++ const uint16_t green = color_map[c->g]; ++ const uint16_t blue = color_map[c->b]; ++ const gpio_bits_t r_bits = designator->r_bit; ++ const gpio_bits_t g_bits = designator->g_bit; ++ const gpio_bits_t b_bits = designator->b_bit; ++ const gpio_bits_t designator_mask = designator->mask; ++ gpio_bits_t *bits = plane_start + pos; ++ for (int plane = min_bit_plane; plane < kBitPlanes; ++plane) { ++ const gpio_bits_t color_bits = ++ (r_bits & -(gpio_bits_t)((red >> plane) & 1)) ++ | (g_bits & -(gpio_bits_t)((green >> plane) & 1)) ++ | (b_bits & -(gpio_bits_t)((blue >> plane) & 1)); ++ *bits = (*bits & designator_mask) | color_bits; ++ bits += columns_; ++ } + } + } + } diff --git a/test/test_install_rgb_checkout.py b/test/test_install_rgb_checkout.py index 274f186f..699baced 100644 --- a/test/test_install_rgb_checkout.py +++ b/test/test_install_rgb_checkout.py @@ -236,3 +236,99 @@ class TestOneShotContract: assert "--recurse-submodules" not in ONE_SHOT.read_text(encoding="utf-8") installer = INSTALLER.read_text(encoding="utf-8") assert re.search(rf"submodule update --init --recursive {SUB}", installer) + + +PATCH_DIR = ROOT / "patches" / "rpi-rgb-led-matrix" + + +def _write_patch(project: Path, name: str, old: str, new: str) -> Path: + """A git patch rewriting the fake library's Makefile from `old` to `new`.""" + patch = project / "patches" / "rpi-rgb-led-matrix" / name + patch.parent.mkdir(parents=True, exist_ok=True) + patch.write_text( + "A note before the diff, as the shipped patches carry.\n\n" + "diff --git a/Makefile b/Makefile\n" + "--- a/Makefile\n" + "+++ b/Makefile\n" + "@@ -1 +1 @@\n" + f"-{old}\n" + "\\ No newline at end of file\n" + f"+{new}\n" + "\\ No newline at end of file\n", + encoding="utf-8", + ) + return patch + + +def _makefile(project: Path) -> str: + return (project / SUB / "Makefile").read_text(encoding="utf-8") + + +def _status(project: Path, env: dict) -> str: + return git("status", "--porcelain", cwd=project / SUB, env=env) + + +class TestLibraryPatches: + """patches/rpi-rgb-led-matrix/*.patch go in for the build and come back out.""" + + def test_applied_for_the_build_and_reverted_after(self, make_project, git_env): + project = make_project("B") + _write_patch(project, "0001-x.patch", "A", "patched") + result = run_helpers( + f'_apply_rgb_patches; cat "{project / SUB / "Makefile"}"; echo; _revert_rgb_patches', + project, git_env) + assert result.returncode == 0, result.stderr + assert "Applied library patch 0001-x.patch" in result.stdout + assert "patched" in result.stdout # what the build would see + assert _makefile(project) == "A" # and the checkout afterwards + assert _status(project, git_env) == "" + + def test_already_applied_is_left_alone(self, make_project, git_env): + project = make_project("B") + _write_patch(project, "0001-x.patch", "A", "patched") + (project / SUB / "Makefile").write_text("patched", encoding="utf-8") + result = run_helpers("_apply_rgb_patches; _revert_rgb_patches", project, git_env) + assert result.returncode == 0, result.stderr + assert "already applied" in result.stdout + assert _makefile(project) == "patched" # not reverted: it was not ours + + def test_a_patch_that_does_not_apply_is_skipped_not_fatal(self, make_project, git_env): + project = make_project("B") + _write_patch(project, "0001-x.patch", "something else", "patched") + result = run_helpers("_apply_rgb_patches; _revert_rgb_patches; echo REACHED", project, git_env) + assert result.returncode == 0, result.stderr + assert "ERR_TRAP_FIRED" not in result.stderr + assert "does not apply" in result.stdout and "REACHED" in result.stdout + assert _makefile(project) == "A" + + def test_no_patch_directory_is_a_no_op(self, make_project, git_env): + project = make_project("B") + result = run_helpers("_apply_rgb_patches; _revert_rgb_patches; echo REACHED", project, git_env) + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == "REACHED" + + def test_revert_twice_is_harmless(self, make_project, git_env): + # The EXIT trap runs _revert_rgb_patches again after Step 6 already has. + project = make_project("B") + _write_patch(project, "0001-x.patch", "A", "patched") + result = run_helpers("_apply_rgb_patches; _revert_rgb_patches; _revert_rgb_patches", project, git_env) + assert result.returncode == 0, result.stderr + assert _makefile(project) == "A" and "Could not revert" not in result.stdout + + def test_build_is_wrapped_and_the_exit_trap_reverts(self): + installer = INSTALLER.read_text(encoding="utf-8") + apply_at = installer.index("_apply_rgb_patches\n if run_rgbmatrix_build") + assert installer.index("_revert_rgb_patches\n cat \"$BUILD_OUTPUT\"") > apply_at + assert re.search(r"^trap '[^']*_revert_rgb_patches[^']*' EXIT", installer, re.M) + + def test_shipped_patches_are_git_patches_against_the_library(self, tmp_path, git_env): + patches = sorted(PATCH_DIR.glob("*.patch")) + assert patches, "no shipped library patches" + repo = tmp_path / "r" + repo.mkdir() + git("init", "-q", ".", cwd=repo, env=git_env) + for patch in patches: + stat = git("apply", "--numstat", str(patch), cwd=repo, env=git_env) + files = {line.split("\t")[2] for line in stat.splitlines()} + assert files <= {"lib/framebuffer.cc", "bindings/python/rgbmatrix/core.pyx", + "bindings/python/rgbmatrix/cppinc.pxd"}, files