diff --git a/CHANGELOG.md b/CHANGELOG.md index fb660b04c..f7ce8eb4d 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 a149fd8c2..65cd66fc8 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 000000000..c378c2b95 --- /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 274f186f5..699bacedf 100644 --- a/test/test_install_rgb_checkout.py +++ b/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