Take the byte order in the constructor, keep ESP8266 per pixel, guard software rotation in mipi_spi and qspi_dbi

esphome/components/animation/animation.cpp

@@ -5,8 +5,8 @@
 namespace esphome::animation {
 
 Animation::Animation(const uint8_t *data_start, int width, int height, uint32_t animation_frame_count,
-                     image::ImageType type, image::Transparency transparent)
-    : Image(data_start, width, height, type, transparent),
+                     image::ImageType type, image::Transparency transparent, bool big_endian)
+    : Image(data_start, width, height, type, transparent, big_endian),
       animation_data_start_(data_start),
       current_frame_(0),
       animation_frame_count_(animation_frame_count),

esphome/components/animation/animation.h

@@ -8,7 +8,7 @@ namespace esphome::animation {
 class Animation final : public image::Image {
  public:
   Animation(const uint8_t *data_start, int width, int height, uint32_t animation_frame_count, image::ImageType type,
-            image::Transparency transparent);
+            image::Transparency transparent, bool big_endian = false);
 
   uint32_t get_animation_frame_count() const;
   int get_current_frame() const;

esphome/components/animation/image.py

@@ -114,9 +114,8 @@ async def setup_animation(config: ConfigType) -> None:
         frame_count,
         image_type,
         trans_value,
+        config.get(CONF_BYTE_ORDER) == "BIG_ENDIAN",
     )
-    if config.get(CONF_BYTE_ORDER) == "BIG_ENDIAN":
-        cg.add(var.set_big_endian(True))
     if loop_config := config.get(CONF_LOOP):
         start = loop_config[CONF_START_FRAME]
         end = loop_config.get(CONF_END_FRAME, frame_count)

esphome/components/display/display.h

@@ -360,6 +360,11 @@ class Display : public PollingComponent {
    * \param x_pad How many pixels are in each line after the end of the pixels to be copied.
    *
    * The length of each source buffer line (stride) will be x_offset + w + x_pad.
+   *
+   * The destination is in drawn coordinates, so an override that copies the block straight into
+   * its buffer must hand anything it cannot copy as is (another pixel format or byte order, a
+   * software rotation, an active clipping rectangle) to this base implementation, which goes
+   * through draw_pixel_at(). Callers keep the block on screen.
    */
   virtual 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);

esphome/components/file/image.py

@@ -254,10 +254,14 @@ async def new_image(config: ConfigType) -> MockObj:
     """
     prog_arr, width, height, image_type, trans_value, _ = await write_image(config)
     var = cg.new_Pvariable(
-        config[CONF_ID], prog_arr, width, height, image_type, trans_value
+        config[CONF_ID],
+        prog_arr,
+        width,
+        height,
+        image_type,
+        trans_value,
+        config.get(CONF_BYTE_ORDER) == "BIG_ENDIAN",
     )
-    if config.get(CONF_BYTE_ORDER) == "BIG_ENDIAN":
-        cg.add(var.set_big_endian(True))
     add_metadata(
         config[CONF_ID], width, height, config[CONF_TYPE], config[CONF_TRANSPARENCY]
     )

esphome/components/image/image.cpp

@@ -42,28 +42,27 @@ void Image::draw(int x, int y, display::Display *display, Color color_on, Color
       y_end = clipping.y2() - y;
   }
 
-  // Clamp to the display so the bulk path below never writes off screen; the
-  // per pixel path bounds checks itself, this just saves it the work.
-  if (x < 0)
-    x_start = std::max(x_start, -x);
-  if (y < 0)
-    y_start = std::max(y_start, -y);
-  x_end = std::min(x_end, display->get_width() - x);
-  y_end = std::min(y_end, display->get_height() - y);
-  if (x_end <= x_start || y_end <= y_start)
-    return;
-
+#ifndef USE_ESP8266
   // Opaque RGB565 and RGB pixels are already in the layout draw_pixels_at()
   // takes, so hand the window to the display: drivers with a bulk path blit
-  // it, the others walk it per pixel as before.
+  // it, the others walk it per pixel as before. Not on the ESP8266, where the
+  // pixels live in flash and only progmem_read_byte() may read them.
   if (this->transparency_ == TRANSPARENCY_OPAQUE &&
       (this->type_ == IMAGE_TYPE_RGB565 || this->type_ == IMAGE_TYPE_RGB)) {
+    // Clamp to the display so the block never reaches off screen
+    x_start = std::max(x_start, -x);
+    y_start = std::max(y_start, -y);
+    x_end = std::min(x_end, display->get_width() - x);
+    y_end = std::min(y_end, display->get_height() - y);
+    if (x_end <= x_start || y_end <= y_start)
+      return;
     const bool rgb565 = this->type_ == IMAGE_TYPE_RGB565;
     display->draw_pixels_at(x + x_start, y + y_start, x_end - x_start, y_end - y_start, this->data_start_,
                             display::COLOR_ORDER_RGB, rgb565 ? display::COLOR_BITNESS_565 : display::COLOR_BITNESS_888,
-                            rgb565 && this->big_endian_, x_start, y_start, this->width_ - x_end);
+                            this->big_endian_, x_start, y_start, this->width_ - x_end);
     return;
   }
+#endif
 
   // 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
@@ -204,7 +203,7 @@ bool Image::get_binary_pixel_(int x, int y) const {
   return progmem_read_byte(this->data_start_ + (pos / 8u)) & (0x80 >> (pos % 8u));
 }
 Color Image::get_rgb_pixel_(int x, int y) const {
-  const uint32_t pos = (x + y * this->width_) * this->bpp_ / 8;
+  const uint32_t pos = (x + y * this->width_) * static_cast<size_t>(this->bpp_) / 8;
   Color color = Color(progmem_read_byte(this->data_start_ + pos + 2), progmem_read_byte(this->data_start_ + pos + 1),
                       progmem_read_byte(this->data_start_ + pos + 0), 0xFF);
 
@@ -224,9 +223,10 @@ Color Image::get_rgb_pixel_(int x, int y) const {
   return color;
 }
 Color Image::get_rgb565_pixel_(int x, int y) const {
-  const uint8_t *pos = this->data_start_ + (x + y * this->width_) * this->bpp_ / 8;
-  const uint16_t rgb565 = this->big_endian_ ? encode_uint16(progmem_read_byte(pos), progmem_read_byte(pos + 1))
-                                            : encode_uint16(progmem_read_byte(pos + 1), progmem_read_byte(pos));
+  const uint8_t *pos = this->data_start_ + (x + y * this->width_) * static_cast<size_t>(this->bpp_) / 8;
+  // The high byte sits at pos when big endian, at pos + 1 otherwise
+  const uint8_t high = this->big_endian_ ? 0 : 1;
+  const uint16_t rgb565 = encode_uint16(progmem_read_byte(pos + high), progmem_read_byte(pos + 1 - high));
   auto r = (rgb565 & 0xF800) >> 11;
   auto g = (rgb565 & 0x07E0) >> 5;
   auto b = rgb565 & 0x001F;
@@ -262,8 +262,14 @@ Color Image::get_grayscale_pixel_(int x, int y) const {
 int Image::get_width() const { return this->width_; }
 int Image::get_height() const { return this->height_; }
 ImageType Image::get_type() const { return this->type_; }
-Image::Image(const uint8_t *data_start, int width, int height, ImageType type, Transparency transparency)
-    : width_(width), height_(height), type_(type), data_start_(data_start), transparency_(transparency) {
+Image::Image(const uint8_t *data_start, int width, int height, ImageType type, Transparency transparency,
+             bool big_endian)
+    : width_(width),
+      height_(height),
+      type_(type),
+      data_start_(data_start),
+      transparency_(transparency),
+      big_endian_(big_endian && type == IMAGE_TYPE_RGB565) {
   switch (this->type_) {
     case IMAGE_TYPE_BINARY:
       this->bpp_ = 1;

esphome/components/image/image.h

@@ -23,7 +23,9 @@ enum Transparency {
 
 class Image : public display::BaseImage {
  public:
-  Image(const uint8_t *data_start, int width, int height, ImageType type, Transparency transparency);
+  /// big_endian is the RGB565 byte order; codegen writes little endian unless configured otherwise.
+  Image(const uint8_t *data_start, int width, int height, ImageType type, Transparency transparency,
+        bool big_endian = false);
   Color get_pixel(int x, int y, Color color_on = display::COLOR_ON, Color color_off = display::COLOR_OFF) const;
   int get_width() const override;
   int get_height() const override;
@@ -38,8 +40,6 @@ class Image : public display::BaseImage {
   void draw(int x, int y, display::Display *display, Color color_on, Color color_off) override;
 
   bool has_transparency() const { return this->transparency_ != TRANSPARENCY_OPAQUE; }
-  /// RGB565 pixel byte order; static images default to little endian.
-  void set_big_endian(bool big_endian) { this->big_endian_ = big_endian; }
 
 #ifdef USE_LVGL
   lv_image_dsc_t *get_lv_image_dsc();
@@ -59,8 +59,8 @@ class Image : public display::BaseImage {
   ImageType type_;
   const uint8_t *data_start_;
   Transparency transparency_;
-  bool big_endian_{false};
-  size_t bpp_{};
+  bool big_endian_;
+  uint8_t bpp_{};
 #ifdef USE_LVGL
   lv_img_dsc_t dsc_{};
 #endif

esphome/components/mipi_spi/mipi_spi.h

@@ -188,8 +188,9 @@ class MipiSpi : public display::Display,
       return;
     if (w <= 0 || h <= 0)
       return;
-    if (get_pixel_mode(bitness) != BUFFERPIXEL || big_endian != IS_BIG_ENDIAN) {
-      // Not the buffer's own format: convert per pixel through draw_pixel_at()
+    if (get_pixel_mode(bitness) != BUFFERPIXEL || big_endian != IS_BIG_ENDIAN ||
+        (!HAS_HARDWARE_ROTATION && this->rotation_ != display::DISPLAY_ROTATION_0_DEGREES)) {
+      // Not the buffer's own format, or rotated in software: go per pixel through draw_pixel_at()
       display::Display::draw_pixels_at(x_start, y_start, w, h, ptr, order, bitness, big_endian, x_offset, y_offset,
                                        x_pad);
       return;

esphome/components/qspi_dbi/qspi_dbi.cpp

@@ -139,7 +139,8 @@ void QspiDbi::draw_pixels_at(int x_start, int y_start, int w, int h, const uint8
   if (w <= 0 || h <= 0)
     return;
   if (bitness != display::COLOR_BITNESS_565 || order != this->color_mode_ ||
-      big_endian != (this->bit_order_ == spi::BIT_ORDER_MSB_FIRST)) {
+      big_endian != (this->bit_order_ == spi::BIT_ORDER_MSB_FIRST) ||
+      this->rotation_ != display::DISPLAY_ROTATION_0_DEGREES) {
     Display::draw_pixels_at(x_start, y_start, w, h, ptr, order, bitness, big_endian, x_offset, y_offset, x_pad);
     return;
   } else if (this->draw_from_origin_) {

esphome/components/runtime_image/runtime_image.cpp

@@ -41,14 +41,11 @@ inline bool is_color_on(const Color &color) {
 
 RuntimeImage::RuntimeImage(ImageFormat format, image::ImageType type, image::Transparency transparency,
                            image::Image *placeholder, bool is_big_endian, int fixed_width, int fixed_height)
-    : Image(nullptr, 0, 0, type, transparency),
+    : Image(nullptr, 0, 0, type, transparency, is_big_endian),
       format_(format),
       fixed_width_(fixed_width),
       fixed_height_(fixed_height),
-      placeholder_(placeholder),
-      is_big_endian_(is_big_endian) {
-  this->big_endian_ = is_big_endian;
-}
+      placeholder_(placeholder) {}
 
 RuntimeImage::~RuntimeImage() { this->release(); }
 
@@ -123,7 +120,7 @@ void RuntimeImage::draw_pixel(int x, int y, const Color &color) {
       Color mapped_color = color;
       this->map_chroma_key(mapped_color);
       uint16_t rgb565 = display::ColorUtil::color_to_565(mapped_color);
-      if (this->is_big_endian_) {
+      if (this->big_endian_) {
         this->buffer_[pos + 0] = static_cast<uint8_t>((rgb565 >> 8) & 0xFF);
         this->buffer_[pos + 1] = static_cast<uint8_t>(rgb565 & 0xFF);
       } else {

esphome/components/runtime_image/runtime_image.h

@@ -227,11 +227,6 @@ class RuntimeImage : public image::Image {
 
   // Configuration
   bool progressive_display_{false};
-  /**
-   * Whether the image is stored in big-endian format.
-   * This is used to determine how to store 16 bit colors in the buffer.
-   */
-  bool is_big_endian_{false};
   /** Whether buffer_ belongs to the caller, so it must not be freed or resized here. */
   bool external_buffer_{false};
 };

tests/components/image/test_image_draw.cpp

@@ -7,6 +7,14 @@
 
 namespace esphome::image::testing {
 
+class TestDisplay : public display::Display {
+ public:
+  void update() override {}
+  display::DisplayType get_display_type() override { return display::DisplayType::DISPLAY_TYPE_COLOR; }
+  int get_width_internal() override { return 64; }
+  int get_height_internal() override { return 48; }
+};
+
 struct Blit {
   int x, y, w, h;
   const uint8_t *ptr;
@@ -15,32 +23,23 @@ struct Blit {
   int x_offset, y_offset, x_pad;
 };
 
-// Records the bulk calls and the per pixel calls Image::draw() makes.
-class RecordingDisplay : public display::Display {
+// Records the bulk calls and counts the per pixel calls Image::draw() makes.
+class RecordingDisplay : public TestDisplay {
  public:
   void draw_pixels_at(int x_start, int y_start, int w, int h, const uint8_t *ptr, display::ColorOrder order,
                       display::ColorBitness bitness, bool big_endian, int x_offset, int y_offset, int x_pad) override {
     this->blits.push_back({x_start, y_start, w, h, ptr, bitness, big_endian, x_offset, y_offset, x_pad});
   }
   void draw_pixel_at(int x, int y, Color color) override { this->pixels++; }
-  void update() override {}
-  display::DisplayType get_display_type() override { return display::DisplayType::DISPLAY_TYPE_COLOR; }
-  int get_width_internal() override { return 64; }
-  int get_height_internal() override { return 48; }
 
   std::vector<Blit> blits;
   int pixels{0};
 };
 
-// Same display, but the bulk call falls through to the base class so the
-// pixels it produces can be checked.
-class FallbackDisplay : public display::Display {
+// Lets the bulk call fall through to the base class and keeps the pixels it produces.
+class FallbackDisplay : public TestDisplay {
  public:
   void draw_pixel_at(int x, int y, Color color) override { this->pixels.push_back({x, y, color.raw_32}); }
-  void update() override {}
-  display::DisplayType get_display_type() override { return display::DisplayType::DISPLAY_TYPE_COLOR; }
-  int get_width_internal() override { return 64; }
-  int get_height_internal() override { return 48; }
 
   struct Pixel {
     int x, y;
@@ -59,30 +58,45 @@ static std::vector<uint8_t> make_data(int bytes_per_pixel) {
   return data;
 }
 
+// Draws an opaque RGB565 image at (x, y) and returns the single bulk call it made.
+static Blit draw_one(int x, int y, const std::vector<uint8_t> &data, RecordingDisplay &display,
+                     bool big_endian = false) {
+  Image image(data.data(), W, H, IMAGE_TYPE_RGB565, TRANSPARENCY_OPAQUE, big_endian);
+  image.draw(x, y, &display, Color::WHITE, Color::BLACK);
+  EXPECT_EQ(display.blits.size(), 1u);
+  EXPECT_EQ(display.pixels, 0);
+  return display.blits.at(0);
+}
+
+static void expect_window(const Blit &blit, int x, int y, int w, int h, int x_offset, int y_offset, int x_pad) {
+  EXPECT_EQ(blit.x, x);
+  EXPECT_EQ(blit.y, y);
+  EXPECT_EQ(blit.w, w);
+  EXPECT_EQ(blit.h, h);
+  EXPECT_EQ(blit.x_offset, x_offset);
+  EXPECT_EQ(blit.y_offset, y_offset);
+  EXPECT_EQ(blit.x_pad, x_pad);
+}
+
 TEST(ImageDraw, OpaqueRgb565IsOneBulkCall) {
   const auto data = make_data(2);
-  Image image(data.data(), W, H, IMAGE_TYPE_RGB565, TRANSPARENCY_OPAQUE);
   RecordingDisplay display;
-  image.draw(10, 20, &display, Color::WHITE, Color::BLACK);
-  ASSERT_EQ(display.blits.size(), 1u);
-  EXPECT_EQ(display.pixels, 0);
-  const auto &blit = display.blits[0];
-  EXPECT_EQ(blit.x, 10);
-  EXPECT_EQ(blit.y, 20);
-  EXPECT_EQ(blit.w, W);
-  EXPECT_EQ(blit.h, H);
+  const Blit blit = draw_one(10, 20, data, display);
+  expect_window(blit, 10, 20, W, H, 0, 0, 0);
   EXPECT_EQ(blit.ptr, data.data());
   EXPECT_EQ(blit.bitness, display::COLOR_BITNESS_565);
   EXPECT_FALSE(blit.big_endian);
-  EXPECT_EQ(blit.x_offset, 0);
-  EXPECT_EQ(blit.y_offset, 0);
-  EXPECT_EQ(blit.x_pad, 0);
+}
+
+TEST(ImageDraw, BigEndianRgb565IsPassedThrough) {
+  const auto data = make_data(2);
+  RecordingDisplay display;
+  EXPECT_TRUE(draw_one(0, 0, data, display, true).big_endian);
 }
 
 TEST(ImageDraw, OpaqueRgbIsOneBulkCall) {
   const auto data = make_data(3);
-  Image image(data.data(), W, H, IMAGE_TYPE_RGB, TRANSPARENCY_OPAQUE);
-  image.set_big_endian(true);  // only meaningful for 16 bit pixels
+  Image image(data.data(), W, H, IMAGE_TYPE_RGB, TRANSPARENCY_OPAQUE, true);  // byte order only applies to 565
   RecordingDisplay display;
   image.draw(0, 0, &display, Color::WHITE, Color::BLACK);
   ASSERT_EQ(display.blits.size(), 1u);
@@ -90,64 +104,23 @@ TEST(ImageDraw, OpaqueRgbIsOneBulkCall) {
   EXPECT_FALSE(display.blits[0].big_endian);
 }
 
-TEST(ImageDraw, BigEndianRgb565IsPassedThrough) {
-  const auto data = make_data(2);
-  Image image(data.data(), W, H, IMAGE_TYPE_RGB565, TRANSPARENCY_OPAQUE);
-  image.set_big_endian(true);
-  RecordingDisplay display;
-  image.draw(0, 0, &display, Color::WHITE, Color::BLACK);
-  ASSERT_EQ(display.blits.size(), 1u);
-  EXPECT_TRUE(display.blits[0].big_endian);
-}
-
 TEST(ImageDraw, ClippingShrinksTheWindow) {
   const auto data = make_data(2);
-  Image image(data.data(), W, H, IMAGE_TYPE_RGB565, TRANSPARENCY_OPAQUE);
   RecordingDisplay display;
   display.start_clipping(12, 22, 15, 24);  // right and bottom are exclusive
-  image.draw(10, 20, &display, Color::WHITE, Color::BLACK);
-  display.end_clipping();
-  ASSERT_EQ(display.blits.size(), 1u);
-  const auto &blit = display.blits[0];
-  EXPECT_EQ(blit.x, 12);
-  EXPECT_EQ(blit.y, 22);
-  EXPECT_EQ(blit.w, 3);
-  EXPECT_EQ(blit.h, 2);
-  EXPECT_EQ(blit.x_offset, 2);
-  EXPECT_EQ(blit.y_offset, 2);
-  EXPECT_EQ(blit.x_pad, W - 5);
+  expect_window(draw_one(10, 20, data, display), 12, 22, 3, 2, 2, 2, W - 5);
 }
 
 TEST(ImageDraw, PartlyOffTheTopLeftIsClamped) {
   const auto data = make_data(2);
-  Image image(data.data(), W, H, IMAGE_TYPE_RGB565, TRANSPARENCY_OPAQUE);
   RecordingDisplay display;
-  image.draw(-3, -2, &display, Color::WHITE, Color::BLACK);
-  ASSERT_EQ(display.blits.size(), 1u);
-  const auto &blit = display.blits[0];
-  EXPECT_EQ(blit.x, 0);
-  EXPECT_EQ(blit.y, 0);
-  EXPECT_EQ(blit.w, W - 3);
-  EXPECT_EQ(blit.h, H - 2);
-  EXPECT_EQ(blit.x_offset, 3);
-  EXPECT_EQ(blit.y_offset, 2);
-  EXPECT_EQ(blit.x_pad, 0);
+  expect_window(draw_one(-3, -2, data, display), 0, 0, W - 3, H - 2, 3, 2, 0);
 }
 
 TEST(ImageDraw, PartlyOffTheBottomRightIsClamped) {
   const auto data = make_data(2);
-  Image image(data.data(), W, H, IMAGE_TYPE_RGB565, TRANSPARENCY_OPAQUE);
   RecordingDisplay display;
-  image.draw(60, 44, &display, Color::WHITE, Color::BLACK);
-  ASSERT_EQ(display.blits.size(), 1u);
-  const auto &blit = display.blits[0];
-  EXPECT_EQ(blit.x, 60);
-  EXPECT_EQ(blit.y, 44);
-  EXPECT_EQ(blit.w, 4);
-  EXPECT_EQ(blit.h, 4);
-  EXPECT_EQ(blit.x_offset, 0);
-  EXPECT_EQ(blit.y_offset, 0);
-  EXPECT_EQ(blit.x_pad, W - 4);
+  expect_window(draw_one(60, 44, data, display), 60, 44, 4, 4, 0, 0, W - 4);
 }
 
 TEST(ImageDraw, FullyOffScreenDrawsNothing) {
@@ -172,18 +145,16 @@ TEST(ImageDraw, TransparentAndOtherFormatsStayPerPixel) {
   EXPECT_GT(display.pixels, 0);
 }
 
-// The base class fallback must decode the pixels the same way the per pixel
-// readers do, in both byte orders.
+// The base class fallback must read the pixels in the byte order the image declares.
 static void expect_fallback_decodes(bool big_endian) {
   const auto data = make_data(2);
-  Image image(data.data(), W, H, IMAGE_TYPE_RGB565, TRANSPARENCY_OPAQUE);
-  image.set_big_endian(big_endian);
+  Image image(data.data(), W, H, IMAGE_TYPE_RGB565, TRANSPARENCY_OPAQUE, big_endian);
   FallbackDisplay display;
   image.draw(0, 0, &display, Color::WHITE, Color::BLACK);
   ASSERT_EQ(display.pixels.size(), static_cast<size_t>(W * H));
   for (const auto &pixel : display.pixels) {
-    const uint8_t *pos = data.data() + (pixel.x + pixel.y * W) * 2;
-    const uint16_t rgb565 = big_endian ? (pos[0] << 8) | pos[1] : (pos[1] << 8) | pos[0];
+    const uint32_t rgb565 =
+        display::ColorUtil::read_packed<display::COLOR_BITNESS_565>(data.data(), pixel.x + pixel.y * W, big_endian);
     const Color expected = display::ColorUtil::to_color(rgb565, display::COLOR_ORDER_RGB, display::COLOR_BITNESS_565);
     EXPECT_EQ(pixel.raw, expected.raw_32) << "at " << pixel.x << "," << pixel.y;
   }