From e0bd42be9590b967d0b5788ce7537a861a35ba74 Mon Sep 17 00:00:00 2001 From: Zachary Huang <55601738+zack466@users.noreply.github.com> Date: Wed, 18 Sep 2024 17:37:44 -0400 Subject: [PATCH] ImageDecoder+LibGfx: Collate decoded bitmaps before sending over IPC There is an issue where gifs with many frames cannot be loaded, as each bitmap is sent over IPC using a separate file descriptor, and there is limit on the maximum number of descriptors per IPC message. Thus, trying to load gifs with more than 64 frames (the current limit) causes the image decoder process to die. This commit introduces the BitmapSequence class, which is a thin wrapper around the type Vector>> and provides an IPC encode/decode routine that collates all bitmap data into a single buffer so that only a single file descriptor is required per IPC transfer, even if multiple frames are being sent. --- .../Userland/Libraries/LibGfx/BUILD.gn | 1 + Userland/Libraries/LibGfx/BitmapSequence.cpp | 134 ++++++++++++++++++ Userland/Libraries/LibGfx/BitmapSequence.h | 44 ++++++ Userland/Libraries/LibGfx/CMakeLists.txt | 1 + .../LibImageDecoderClient/Client.cpp | 3 +- .../Libraries/LibImageDecoderClient/Client.h | 2 +- .../ImageDecoder/ConnectionFromClient.cpp | 8 +- .../ImageDecoder/ConnectionFromClient.h | 3 +- .../ImageDecoder/ImageDecoderClient.ipc | 4 +- 9 files changed, 193 insertions(+), 7 deletions(-) create mode 100644 Userland/Libraries/LibGfx/BitmapSequence.cpp create mode 100644 Userland/Libraries/LibGfx/BitmapSequence.h diff --git a/Meta/gn/secondary/Userland/Libraries/LibGfx/BUILD.gn b/Meta/gn/secondary/Userland/Libraries/LibGfx/BUILD.gn index 934db9954bf..1124fc23af7 100644 --- a/Meta/gn/secondary/Userland/Libraries/LibGfx/BUILD.gn +++ b/Meta/gn/secondary/Userland/Libraries/LibGfx/BUILD.gn @@ -27,6 +27,7 @@ shared_library("LibGfx") { "AffineTransform.cpp", "AntiAliasingPainter.cpp", "Bitmap.cpp", + "BitmapSequence.cpp", "CMYKBitmap.cpp", "Color.cpp", "DeltaE.cpp", diff --git a/Userland/Libraries/LibGfx/BitmapSequence.cpp b/Userland/Libraries/LibGfx/BitmapSequence.cpp new file mode 100644 index 00000000000..ce0593ac1fd --- /dev/null +++ b/Userland/Libraries/LibGfx/BitmapSequence.cpp @@ -0,0 +1,134 @@ +/* + * Copyright (c) 2024, Zachary Huang + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +namespace Gfx { + +static BitmapMetadata get_metadata(Bitmap const& bitmap) +{ + return BitmapMetadata { .format = bitmap.format(), .alpha_type = bitmap.alpha_type(), .size = bitmap.size(), .size_in_bytes = bitmap.size_in_bytes() }; +} + +} + +namespace IPC { + +template<> +ErrorOr encode(Encoder& encoder, Gfx::BitmapMetadata const& metadata) +{ + TRY(encoder.encode(static_cast(metadata.format))); + TRY(encoder.encode(static_cast(metadata.alpha_type))); + TRY(encoder.encode(metadata.size_in_bytes)); + TRY(encoder.encode(metadata.size)); + + return {}; +} + +template<> +ErrorOr decode(Decoder& decoder) +{ + auto raw_bitmap_format = TRY(decoder.decode()); + if (!Gfx::is_valid_bitmap_format(raw_bitmap_format)) + return Error::from_string_literal("IPC: Invalid Gfx::BitmapSequence format"); + auto format = static_cast(raw_bitmap_format); + + auto raw_alpha_type = TRY(decoder.decode()); + if (!Gfx::is_valid_alpha_type(raw_alpha_type)) + return Error::from_string_literal("IPC: Invalid Gfx::BitmapSequence alpha type"); + auto alpha_type = static_cast(raw_alpha_type); + + auto size_in_bytes = TRY(decoder.decode()); + auto size = TRY(decoder.decode()); + + return Gfx::BitmapMetadata { format, alpha_type, size, size_in_bytes }; +} + +template<> +ErrorOr encode(Encoder& encoder, Gfx::BitmapSequence const& bitmap_sequence) +{ + auto const& bitmaps = bitmap_sequence.bitmaps; + + Vector> metadata; + metadata.ensure_capacity(bitmaps.size()); + + size_t total_buffer_size = 0; + + for (auto const& bitmap_option : bitmaps) { + Optional data = {}; + + if (bitmap_option.has_value()) { + data = get_metadata(bitmap_option.value()); + total_buffer_size += data->size_in_bytes; + } + + metadata.unchecked_append(data); + } + + TRY(encoder.encode(metadata)); + + // collate all of the bitmap data into one contiguous buffer + auto collated_buffer = TRY(Core::AnonymousBuffer::create_with_size(total_buffer_size)); + + auto* write_pointer = collated_buffer.data(); + for (auto const& bitmap_option : bitmaps) { + if (bitmap_option.has_value()) { + auto const& bitmap = bitmap_option.value(); + memcpy(write_pointer, bitmap->scanline(0), bitmap->size_in_bytes()); + write_pointer += bitmap->size_in_bytes(); + } + } + + TRY(encoder.encode(collated_buffer)); + + return {}; +} + +template<> +ErrorOr decode(Decoder& decoder) +{ + auto metadata = TRY(decoder.decode>>()); + auto collated_buffer = TRY(decoder.decode()); + + Vector>> bitmaps; + bitmaps.ensure_capacity(metadata.size()); + + ReadonlyBytes bytes = ReadonlyBytes(collated_buffer.data(), collated_buffer.size()); + size_t bytes_read = 0; + + // sequentially read each valid bitmap's data from the collated buffer + for (auto const& metadata_option : metadata) { + Optional> bitmap = {}; + + if (metadata_option.has_value()) { + auto metadata = metadata_option.value(); + size_t size_in_bytes = metadata.size_in_bytes; + + if (bytes_read + size_in_bytes > bytes.size()) + return Error::from_string_literal("IPC: Invalid Gfx::BitmapSequence buffer data"); + + auto buffer = TRY(Core::AnonymousBuffer::create_with_size(size_in_bytes)); + + memcpy(buffer.data(), bytes.slice(bytes_read, size_in_bytes).data(), size_in_bytes); + bytes_read += size_in_bytes; + + bitmap = TRY(Gfx::Bitmap::create_with_anonymous_buffer(metadata.format, metadata.alpha_type, move(buffer), metadata.size)); + } + + bitmaps.append(bitmap); + } + + return Gfx::BitmapSequence { bitmaps }; +} + +} diff --git a/Userland/Libraries/LibGfx/BitmapSequence.h b/Userland/Libraries/LibGfx/BitmapSequence.h new file mode 100644 index 00000000000..ae440766b2f --- /dev/null +++ b/Userland/Libraries/LibGfx/BitmapSequence.h @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2024, Zachary Huang + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#pragma once + +#include +#include +#include +#include + +namespace Gfx { + +struct BitmapSequence { + Vector>> bitmaps; +}; + +// a struct to temporarily store bitmap fields before the buffer data is decoded +struct BitmapMetadata { + Gfx::BitmapFormat format; + Gfx::AlphaType alpha_type; + Gfx::IntSize size; + size_t size_in_bytes; +}; + +} + +namespace IPC { + +template<> +ErrorOr encode(Encoder&, Gfx::BitmapMetadata const&); + +template<> +ErrorOr decode(Decoder&); + +template<> +ErrorOr encode(Encoder&, Gfx::BitmapSequence const&); + +template<> +ErrorOr decode(Decoder&); + +} diff --git a/Userland/Libraries/LibGfx/CMakeLists.txt b/Userland/Libraries/LibGfx/CMakeLists.txt index 91d62b5ac4b..a847fd1a108 100644 --- a/Userland/Libraries/LibGfx/CMakeLists.txt +++ b/Userland/Libraries/LibGfx/CMakeLists.txt @@ -4,6 +4,7 @@ set(SOURCES AffineTransform.cpp AntiAliasingPainter.cpp Bitmap.cpp + BitmapSequence.cpp CMYKBitmap.cpp Color.cpp DeltaE.cpp diff --git a/Userland/Libraries/LibImageDecoderClient/Client.cpp b/Userland/Libraries/LibImageDecoderClient/Client.cpp index 2550aedddf6..95f579fd950 100644 --- a/Userland/Libraries/LibImageDecoderClient/Client.cpp +++ b/Userland/Libraries/LibImageDecoderClient/Client.cpp @@ -57,8 +57,9 @@ NonnullRefPtr> Client::decode_image(ReadonlyBytes en return promise; } -void Client::did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Vector>> const& bitmaps, 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) { + auto const& bitmaps = bitmap_sequence.bitmaps; VERIFY(!bitmaps.is_empty()); auto maybe_promise = m_pending_decoded_images.take(image_id); diff --git a/Userland/Libraries/LibImageDecoderClient/Client.h b/Userland/Libraries/LibImageDecoderClient/Client.h index a9b3cb2995d..d40ac4aea07 100644 --- a/Userland/Libraries/LibImageDecoderClient/Client.h +++ b/Userland/Libraries/LibImageDecoderClient/Client.h @@ -41,7 +41,7 @@ public: private: virtual void die() override; - virtual void did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Vector>> const& bitmaps, 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) override; virtual void did_fail_to_decode_image(i64 image_id, String const& error_message) override; HashMap>> m_pending_decoded_images; diff --git a/Userland/Services/ImageDecoder/ConnectionFromClient.cpp b/Userland/Services/ImageDecoder/ConnectionFromClient.cpp index 94604ca2e3c..40d2a6acf73 100644 --- a/Userland/Services/ImageDecoder/ConnectionFromClient.cpp +++ b/Userland/Services/ImageDecoder/ConnectionFromClient.cpp @@ -104,6 +104,8 @@ static ErrorOr decode_image_to_details(Core: result.is_animated = decoder->is_animated(); result.loop_count = decoder->loop_count(); + Vector>> bitmaps; + if (auto maybe_metadata = decoder->metadata(); maybe_metadata.has_value() && is(*maybe_metadata)) { auto const& exif = static_cast(maybe_metadata.value()); if (exif.x_resolution().has_value() && exif.y_resolution().has_value()) { @@ -116,11 +118,13 @@ static ErrorOr decode_image_to_details(Core: } } - decode_image_to_bitmaps_and_durations_with_decoder(*decoder, move(ideal_size), result.bitmaps, result.durations); + decode_image_to_bitmaps_and_durations_with_decoder(*decoder, move(ideal_size), bitmaps, result.durations); - if (result.bitmaps.is_empty()) + if (bitmaps.is_empty()) return Error::from_string_literal("Could not decode image"); + result.bitmaps = Gfx::BitmapSequence { bitmaps }; + return result; } diff --git a/Userland/Services/ImageDecoder/ConnectionFromClient.h b/Userland/Services/ImageDecoder/ConnectionFromClient.h index fbc6f1933ec..ed3e24c9093 100644 --- a/Userland/Services/ImageDecoder/ConnectionFromClient.h +++ b/Userland/Services/ImageDecoder/ConnectionFromClient.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -28,7 +29,7 @@ public: bool is_animated = false; u32 loop_count = 0; Gfx::FloatPoint scale { 1, 1 }; - Vector>> bitmaps; + Gfx::BitmapSequence bitmaps; Vector durations; }; diff --git a/Userland/Services/ImageDecoder/ImageDecoderClient.ipc b/Userland/Services/ImageDecoder/ImageDecoderClient.ipc index fe4b5799682..1177d59fddb 100644 --- a/Userland/Services/ImageDecoder/ImageDecoderClient.ipc +++ b/Userland/Services/ImageDecoder/ImageDecoderClient.ipc @@ -1,7 +1,7 @@ -#include +#include endpoint ImageDecoderClient { - did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Vector>> 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) =| did_fail_to_decode_image(i64 image_id, String error_message) =| }