From d5e8bdb7daeb4ec8c50a19ca509bfa29f7d6f00a 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 | 129 ++++++++++++++++++ Userland/Libraries/LibGfx/BitmapSequence.h | 40 ++++++ Userland/Libraries/LibGfx/CMakeLists.txt | 1 + .../LibImageDecoderClient/Client.cpp | 4 +- .../Libraries/LibImageDecoderClient/Client.h | 2 +- .../ImageDecoder/ConnectionFromClient.cpp | 8 +- .../ImageDecoder/ConnectionFromClient.h | 3 +- .../ImageDecoder/ImageDecoderClient.ipc | 4 +- 9 files changed, 185 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 57ea6d761215..d37879f55e2f 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 000000000000..2a0b5f9d843e --- /dev/null +++ b/Userland/Libraries/LibGfx/BitmapSequence.cpp @@ -0,0 +1,129 @@ +/* + * Copyright (c) 2024, Zachary Huang + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include +#include +#include +#include +#include +#include + +namespace Gfx { + +BitmapSequence::BitmapSequence(Vector>> bitmaps) + : m_bitmaps(move(bitmaps)) +{ +} + +} + +namespace IPC { + +template<> +ErrorOr encode(Encoder& encoder, Gfx::BitmapSequence const& bitmap_sequence) +{ + Vector>> bitmaps = bitmap_sequence.bitmaps(); + + TRY(encoder.encode_size(bitmaps.size())); + + size_t total_buffer_size = 0; + + for (auto const& bitmap_option : bitmaps) { + TRY(encoder.encode(bitmap_option.has_value())); + + if (bitmap_option.has_value()) { + auto bitmap = bitmap_option.value(); + TRY(encoder.encode(static_cast(bitmap->format()))); + TRY(encoder.encode(static_cast(bitmap->alpha_type()))); + TRY(encoder.encode(bitmap->size_in_bytes())); + TRY(encoder.encode(bitmap->size())); + + total_buffer_size += bitmap_option.value()->size_in_bytes(); + } + } + + // 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 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) +{ + // 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; + }; + + Vector> bitmaps_metadata; + + size_t num_bitmaps = TRY(decoder.decode_size()); + + for (size_t i = 0; i < num_bitmaps; i++) { + auto valid = TRY(decoder.decode()); + + if (valid) { + 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()); + + bitmaps_metadata.append(BitmapMetadata { format, alpha_type, size, size_in_bytes }); + } else { + bitmaps_metadata.append({}); + } + } + + auto collated_buffer = TRY(decoder.decode()); + auto* read_pointer = collated_buffer.data(); + + Vector>> bitmaps; + + // sequentially read each valid bitmap's data from the collated buffer + for (auto metadata_option : bitmaps_metadata) { + if (metadata_option.has_value()) { + auto metadata = metadata_option.value(); + size_t size_in_bytes = metadata.size_in_bytes; + + auto buffer = TRY(Core::AnonymousBuffer::create_with_size(size_in_bytes)); + memcpy(buffer.data(), read_pointer, size_in_bytes); + read_pointer += size_in_bytes; + + auto bitmap = TRY(Gfx::Bitmap::create_with_anonymous_buffer(metadata.format, metadata.alpha_type, move(buffer), metadata.size)); + bitmaps.append(bitmap); + } else { + bitmaps.append({}); + } + } + + return Gfx::BitmapSequence { bitmaps }; +} + +} diff --git a/Userland/Libraries/LibGfx/BitmapSequence.h b/Userland/Libraries/LibGfx/BitmapSequence.h new file mode 100644 index 000000000000..20cb3775d510 --- /dev/null +++ b/Userland/Libraries/LibGfx/BitmapSequence.h @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2024, Zachary Huang + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#pragma once + +#include +#include +#include +#include + +namespace Gfx { + +class BitmapSequence { +public: + BitmapSequence() = default; + + BitmapSequence(Vector>>); + + Vector>> bitmaps() const { return m_bitmaps; } + +private: + // friend class Bitmap; + + Vector>> m_bitmaps; +}; + +} + +namespace IPC { + +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 91d62b5ac4bc..a847fd1a1088 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 2550aedddf6f..a307f4a4ab06 100644 --- a/Userland/Libraries/LibImageDecoderClient/Client.cpp +++ b/Userland/Libraries/LibImageDecoderClient/Client.cpp @@ -57,8 +57,10 @@ 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 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 a9b3cb2995dc..d40ac4aea073 100644 --- a/Userland/Libraries/LibImageDecoderClient/Client.h +++ b/Userland/Libraries/LibImageDecoderClient/Client.h @@ -41,7 +41,7 @@ class Client final 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 94604ca2e3c8..40d2a6acf739 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 fbc6f1933ecc..ed3e24c90934 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 @@ class ConnectionFromClient final 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 fe4b57996823..1177d59fddb5 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) =| }