From 1389ae02be94a61747c2f864c03c4a94f23080c8 Mon Sep 17 00:00:00 2001 From: Pavel Shliak Date: Sun, 3 Nov 2024 03:12:14 +0400 Subject: [PATCH] LibGfx: Fix 24bit BMPs being transparent and send them as RGBx to Skia --- Tests/LibGfx/TestImageDecoder.cpp | 5 ++++- Userland/Libraries/LibGfx/Bitmap.cpp | 1 + Userland/Libraries/LibGfx/Bitmap.h | 17 +++++++++++++++++ .../Libraries/LibGfx/ImageFormats/BMPLoader.cpp | 12 ++++++++++-- Userland/Libraries/LibGfx/PainterSkia.cpp | 2 ++ .../LibWeb/Painting/DisplayListPlayerSkia.cpp | 2 ++ Userland/Utilities/image.cpp | 10 ++++++++++ 7 files changed, 46 insertions(+), 3 deletions(-) diff --git a/Tests/LibGfx/TestImageDecoder.cpp b/Tests/LibGfx/TestImageDecoder.cpp index a723775c309..f9287720d77 100644 --- a/Tests/LibGfx/TestImageDecoder.cpp +++ b/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) diff --git a/Userland/Libraries/LibGfx/Bitmap.cpp b/Userland/Libraries/LibGfx/Bitmap.cpp index 1a84f0a9b68..01be0a51964 100644 --- a/Userland/Libraries/LibGfx/Bitmap.cpp +++ b/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; diff --git a/Userland/Libraries/LibGfx/Bitmap.h b/Userland/Libraries/LibGfx/Bitmap.h index 6bf9d8cc9d5..29ae045bb13 100644 --- a/Userland/Libraries/LibGfx/Bitmap.h +++ b/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(int x, int y, Colo scanline(y)[x] = rgba; } +template<> +ALWAYS_INLINE void Bitmap::set_pixel(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(x, y, color); break; + case StorageFormat::RGBx8888: + set_pixel(x, y, color); + break; default: VERIFY_NOT_REACHED(); } diff --git a/Userland/Libraries/LibGfx/ImageFormats/BMPLoader.cpp b/Userland/Libraries/LibGfx/ImageFormats/BMPLoader.cpp index 490f0e2437e..62f42eb7a12 100644 --- a/Userland/Libraries/LibGfx/ImageFormats/BMPLoader.cpp +++ b/Userland/Libraries/LibGfx/ImageFormats/BMPLoader.cpp @@ -1,6 +1,7 @@ /* * Copyright (c) 2020, Matthew Olsson * Copyright (c) 2022, Bruno Conde + * Copyright (c) 2024, Pavel Shliak * * SPDX-License-Identifier: BSD-2-Clause */ @@ -1256,7 +1257,7 @@ static ErrorOr 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 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: diff --git a/Userland/Libraries/LibGfx/PainterSkia.cpp b/Userland/Libraries/LibGfx/PainterSkia.cpp index 03f10765ae3..b68689bd1cc 100644 --- a/Userland/Libraries/LibGfx/PainterSkia.cpp +++ b/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; } diff --git a/Userland/Libraries/LibWeb/Painting/DisplayListPlayerSkia.cpp b/Userland/Libraries/LibWeb/Painting/DisplayListPlayerSkia.cpp index d72ce363604..de791f1b6dc 100644 --- a/Userland/Libraries/LibWeb/Painting/DisplayListPlayerSkia.cpp +++ b/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; } diff --git a/Userland/Utilities/image.cpp b/Userland/Utilities/image.cpp index 67711b33a36..6389ff868af 100644 --- a/Userland/Utilities/image.cpp +++ b/Userland/Utilities/image.cpp @@ -88,6 +88,10 @@ static ErrorOr 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 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 {}; }