From 6e6999ce5767121a6904b6c36454723e39b9bf24 Mon Sep 17 00:00:00 2001 From: Liav A Date: Fri, 20 Jan 2023 03:27:10 +0200 Subject: [PATCH] LibGfx: Re-work the abstractions of sending image for decoding over IPC Originally I simply thought that passing file paths is quite OK, but as Linus pointed to, it turned out that passing file paths to ensure some files are able to be decoded is awkward because it does not work with images being served over HTTP. Therefore, ideally we should just use the MIME type as an optional argument to ensure that we can always fallback to use that in case sniffing for the correct image type has failed so we can still detect files like with the TGA format, which has no magic bytes. --- .../Applications/ImageViewer/ViewWidget.cpp | 5 ++- Userland/Libraries/LibGUI/ImageWidget.cpp | 4 +- Userland/Libraries/LibGfx/Bitmap.cpp | 4 +- Userland/Libraries/LibGfx/ImageDecoder.cpp | 28 ++++++------- Userland/Libraries/LibGfx/ImageDecoder.h | 3 +- .../LibImageDecoderClient/Client.cpp | 41 +------------------ .../Libraries/LibImageDecoderClient/Client.h | 3 +- .../ImageDecoder/ConnectionFromClient.cpp | 28 ++----------- .../ImageDecoder/ConnectionFromClient.h | 3 +- .../ImageDecoder/ImageDecoderServer.ipc | 3 +- Userland/Utilities/file.cpp | 3 +- 11 files changed, 34 insertions(+), 91 deletions(-) diff --git a/Userland/Applications/ImageViewer/ViewWidget.cpp b/Userland/Applications/ImageViewer/ViewWidget.cpp index 44c4ccb7748..52f7a454bb9 100644 --- a/Userland/Applications/ImageViewer/ViewWidget.cpp +++ b/Userland/Applications/ImageViewer/ViewWidget.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -167,8 +168,8 @@ void ViewWidget::load_from_file(DeprecatedString const& path) // Spawn a new ImageDecoder service process and connect to it. auto client = ImageDecoderClient::Client::try_create().release_value_but_fixme_should_propagate_errors(); - - auto decoded_image_or_error = client->decode_image_with_known_path(path, mapped_file.bytes()); + auto mime_type = Core::guess_mime_type_based_on_filename(path); + auto decoded_image_or_error = client->decode_image(mapped_file.bytes(), mime_type); if (!decoded_image_or_error.has_value()) { show_error(); return; diff --git a/Userland/Libraries/LibGUI/ImageWidget.cpp b/Userland/Libraries/LibGUI/ImageWidget.cpp index 2800fe4f757..3de928bc469 100644 --- a/Userland/Libraries/LibGUI/ImageWidget.cpp +++ b/Userland/Libraries/LibGUI/ImageWidget.cpp @@ -6,6 +6,7 @@ */ #include +#include #include #include #include @@ -76,7 +77,8 @@ void ImageWidget::load_from_file(StringView path) return; auto& mapped_file = *file_or_error.value(); - m_image_decoder = Gfx::ImageDecoder::try_create_for_raw_bytes_with_known_path(path, mapped_file.bytes()); + auto mime_type = Core::guess_mime_type_based_on_filename(path); + m_image_decoder = Gfx::ImageDecoder::try_create_for_raw_bytes(mapped_file.bytes(), mime_type); VERIFY(m_image_decoder); auto frame = m_image_decoder->frame(0).release_value_but_fixme_should_propagate_errors(); diff --git a/Userland/Libraries/LibGfx/Bitmap.cpp b/Userland/Libraries/LibGfx/Bitmap.cpp index 30073c6ae0d..e46161f321a 100644 --- a/Userland/Libraries/LibGfx/Bitmap.cpp +++ b/Userland/Libraries/LibGfx/Bitmap.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -143,7 +144,8 @@ ErrorOr> Bitmap::try_load_from_file(StringView path, int s ErrorOr> Bitmap::try_load_from_fd_and_close(int fd, StringView path) { auto file = TRY(Core::MappedFile::map_from_fd_and_close(fd, path)); - if (auto decoder = ImageDecoder::try_create_for_raw_bytes_with_known_path(path, file->bytes())) { + auto mime_type = Core::guess_mime_type_based_on_filename(path); + if (auto decoder = ImageDecoder::try_create_for_raw_bytes(file->bytes(), mime_type)) { auto frame = TRY(decoder->frame(0)); if (auto& bitmap = frame.image) return bitmap.release_nonnull(); diff --git a/Userland/Libraries/LibGfx/ImageDecoder.cpp b/Userland/Libraries/LibGfx/ImageDecoder.cpp index 90741729598..9d293317b38 100644 --- a/Userland/Libraries/LibGfx/ImageDecoder.cpp +++ b/Userland/Libraries/LibGfx/ImageDecoder.cpp @@ -69,21 +69,12 @@ static OwnPtr probe_and_sniff_for_appropriate_plugin(Readonl return {}; } -RefPtr ImageDecoder::try_create_for_raw_bytes(ReadonlyBytes bytes) +static OwnPtr probe_and_sniff_for_appropriate_plugin_with_known_mime_type(StringView mime_type, ReadonlyBytes bytes) { - OwnPtr plugin = probe_and_sniff_for_appropriate_plugin(bytes); - if (!plugin) - return {}; - return adopt_ref_if_nonnull(new (nothrow) ImageDecoder(plugin.release_nonnull())); -} - -static OwnPtr probe_and_sniff_for_appropriate_plugin_with_known_path(StringView path, ReadonlyBytes bytes) -{ - LexicalPath lexical_mapped_file_path(path); auto* data = bytes.data(); auto size = bytes.size(); OwnPtr plugin; - if (lexical_mapped_file_path.extension() == "tga"sv) { + if (mime_type == "image/x-targa"sv) { plugin = make(data, size); if (plugin->sniff()) return plugin; @@ -91,11 +82,18 @@ static OwnPtr probe_and_sniff_for_appropriate_plugin_with_kn return {}; } -RefPtr ImageDecoder::try_create_for_raw_bytes_with_known_path(StringView path, ReadonlyBytes bytes) +RefPtr ImageDecoder::try_create_for_raw_bytes(ReadonlyBytes bytes, Optional mime_type) { - OwnPtr plugin = probe_and_sniff_for_appropriate_plugin_with_known_path(path, bytes); - if (!plugin) - return try_create_for_raw_bytes(bytes); + OwnPtr plugin = probe_and_sniff_for_appropriate_plugin(bytes); + if (!plugin) { + if (mime_type.has_value()) { + plugin = probe_and_sniff_for_appropriate_plugin_with_known_mime_type(mime_type.value(), bytes); + if (!plugin) + return {}; + } else { + return {}; + } + } return adopt_ref_if_nonnull(new (nothrow) ImageDecoder(plugin.release_nonnull())); } diff --git a/Userland/Libraries/LibGfx/ImageDecoder.h b/Userland/Libraries/LibGfx/ImageDecoder.h index 5b0731b9a26..9d622ec8aca 100644 --- a/Userland/Libraries/LibGfx/ImageDecoder.h +++ b/Userland/Libraries/LibGfx/ImageDecoder.h @@ -47,8 +47,7 @@ protected: class ImageDecoder : public RefCounted { public: - static RefPtr try_create_for_raw_bytes(ReadonlyBytes); - static RefPtr try_create_for_raw_bytes_with_known_path(StringView path, ReadonlyBytes); + static RefPtr try_create_for_raw_bytes(ReadonlyBytes, Optional mime_type = {}); ~ImageDecoder() = default; IntSize size() const { return m_plugin->size(); } diff --git a/Userland/Libraries/LibImageDecoderClient/Client.cpp b/Userland/Libraries/LibImageDecoderClient/Client.cpp index abb151c279e..63e1216d006 100644 --- a/Userland/Libraries/LibImageDecoderClient/Client.cpp +++ b/Userland/Libraries/LibImageDecoderClient/Client.cpp @@ -20,7 +20,7 @@ void Client::die() on_death(); } -Optional Client::decode_image_with_known_path(DeprecatedString const& path, ReadonlyBytes encoded_data) +Optional Client::decode_image(ReadonlyBytes encoded_data, Optional mime_type) { if (encoded_data.is_empty()) return {}; @@ -33,44 +33,7 @@ Optional Client::decode_image_with_known_path(DeprecatedString con auto encoded_buffer = encoded_buffer_or_error.release_value(); memcpy(encoded_buffer.data(), encoded_data.data(), encoded_data.size()); - auto response_or_error = try_decode_image_with_known_path(path, move(encoded_buffer)); - - if (response_or_error.is_error()) { - dbgln("ImageDecoder died heroically"); - return {}; - } - - auto& response = response_or_error.value(); - - if (response.bitmaps().is_empty()) - return {}; - - DecodedImage image; - image.is_animated = response.is_animated(); - image.loop_count = response.loop_count(); - image.frames.resize(response.bitmaps().size()); - for (size_t i = 0; i < image.frames.size(); ++i) { - auto& frame = image.frames[i]; - frame.bitmap = response.bitmaps()[i].bitmap(); - frame.duration = response.durations()[i]; - } - return image; -} - -Optional Client::decode_image(ReadonlyBytes encoded_data) -{ - if (encoded_data.is_empty()) - return {}; - - auto encoded_buffer_or_error = Core::AnonymousBuffer::create_with_size(encoded_data.size()); - if (encoded_buffer_or_error.is_error()) { - dbgln("Could not allocate encoded buffer"); - return {}; - } - auto encoded_buffer = encoded_buffer_or_error.release_value(); - - memcpy(encoded_buffer.data(), encoded_data.data(), encoded_data.size()); - auto response_or_error = try_decode_image(move(encoded_buffer)); + auto response_or_error = try_decode_image(move(encoded_buffer), mime_type); if (response_or_error.is_error()) { dbgln("ImageDecoder died heroically"); diff --git a/Userland/Libraries/LibImageDecoderClient/Client.h b/Userland/Libraries/LibImageDecoderClient/Client.h index 4e15e9ea2e7..5464fed0e79 100644 --- a/Userland/Libraries/LibImageDecoderClient/Client.h +++ b/Userland/Libraries/LibImageDecoderClient/Client.h @@ -30,8 +30,7 @@ class Client final IPC_CLIENT_CONNECTION(Client, "/tmp/session/%sid/portal/image"sv); public: - Optional decode_image(ReadonlyBytes); - Optional decode_image_with_known_path(DeprecatedString const& path, ReadonlyBytes); + Optional decode_image(ReadonlyBytes, Optional mime_type = {}); Function on_death; diff --git a/Userland/Services/ImageDecoder/ConnectionFromClient.cpp b/Userland/Services/ImageDecoder/ConnectionFromClient.cpp index 9f0bee865bd..c0d4d203f23 100644 --- a/Userland/Services/ImageDecoder/ConnectionFromClient.cpp +++ b/Userland/Services/ImageDecoder/ConnectionFromClient.cpp @@ -37,19 +37,14 @@ static void decode_image_to_bitmaps_and_durations_with_decoder(Gfx::ImageDecoder } } -static void decode_image_to_details(Core::AnonymousBuffer const& encoded_buffer, Optional known_path, bool& is_animated, u32& loop_count, Vector& bitmaps, Vector& durations) +static void decode_image_to_details(Core::AnonymousBuffer const& encoded_buffer, Optional const& known_mime_type, bool& is_animated, u32& loop_count, Vector& bitmaps, Vector& durations) { VERIFY(bitmaps.size() == 0); VERIFY(durations.size() == 0); VERIFY(!is_animated); VERIFY(loop_count == 0); - RefPtr decoder; - if (known_path.has_value()) - decoder = Gfx::ImageDecoder::try_create_for_raw_bytes_with_known_path(known_path.value(), ReadonlyBytes { encoded_buffer.data(), encoded_buffer.size() }); - else - decoder = Gfx::ImageDecoder::try_create_for_raw_bytes(ReadonlyBytes { encoded_buffer.data(), encoded_buffer.size() }); - + auto decoder = Gfx::ImageDecoder::try_create_for_raw_bytes(ReadonlyBytes { encoded_buffer.data(), encoded_buffer.size() }, known_mime_type); if (!decoder) { dbgln_if(IMAGE_DECODER_DEBUG, "Could not find suitable image decoder plugin for data"); return; @@ -62,7 +57,7 @@ static void decode_image_to_details(Core::AnonymousBuffer const& encoded_buffer, decode_image_to_bitmaps_and_durations_with_decoder(*decoder, bitmaps, durations); } -Messages::ImageDecoderServer::DecodeImageWithKnownPathResponse ConnectionFromClient::decode_image_with_known_path(DeprecatedString const& path, Core::AnonymousBuffer const& encoded_buffer) +Messages::ImageDecoderServer::DecodeImageResponse ConnectionFromClient::decode_image(Core::AnonymousBuffer const& encoded_buffer, Optional const& mime_type) { if (!encoded_buffer.is_valid()) { dbgln_if(IMAGE_DECODER_DEBUG, "Encoded data is invalid"); @@ -73,22 +68,7 @@ Messages::ImageDecoderServer::DecodeImageWithKnownPathResponse ConnectionFromCli u32 loop_count = 0; Vector bitmaps; Vector durations; - decode_image_to_details(encoded_buffer, path.substring_view(0), is_animated, loop_count, bitmaps, durations); - return { is_animated, loop_count, bitmaps, durations }; -} - -Messages::ImageDecoderServer::DecodeImageResponse ConnectionFromClient::decode_image(Core::AnonymousBuffer const& encoded_buffer) -{ - if (!encoded_buffer.is_valid()) { - dbgln_if(IMAGE_DECODER_DEBUG, "Encoded data is invalid"); - return nullptr; - } - - bool is_animated = false; - u32 loop_count = 0; - Vector bitmaps; - Vector durations; - decode_image_to_details(encoded_buffer, {}, is_animated, loop_count, bitmaps, durations); + decode_image_to_details(encoded_buffer, mime_type, is_animated, loop_count, bitmaps, durations); return { is_animated, loop_count, bitmaps, durations }; } diff --git a/Userland/Services/ImageDecoder/ConnectionFromClient.h b/Userland/Services/ImageDecoder/ConnectionFromClient.h index f137af3dfeb..a9ae1919bfc 100644 --- a/Userland/Services/ImageDecoder/ConnectionFromClient.h +++ b/Userland/Services/ImageDecoder/ConnectionFromClient.h @@ -26,8 +26,7 @@ public: private: explicit ConnectionFromClient(NonnullOwnPtr); - virtual Messages::ImageDecoderServer::DecodeImageResponse decode_image(Core::AnonymousBuffer const&) override; - virtual Messages::ImageDecoderServer::DecodeImageWithKnownPathResponse decode_image_with_known_path(DeprecatedString const& path, Core::AnonymousBuffer const&) override; + virtual Messages::ImageDecoderServer::DecodeImageResponse decode_image(Core::AnonymousBuffer const&, Optional const& mime_type) override; }; } diff --git a/Userland/Services/ImageDecoder/ImageDecoderServer.ipc b/Userland/Services/ImageDecoder/ImageDecoderServer.ipc index 9492f685a06..a2d24f1952d 100644 --- a/Userland/Services/ImageDecoder/ImageDecoderServer.ipc +++ b/Userland/Services/ImageDecoder/ImageDecoderServer.ipc @@ -3,6 +3,5 @@ endpoint ImageDecoderServer { - decode_image(Core::AnonymousBuffer data) => (bool is_animated, u32 loop_count, Vector bitmaps, Vector durations) - decode_image_with_known_path(DeprecatedString path, Core::AnonymousBuffer data) => (bool is_animated, u32 loop_count, Vector bitmaps, Vector durations) + decode_image(Core::AnonymousBuffer data, Optional mime_type) => (bool is_animated, u32 loop_count, Vector bitmaps, Vector durations) } diff --git a/Userland/Utilities/file.cpp b/Userland/Utilities/file.cpp index 8c4f0dd68ff..e5902175095 100644 --- a/Userland/Utilities/file.cpp +++ b/Userland/Utilities/file.cpp @@ -33,7 +33,8 @@ static Optional image_details(DeprecatedString const& descript return {}; auto& mapped_file = *file_or_error.value(); - auto image_decoder = Gfx::ImageDecoder::try_create_for_raw_bytes_with_known_path(path, mapped_file.bytes()); + auto mime_type = Core::guess_mime_type_based_on_filename(path); + auto image_decoder = Gfx::ImageDecoder::try_create_for_raw_bytes(mapped_file.bytes(), mime_type); if (!image_decoder) return {};