From 166e603c5eb0a103eea148baf97a075fe5fea964 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Wed, 17 Jul 2024 19:14:44 +0200 Subject: [PATCH] ImageDecoder: Pass decoded images as Gfx::Bitmap over IPC Before this change, we were passing them as Gfx::ShareableBitmap. The problem is that shareable bitmaps keep their underlying file descriptor open, so that they can be shared again with someone else. When a Gfx::Bitmap is decoded from an IPC message, the file descriptor is closed and recovered immediately. This fixes an issue where we'd accumulate one file descriptor for every image decoded. This eventually led to descriptor starvation after enough images were loaded and still referenced at the same time. --- Userland/Libraries/LibImageDecoderClient/Client.cpp | 6 +++--- Userland/Libraries/LibImageDecoderClient/Client.h | 2 +- Userland/Services/ImageDecoder/ConnectionFromClient.cpp | 6 +++--- Userland/Services/ImageDecoder/ConnectionFromClient.h | 2 +- Userland/Services/ImageDecoder/ImageDecoderClient.ipc | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Userland/Libraries/LibImageDecoderClient/Client.cpp b/Userland/Libraries/LibImageDecoderClient/Client.cpp index f9f2e23d9d..2550aedddf 100644 --- a/Userland/Libraries/LibImageDecoderClient/Client.cpp +++ b/Userland/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, Vector const& bitmaps, Vector const& durations, Gfx::FloatPoint scale) +void Client::did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Vector>> const& bitmaps, Vector const& durations, Gfx::FloatPoint scale) { VERIFY(!bitmaps.is_empty()); @@ -74,13 +74,13 @@ void Client::did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Ve image.scale = scale; image.frames.ensure_capacity(bitmaps.size()); for (size_t i = 0; i < bitmaps.size(); ++i) { - if (!bitmaps[i].is_valid()) { + if (!bitmaps[i].has_value()) { dbgln("ImageDecoderClient: Invalid bitmap for request {} at index {}", image_id, i); promise->reject(Error::from_string_literal("Invalid bitmap")); return; } - image.frames.empend(*bitmaps[i].bitmap(), durations[i]); + image.frames.empend(*bitmaps[i], durations[i]); } promise->resolve(move(image)); diff --git a/Userland/Libraries/LibImageDecoderClient/Client.h b/Userland/Libraries/LibImageDecoderClient/Client.h index f4c8ab42fb..a9b3cb2995 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, Vector>> const& bitmaps, 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 f51ee99e62..94604ca2e3 100644 --- a/Userland/Services/ImageDecoder/ConnectionFromClient.cpp +++ b/Userland/Services/ImageDecoder/ConnectionFromClient.cpp @@ -75,16 +75,16 @@ Messages::ImageDecoderServer::ConnectNewClientsResponse ConnectionFromClient::co return files; } -static void decode_image_to_bitmaps_and_durations_with_decoder(Gfx::ImageDecoder const& decoder, Optional ideal_size, Vector& bitmaps, Vector& durations) +static void decode_image_to_bitmaps_and_durations_with_decoder(Gfx::ImageDecoder const& decoder, Optional ideal_size, Vector>>& bitmaps, Vector& durations) { for (size_t i = 0; i < decoder.frame_count(); ++i) { auto frame_or_error = decoder.frame(i, ideal_size); if (frame_or_error.is_error()) { - bitmaps.append(Gfx::ShareableBitmap {}); + bitmaps.append({}); durations.append(0); } else { auto frame = frame_or_error.release_value(); - bitmaps.append(frame.image->to_shareable_bitmap()); + bitmaps.append(frame.image.release_nonnull()); durations.append(frame.duration); } } diff --git a/Userland/Services/ImageDecoder/ConnectionFromClient.h b/Userland/Services/ImageDecoder/ConnectionFromClient.h index baacecd877..fbc6f1933e 100644 --- a/Userland/Services/ImageDecoder/ConnectionFromClient.h +++ b/Userland/Services/ImageDecoder/ConnectionFromClient.h @@ -28,7 +28,7 @@ public: bool is_animated = false; u32 loop_count = 0; Gfx::FloatPoint scale { 1, 1 }; - Vector bitmaps; + Vector>> bitmaps; Vector durations; }; diff --git a/Userland/Services/ImageDecoder/ImageDecoderClient.ipc b/Userland/Services/ImageDecoder/ImageDecoderClient.ipc index f47c27cb54..fe4b579968 100644 --- a/Userland/Services/ImageDecoder/ImageDecoderClient.ipc +++ b/Userland/Services/ImageDecoder/ImageDecoderClient.ipc @@ -2,6 +2,6 @@ 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, Vector>> bitmaps, Vector durations, Gfx::FloatPoint scale) =| did_fail_to_decode_image(i64 image_id, String error_message) =| }