perf(install): build rpi-rgb-led-matrix with a faster SetImage (3x cheaper frame copy)

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,

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"

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):
+         (<cppinc.FrameCanvas*>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 = <cppinc.FrameCanvas*>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 = <cppinc.Color*>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 (<cppinc.FrameCanvas*>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_;
++      }
+     }
+   }
+ }

test/test_install_rgb_checkout.py

@@ -236,3 +236,99 @@ def test_installer_initialises_the_submodule_itself(self):
         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