[image] Check the rotated benchmark frame against the plain one

The benchmark now draws once at each rotation and requires the rotated
frame to be the transpose of the plain one before timing, so a wrong
traversal fails the run instead of passing silently.

esphome/components/display/display.h

@@ -137,6 +137,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;
@@ -710,6 +715,12 @@ class Display : public PollingComponent {
 
   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/epaper_spi/epaper_spi.h

@@ -113,6 +113,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;
 
  protected:

esphome/components/image/image.cpp

@@ -5,81 +5,107 @@
 
 namespace esphome::image {
 
+// Walks the clipped image range with rows (ROWS_OUTER) or columns as the outer
+// loop. The coordinate selection folds at compile time, and the pixel body must
+// be ESPHOME_ALWAYS_INLINE: at -Os GCC otherwise leaves it out of line and calls
+// it once per pixel.
+template<bool ROWS_OUTER, typename F>
+static void for_each_pixel(int x_start, int x_end, int y_start, int y_end, F &&f) {
+  const int outer_start = ROWS_OUTER ? y_start : x_start;
+  const int outer_end = ROWS_OUTER ? y_end : x_end;
+  const int inner_start = ROWS_OUTER ? x_start : y_start;
+  const int inner_end = ROWS_OUTER ? x_end : y_end;
+  for (int outer = outer_start; outer < outer_end; outer++) {
+    for (int inner = inner_start; inner < inner_end; inner++) {
+      f(ROWS_OUTER ? inner : outer, ROWS_OUTER ? outer : inner);
+    }
+  }
+}
+
 void Image::draw(int x, int y, display::Display *display, Color color_on, Color color_off) {
-  int img_x0 = 0;
-  int img_y0 = 0;
-  int w = width_;
-  int h = height_;
+  int x_start = 0;
+  int y_start = 0;
+  int x_end = width_;
+  int y_end = height_;
 
   auto clipping = display->get_clipping();
   if (clipping.is_set()) {
     if (clipping.x > x)
-      img_x0 += clipping.x - x;
+      x_start += clipping.x - x;
     if (clipping.y > y)
-      img_y0 += clipping.y - y;
-    if (w > clipping.x2() - x)
-      w = clipping.x2() - x;
-    if (h > clipping.y2() - y)
-      h = clipping.y2() - y;
+      y_start += clipping.y - y;
+    if (x_end > clipping.x2() - x)
+      x_end = clipping.x2() - x;
+    if (y_end > clipping.y2() - y)
+      y_end = clipping.y2() - y;
   }
 
-  switch (type_) {
-    case IMAGE_TYPE_BINARY: {
-      for (int img_x = img_x0; img_x < w; img_x++) {
-        for (int img_y = img_y0; img_y < h; img_y++) {
-          if (this->get_binary_pixel_(img_x, img_y)) {
-            display->draw_pixel_at(x + img_x, y + img_y, color_on);
-          } else if (!this->transparency_) {
-            display->draw_pixel_at(x + img_x, y + img_y, color_off);
-          }
+  // Pixel data and frame buffers are row-major, so walking rows keeps the
+  // image read and the frame buffer write sequential. When the display swaps
+  // the axes before writing, walking columns keeps the write sequential
+  // instead; a strided write costs more than a strided read.
+#ifdef USE_PSRAM
+  if (display->pixel_axes_swapped()) {
+    this->draw_<false>(x, y, display, color_on, color_off, x_start, x_end, y_start, y_end);
+    return;
+  }
+#endif
+  // Without PSRAM the frame buffer is in uncached RAM, where the strided write
+  // is free, so one copy of the loops is enough.
+  this->draw_<true>(x, y, display, color_on, color_off, x_start, x_end, y_start, y_end);
+}
+
+template<bool ROWS_OUTER>
+void Image::draw_(int x, int y, display::Display *display, Color color_on, Color color_off, int x_start, int x_end,
+                  int y_start, int y_end) {
+  // Local copies: draw_pixel_at() is an opaque virtual call, so members read
+  // in the loop would otherwise be reloaded every pixel.
+  const Transparency transparency = this->transparency_;
+  switch (this->type_) {
+    case IMAGE_TYPE_BINARY:
+      for_each_pixel<ROWS_OUTER>(x_start, x_end, y_start, y_end, [&](int img_x, int img_y) ESPHOME_ALWAYS_INLINE {
+        if (this->get_binary_pixel_(img_x, img_y)) {
+          display->draw_pixel_at(x + img_x, y + img_y, color_on);
+        } else if (transparency == TRANSPARENCY_OPAQUE) {
+          display->draw_pixel_at(x + img_x, y + img_y, color_off);
         }
-      }
+      });
+      break;
+    case IMAGE_TYPE_GRAYSCALE: {
+      const int width = this->width_;
+      const uint8_t *const data = this->data_start_;
+      for_each_pixel<ROWS_OUTER>(x_start, x_end, y_start, y_end, [&](int img_x, int img_y) ESPHOME_ALWAYS_INLINE {
+        const uint8_t gray = progmem_read_byte(data + img_x + img_y * width);
+        Color color = Color(gray, gray, gray, 0xFF);
+        if (transparency == TRANSPARENCY_CHROMA_KEY) {
+          if (gray == 1) {
+            return;  // transparent pixel, skip drawing
+          }
+        } else if (transparency == TRANSPARENCY_ALPHA_CHANNEL) {
+          // gray is the alpha: blend from color_off to color_on, drawn opaque
+          color = Color(Color::blend_channel(color_off.r, color_on.r, gray),
+                        Color::blend_channel(color_off.g, color_on.g, gray),
+                        Color::blend_channel(color_off.b, color_on.b, gray), 0xFF);
+        }
+        display->draw_pixel_at(x + img_x, y + img_y, color);
+      });
       break;
     }
-    case IMAGE_TYPE_GRAYSCALE:
-      for (int img_x = img_x0; img_x < w; img_x++) {
-        for (int img_y = img_y0; img_y < h; img_y++) {
-          const uint32_t pos = (img_x + img_y * this->width_);
-          const uint8_t gray = progmem_read_byte(this->data_start_ + pos);
-          Color color = Color(gray, gray, gray, 0xFF);
-          switch (this->transparency_) {
-            case TRANSPARENCY_CHROMA_KEY:
-              if (gray == 1) {
-                continue;  // skip drawing
-              }
-              break;
-            case TRANSPARENCY_ALPHA_CHANNEL:
-              // gray is the alpha: blend from color_off to color_on, drawn opaque
-              color = Color(Color::blend_channel(color_off.r, color_on.r, gray),
-                            Color::blend_channel(color_off.g, color_on.g, gray),
-                            Color::blend_channel(color_off.b, color_on.b, gray), 0xFF);
-              break;
-            default:
-              break;
-          }
+    case IMAGE_TYPE_RGB565:
+      for_each_pixel<ROWS_OUTER>(x_start, x_end, y_start, y_end, [&](int img_x, int img_y) ESPHOME_ALWAYS_INLINE {
+        auto color = this->get_rgb565_pixel_(img_x, img_y);
+        if (color.w >= 0x80) {
           display->draw_pixel_at(x + img_x, y + img_y, color);
         }
-      }
-      break;
-    case IMAGE_TYPE_RGB565:
-      for (int img_x = img_x0; img_x < w; img_x++) {
-        for (int img_y = img_y0; img_y < h; img_y++) {
-          auto color = this->get_rgb565_pixel_(img_x, img_y);
-          if (color.w >= 0x80) {
-            display->draw_pixel_at(x + img_x, y + img_y, color);
-          }
-        }
-      }
+      });
       break;
     case IMAGE_TYPE_RGB:
-      for (int img_x = img_x0; img_x < w; img_x++) {
-        for (int img_y = img_y0; img_y < h; img_y++) {
-          auto color = this->get_rgb_pixel_(img_x, img_y);
-          if (color.w >= 0x80) {
-            display->draw_pixel_at(x + img_x, y + img_y, color);
-          }
+      for_each_pixel<ROWS_OUTER>(x_start, x_end, y_start, y_end, [&](int img_x, int img_y) ESPHOME_ALWAYS_INLINE {
+        auto color = this->get_rgb_pixel_(img_x, img_y);
+        if (color.w >= 0x80) {
+          display->draw_pixel_at(x + img_x, y + img_y, color);
         }
-      }
+      });
       break;
   }
 }

esphome/components/image/image.h

@@ -43,6 +43,10 @@ class Image : public display::BaseImage {
   lv_image_dsc_t *get_lv_image_dsc();
 #endif
  protected:
+  /// Per-pixel draw of the clipped range, rows (true) or columns (false) outer.
+  template<bool ROWS_OUTER>
+  void draw_(int x, int y, display::Display *display, Color color_on, Color color_off, int x_start, int x_end,
+             int y_start, int y_end);
   bool get_binary_pixel_(int x, int y) const;
   Color get_rgb_pixel_(int x, int y) const;
   Color get_rgb565_pixel_(int x, int y) const;

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

@@ -119,19 +119,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 std::vector<uint8_t> &sequence) { this->init_sequence_ = sequence; }
 
@@ -602,6 +591,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/benchmarks/components/image/__init__.py

@@ -0,0 +1,17 @@
+import esphome.codegen as cg
+from tests.testing_helpers import ComponentManifestOverride
+
+
+def override_manifest(manifest: ComponentManifestOverride) -> None:
+    # The column walk in Image::draw() is only built with PSRAM; define it so
+    # the rotated benchmark exercises that loop order on the host.
+    manifest.enable_codegen()
+    original_to_code = manifest.to_code
+
+    async def to_code(config):
+        await original_to_code(config)
+        cg.add_define("USE_PSRAM")
+
+    if hasattr(original_to_code, "priority"):
+        to_code.priority = original_to_code.priority
+    manifest.to_code = to_code

tests/benchmarks/components/image/bench_image_draw.cpp

@@ -0,0 +1,124 @@
+#include <benchmark/benchmark.h>
+
+#include <cstdint>
+#include <memory>
+
+#include "esphome/components/display/display_buffer.h"
+#include "esphome/components/image/image.h"
+
+namespace esphome::image::benchmarks {
+
+static constexpr int kWidth = 128;
+static constexpr int kHeight = 96;
+// Square frame so a rotated draw of the image stays inside it.
+static constexpr int kStride = kWidth > kHeight ? kWidth : kHeight;
+
+// One byte per pixel frame buffer with no hardware behind it, so the
+// measurement is Image::draw() plus the DisplayBuffer pixel path every
+// buffered display shares. Everything fits the host cache, so this counts
+// the per pixel work; the memory locality win only shows on hardware.
+class BenchDisplay : public display::DisplayBuffer {
+ public:
+  BenchDisplay() : frame_(std::make_unique<uint8_t[]>(kStride * kStride)) {}
+  void update() override {}
+  display::DisplayType get_display_type() override { return display::DisplayType::DISPLAY_TYPE_COLOR; }
+  int get_width_internal() override { return kStride; }
+  int get_height_internal() override { return kStride; }
+
+ protected:
+  void draw_absolute_pixel_internal(int x, int y, Color color) override { this->frame_[y * kStride + x] = color.r; }
+
+  std::unique_ptr<uint8_t[]> frame_;
+
+ public:
+  uint8_t pixel(int x, int y) const { return this->frame_[y * kStride + x]; }
+};
+
+// Draws the image at rotation 0 and 90 and checks the second frame is the
+// transpose of the first, which is what a wrong traversal order would break.
+static bool frames_match(const Image &image) {
+  BenchDisplay plain;
+  BenchDisplay rotated;
+  rotated.set_rotation(display::DISPLAY_ROTATION_90_DEGREES);
+  const_cast<Image &>(image).draw(0, 0, &plain, display::COLOR_ON, display::COLOR_OFF);
+  const_cast<Image &>(image).draw(0, 0, &rotated, display::COLOR_ON, display::COLOR_OFF);
+  for (int y = 0; y < kHeight; y++) {
+    for (int x = 0; x < kWidth; x++) {
+      // DisplayBuffer maps (x, y) to (width - y - 1, x) at 90 degrees
+      if (plain.pixel(x, y) != rotated.pixel(kStride - y - 1, x))
+        return false;
+    }
+  }
+  return true;
+}
+
+// Deterministic pseudo random pixel bytes so the alpha branch sees a mix of
+// pixels. Sized for the widest format, the image reads only what it needs.
+static std::unique_ptr<uint8_t[]> make_pixels() {
+  constexpr size_t bytes = kWidth * kHeight * 4;
+  auto data = std::make_unique<uint8_t[]>(bytes);
+  uint32_t seed = 11897;
+  for (size_t i = 0; i < bytes; i++) {
+    seed = seed * 1103515245u + 12345u;
+    data[i] = static_cast<uint8_t>(seed >> 16);
+  }
+  return data;
+}
+
+static void draw_image(benchmark::State &state, ImageType type, Transparency transparency,
+                       display::DisplayRotation rotation = display::DISPLAY_ROTATION_0_DEGREES) {
+  auto data = make_pixels();
+  Image image(data.get(), kWidth, kHeight, type, transparency);
+  BenchDisplay display;
+  display.set_rotation(rotation);
+  if (!frames_match(image)) {
+    state.SkipWithError("rotated draw is not the transpose of the plain draw");
+    return;
+  }
+
+  for (auto _ : state) {
+    image.draw(0, 0, &display, display::COLOR_ON, display::COLOR_OFF);
+  }
+  state.SetItemsProcessed(state.iterations() * kWidth * kHeight);
+}
+
+static void ImageDraw_Binary(benchmark::State &state) { draw_image(state, IMAGE_TYPE_BINARY, TRANSPARENCY_OPAQUE); }
+BENCHMARK(ImageDraw_Binary);
+
+static void ImageDraw_Grayscale(benchmark::State &state) {
+  draw_image(state, IMAGE_TYPE_GRAYSCALE, TRANSPARENCY_OPAQUE);
+}
+BENCHMARK(ImageDraw_Grayscale);
+
+static void ImageDraw_RGB565(benchmark::State &state) { draw_image(state, IMAGE_TYPE_RGB565, TRANSPARENCY_OPAQUE); }
+BENCHMARK(ImageDraw_RGB565);
+
+static void ImageDraw_RGB(benchmark::State &state) { draw_image(state, IMAGE_TYPE_RGB, TRANSPARENCY_OPAQUE); }
+BENCHMARK(ImageDraw_RGB);
+
+static void ImageDraw_RGB_Rotated(benchmark::State &state) {
+  draw_image(state, IMAGE_TYPE_RGB, TRANSPARENCY_OPAQUE, display::DISPLAY_ROTATION_90_DEGREES);
+}
+BENCHMARK(ImageDraw_RGB_Rotated);
+
+static void ImageDraw_Binary_Transparent(benchmark::State &state) {
+  draw_image(state, IMAGE_TYPE_BINARY, TRANSPARENCY_CHROMA_KEY);
+}
+BENCHMARK(ImageDraw_Binary_Transparent);
+
+static void ImageDraw_Grayscale_ChromaKey(benchmark::State &state) {
+  draw_image(state, IMAGE_TYPE_GRAYSCALE, TRANSPARENCY_CHROMA_KEY);
+}
+BENCHMARK(ImageDraw_Grayscale_ChromaKey);
+
+static void ImageDraw_Grayscale_Alpha(benchmark::State &state) {
+  draw_image(state, IMAGE_TYPE_GRAYSCALE, TRANSPARENCY_ALPHA_CHANNEL);
+}
+BENCHMARK(ImageDraw_Grayscale_Alpha);
+
+static void ImageDraw_RGB_Alpha(benchmark::State &state) {
+  draw_image(state, IMAGE_TYPE_RGB, TRANSPARENCY_ALPHA_CHANNEL);
+}
+BENCHMARK(ImageDraw_RGB_Alpha);
+
+}  // namespace esphome::image::benchmarks