From b86eec4a27ed468866f447c64c160cbd25c5557d Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Mon, 4 May 2026 17:42:02 +0200 Subject: [PATCH] LibGfx: Keep ImmutableBitmap source data context-neutral With the SkImage cache extracted, ImmutableBitmap no longer needs to hold a SkiaBackendContext, an SkImage, or an SkBitmap. Reduce it to just the source pixels (a Bitmap or YUVData) and a color space, so the sam ImmutableBitmap can be consumed by any Skia context without coordination. Per-context locking and ensure_sk_image() are no longer needed and go away with this change; the cache rebuilds an SkImage from the source data the first time each bitmap is drawn against a context. --- Libraries/LibGfx/ColorSpace.cpp | 6 + Libraries/LibGfx/ColorSpace.h | 2 + Libraries/LibGfx/Filter.cpp | 7 +- Libraries/LibGfx/Forward.h | 1 + Libraries/LibGfx/ImmutableBitmap.cpp | 173 +++++------------- Libraries/LibGfx/ImmutableBitmap.h | 12 +- .../LibGfx/ImmutableBitmapSkiaImageCache.cpp | 17 +- Libraries/LibGfx/SkiaUtils.cpp | 14 ++ Libraries/LibGfx/SkiaUtils.h | 5 + 9 files changed, 95 insertions(+), 142 deletions(-) diff --git a/Libraries/LibGfx/ColorSpace.cpp b/Libraries/LibGfx/ColorSpace.cpp index a03890d99f..606158779c 100644 --- a/Libraries/LibGfx/ColorSpace.cpp +++ b/Libraries/LibGfx/ColorSpace.cpp @@ -162,6 +162,12 @@ sk_sp& ColorSpace::color_space() return m_color_space->color_space; } +template<> +sk_sp const& ColorSpace::color_space() const +{ + return m_color_space->color_space; +} + } namespace IPC { diff --git a/Libraries/LibGfx/ColorSpace.h b/Libraries/LibGfx/ColorSpace.h index 939a6e7d58..f52149d57a 100644 --- a/Libraries/LibGfx/ColorSpace.h +++ b/Libraries/LibGfx/ColorSpace.h @@ -37,6 +37,8 @@ public: // and only provide a specialization for sk_sp. template T& color_space(); + template + T const& color_space() const; private: template diff --git a/Libraries/LibGfx/Filter.cpp b/Libraries/LibGfx/Filter.cpp index 17416aee0c..aedfc334fa 100644 --- a/Libraries/LibGfx/Filter.cpp +++ b/Libraries/LibGfx/Filter.cpp @@ -292,7 +292,12 @@ Filter Filter::image(Gfx::ImmutableBitmap const& bitmap, Gfx::IntRect const& src auto skia_dest_rect = to_skia_rect(dest_rect); auto sampling_options = to_skia_sampling_options(scaling_mode); - return Filter(Impl::create(SkImageFilters::Image(sk_ref_sp(bitmap.sk_image()), skia_src_rect, skia_dest_rect, sampling_options))); + auto source_bitmap = bitmap.bitmap(); + if (!source_bitmap) + return Filter(Impl::create(nullptr)); + + auto image = sk_image_from_bitmap(*source_bitmap, bitmap.color_space()); + return Filter(Impl::create(SkImageFilters::Image(move(image), skia_src_rect, skia_dest_rect, sampling_options))); } Filter Filter::merge(Vector> const& inputs) diff --git a/Libraries/LibGfx/Forward.h b/Libraries/LibGfx/Forward.h index a61e0b20f0..2c414883b3 100644 --- a/Libraries/LibGfx/Forward.h +++ b/Libraries/LibGfx/Forward.h @@ -10,6 +10,7 @@ namespace Gfx { class Bitmap; class CMYKBitmap; +class ColorSpace; class ImmutableBitmap; class Color; diff --git a/Libraries/LibGfx/ImmutableBitmap.cpp b/Libraries/LibGfx/ImmutableBitmap.cpp index 299d715e7c..aceb0a1629 100644 --- a/Libraries/LibGfx/ImmutableBitmap.cpp +++ b/Libraries/LibGfx/ImmutableBitmap.cpp @@ -6,20 +6,16 @@ */ #include +#include #include #include -#include #include #include -#include #include #include #include #include -#include -#include -#include namespace Gfx { @@ -36,21 +32,25 @@ StringView export_format_name(ExportFormat format) } struct ImmutableBitmapImpl { - RefPtr context; - sk_sp sk_image; - SkBitmap sk_bitmap; RefPtr bitmap; + OwnPtr yuv_data; ColorSpace color_space; }; int ImmutableBitmap::width() const { - return m_impl->sk_image->width(); + if (m_impl->bitmap) + return m_impl->bitmap->width(); + VERIFY(m_impl->yuv_data); + return m_impl->yuv_data->size().width(); } int ImmutableBitmap::height() const { - return m_impl->sk_image->height(); + if (m_impl->bitmap) + return m_impl->bitmap->height(); + VERIFY(m_impl->yuv_data); + return m_impl->yuv_data->size().height(); } IntRect ImmutableBitmap::rect() const @@ -65,14 +65,20 @@ IntSize ImmutableBitmap::size() const AlphaType ImmutableBitmap::alpha_type() const { - // We assume premultiplied alpha type for opaque surfaces since that is Skia's preferred alpha type and the - // effective pixel data is identical between premultiplied and unpremultiplied in that case. - return m_impl->sk_image->alphaType() == kUnpremul_SkAlphaType ? AlphaType::Unpremultiplied : AlphaType::Premultiplied; + if (m_impl->bitmap) + return m_impl->bitmap->alpha_type(); + + return AlphaType::Premultiplied; } -SkImage const* ImmutableBitmap::sk_image() const +YUVData const* ImmutableBitmap::yuv_data() const { - return m_impl->sk_image.get(); + return m_impl->yuv_data.ptr(); +} + +ColorSpace const& ImmutableBitmap::color_space() const +{ + return m_impl->color_space; } static int bytes_per_pixel_for_export_format(ExportFormat format) @@ -120,9 +126,6 @@ static SkColorType export_format_to_skia_color_type(ExportFormat format) ErrorOr ImmutableBitmap::export_to_byte_buffer(ExportFormat format, int flags, Optional target_width, Optional target_height) const { - if (SkiaBackendContext::the_main_thread_context() && !ensure_sk_image(*SkiaBackendContext::the_main_thread_context())) - return Error::from_string_literal("Failed to create a Skia image for this ImmutableBitmap"); - int width = target_width.value_or(this->width()); int height = target_height.value_or(this->height()); @@ -158,6 +161,14 @@ ErrorOr ImmutableBitmap::export_to_byte_buffer(ExportFormat } } } else { + auto bitmap = this->bitmap(); + if (!bitmap) + return Error::from_string_literal("Failed to create a Bitmap for this ImmutableBitmap"); + + auto image = sk_image_from_bitmap(*bitmap, m_impl->color_space); + if (!image) + return Error::from_string_literal("Failed to create a Skia image for this ImmutableBitmap"); + auto skia_format = export_format_to_skia_color_type(format); auto color_space = SkColorSpace::MakeSRGB(); @@ -172,7 +183,7 @@ ErrorOr ImmutableBitmap::export_to_byte_buffer(ExportFormat surface_canvas->scale(1, -1); } - surface_canvas->drawImageRect(sk_image(), dst_rect, Gfx::to_skia_sampling_options(Gfx::ScalingMode::NearestNeighbor)); + surface_canvas->drawImageRect(image.get(), dst_rect, Gfx::to_skia_sampling_options(Gfx::ScalingMode::NearestNeighbor)); } } else { VERIFY(buffer.is_empty()); @@ -187,16 +198,11 @@ ErrorOr ImmutableBitmap::export_to_byte_buffer(ExportFormat RefPtr ImmutableBitmap::bitmap() const { - if (!m_impl->bitmap && m_impl->sk_image) { - auto bitmap = MUST(Gfx::Bitmap::create(Gfx::BitmapFormat::BGRA8888, Gfx::AlphaType::Premultiplied, { m_impl->sk_image->width(), m_impl->sk_image->height() })); - auto image_info = SkImageInfo::Make(bitmap->width(), bitmap->height(), kBGRA_8888_SkColorType, kPremul_SkAlphaType, SkColorSpace::MakeSRGB()); - SkPixmap pixmap(image_info, bitmap->begin(), bitmap->pitch()); - if (m_impl->context) - m_impl->context->lock(); - m_impl->sk_image->readPixels(pixmap, 0, 0); - if (m_impl->context) - m_impl->context->unlock(); - m_impl->bitmap = move(bitmap); + if (!m_impl->bitmap && m_impl->yuv_data) { + auto bitmap_or_error = m_impl->yuv_data->to_bitmap(); + if (bitmap_or_error.is_error()) + return nullptr; + m_impl->bitmap = bitmap_or_error.release_value(); } return m_impl->bitmap; } @@ -205,94 +211,29 @@ ErrorOr> ImmutableBitmap::create_from_yuv(Nonnull { auto color_space = TRY(ColorSpace::from_cicp(yuv_data->cicp())); - auto context = SkiaBackendContext::the_main_thread_context(); - auto* gr_context = context ? context->sk_context() : nullptr; - - if (!gr_context) { - auto bitmap = TRY(yuv_data->to_bitmap()); - return create(move(bitmap), move(color_space)); - } - if (yuv_data->bit_depth() > 8) yuv_data->expand_samples_to_full_16_bit_range(); - context->lock(); - auto sk_image = SkImages::TextureFromYUVAPixmaps( - gr_context, - yuv_data->make_pixmaps(), - skgpu::Mipmapped::kNo, - false, - color_space.color_space>()); - context->unlock(); - - if (!sk_image) - return Error::from_string_literal("Failed to upload YUV data"); - ImmutableBitmapImpl impl { - .context = context, - .sk_image = move(sk_image), - .sk_bitmap = {}, .bitmap = nullptr, - .color_space = {}, + .yuv_data = move(yuv_data), + .color_space = move(color_space), }; return adopt_ref(*new ImmutableBitmap(make(move(impl)))); } -bool ImmutableBitmap::ensure_sk_image(SkiaBackendContext& context) const -{ - if (m_impl->context) { - VERIFY(m_impl->context.ptr() == &context); - return true; - } - - context.lock(); - ScopeGuard unlock_guard = [&context] { - context.unlock(); - }; - - auto* gr_context = context.sk_context(); - - VERIFY(m_impl->sk_image); - if (!gr_context) - return true; // No GPU, but raster image is still usable - auto gpu_image = SkImages::TextureFromImage(gr_context, m_impl->sk_image.get(), skgpu::Mipmapped::kNo, skgpu::Budgeted::kYes); - if (gpu_image) { - m_impl->context = context; - m_impl->sk_image = move(gpu_image); - } - return true; -} - Color ImmutableBitmap::get_pixel(int x, int y) const { - return m_impl->bitmap->get_pixel(x, y); -} - -static SkAlphaType to_skia_alpha_type(Gfx::AlphaType alpha_type) -{ - switch (alpha_type) { - case AlphaType::Premultiplied: - return kPremul_SkAlphaType; - case AlphaType::Unpremultiplied: - return kUnpremul_SkAlphaType; - default: - VERIFY_NOT_REACHED(); - } + auto bitmap = this->bitmap(); + VERIFY(bitmap); + return bitmap->get_pixel(x, y); } NonnullRefPtr ImmutableBitmap::create(NonnullRefPtr const& bitmap, ColorSpace color_space) { - SkBitmap sk_bitmap; - auto info = SkImageInfo::Make(bitmap->width(), bitmap->height(), to_skia_color_type(bitmap->format()), to_skia_alpha_type(bitmap->alpha_type()), color_space.color_space>()); - sk_bitmap.installPixels(info, const_cast(static_cast(bitmap->scanline(0))), bitmap->pitch()); - sk_bitmap.setImmutable(); - auto sk_image = sk_bitmap.asImage(); - ImmutableBitmapImpl impl { - .context = nullptr, - .sk_image = move(sk_image), - .sk_bitmap = move(sk_bitmap), .bitmap = bitmap, + .yuv_data = nullptr, .color_space = move(color_space), }; return adopt_ref(*new ImmutableBitmap(make(move(impl)))); @@ -315,18 +256,9 @@ NonnullRefPtr ImmutableBitmap::create(NonnullRefPtr ImmutableBitmap::create_snapshot_from_painting_surface(NonnullRefPtr const& painting_surface) { - painting_surface->lock_context(); - auto sk_image = painting_surface->sk_image_snapshot>(); - painting_surface->unlock_context(); - - ImmutableBitmapImpl impl { - .context = painting_surface->skia_backend_context(), - .sk_image = move(sk_image), - .sk_bitmap = {}, - .bitmap = nullptr, - .color_space = {}, - }; - return adopt_ref(*new ImmutableBitmap(make(move(impl)))); + auto bitmap = MUST(Bitmap::create(BitmapFormat::BGRA8888, AlphaType::Premultiplied, painting_surface->size())); + painting_surface->read_into_bitmap(*bitmap); + return create(bitmap); } ImmutableBitmap::ImmutableBitmap(NonnullOwnPtr&& impl) @@ -336,23 +268,6 @@ ImmutableBitmap::ImmutableBitmap(NonnullOwnPtr&& impl) ImmutableBitmap::~ImmutableBitmap() { - lock_context(); - m_impl->sk_image = nullptr; - unlock_context(); -} - -void ImmutableBitmap::lock_context() -{ - auto& context = m_impl->context; - if (context) - context->lock(); -} - -void ImmutableBitmap::unlock_context() -{ - auto& context = m_impl->context; - if (context) - context->unlock(); } } diff --git a/Libraries/LibGfx/ImmutableBitmap.h b/Libraries/LibGfx/ImmutableBitmap.h index 58c64cdf62..c38f1dfdc5 100644 --- a/Libraries/LibGfx/ImmutableBitmap.h +++ b/Libraries/LibGfx/ImmutableBitmap.h @@ -17,8 +17,6 @@ #include #include -class SkImage; - namespace Gfx { struct ImmutableBitmapImpl; @@ -56,30 +54,26 @@ public: ~ImmutableBitmap(); - bool ensure_sk_image(SkiaBackendContext&) const; - int width() const; int height() const; IntRect rect() const; IntSize size() const; AlphaType alpha_type() const; + YUVData const* yuv_data() const; + ColorSpace const& color_space() const; - SkImage const* sk_image() const; [[nodiscard]] ErrorOr export_to_byte_buffer(ExportFormat format, int flags, Optional target_width, Optional target_height) const; Color get_pixel(int x, int y) const; - // Returns nullptr for YUV-backed bitmaps + // May lazily convert YUV-backed bitmaps to CPU pixels. RefPtr bitmap() const; private: mutable NonnullOwnPtr m_impl; explicit ImmutableBitmap(NonnullOwnPtr&&); - - void lock_context(); - void unlock_context(); }; } diff --git a/Libraries/LibGfx/ImmutableBitmapSkiaImageCache.cpp b/Libraries/LibGfx/ImmutableBitmapSkiaImageCache.cpp index 0b712f4977..30362703b9 100644 --- a/Libraries/LibGfx/ImmutableBitmapSkiaImageCache.cpp +++ b/Libraries/LibGfx/ImmutableBitmapSkiaImageCache.cpp @@ -4,11 +4,16 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include #include +#include +#include #include +#include +#include namespace Gfx { @@ -30,11 +35,17 @@ sk_sp ImmutableBitmapSkiaImageCache::image_for_bitmap(ImmutableBitmap c return it->value.image; } - auto context = m_skia_backend_context ? m_skia_backend_context : SkiaBackendContext::the(); - if (context && !bitmap.ensure_sk_image(*context)) + auto source_bitmap = bitmap.bitmap(); + if (!source_bitmap) return nullptr; - auto image = sk_ref_sp(bitmap.sk_image()); + auto image = sk_image_from_bitmap(*source_bitmap, bitmap.color_space()); + if (auto* gr_context = m_skia_backend_context ? m_skia_backend_context->sk_context() : nullptr) { + auto gpu_image = SkImages::TextureFromImage(gr_context, image.get(), skgpu::Mipmapped::kNo, skgpu::Budgeted::kYes); + if (gpu_image) + image = move(gpu_image); + } + if (!image) return nullptr; diff --git a/Libraries/LibGfx/SkiaUtils.cpp b/Libraries/LibGfx/SkiaUtils.cpp index 7bcc7178e7..2242278402 100644 --- a/Libraries/LibGfx/SkiaUtils.cpp +++ b/Libraries/LibGfx/SkiaUtils.cpp @@ -5,10 +5,15 @@ */ #include +#include +#include #include #include #include +#include #include +#include +#include #include #include #include @@ -25,6 +30,15 @@ sk_sp to_skia_image_filter(Gfx::Filter const& filter) return filter.impl().filter; } +sk_sp sk_image_from_bitmap(Bitmap const& bitmap, ColorSpace const& color_space) +{ + auto info = SkImageInfo::Make(bitmap.width(), bitmap.height(), to_skia_color_type(bitmap.format()), to_skia_alpha_type(bitmap.format(), bitmap.alpha_type()), color_space.color_space>()); + SkBitmap sk_bitmap; + sk_bitmap.installPixels(info, const_cast(static_cast(bitmap.scanline(0))), bitmap.pitch()); + sk_bitmap.setImmutable(); + return sk_bitmap.asImage(); +} + sk_sp to_skia_blender(Gfx::CompositingAndBlendingOperator compositing_and_blending_operator) { switch (compositing_and_blending_operator) { diff --git a/Libraries/LibGfx/SkiaUtils.h b/Libraries/LibGfx/SkiaUtils.h index 32d1e43435..ec9ed411db 100644 --- a/Libraries/LibGfx/SkiaUtils.h +++ b/Libraries/LibGfx/SkiaUtils.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -140,4 +141,8 @@ SkPath to_skia_path(Path const& path); sk_sp to_skia_image_filter(Gfx::Filter const& filter); sk_sp to_skia_blender(Gfx::CompositingAndBlendingOperator compositing_and_blending_operator); +// The returned SkImage references the source bitmap's pixels without copying; the caller +// must keep `bitmap` alive for as long as the SkImage (or anything derived from it) is in use. +sk_sp sk_image_from_bitmap(Bitmap const& bitmap, ColorSpace const& color_space); + }