LibGfx: Fix 24bit BMPs being transparent and send them as RGBx to Skia

Author: https://github.com/shlyakpavel
Commit: 1389ae02be
Pull-request: https://github.com/LadybirdBrowser/ladybird/pull/2132
Reviewed-by: https://github.com/gmta ✅
Reviewed-by: https://github.com/simonkrauter

Tests/LibGfx/TestImageDecoder.cpp

@@ -59,7 +59,10 @@ TEST_CASE(test_bmp_top_down)
     EXPECT(Gfx::BMPImageDecoderPlugin::sniff(file->bytes()));
     auto plugin_decoder = TRY_OR_FAIL(Gfx::BMPImageDecoderPlugin::create(file->bytes()));
 
-    TRY_OR_FAIL(expect_single_frame(*plugin_decoder));
+    auto frame = TRY_OR_FAIL(expect_single_frame(*plugin_decoder));
+    EXPECT_EQ(frame.image->format(), Gfx::BitmapFormat::RGBx8888);
+    // Compares only rgb data
+    EXPECT_EQ(frame.image->begin()[0] & 0x00ffffffU, 0x00dcc1b8U);
 }
 
 TEST_CASE(test_bmp_1bpp)

Userland/Libraries/LibGfx/Bitmap.cpp

@@ -37,6 +37,7 @@ size_t Bitmap::minimum_pitch(size_t width, BitmapFormat format)
     switch (determine_storage_format(format)) {
     case StorageFormat::BGRx8888:
     case StorageFormat::BGRA8888:
+    case StorageFormat::RGBx8888:
     case StorageFormat::RGBA8888:
         element_size = 4;
         break;

Userland/Libraries/LibGfx/Bitmap.h

@@ -23,6 +23,7 @@ enum class BitmapFormat {
     Invalid,
     BGRx8888,
     BGRA8888,
+    RGBx8888,
     RGBA8888,
 };
 
@@ -31,6 +32,7 @@ inline bool is_valid_bitmap_format(unsigned format)
     switch (format) {
     case (unsigned)BitmapFormat::Invalid:
     case (unsigned)BitmapFormat::BGRx8888:
+    case (unsigned)BitmapFormat::RGBx8888:
     case (unsigned)BitmapFormat::BGRA8888:
     case (unsigned)BitmapFormat::RGBA8888:
         return true;
@@ -42,6 +44,7 @@ enum class StorageFormat {
     BGRx8888,
     BGRA8888,
     RGBA8888,
+    RGBx8888,
 };
 
 inline StorageFormat determine_storage_format(BitmapFormat format)
@@ -53,6 +56,8 @@ inline StorageFormat determine_storage_format(BitmapFormat format)
         return StorageFormat::BGRA8888;
     case BitmapFormat::RGBA8888:
         return StorageFormat::RGBA8888;
+    case BitmapFormat::RGBx8888:
+        return StorageFormat::RGBx8888;
     default:
         VERIFY_NOT_REACHED();
     }
@@ -309,6 +314,15 @@ ALWAYS_INLINE void Bitmap::set_pixel<StorageFormat::RGBA8888>(int x, int y, Colo
     scanline(y)[x] = rgba;
 }
 
+template<>
+ALWAYS_INLINE void Bitmap::set_pixel<StorageFormat::RGBx8888>(int x, int y, Color color)
+{
+    VERIFY(x >= 0);
+    VERIFY(x < width());
+    auto rgb = (color.blue() << 16) | (color.green() << 8) | color.red();
+    scanline(y)[x] = rgb;
+}
+
 ALWAYS_INLINE void Bitmap::set_pixel(int x, int y, Color color)
 {
     switch (determine_storage_format(m_format)) {
@@ -321,6 +335,9 @@ ALWAYS_INLINE void Bitmap::set_pixel(int x, int y, Color color)
     case StorageFormat::RGBA8888:
         set_pixel<StorageFormat::RGBA8888>(x, y, color);
         break;
+    case StorageFormat::RGBx8888:
+        set_pixel<StorageFormat::RGBx8888>(x, y, color);
+        break;
     default:
         VERIFY_NOT_REACHED();
     }

Userland/Libraries/LibGfx/ImageFormats/BMPLoader.cpp

@@ -1,6 +1,7 @@
 /*
  * Copyright (c) 2020, Matthew Olsson <mattco@serenityos.org>
  * Copyright (c) 2022, Bruno Conde <brunompconde@gmail.com>
+ * Copyright (c) 2024, Pavel Shliak <shlyakpavel@gmail.com>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -1256,7 +1257,7 @@ static ErrorOr<void> decode_bmp_pixel_data(BMPLoadingContext& context)
                 return BitmapFormat::BGRA8888;
             return BitmapFormat::BGRx8888;
         case 24:
-            return BitmapFormat::BGRx8888;
+            return BitmapFormat::RGBx8888;
         case 32:
             return BitmapFormat::BGRA8888;
         default:
@@ -1372,7 +1373,14 @@ static ErrorOr<void> decode_bmp_pixel_data(BMPLoadingContext& context)
             case 24: {
                 if (!streamer.has_u24())
                     return Error::from_string_literal("Cannot read 24 bits");
-                context.bitmap->scanline(row)[column++] = streamer.read_u24();
+
+                u32 pixel = streamer.read_u24();
+                u8 b = (pixel & 0xFF0000) >> 16;
+                u8 g = (pixel & 0x00FF00) >> 8;
+                u8 r = (pixel & 0x0000FF);
+
+                u32 rgbx_pixel = (r << 16) | (g << 8) | b;
+                context.bitmap->scanline(row)[column++] = rgbx_pixel;
                 break;
             }
             case 32:

Userland/Libraries/LibGfx/PainterSkia.cpp

@@ -38,6 +38,8 @@ static SkColorType to_skia_color_type(Gfx::BitmapFormat format)
         return kBGRA_8888_SkColorType;
     case Gfx::BitmapFormat::RGBA8888:
         return kRGBA_8888_SkColorType;
+    case Gfx::BitmapFormat::RGBx8888:
+        return kRGB_888x_SkColorType;
     default:
         return kUnknown_SkColorType;
     }

Userland/Libraries/LibWeb/Painting/DisplayListPlayerSkia.cpp

@@ -395,6 +395,8 @@ static SkColorType to_skia_color_type(Gfx::BitmapFormat format)
         return kBGRA_8888_SkColorType;
     case Gfx::BitmapFormat::RGBA8888:
         return kRGBA_8888_SkColorType;
+    case Gfx::BitmapFormat::RGBx8888:
+        return kRGB_888x_SkColorType;
     default:
         return kUnknown_SkColorType;
     }

Userland/Utilities/image.cpp

@@ -88,6 +88,10 @@ static ErrorOr<void> move_alpha_to_rgb(LoadedImage& image)
             u8 alpha = pixel >> 24;
             pixel = 0xff000000 | (alpha << 16) | (alpha << 8) | alpha;
         }
+        break;
+    case Gfx::BitmapFormat::RGBx8888:
+        // This should never be the case, as there's no alpha channel in the image
+        return Error::from_string_literal("Can't --move-alpha-to-rgb with RGBx8888 bitmaps");
     }
     return {};
 }
@@ -108,7 +112,13 @@ static ErrorOr<void> strip_alpha(LoadedImage& image)
         return Error::from_string_literal("--strip-alpha not implemented for RGBA8888");
     case Gfx::BitmapFormat::BGRA8888:
     case Gfx::BitmapFormat::BGRx8888:
+        // BGRx8888 is sent as BGRA8888 to Skia,
+        // that's why we need to ensure there's no "alpha channel" left
         frame->strip_alpha_channel();
+        break;
+    case Gfx::BitmapFormat::RGBx8888:
+        // This format means there's no alpha channel, so nothing to do here
+        break;
     }
     return {};
 }