[display] Walk the blit fallback in frame buffer order and resolve the bitness once (#20651)

esphome/components/display/display.cpp

@@ -5,6 +5,7 @@
 #include "display_color_utils.h"
 #include "esphome/core/application.h"
 #include "esphome/core/hal.h"
+#include "esphome/core/helpers.h"
 #include "esphome/core/log.h"
 
 namespace esphome::display {
@@ -52,40 +53,65 @@ void Display::line_at_angle(int x, int y, int angle, int start_radius, int stop_
   this->line(x1, y1, x2, y2, color);
 }
 
-void Display::draw_pixels_at(int x_start, int y_start, int w, int h, const uint8_t *ptr, ColorOrder order,
-                             ColorBitness bitness, bool big_endian, int x_offset, int y_offset, int x_pad) {
-  size_t line_stride = x_offset + w + x_pad;  // length of each source line in pixels
-  uint32_t color_value;
+// flatten keeps to_color() inlined in the pixel loop; in PSRAM builds GCC at
+// -Os stops inlining it once there are two call sites, which costs a call and
+// spills per pixel. Without PSRAM there is one call site and it changes
+// nothing, and draw_pixel_at() stays a virtual call either way.
+template<ColorBitness BITNESS>
+static void __attribute__((flatten))
+draw_packed_pixels(Display *display, int x_start, int y_start, int w, int h, const uint8_t *ptr, ColorOrder order,
+                   bool big_endian, int x_offset, int y_offset, int x_pad) {
+  const size_t line_stride = x_offset + w + x_pad;  // length of each source line in pixels
+  // BITNESS is a constant here, so to_color()'s three per pixel divisions
+  // become a multiply and shift and its bitness switch folds.
+  const auto draw_source_pixel = [&](int x, int y, size_t source_idx) ESPHOME_ALWAYS_INLINE {
+    display->draw_pixel_at(
+        x + x_start, y + y_start,
+        ColorUtil::to_color(ColorUtil::read_packed<BITNESS>(ptr, source_idx, big_endian), order, BITNESS));
+  };
+  // The source and frame buffers are row-major, so walking rows keeps the read
+  // and the write sequential. When the display swaps the axes before writing,
+  // walking columns keeps the write sequential instead; with the frame buffer
+  // in PSRAM a strided write costs more than a strided read.
+#ifdef USE_PSRAM
+  if (display->pixel_axes_swapped()) {
+    size_t column_idx = y_offset * line_stride + x_offset;
+    for (int x = 0; x != w; x++, column_idx++) {
+      size_t source_idx = column_idx;
+      for (int y = 0; y != h; y++, source_idx += line_stride) {
+        draw_source_pixel(x, y, source_idx);
+      }
+    }
+    return;
+  }
+#endif
+  // Row walk: builds without PSRAM, and PSRAM builds whose axes are not swapped.
   for (int y = 0; y != h; y++) {
     size_t source_idx = (y_offset + y) * line_stride + x_offset;
-    size_t source_idx_mod;
     for (int x = 0; x != w; x++, source_idx++) {
-      switch (bitness) {
-        default:
-          color_value = ptr[source_idx];
-          break;
-        case COLOR_BITNESS_565:
-          source_idx_mod = source_idx * 2;
-          if (big_endian) {
-            color_value = (ptr[source_idx_mod] << 8) + ptr[source_idx_mod + 1];
-          } else {
-            color_value = ptr[source_idx_mod] + (ptr[source_idx_mod + 1] << 8);
-          }
-          break;
-        case COLOR_BITNESS_888:
-          source_idx_mod = source_idx * 3;
-          if (big_endian) {
-            color_value = (ptr[source_idx_mod + 0] << 16) + (ptr[source_idx_mod + 1] << 8) + ptr[source_idx_mod + 2];
-          } else {
-            color_value = ptr[source_idx_mod + 0] + (ptr[source_idx_mod + 1] << 8) + (ptr[source_idx_mod + 2] << 16);
-          }
-          break;
-      }
-      this->draw_pixel_at(x + x_start, y + y_start, ColorUtil::to_color(color_value, order, bitness));
+      draw_source_pixel(x, y, source_idx);
     }
   }
 }
 
+void Display::draw_pixels_at(int x_start, int y_start, int w, int h, const uint8_t *ptr, ColorOrder order,
+                             ColorBitness bitness, bool big_endian, int x_offset, int y_offset, int x_pad) {
+  switch (bitness) {
+    case COLOR_BITNESS_565:
+      draw_packed_pixels<COLOR_BITNESS_565>(this, x_start, y_start, w, h, ptr, order, big_endian, x_offset, y_offset,
+                                            x_pad);
+      break;
+    case COLOR_BITNESS_888:
+      draw_packed_pixels<COLOR_BITNESS_888>(this, x_start, y_start, w, h, ptr, order, big_endian, x_offset, y_offset,
+                                            x_pad);
+      break;
+    default:
+      draw_packed_pixels<COLOR_BITNESS_332>(this, x_start, y_start, w, h, ptr, order, big_endian, x_offset, y_offset,
+                                            x_pad);
+      break;
+  }
+}
+
 void HOT Display::horizontal_line(int x, int y, int width, Color color) {
   // Future: Could be made more efficient by manipulating buffer directly in certain rotations.
   for (int i = x; i < x + width; i++)

esphome/components/display/display.h

@@ -139,6 +139,11 @@ enum DisplayRotation {
   DISPLAY_ROTATION_270_DEGREES = 270,
 };
 
+/// Whether a rotation swaps the x and y axes.
+constexpr bool rotation_swaps_axes(DisplayRotation rotation) {
+  return rotation == DISPLAY_ROTATION_90_DEGREES || rotation == DISPLAY_ROTATION_270_DEGREES;
+}
+
 const int EDGES_TRIGON = 3;
 const int EDGES_TRIANGLE = 3;
 const int EDGES_TETRAGON = 4;
@@ -717,6 +722,12 @@ class Display : public PollingComponent {
 
   virtual DisplayRotation get_rotation() const { return this->rotation_; }
 
+  /// True when draw_pixel_at() swaps x and y before writing, so pixels that are
+  /// neighbours along x land a row apart in the frame buffer. Not the same as
+  /// get_width()/get_height() swapping: a display that rotates in hardware
+  /// reports swapped dimensions but writes unswapped pixels.
+  virtual bool pixel_axes_swapped() const { return rotation_swaps_axes(this->rotation_); }
+
   /** Get the type of display that the buffer corresponds to. In case of dynamically configurable displays,
    * returns the type the display is currently configured to.
    */

esphome/components/display/display_color_utils.h

@@ -2,6 +2,7 @@
 #include <array>
 #include <cstdint>
 #include "esphome/core/color.h"
+#include "esphome/core/helpers.h"
 
 namespace esphome::display {
 enum ColorOrder : uint8_t { COLOR_ORDER_RGB = 0, COLOR_ORDER_BGR = 1, COLOR_ORDER_GRB = 2 };
@@ -10,6 +11,27 @@ inline static uint8_t esp_scale(uint8_t i, uint8_t scale, uint8_t max_value = 25
 
 class ColorUtil {
  public:
+  /// Reads one packed pixel of the given bitness from a source buffer into the
+  /// value to_color() takes. BITNESS is a template argument so the byte layout
+  /// folds; pass it on to to_color() as a constant too, and its channel scaling
+  /// divides by a constant, which the compiler turns into a multiply and shift.
+  template<ColorBitness BITNESS>
+  static ESPHOME_ALWAYS_INLINE uint32_t read_packed(const uint8_t *ptr, size_t index, bool big_endian) {
+    if constexpr (BITNESS == COLOR_BITNESS_565) {
+      const size_t idx = index * 2;
+      if (big_endian)
+        return (ptr[idx] << 8) + ptr[idx + 1];
+      return ptr[idx] + (ptr[idx + 1] << 8);
+    } else if constexpr (BITNESS == COLOR_BITNESS_888) {
+      const size_t idx = index * 3;
+      if (big_endian)
+        return (ptr[idx + 0] << 16) + (ptr[idx + 1] << 8) + ptr[idx + 2];
+      return ptr[idx + 0] + (ptr[idx + 1] << 8) + (ptr[idx + 2] << 16);
+    } else {
+      return ptr[index];
+    }
+  }
+
   static Color to_color(uint32_t colorcode, ColorOrder color_order,
                         ColorBitness color_bitness = ColorBitness::COLOR_BITNESS_888, bool right_bit_aligned = true) {
     uint8_t first_color, second_color, third_color;

esphome/components/epaper_spi/epaper_spi.h

@@ -106,6 +106,7 @@ class EPaperBase : public Display,
 
   int get_width() override { return this->effective_transform_ & SWAP_XY ? this->height_ : this->width_; }
   int get_height() override { return this->effective_transform_ & SWAP_XY ? this->width_ : this->height_; }
+  bool pixel_axes_swapped() const override { return this->effective_transform_ & SWAP_XY; }
   void draw_pixel_at(int x, int y, Color color) override;
   void draw_pixels_at(int x_start, int y_start, int w, int h, const uint8_t *ptr, ColorOrder order,
                       ColorBitness bitness, bool big_endian, int x_offset, int y_offset, int x_pad) override;

esphome/components/it8951/it8951.h

@@ -204,6 +204,7 @@ class IT8951Display : public Display,
                       ColorBitness bitness, bool big_endian, int x_offset, int y_offset, int x_pad) override;
   int get_width() override { return (this->effective_transform_ & TRANSFORM_SWAP_XY) ? this->height_ : this->width_; }
   int get_height() override { return (this->effective_transform_ & TRANSFORM_SWAP_XY) ? this->width_ : this->height_; }
+  bool pixel_axes_swapped() const override { return this->effective_transform_ & TRANSFORM_SWAP_XY; }
 
  protected:
   int get_height_internal() override { return this->height_; }

esphome/components/mipi_spi/mipi_spi.h

@@ -130,19 +130,8 @@ class MipiSpi : public display::Display,
   }
   display::DisplayType get_display_type() override { return display::DisplayType::DISPLAY_TYPE_COLOR; }
 
-  int get_width() override {
-    if (this->rotation_ == display::DISPLAY_ROTATION_90_DEGREES ||
-        this->rotation_ == display::DISPLAY_ROTATION_270_DEGREES)
-      return HEIGHT;
-    return WIDTH;
-  }
-
-  int get_height() override {
-    if (this->rotation_ == display::DISPLAY_ROTATION_90_DEGREES ||
-        this->rotation_ == display::DISPLAY_ROTATION_270_DEGREES)
-      return WIDTH;
-    return HEIGHT;
-  }
+  int get_width() override { return display::rotation_swaps_axes(this->rotation_) ? HEIGHT : WIDTH; }
+  int get_height() override { return display::rotation_swaps_axes(this->rotation_) ? WIDTH : HEIGHT; }
 
   void set_init_sequence(const uint8_t *sequence, size_t len) {
     this->init_sequence_ = sequence;
@@ -621,6 +610,9 @@ class MipiSpiBuffer
 #endif
   }
 
+  // With hardware rotation the buffer is written in drawn coordinates.
+  bool pixel_axes_swapped() const override { return !HAS_HARDWARE_ROTATION && display::Display::pixel_axes_swapped(); }
+
   // Draw a pixel at the given coordinates.
   void draw_pixel_at(int x, int y, Color color) override {
     if (this->is_point_clipped(x, y))

tests/components/display/__init__.py

@@ -0,0 +1,20 @@
+from pathlib import Path
+
+import esphome.codegen as cg
+from esphome.const import CONF_ESPHOME, CONF_INCLUDES
+from esphome.core import CORE
+from esphome.types import ConfigType
+from tests.testing_helpers import ComponentManifestOverride
+
+TESTS_DIR = Path(__file__).parent
+
+
+def override_manifest(manifest: ComponentManifestOverride) -> None:
+    # The column walk in Display::draw_pixels_at() is only built with PSRAM; define it
+    # when display's own tests are built so both walk orders run, and leave other
+    # components' test binaries on the plain build.
+    async def to_code_testing(config: ConfigType) -> None:
+        if any(Path(p) == TESTS_DIR for p in CORE.config[CONF_ESPHOME][CONF_INCLUDES]):
+            cg.add_define("USE_PSRAM")
+
+    manifest.to_code = to_code_testing

tests/components/display/test_draw_pixels_at.cpp

@@ -0,0 +1,109 @@
+#include <gtest/gtest.h>
+
+#include <algorithm>
+#include <initializer_list>
+#include <tuple>
+#include <vector>
+
+#include "esphome/components/display/display.h"
+
+namespace esphome::display::testing {
+
+// Records every pixel write; pixel_axes_swapped() picks the walk order.
+class RecordingDisplay : public Display {
+ public:
+  void set_axes_swapped(bool swapped) { this->axes_swapped_ = swapped; }
+  bool pixel_axes_swapped() const override { return this->axes_swapped_; }
+  void draw_pixel_at(int x, int y, Color color) override { this->pixels_.emplace_back(x, y, color.raw_32); }
+  void update() override {}
+  DisplayType get_display_type() override { return DisplayType::DISPLAY_TYPE_COLOR; }
+  int get_width_internal() override { return 64; }
+  int get_height_internal() override { return 64; }
+
+  std::vector<std::tuple<int, int, uint32_t>> sorted_pixels() const {
+    auto sorted = this->pixels_;
+    std::sort(sorted.begin(), sorted.end());
+    return sorted;
+  }
+  size_t count() const { return this->pixels_.size(); }
+  const std::vector<std::tuple<int, int, uint32_t>> &pixels() const { return this->pixels_; }
+
+ protected:
+  bool axes_swapped_{false};
+  std::vector<std::tuple<int, int, uint32_t>> pixels_;
+};
+
+// A 7x5 region with a 2 pixel left offset, a 3 line top offset and 4 pixels of
+// right padding, so the source index arithmetic of both walks is exercised.
+static constexpr int W = 7, H = 5, X_OFFSET = 2, Y_OFFSET = 3, X_PAD = 4;
+static constexpr int LINE_STRIDE = X_OFFSET + W + X_PAD;
+static constexpr int LINES = Y_OFFSET + H;
+
+static std::vector<uint8_t> make_source(int bytes_per_pixel) {
+  std::vector<uint8_t> source(LINE_STRIDE * LINES * bytes_per_pixel);
+  uint8_t value = 1;
+  for (auto &byte : source)
+    byte = value++;
+  return source;
+}
+
+static void expect_walks_match(ColorBitness bitness, int bytes_per_pixel, bool big_endian) {
+  const auto source = make_source(bytes_per_pixel);
+  RecordingDisplay rows;
+  RecordingDisplay columns;
+  columns.set_axes_swapped(true);
+  rows.draw_pixels_at(10, 20, W, H, source.data(), COLOR_ORDER_RGB, bitness, big_endian, X_OFFSET, Y_OFFSET, X_PAD);
+  columns.draw_pixels_at(10, 20, W, H, source.data(), COLOR_ORDER_RGB, bitness, big_endian, X_OFFSET, Y_OFFSET, X_PAD);
+  EXPECT_EQ(rows.count(), static_cast<size_t>(W * H));
+  EXPECT_EQ(columns.count(), static_cast<size_t>(W * H));
+  EXPECT_EQ(rows.sorted_pixels(), columns.sorted_pixels());
+}
+
+TEST(DrawPixelsAt, ColumnWalkMatchesRowWalk332) { expect_walks_match(COLOR_BITNESS_332, 1, true); }
+TEST(DrawPixelsAt, ColumnWalkMatchesRowWalk565BigEndian) { expect_walks_match(COLOR_BITNESS_565, 2, true); }
+TEST(DrawPixelsAt, ColumnWalkMatchesRowWalk565LittleEndian) { expect_walks_match(COLOR_BITNESS_565, 2, false); }
+TEST(DrawPixelsAt, ColumnWalkMatchesRowWalk888BigEndian) { expect_walks_match(COLOR_BITNESS_888, 3, true); }
+TEST(DrawPixelsAt, ColumnWalkMatchesRowWalk888LittleEndian) { expect_walks_match(COLOR_BITNESS_888, 3, false); }
+
+// One known pixel per bitness and byte order, so a decode that swapped bytes or channels
+// could not pass the walk comparison above by being wrong the same way twice.
+static void expect_golden(ColorBitness bitness, std::initializer_list<uint8_t> bytes, bool big_endian, Color expected) {
+  const std::vector<uint8_t> source(bytes);
+  RecordingDisplay display;
+  display.draw_pixels_at(3, 4, 1, 1, source.data(), COLOR_ORDER_RGB, bitness, big_endian, 0, 0, 0);
+  ASSERT_EQ(display.count(), 1u);
+  EXPECT_EQ(display.pixels()[0], std::make_tuple(3, 4, expected.raw_32));
+}
+
+TEST(DrawPixelsAt, Golden565BigEndian) {
+  expect_golden(COLOR_BITNESS_565, {0xE0, 0x07}, true, Color(230, 0, 57));  // 0xE007: r 28, g 0, b 7
+}
+TEST(DrawPixelsAt, Golden565LittleEndian) {
+  expect_golden(COLOR_BITNESS_565, {0xE0, 0x07}, false, Color(0, 255, 0));  // 0x07E0: pure green
+}
+TEST(DrawPixelsAt, Golden888BigEndian) {
+  expect_golden(COLOR_BITNESS_888, {0x10, 0x20, 0x30}, true, Color(0x10, 0x20, 0x30));
+}
+TEST(DrawPixelsAt, Golden888LittleEndian) {
+  expect_golden(COLOR_BITNESS_888, {0x10, 0x20, 0x30}, false, Color(0x30, 0x20, 0x10));
+}
+TEST(DrawPixelsAt, Golden332) {
+  expect_golden(COLOR_BITNESS_332, {0xE3}, true, Color(255, 0, 255));  // r 7 of 7, g 0, b 3 of 3
+}
+
+// The column walk really is a different order, otherwise the tests above prove nothing.
+TEST(DrawPixelsAt, ColumnWalkChangesOrder) {
+  const auto source = make_source(1);
+  RecordingDisplay rows;
+  RecordingDisplay columns;
+  columns.set_axes_swapped(true);
+  rows.draw_pixels_at(0, 0, W, H, source.data(), COLOR_ORDER_RGB, COLOR_BITNESS_332, true, X_OFFSET, Y_OFFSET, X_PAD);
+  columns.draw_pixels_at(0, 0, W, H, source.data(), COLOR_ORDER_RGB, COLOR_BITNESS_332, true, X_OFFSET, Y_OFFSET,
+                         X_PAD);
+  ASSERT_GT(rows.count(), 1u);
+  ASSERT_GT(columns.count(), 1u);
+  EXPECT_EQ(std::get<0>(rows.pixels()[1]), 1);     // the row walk steps x first
+  EXPECT_EQ(std::get<1>(columns.pixels()[1]), 1);  // the column walk steps y first
+}
+
+}  // namespace esphome::display::testing