Skip to content

Commit

Permalink
ImageDecoder+LibGfx: Collate decoded bitmaps before sending over IPC
Browse files Browse the repository at this point in the history
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<Optional<NonnullRefPtr<Gfx::Bitmap>>> 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.
  • Loading branch information
zack466 committed Sep 18, 2024
1 parent 259e798 commit 801665a
Show file tree
Hide file tree
Showing 7 changed files with 181 additions and 6 deletions.
129 changes: 129 additions & 0 deletions Userland/Libraries/LibGfx/BitmapSequence.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
/*
* Copyright (c) 2024, Zachary Huang <[email protected]>
*
* SPDX-License-Identifier: BSD-2-Clause
*/

#include <LibCore/AnonymousBuffer.h>
#include <LibGfx/Bitmap.h>
#include <LibGfx/BitmapSequence.h>
#include <LibGfx/Size.h>
#include <LibIPC/Decoder.h>
#include <LibIPC/Encoder.h>
#include <LibIPC/File.h>

namespace Gfx {

BitmapSequence::BitmapSequence(Vector<Optional<NonnullRefPtr<Bitmap>>> bitmaps)
: m_bitmaps(move(bitmaps))
{
}

}

namespace IPC {

template<>
ErrorOr<void> encode(Encoder& encoder, Gfx::BitmapSequence const& bitmap_sequence)
{
Vector<Optional<NonnullRefPtr<Gfx::Bitmap>>> 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<u32>(bitmap->format())));
TRY(encoder.encode(static_cast<u32>(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<u8>();
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<Gfx::BitmapSequence> 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<Optional<BitmapMetadata>> bitmaps_metadata;

size_t num_bitmaps = TRY(decoder.decode_size());

for (size_t i = 0; i < num_bitmaps; i++) {
auto valid = TRY(decoder.decode<bool>());

if (valid) {
auto raw_bitmap_format = TRY(decoder.decode<u32>());
if (!Gfx::is_valid_bitmap_format(raw_bitmap_format))
return Error::from_string_literal("IPC: Invalid Gfx::BitmapSequence format");
auto format = static_cast<Gfx::BitmapFormat>(raw_bitmap_format);

auto raw_alpha_type = TRY(decoder.decode<u32>());
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<Gfx::AlphaType>(raw_alpha_type);

auto size_in_bytes = TRY(decoder.decode<size_t>());
auto size = TRY(decoder.decode<Gfx::IntSize>());

bitmaps_metadata.append(BitmapMetadata { format, alpha_type, size, size_in_bytes });
} else {
bitmaps_metadata.append({});
}
}

auto collated_buffer = TRY(decoder.decode<Core::AnonymousBuffer>());
auto* read_pointer = collated_buffer.data<u8>();

Vector<Optional<NonnullRefPtr<Gfx::Bitmap>>> 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<u8>(), 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 };
}

}
40 changes: 40 additions & 0 deletions Userland/Libraries/LibGfx/BitmapSequence.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright (c) 2024, Zachary Huang <[email protected]>
*
* SPDX-License-Identifier: BSD-2-Clause
*/

#pragma once

#include <AK/RefPtr.h>
#include <LibGfx/Bitmap.h>
#include <LibGfx/Size.h>
#include <LibIPC/Forward.h>

namespace Gfx {

class BitmapSequence {
public:
BitmapSequence() = default;

BitmapSequence(Vector<Optional<NonnullRefPtr<Gfx::Bitmap>>>);

Vector<Optional<NonnullRefPtr<Gfx::Bitmap>>> bitmaps() const { return m_bitmaps; }

private:
// friend class Bitmap;

Vector<Optional<NonnullRefPtr<Gfx::Bitmap>>> m_bitmaps;
};

}

namespace IPC {

template<>
ErrorOr<void> encode(Encoder&, Gfx::BitmapSequence const&);

template<>
ErrorOr<Gfx::BitmapSequence> decode(Decoder&);

}
1 change: 1 addition & 0 deletions Userland/Libraries/LibGfx/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ set(SOURCES
AffineTransform.cpp
AntiAliasingPainter.cpp
Bitmap.cpp
BitmapSequence.cpp
CMYKBitmap.cpp
Color.cpp
DeltaE.cpp
Expand Down
2 changes: 1 addition & 1 deletion Userland/Libraries/LibImageDecoderClient/Client.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<Optional<NonnullRefPtr<Gfx::Bitmap>>> const& bitmaps, Vector<u32> 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<u32> const& durations, Gfx::FloatPoint scale) override;
virtual void did_fail_to_decode_image(i64 image_id, String const& error_message) override;

HashMap<i64, NonnullRefPtr<Core::Promise<DecodedImage>>> m_pending_decoded_images;
Expand Down
8 changes: 6 additions & 2 deletions Userland/Services/ImageDecoder/ConnectionFromClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ static ErrorOr<ConnectionFromClient::DecodeResult> decode_image_to_details(Core:
result.is_animated = decoder->is_animated();
result.loop_count = decoder->loop_count();

Vector<Optional<NonnullRefPtr<Gfx::Bitmap>>> bitmaps;

if (auto maybe_metadata = decoder->metadata(); maybe_metadata.has_value() && is<Gfx::ExifMetadata>(*maybe_metadata)) {
auto const& exif = static_cast<Gfx::ExifMetadata const&>(maybe_metadata.value());
if (exif.x_resolution().has_value() && exif.y_resolution().has_value()) {
Expand All @@ -116,11 +118,13 @@ static ErrorOr<ConnectionFromClient::DecodeResult> 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;
}

Expand Down
3 changes: 2 additions & 1 deletion Userland/Services/ImageDecoder/ConnectionFromClient.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <ImageDecoder/Forward.h>
#include <ImageDecoder/ImageDecoderClientEndpoint.h>
#include <ImageDecoder/ImageDecoderServerEndpoint.h>
#include <LibGfx/BitmapSequence.h>
#include <LibIPC/ConnectionFromClient.h>
#include <LibThreading/BackgroundAction.h>

Expand All @@ -28,7 +29,7 @@ class ConnectionFromClient final
bool is_animated = false;
u32 loop_count = 0;
Gfx::FloatPoint scale { 1, 1 };
Vector<Optional<NonnullRefPtr<Gfx::Bitmap>>> bitmaps;
Gfx::BitmapSequence bitmaps;
Vector<u32> durations;
};

Expand Down
4 changes: 2 additions & 2 deletions Userland/Services/ImageDecoder/ImageDecoderClient.ipc
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#include <LibGfx/ShareableBitmap.h>
#include <LibGfx/BitmapSequence.h>

endpoint ImageDecoderClient
{
did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Vector<Optional<NonnullRefPtr<Gfx::Bitmap>>> bitmaps, Vector<u32> durations, Gfx::FloatPoint scale) =|
did_decode_image(i64 image_id, bool is_animated, u32 loop_count, Gfx::BitmapSequence bitmaps, Vector<u32> durations, Gfx::FloatPoint scale) =|
did_fail_to_decode_image(i64 image_id, String error_message) =|
}

0 comments on commit 801665a

Please sign in to comment.