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()

Author: nico

Tests/LibGfx/TestImageWriter.cpp

@@ -6,6 +6,9 @@
 
 #include <AK/MemoryStream.h>
 #include <LibGfx/Bitmap.h>
+#include <LibGfx/ICC/BinaryWriter.h>
+#include <LibGfx/ICC/Profile.h>
+#include <LibGfx/ICC/WellKnownProfiles.h>
 #include <LibGfx/ImageFormats/BMPLoader.h>
 #include <LibGfx/ImageFormats/BMPWriter.h>
 #include <LibGfx/ImageFormats/JPEGLoader.h>
@@ -37,31 +40,38 @@ static ErrorOr<NonnullRefPtr<Gfx::Bitmap>> expect_single_frame_of_size(Gfx::Imag
     return frame;
 }
 
-template<class Writer, class Loader>
-static ErrorOr<NonnullRefPtr<Gfx::Bitmap>> get_roundtrip_bitmap(Gfx::Bitmap const& bitmap)
+template<class Writer, class... ExtraArgs>
+static ErrorOr<ByteBuffer> encode_bitmap(Gfx::Bitmap const& bitmap, ExtraArgs... extra_args)
 {
-    ByteBuffer encoded_data;
-    if constexpr (requires(AllocatingMemoryStream stream) { Writer::encode(stream, bitmap); }) {
+    if constexpr (requires(AllocatingMemoryStream stream) { Writer::encode(stream, bitmap, extra_args...); }) {
         AllocatingMemoryStream stream;
-        TRY(Writer::encode(stream, bitmap));
-        encoded_data = TRY(stream.read_until_eof());
+        TRY(Writer::encode(stream, bitmap, extra_args...));
+        return stream.read_until_eof();
     } else {
-        encoded_data = TRY(Writer::encode(bitmap));
+        return Writer::encode(bitmap, extra_args...);
     }
+}
+
+template<class Writer, class Loader>
+static ErrorOr<NonnullRefPtr<Gfx::Bitmap>> get_roundtrip_bitmap(Gfx::Bitmap const& bitmap)
+{
+    auto encoded_data = TRY(encode_bitmap<Writer>(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<class Writer, class Loader>
 static ErrorOr<void> test_roundtrip(Gfx::Bitmap const& bitmap)
 {
     auto decoded = TRY((get_roundtrip_bitmap<Writer, Loader>(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<Gfx::WebPWriter, Gfx::WebPImageDecoderPlugin>(TRY_OR_FAIL(create_test_rgb_bitmap()))));
     TRY_OR_FAIL((test_roundtrip<Gfx::WebPWriter, Gfx::WebPImageDecoderPlugin>(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<Gfx::WebPWriter>(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);
+}

Userland/Libraries/LibGfx/ImageFormats/WebPWriter.cpp

@@ -215,33 +215,41 @@ static ErrorOr<void> write_VP8X_header(Stream& stream, VP8XHeader const& header)
 
     LittleEndianOutputBitStream bit_stream { MaybeOwned<Stream>(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<void> 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;