From bd9328581178e66d10620b7989cc67fa180e2d48 Mon Sep 17 00:00:00 2001 From: Lucas CHOLLET Date: Mon, 2 Dec 2024 18:55:43 -0500 Subject: [PATCH] LibGfx+LibWeb: Do some color management on images with an ICC profile This patch introduces the `Gfx::ColorSpace` class, this is basically a serializable wrapper for skia's SkColorSpace. Creation of the instances of this class (and thus ICC profiles parsing) is performed in the ImageDecoder process. Then the object is serialized and sent through IPC, to finally be handed to skia for rendering. However, to make sure that we're not making all LibGfx's users dependent on Skia as well, we need to ensure the `Gfx::ColorSpace` object has no dependency on objects from Skia. To that end, the only member of the `ColorSpace` class is the opaque `ColorSpaceImpl` struct. Though, there is on issue with that design, the code in `DisplayListPlayer.cpp` needs access to the underlying `sk_sp`. It is provided by a template function, that is only specialized for this type. Doing this work allows us to pass the following WPT tests: - https://wpt.live/css/css-color/tagged-images-001.html - https://wpt.live/css/css-color/tagged-images-003.html - https://wpt.live/css/css-color/tagged-images-004.html - https://wpt.live/css/css-color/untagged-images-001.html Other test cases can also be found here: - https://github.com/svgeesus/PNG-ICC-tests Note that SkColorSpace support quite a limited amount of color spaces, so color profiles like the ones in [1] or the v4 profiles in [2] are not supported yet. In fact, SkColorSpace only accepts skcms_ICCProfile with a linear conversion to XYZ D50. [1] https://www.color.org/browsertest.xalter [2] https://www.color.org/version4html.xalter --- Libraries/LibGfx/CMakeLists.txt | 1 + Libraries/LibGfx/ColorSpace.cpp | 97 +++++++++++++++++++ Libraries/LibGfx/ColorSpace.h | 60 ++++++++++++ .../LibGfx/ImageFormats/ImageDecoder.cpp | 8 ++ Libraries/LibGfx/ImageFormats/ImageDecoder.h | 2 + Libraries/LibGfx/ImmutableBitmap.cpp | 7 +- Libraries/LibGfx/ImmutableBitmap.h | 3 +- Libraries/LibImageDecoderClient/Client.cpp | 3 +- Libraries/LibImageDecoderClient/Client.h | 4 +- .../LibWeb/HTML/SharedResourceRequest.cpp | 2 +- Libraries/LibWeb/Platform/ImageCodecPlugin.h | 2 + .../LibWebView/Plugins/ImageCodecPlugin.cpp | 1 + .../ImageDecoder/ConnectionFromClient.cpp | 5 +- Services/ImageDecoder/ConnectionFromClient.h | 2 + Services/ImageDecoder/ImageDecoderClient.ipc | 3 +- 15 files changed, 192 insertions(+), 8 deletions(-) create mode 100644 Libraries/LibGfx/ColorSpace.cpp create mode 100644 Libraries/LibGfx/ColorSpace.h diff --git a/Libraries/LibGfx/CMakeLists.txt b/Libraries/LibGfx/CMakeLists.txt index e0460f56490..c88d0ace897 100644 --- a/Libraries/LibGfx/CMakeLists.txt +++ b/Libraries/LibGfx/CMakeLists.txt @@ -7,6 +7,7 @@ set(SOURCES BitmapSequence.cpp CMYKBitmap.cpp Color.cpp + ColorSpace.cpp DeltaE.cpp FontCascadeList.cpp Font/Font.cpp diff --git a/Libraries/LibGfx/ColorSpace.cpp b/Libraries/LibGfx/ColorSpace.cpp new file mode 100644 index 00000000000..88ef5fa22f5 --- /dev/null +++ b/Libraries/LibGfx/ColorSpace.cpp @@ -0,0 +1,97 @@ +/* + * Copyright (c) 2024, Lucas Chollet + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include +#include +#include + +#include +#include + +namespace Gfx { + +namespace Details { + +struct ColorSpaceImpl { + sk_sp color_space; +}; + +} + +ColorSpace::ColorSpace() + : m_color_space(make()) +{ +} + +ColorSpace::ColorSpace(ColorSpace const& other) + : m_color_space(make(other.m_color_space->color_space)) +{ +} + +ColorSpace& ColorSpace::operator=(ColorSpace const& other) +{ + m_color_space = make(other.m_color_space->color_space); + return *this; +} + +ColorSpace::ColorSpace(ColorSpace&& other) = default; +ColorSpace& ColorSpace::operator=(ColorSpace&&) = default; +ColorSpace::~ColorSpace() = default; + +ColorSpace::ColorSpace(NonnullOwnPtr&& color_space) + : m_color_space(move(color_space)) +{ +} + +ErrorOr ColorSpace::load_from_icc_bytes(ReadonlyBytes icc_bytes) +{ + if (icc_bytes.size() != 0) { + skcms_ICCProfile icc_profile {}; + if (!skcms_Parse(icc_bytes.data(), icc_bytes.size(), &icc_profile)) + return Error::from_string_literal("Failed to parse the ICC profile"); + + return ColorSpace { make(SkColorSpace::Make(icc_profile)) }; + } + return ColorSpace {}; +} + +template<> +sk_sp& ColorSpace::color_space() +{ + return m_color_space->color_space; +} + +} + +namespace IPC { +template<> +ErrorOr encode(Encoder& encoder, Gfx::ColorSpace const& color_space) +{ + if (!color_space.m_color_space->color_space) { + TRY(encoder.encode(0)); + return {}; + } + auto serialized = color_space.m_color_space->color_space->serialize(); + TRY(encoder.encode(serialized->size())); + TRY(encoder.append(serialized->bytes(), serialized->size())); + return {}; +} + +template<> +ErrorOr decode(Decoder& decoder) +{ + auto size = TRY(decoder.decode()); + if (size == 0) + return Gfx::ColorSpace {}; + + auto buffer = TRY(ByteBuffer::create_uninitialized(size)); + TRY(decoder.decode_into(buffer.bytes())); + + auto color_space = SkColorSpace::Deserialize(buffer.data(), buffer.size()); + return Gfx::ColorSpace { make<::Gfx::Details::ColorSpaceImpl>(move(color_space)) }; +} +} diff --git a/Libraries/LibGfx/ColorSpace.h b/Libraries/LibGfx/ColorSpace.h new file mode 100644 index 00000000000..9ef3bb06270 --- /dev/null +++ b/Libraries/LibGfx/ColorSpace.h @@ -0,0 +1,60 @@ +/* + * Copyright (c) 2024, Lucas Chollet + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#pragma once + +#include +#include +#include +#include + +namespace Gfx { + +namespace Details { + +struct ColorSpaceImpl; + +} + +class ColorSpace { +public: + ColorSpace(); + ColorSpace(ColorSpace const&); + ColorSpace(ColorSpace&&); + ColorSpace& operator=(ColorSpace const&); + ColorSpace& operator=(ColorSpace&&); + ~ColorSpace(); + + static ErrorOr load_from_icc_bytes(ReadonlyBytes); + + // In order to keep this file free of Skia types, this function can't return + // a sk_sp. To work around that issue, we define a template here + // and only provide a specialization for sk_sp. + template + T& color_space(); + +private: + template + friend ErrorOr IPC::encode(IPC::Encoder&, T const&); + template + friend ErrorOr IPC::decode(IPC::Decoder&); + + explicit ColorSpace(NonnullOwnPtr&& color_pace); + + NonnullOwnPtr m_color_space; +}; + +} + +namespace IPC { + +template<> +ErrorOr encode(Encoder&, Gfx::ColorSpace const&); + +template<> +ErrorOr decode(Decoder&); + +} diff --git a/Libraries/LibGfx/ImageFormats/ImageDecoder.cpp b/Libraries/LibGfx/ImageFormats/ImageDecoder.cpp index 8d95998c073..5981f998d4e 100644 --- a/Libraries/LibGfx/ImageFormats/ImageDecoder.cpp +++ b/Libraries/LibGfx/ImageFormats/ImageDecoder.cpp @@ -47,6 +47,14 @@ static ErrorOr> probe_and_sniff_for_appropriate_plugi return OwnPtr {}; } +ErrorOr ImageDecoder::color_space() +{ + auto maybe_icc_data = TRY(icc_data()); + if (!maybe_icc_data.has_value()) + return ColorSpace {}; + return ColorSpace::load_from_icc_bytes(maybe_icc_data.value()); +} + ErrorOr> ImageDecoder::try_create_for_raw_bytes(ReadonlyBytes bytes, [[maybe_unused]] Optional mime_type) { if (auto plugin = TRY(probe_and_sniff_for_appropriate_plugin(bytes)); plugin) diff --git a/Libraries/LibGfx/ImageFormats/ImageDecoder.h b/Libraries/LibGfx/ImageFormats/ImageDecoder.h index 3d1410e8d47..af7a0cc9070 100644 --- a/Libraries/LibGfx/ImageFormats/ImageDecoder.h +++ b/Libraries/LibGfx/ImageFormats/ImageDecoder.h @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -111,6 +112,7 @@ public: ErrorOr frame(size_t index, Optional ideal_size = {}) const { return m_plugin->frame(index, ideal_size); } Optional metadata() const { return m_plugin->metadata(); } + ErrorOr color_space(); ErrorOr> icc_data() const { return m_plugin->icc_data(); } NaturalFrameFormat natural_frame_format() { return m_plugin->natural_frame_format(); } diff --git a/Libraries/LibGfx/ImmutableBitmap.cpp b/Libraries/LibGfx/ImmutableBitmap.cpp index 37cce11f740..720b411abcf 100644 --- a/Libraries/LibGfx/ImmutableBitmap.cpp +++ b/Libraries/LibGfx/ImmutableBitmap.cpp @@ -9,6 +9,7 @@ #include #include +#include #include namespace Gfx { @@ -17,6 +18,7 @@ struct ImmutableBitmapImpl { sk_sp sk_image; SkBitmap sk_bitmap; Variant, NonnullRefPtr, Empty> source; + ColorSpace color_space; }; int ImmutableBitmap::width() const @@ -73,14 +75,15 @@ static SkAlphaType to_skia_alpha_type(Gfx::AlphaType alpha_type) } } -NonnullRefPtr ImmutableBitmap::create(NonnullRefPtr bitmap) +NonnullRefPtr ImmutableBitmap::create(NonnullRefPtr bitmap, ColorSpace color_space) { ImmutableBitmapImpl impl; - auto info = SkImageInfo::Make(bitmap->width(), bitmap->height(), to_skia_color_type(bitmap->format()), to_skia_alpha_type(bitmap->alpha_type())); + auto info = SkImageInfo::Make(bitmap->width(), bitmap->height(), to_skia_color_type(bitmap->format()), to_skia_alpha_type(bitmap->alpha_type()), color_space.color_space>()); impl.sk_bitmap.installPixels(info, const_cast(static_cast(bitmap->scanline(0))), bitmap->pitch()); impl.sk_bitmap.setImmutable(); impl.sk_image = impl.sk_bitmap.asImage(); impl.source = bitmap; + impl.color_space = move(color_space); return adopt_ref(*new ImmutableBitmap(make(impl))); } diff --git a/Libraries/LibGfx/ImmutableBitmap.h b/Libraries/LibGfx/ImmutableBitmap.h index e5eca00f760..d1565ba80c5 100644 --- a/Libraries/LibGfx/ImmutableBitmap.h +++ b/Libraries/LibGfx/ImmutableBitmap.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -21,7 +22,7 @@ struct ImmutableBitmapImpl; class ImmutableBitmap final : public RefCounted { public: - static NonnullRefPtr create(NonnullRefPtr bitmap); + static NonnullRefPtr create(NonnullRefPtr bitmap, ColorSpace color_space = {}); static NonnullRefPtr create_snapshot_from_painting_surface(NonnullRefPtr); ~ImmutableBitmap(); diff --git a/Libraries/LibImageDecoderClient/Client.cpp b/Libraries/LibImageDecoderClient/Client.cpp index 28ccbab3dc4..d71a4887b14 100644 --- a/Libraries/LibImageDecoderClient/Client.cpp +++ b/Libraries/LibImageDecoderClient/Client.cpp @@ -57,7 +57,7 @@ NonnullRefPtr> Client::decode_image(ReadonlyBytes en return promise; } -void Client::did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Gfx::BitmapSequence const& bitmap_sequence, Vector const& durations, Gfx::FloatPoint scale) +void Client::did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Gfx::BitmapSequence const& bitmap_sequence, Vector const& durations, Gfx::FloatPoint scale, Gfx::ColorSpace const& color_space) { auto const& bitmaps = bitmap_sequence.bitmaps; VERIFY(!bitmaps.is_empty()); @@ -74,6 +74,7 @@ void Client::did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Gf image.loop_count = loop_count; image.scale = scale; image.frames.ensure_capacity(bitmaps.size()); + image.color_space = move(color_space); for (size_t i = 0; i < bitmaps.size(); ++i) { if (!bitmaps[i].has_value()) { dbgln("ImageDecoderClient: Invalid bitmap for request {} at index {}", image_id, i); diff --git a/Libraries/LibImageDecoderClient/Client.h b/Libraries/LibImageDecoderClient/Client.h index 5af4811165d..49da13b942b 100644 --- a/Libraries/LibImageDecoderClient/Client.h +++ b/Libraries/LibImageDecoderClient/Client.h @@ -10,6 +10,7 @@ #include #include #include +#include #include namespace ImageDecoderClient { @@ -24,6 +25,7 @@ struct DecodedImage { Gfx::FloatPoint scale { 1, 1 }; u32 loop_count { 0 }; Vector frames; + Gfx::ColorSpace color_space; }; class Client final @@ -41,7 +43,7 @@ public: private: virtual void die() override; - virtual void did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Gfx::BitmapSequence const& bitmap_sequence, Vector const& durations, Gfx::FloatPoint scale) override; + virtual void did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Gfx::BitmapSequence const& bitmap_sequence, Vector const& durations, Gfx::FloatPoint scale, Gfx::ColorSpace const& color_profile) override; virtual void did_fail_to_decode_image(i64 image_id, String const& error_message) override; HashMap>> m_pending_decoded_images; diff --git a/Libraries/LibWeb/HTML/SharedResourceRequest.cpp b/Libraries/LibWeb/HTML/SharedResourceRequest.cpp index 208c1396131..803de936c80 100644 --- a/Libraries/LibWeb/HTML/SharedResourceRequest.cpp +++ b/Libraries/LibWeb/HTML/SharedResourceRequest.cpp @@ -161,7 +161,7 @@ void SharedResourceRequest::handle_successful_fetch(URL::URL const& url_string, Vector frames; for (auto& frame : result.frames) { frames.append(AnimatedBitmapDecodedImageData::Frame { - .bitmap = Gfx::ImmutableBitmap::create(*frame.bitmap), + .bitmap = Gfx::ImmutableBitmap::create(*frame.bitmap, move(result.color_space)), .duration = static_cast(frame.duration), }); } diff --git a/Libraries/LibWeb/Platform/ImageCodecPlugin.h b/Libraries/LibWeb/Platform/ImageCodecPlugin.h index 9709c172089..9cd680b6ec4 100644 --- a/Libraries/LibWeb/Platform/ImageCodecPlugin.h +++ b/Libraries/LibWeb/Platform/ImageCodecPlugin.h @@ -10,6 +10,7 @@ #include #include #include +#include #include namespace Web::Platform { @@ -23,6 +24,7 @@ struct DecodedImage { bool is_animated { false }; u32 loop_count { 0 }; Vector frames; + Gfx::ColorSpace color_space; }; class ImageCodecPlugin { diff --git a/Libraries/LibWebView/Plugins/ImageCodecPlugin.cpp b/Libraries/LibWebView/Plugins/ImageCodecPlugin.cpp index 90f79b962af..4ad56fd1965 100644 --- a/Libraries/LibWebView/Plugins/ImageCodecPlugin.cpp +++ b/Libraries/LibWebView/Plugins/ImageCodecPlugin.cpp @@ -54,6 +54,7 @@ NonnullRefPtr> ImageCodecPlugin::deco for (auto& frame : result.frames) { decoded_image.frames.empend(move(frame.bitmap), frame.duration); } + decoded_image.color_space = move(result.color_space); promise->resolve(move(decoded_image)); return {}; }, diff --git a/Services/ImageDecoder/ConnectionFromClient.cpp b/Services/ImageDecoder/ConnectionFromClient.cpp index 164dc09ab6d..67e4b600f9d 100644 --- a/Services/ImageDecoder/ConnectionFromClient.cpp +++ b/Services/ImageDecoder/ConnectionFromClient.cpp @@ -104,6 +104,9 @@ static ErrorOr decode_image_to_details(Core: result.is_animated = decoder->is_animated(); result.loop_count = decoder->loop_count(); + if (auto maybe_icc_data = decoder->color_space(); !maybe_icc_data.is_error()) + result.color_profile = maybe_icc_data.value(); + Vector>> bitmaps; if (auto maybe_metadata = decoder->metadata(); maybe_metadata.has_value() && is(*maybe_metadata)) { @@ -135,7 +138,7 @@ NonnullRefPtr ConnectionFromClient::make_decode_image return TRY(decode_image_to_details(encoded_buffer, ideal_size, mime_type)); }, [strong_this = NonnullRefPtr(*this), image_id](DecodeResult result) -> ErrorOr { - strong_this->async_did_decode_image(image_id, result.is_animated, result.loop_count, result.bitmaps, result.durations, result.scale); + strong_this->async_did_decode_image(image_id, result.is_animated, result.loop_count, result.bitmaps, result.durations, result.scale, result.color_profile); strong_this->m_pending_jobs.remove(image_id); return {}; }, diff --git a/Services/ImageDecoder/ConnectionFromClient.h b/Services/ImageDecoder/ConnectionFromClient.h index 380a995363f..3dea9b9e7ce 100644 --- a/Services/ImageDecoder/ConnectionFromClient.h +++ b/Services/ImageDecoder/ConnectionFromClient.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -31,6 +32,7 @@ public: Gfx::FloatPoint scale { 1, 1 }; Gfx::BitmapSequence bitmaps; Vector durations; + Gfx::ColorSpace color_profile; }; private: diff --git a/Services/ImageDecoder/ImageDecoderClient.ipc b/Services/ImageDecoder/ImageDecoderClient.ipc index 1177d59fddb..601cb34ce91 100644 --- a/Services/ImageDecoder/ImageDecoderClient.ipc +++ b/Services/ImageDecoder/ImageDecoderClient.ipc @@ -1,7 +1,8 @@ #include +#include endpoint ImageDecoderClient { - did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Gfx::BitmapSequence bitmaps, Vector durations, Gfx::FloatPoint scale) =| + did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Gfx::BitmapSequence bitmaps, Vector durations, Gfx::FloatPoint scale, Gfx::ColorSpace color_profile) =| did_fail_to_decode_image(i64 image_id, String error_message) =| }