From b505b29f6c0ca332938b819d3308db7215941a76 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Thu, 3 Apr 2025 18:03:17 +0200 Subject: [PATCH] LibWeb: Move painting surface allocation into rendering thread Skia has a check in debug mode to verify that surface is only used within one thread. Before this change we were violating this by allocating surfaces on the main thread while using and destructing them on the rendering thread. --- Libraries/LibGfx/SkiaBackendContext.h | 4 +- Libraries/LibWeb/HTML/RenderingThread.cpp | 53 +++++++++++++++++-- Libraries/LibWeb/HTML/RenderingThread.h | 16 ++++-- .../LibWeb/HTML/TraversableNavigable.cpp | 40 ++------------ Libraries/LibWeb/HTML/TraversableNavigable.h | 9 ++-- Services/WebContent/PageClient.cpp | 3 +- 6 files changed, 73 insertions(+), 52 deletions(-) diff --git a/Libraries/LibGfx/SkiaBackendContext.h b/Libraries/LibGfx/SkiaBackendContext.h index 50ddf8ad647..af8c8760372 100644 --- a/Libraries/LibGfx/SkiaBackendContext.h +++ b/Libraries/LibGfx/SkiaBackendContext.h @@ -6,8 +6,8 @@ #pragma once +#include #include -#include #include #ifdef USE_VULKAN @@ -25,7 +25,7 @@ namespace Gfx { class MetalContext; -class SkiaBackendContext : public RefCounted { +class SkiaBackendContext : public AtomicRefCounted { AK_MAKE_NONCOPYABLE(SkiaBackendContext); AK_MAKE_NONMOVABLE(SkiaBackendContext); diff --git a/Libraries/LibWeb/HTML/RenderingThread.cpp b/Libraries/LibWeb/HTML/RenderingThread.cpp index 80097e7c7aa..41effe7809b 100644 --- a/Libraries/LibWeb/HTML/RenderingThread.cpp +++ b/Libraries/LibWeb/HTML/RenderingThread.cpp @@ -6,6 +6,8 @@ #include #include +#include +#include namespace Web::HTML { @@ -21,8 +23,9 @@ RenderingThread::~RenderingThread() (void)m_thread->join(); } -void RenderingThread::start() +void RenderingThread::start(DisplayListPlayerType display_list_player_type) { + m_display_list_player_type = display_list_player_type; VERIFY(m_skia_player); m_thread = Threading::Thread::construct([this] { rendering_thread_loop(); @@ -36,6 +39,10 @@ void RenderingThread::rendering_thread_loop() while (true) { auto task = [this]() -> Optional { Threading::MutexLocker const locker { m_rendering_task_mutex }; + if (m_needs_to_clear_bitmap_to_surface_cache) { + m_bitmap_to_surface.clear(); + m_needs_to_clear_bitmap_to_surface_cache = false; + } while (m_rendering_tasks.is_empty() && !m_exit) { m_rendering_task_ready_wake_condition.wait(); } @@ -49,18 +56,56 @@ void RenderingThread::rendering_thread_loop() break; } - m_skia_player->execute(*task->display_list, task->painting_surface); + auto painting_surface = painting_surface_for_backing_store(task->backing_store); + m_skia_player->execute(*task->display_list, painting_surface); m_main_thread_event_loop.deferred_invoke([callback = move(task->callback)] { callback(); }); } } -void RenderingThread::enqueue_rendering_task(NonnullRefPtr display_list, NonnullRefPtr painting_surface, Function&& callback) +void RenderingThread::enqueue_rendering_task(NonnullRefPtr display_list, NonnullRefPtr backing_store, Function&& callback) { Threading::MutexLocker const locker { m_rendering_task_mutex }; - m_rendering_tasks.enqueue(Task { move(display_list), move(painting_surface), move(callback) }); + m_rendering_tasks.enqueue(Task { move(display_list), move(backing_store), move(callback) }); m_rendering_task_ready_wake_condition.signal(); } +NonnullRefPtr RenderingThread::painting_surface_for_backing_store(Painting::BackingStore& backing_store) +{ + auto& bitmap = backing_store.bitmap(); + auto cached_surface = m_bitmap_to_surface.find(&bitmap); + if (cached_surface != m_bitmap_to_surface.end()) + return cached_surface->value; + + RefPtr new_surface; + if (m_display_list_player_type == DisplayListPlayerType::SkiaGPUIfAvailable && m_skia_backend_context) { +#ifdef USE_VULKAN + // Vulkan: Try to create an accelerated surface. + new_surface = Gfx::PaintingSurface::create_with_size(m_skia_backend_context, backing_store.size(), Gfx::BitmapFormat::BGRA8888, Gfx::AlphaType::Premultiplied); + new_surface->on_flush = [backing_store = static_cast>(backing_store)](auto& surface) { surface.read_into_bitmap(backing_store->bitmap()); }; +#endif +#ifdef AK_OS_MACOS + // macOS: Wrap an IOSurface if available. + if (is(backing_store)) { + auto& iosurface_backing_store = static_cast(backing_store); + new_surface = Gfx::PaintingSurface::wrap_iosurface(iosurface_backing_store.iosurface_handle(), *m_skia_backend_context); + } +#endif + } + + // CPU and fallback: wrap the backing store bitmap directly. + if (!new_surface) + new_surface = Gfx::PaintingSurface::wrap_bitmap(bitmap); + + m_bitmap_to_surface.set(&bitmap, *new_surface); + return *new_surface; +} + +void RenderingThread::clear_bitmap_to_surface_cache() +{ + Threading::MutexLocker const locker { m_rendering_task_mutex }; + m_needs_to_clear_bitmap_to_surface_cache = true; +} + } diff --git a/Libraries/LibWeb/HTML/RenderingThread.h b/Libraries/LibWeb/HTML/RenderingThread.h index fe4c2a36222..2aecc153093 100644 --- a/Libraries/LibWeb/HTML/RenderingThread.h +++ b/Libraries/LibWeb/HTML/RenderingThread.h @@ -11,6 +11,8 @@ #include #include #include +#include +#include #include namespace Web::HTML { @@ -23,23 +25,28 @@ public: RenderingThread(); ~RenderingThread(); - void start(); + void start(DisplayListPlayerType); void set_skia_player(OwnPtr&& player) { m_skia_player = move(player); } - void enqueue_rendering_task(NonnullRefPtr, NonnullRefPtr, Function&& callback); + void set_skia_backend_context(RefPtr context) { m_skia_backend_context = move(context); } + void enqueue_rendering_task(NonnullRefPtr, NonnullRefPtr, Function&& callback); + void clear_bitmap_to_surface_cache(); private: void rendering_thread_loop(); + NonnullRefPtr painting_surface_for_backing_store(Painting::BackingStore& backing_store); Core::EventLoop& m_main_thread_event_loop; + DisplayListPlayerType m_display_list_player_type; OwnPtr m_skia_player; + RefPtr m_skia_backend_context; RefPtr m_thread; Atomic m_exit { false }; struct Task { NonnullRefPtr display_list; - NonnullRefPtr painting_surface; + NonnullRefPtr backing_store; Function callback; }; // NOTE: Queue will only contain multiple items in case tasks were scheduled by screenshot requests. @@ -47,6 +54,9 @@ private: Queue m_rendering_tasks; Threading::Mutex m_rendering_task_mutex; Threading::ConditionVariable m_rendering_task_ready_wake_condition { m_rendering_task_mutex }; + + HashMap> m_bitmap_to_surface; + bool m_needs_to_clear_bitmap_to_surface_cache { false }; }; } diff --git a/Libraries/LibWeb/HTML/TraversableNavigable.cpp b/Libraries/LibWeb/HTML/TraversableNavigable.cpp index a8519eab177..ecae592a888 100644 --- a/Libraries/LibWeb/HTML/TraversableNavigable.cpp +++ b/Libraries/LibWeb/HTML/TraversableNavigable.cpp @@ -62,7 +62,8 @@ TraversableNavigable::TraversableNavigable(GC::Ref page) } m_rendering_thread.set_skia_player(move(skia_player)); - m_rendering_thread.start(); + m_rendering_thread.set_skia_backend_context(m_skia_backend_context); + m_rendering_thread.start(display_list_player_type); } TraversableNavigable::~TraversableNavigable() = default; @@ -1399,38 +1400,7 @@ void TraversableNavigable::set_viewport_size(CSSPixelSize size) Navigable::set_viewport_size(size); // Invalidate the surface cache if the traversable changed size. - m_bitmap_to_surface.clear(); -} - -NonnullRefPtr TraversableNavigable::painting_surface_for_backing_store(Painting::BackingStore& backing_store) -{ - auto& bitmap = backing_store.bitmap(); - auto cached_surface = m_bitmap_to_surface.find(&bitmap); - if (cached_surface != m_bitmap_to_surface.end()) - return cached_surface->value; - - RefPtr new_surface; - if (page().client().display_list_player_type() == DisplayListPlayerType::SkiaGPUIfAvailable && m_skia_backend_context) { -#ifdef USE_VULKAN - // Vulkan: Try to create an accelerated surface. - new_surface = Gfx::PaintingSurface::create_with_size(m_skia_backend_context, backing_store.size(), Gfx::BitmapFormat::BGRA8888, Gfx::AlphaType::Premultiplied); - new_surface->on_flush = [backing_store = static_cast>(backing_store)](auto& surface) { surface.read_into_bitmap(backing_store->bitmap()); }; -#endif -#ifdef AK_OS_MACOS - // macOS: Wrap an IOSurface if available. - if (is(backing_store)) { - auto& iosurface_backing_store = static_cast(backing_store); - new_surface = Gfx::PaintingSurface::wrap_iosurface(iosurface_backing_store.iosurface_handle(), *m_skia_backend_context); - } -#endif - } - - // CPU and fallback: wrap the backing store bitmap directly. - if (!new_surface) - new_surface = Gfx::PaintingSurface::wrap_bitmap(bitmap); - - m_bitmap_to_surface.set(&bitmap, *new_surface); - return *new_surface; + m_rendering_thread.clear_bitmap_to_surface_cache(); } RefPtr TraversableNavigable::record_display_list(DevicePixelRect const& content_rect, PaintOptions paint_options) @@ -1454,9 +1424,9 @@ RefPtr TraversableNavigable::record_display_list(DevicePi return document->record_display_list(paint_config); } -void TraversableNavigable::start_display_list_rendering(NonnullRefPtr display_list, NonnullRefPtr painting_surface, Function&& callback) +void TraversableNavigable::start_display_list_rendering(NonnullRefPtr display_list, NonnullRefPtr backing_store, Function&& callback) { - m_rendering_thread.enqueue_rendering_task(move(display_list), move(painting_surface), move(callback)); + m_rendering_thread.enqueue_rendering_task(move(display_list), move(backing_store), move(callback)); } } diff --git a/Libraries/LibWeb/HTML/TraversableNavigable.h b/Libraries/LibWeb/HTML/TraversableNavigable.h index 1bd18b8b092..542c642c92d 100644 --- a/Libraries/LibWeb/HTML/TraversableNavigable.h +++ b/Libraries/LibWeb/HTML/TraversableNavigable.h @@ -98,7 +98,7 @@ public: [[nodiscard]] GC::Ptr currently_focused_area(); RefPtr record_display_list(DevicePixelRect const&, PaintOptions); - void start_display_list_rendering(NonnullRefPtr display_list, NonnullRefPtr painting_surface, Function&& callback); + void start_display_list_rendering(NonnullRefPtr, NonnullRefPtr, Function&& callback); enum class CheckIfUnloadingIsCanceledResult { CanceledByBeforeUnload, @@ -117,8 +117,6 @@ public: bool needs_repaint() const { return m_needs_repaint; } void set_needs_repaint() { m_needs_repaint = true; } - NonnullRefPtr painting_surface_for_backing_store(Painting::BackingStore&); - private: TraversableNavigable(GC::Ref); @@ -140,6 +138,8 @@ private: [[nodiscard]] bool can_go_forward() const; + RenderingThread m_rendering_thread; + // https://html.spec.whatwg.org/multipage/document-sequences.html#tn-current-session-history-step int m_current_session_history_step { 0 }; @@ -163,11 +163,8 @@ private: String m_window_handle; RefPtr m_skia_backend_context; - HashMap> m_bitmap_to_surface; bool m_needs_repaint { true }; - - RenderingThread m_rendering_thread; }; struct BrowsingContextAndDocument { diff --git a/Services/WebContent/PageClient.cpp b/Services/WebContent/PageClient.cpp index 698521fced3..57b57b7ce55 100644 --- a/Services/WebContent/PageClient.cpp +++ b/Services/WebContent/PageClient.cpp @@ -246,8 +246,7 @@ void PageClient::start_display_list_rendering(Web::DevicePixelRect const& conten callback(); return; } - auto painting_surface = traversable.painting_surface_for_backing_store(target); - traversable.start_display_list_rendering(*display_list, painting_surface, move(callback)); + traversable.start_display_list_rendering(*display_list, target, move(callback)); } Queue& PageClient::input_event_queue()