From d988b6facc8f64ba814b8aea7b76c4e04c35d1fa Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Tue, 7 May 2024 18:00:33 -0400 Subject: [PATCH] LibGfx/WebPWriter+TestImageWriter: Fix bugs writing VP8X header Two bugs: 1. Correctly set bits in VP8X header. Turns out these were set in the wrong order. 2. Correctly set the `has_alpha` flag. Also add a test for writing webp files with icc data. With the additional checks in other commits in this PR, this test catches the bug in WebPWriter. Rearrange some existing functions to make it easier to write this test: * Extract encode_bitmap() from get_roundtrip_bitmap(). encode_bitmap() allows passing extra_args that the test uses to pass in ICC data. * Extract expect_bitmaps_equal() from test_roundtrip() --- Tests/LibGfx/TestImageWriter.cpp | 55 ++++++++++++++----- .../LibGfx/ImageFormats/WebPWriter.cpp | 24 +++++--- 2 files changed, 56 insertions(+), 23 deletions(-) diff --git a/Tests/LibGfx/TestImageWriter.cpp b/Tests/LibGfx/TestImageWriter.cpp index 01f793ce37f..0d7b5e52fa8 100644 --- a/Tests/LibGfx/TestImageWriter.cpp +++ b/Tests/LibGfx/TestImageWriter.cpp @@ -6,6 +6,9 @@ #include #include +#include +#include +#include #include #include #include @@ -37,31 +40,38 @@ static ErrorOr> expect_single_frame_of_size(Gfx::Imag return frame; } +template +static ErrorOr encode_bitmap(Gfx::Bitmap const& bitmap, ExtraArgs... extra_args) +{ + if constexpr (requires(AllocatingMemoryStream stream) { Writer::encode(stream, bitmap, extra_args...); }) { + AllocatingMemoryStream stream; + TRY(Writer::encode(stream, bitmap, extra_args...)); + return stream.read_until_eof(); + } else { + return Writer::encode(bitmap, extra_args...); + } +} + template static ErrorOr> get_roundtrip_bitmap(Gfx::Bitmap const& bitmap) { - ByteBuffer encoded_data; - if constexpr (requires(AllocatingMemoryStream stream) { Writer::encode(stream, bitmap); }) { - AllocatingMemoryStream stream; - TRY(Writer::encode(stream, bitmap)); - encoded_data = TRY(stream.read_until_eof()); - } else { - encoded_data = TRY(Writer::encode(bitmap)); - } + auto encoded_data = TRY(encode_bitmap(bitmap)); + return expect_single_frame_of_size(*TRY(Loader::create(encoded_data)), bitmap.size()); +} - auto plugin = TRY(Loader::create(encoded_data)); - return expect_single_frame_of_size(*plugin, bitmap.size()); +static void expect_bitmaps_equal(Gfx::Bitmap const& a, Gfx::Bitmap const& b) +{ + VERIFY(a.size() == b.size()); + for (int y = 0; y < a.height(); ++y) + for (int x = 0; x < a.width(); ++x) + EXPECT_EQ(a.get_pixel(x, y), b.get_pixel(x, y)); } template static ErrorOr test_roundtrip(Gfx::Bitmap const& bitmap) { auto decoded = TRY((get_roundtrip_bitmap(bitmap))); - - for (int y = 0; y < bitmap.height(); ++y) - for (int x = 0; x < bitmap.width(); ++x) - EXPECT_EQ(decoded->get_pixel(x, y), bitmap.get_pixel(x, y)); - + expect_bitmaps_equal(*decoded, bitmap); return {}; } @@ -120,3 +130,18 @@ TEST_CASE(test_webp) TRY_OR_FAIL((test_roundtrip(TRY_OR_FAIL(create_test_rgb_bitmap())))); TRY_OR_FAIL((test_roundtrip(TRY_OR_FAIL(create_test_rgba_bitmap())))); } + +TEST_CASE(test_webp_icc) +{ + auto sRGB_icc_profile = MUST(Gfx::ICC::sRGB()); + auto sRGB_icc_data = MUST(Gfx::ICC::encode(sRGB_icc_profile)); + + auto rgba_bitmap = TRY_OR_FAIL(create_test_rgba_bitmap()); + auto encoded_rgba_bitmap = TRY_OR_FAIL((encode_bitmap(rgba_bitmap, Gfx::WebPEncoderOptions { .icc_data = sRGB_icc_data }))); + + auto decoded_rgba_plugin = TRY_OR_FAIL(Gfx::WebPImageDecoderPlugin::create(encoded_rgba_bitmap)); + expect_bitmaps_equal(*TRY_OR_FAIL(expect_single_frame_of_size(*decoded_rgba_plugin, rgba_bitmap->size())), rgba_bitmap); + auto decoded_rgba_profile = TRY_OR_FAIL(Gfx::ICC::Profile::try_load_from_externally_owned_memory(TRY_OR_FAIL(decoded_rgba_plugin->icc_data()).value())); + auto reencoded_icc_data = TRY_OR_FAIL(Gfx::ICC::encode(decoded_rgba_profile)); + EXPECT_EQ(sRGB_icc_data, reencoded_icc_data); +} diff --git a/Userland/Libraries/LibGfx/ImageFormats/WebPWriter.cpp b/Userland/Libraries/LibGfx/ImageFormats/WebPWriter.cpp index bb869e6c2f6..08b4f593e41 100644 --- a/Userland/Libraries/LibGfx/ImageFormats/WebPWriter.cpp +++ b/Userland/Libraries/LibGfx/ImageFormats/WebPWriter.cpp @@ -215,33 +215,41 @@ static ErrorOr write_VP8X_header(Stream& stream, VP8XHeader const& header) LittleEndianOutputBitStream bit_stream { MaybeOwned(stream) }; + // Don't use bit_stream.write_bits() to write individual flags here: + // The spec describes bit flags in MSB to LSB order, but write_bits() writes LSB to MSB. + u8 flags = 0; // "Reserved (Rsv): 2 bits // MUST be 0. Readers MUST ignore this field." - TRY(bit_stream.write_bits(0u, 2u)); // "ICC profile (I): 1 bit // Set if the file contains an 'ICCP' Chunk." - TRY(bit_stream.write_bits(header.has_icc, 1u)); + if (header.has_icc) + flags |= 0x20; // "Alpha (L): 1 bit // Set if any of the frames of the image contain transparency information ("alpha")." - TRY(bit_stream.write_bits(header.has_alpha, 1u)); + if (header.has_alpha) + flags |= 0x10; // "Exif metadata (E): 1 bit // Set if the file contains Exif metadata." - TRY(bit_stream.write_bits(header.has_exif, 1u)); + if (header.has_exif) + flags |= 0x8; // "XMP metadata (X): 1 bit // Set if the file contains XMP metadata." - TRY(bit_stream.write_bits(header.has_xmp, 1u)); + if (header.has_xmp) + flags |= 0x4; // "Animation (A): 1 bit // Set if this is an animated image. Data in 'ANIM' and 'ANMF' Chunks should be used to control the animation." - TRY(bit_stream.write_bits(header.has_animation, 1u)); + if (header.has_animation) + flags |= 0x2; // "Reserved (R): 1 bit // MUST be 0. Readers MUST ignore this field." - TRY(bit_stream.write_bits(0u, 1u)); + + TRY(bit_stream.write_bits(flags, 8u)); // "Reserved: 24 bits // MUST be 0. Readers MUST ignore this field." @@ -304,7 +312,7 @@ ErrorOr WebPWriter::encode(Stream& stream, Bitmap const& bitmap, Options c iccp_chunk_bytes = TRY(iccp_chunk_stream.read_until_eof()); AllocatingMemoryStream vp8x_header_stream; - TRY(write_VP8X_header(vp8x_header_stream, { .has_icc = true, .width = (u32)bitmap.width(), .height = (u32)bitmap.height() })); + TRY(write_VP8X_header(vp8x_header_stream, { .has_icc = true, .has_alpha = alpha_is_used_hint, .width = (u32)bitmap.width(), .height = (u32)bitmap.height() })); auto vp8x_header_bytes = TRY(vp8x_header_stream.read_until_eof()); AllocatingMemoryStream vp8x_chunk_stream;