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

@@ -6,8 +6,8 @@
 
 #pragma once
 
+#include <AK/AtomicRefCounted.h>
 #include <AK/Noncopyable.h>
-#include <AK/RefCounted.h>
 #include <LibThreading/Mutex.h>
 
 #ifdef USE_VULKAN
@@ -25,7 +25,7 @@ namespace Gfx {
 
 class MetalContext;
 
-class SkiaBackendContext : public RefCounted<SkiaBackendContext> {
+class SkiaBackendContext : public AtomicRefCounted<SkiaBackendContext> {
     AK_MAKE_NONCOPYABLE(SkiaBackendContext);
     AK_MAKE_NONMOVABLE(SkiaBackendContext);
 

Libraries/LibWeb/HTML/RenderingThread.cpp

@@ -6,6 +6,8 @@
 
 #include <LibCore/EventLoop.h>
 #include <LibWeb/HTML/RenderingThread.h>
+#include <LibWeb/HTML/TraversableNavigable.h>
+#include <LibWeb/Painting/BackingStore.h>
 
 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<Task> {
             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<Painting::DisplayList> display_list, NonnullRefPtr<Gfx::PaintingSurface> painting_surface, Function<void()>&& callback)
+void RenderingThread::enqueue_rendering_task(NonnullRefPtr<Painting::DisplayList> display_list, NonnullRefPtr<Painting::BackingStore> backing_store, Function<void()>&& 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<Gfx::PaintingSurface> 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<Gfx::PaintingSurface> 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<NonnullRefPtr<Painting::BackingStore>>(backing_store)](auto& surface) { surface.read_into_bitmap(backing_store->bitmap()); };
+#endif
+#ifdef AK_OS_MACOS
+        // macOS: Wrap an IOSurface if available.
+        if (is<Painting::IOSurfaceBackingStore>(backing_store)) {
+            auto& iosurface_backing_store = static_cast<Painting::IOSurfaceBackingStore&>(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;
+}
+
 }

Libraries/LibWeb/HTML/RenderingThread.h

@@ -11,6 +11,8 @@
 #include <LibThreading/ConditionVariable.h>
 #include <LibThreading/Mutex.h>
 #include <LibThreading/Thread.h>
+#include <LibWeb/Forward.h>
+#include <LibWeb/Page/Page.h>
 #include <LibWeb/Painting/DisplayListPlayerSkia.h>
 
 namespace Web::HTML {
@@ -23,23 +25,28 @@ public:
     RenderingThread();
     ~RenderingThread();
 
-    void start();
+    void start(DisplayListPlayerType);
     void set_skia_player(OwnPtr<Painting::DisplayListPlayerSkia>&& player) { m_skia_player = move(player); }
-    void enqueue_rendering_task(NonnullRefPtr<Painting::DisplayList>, NonnullRefPtr<Gfx::PaintingSurface>, Function<void()>&& callback);
+    void set_skia_backend_context(RefPtr<Gfx::SkiaBackendContext> context) { m_skia_backend_context = move(context); }
+    void enqueue_rendering_task(NonnullRefPtr<Painting::DisplayList>, NonnullRefPtr<Painting::BackingStore>, Function<void()>&& callback);
+    void clear_bitmap_to_surface_cache();
 
 private:
     void rendering_thread_loop();
+    NonnullRefPtr<Gfx::PaintingSurface> painting_surface_for_backing_store(Painting::BackingStore& backing_store);
 
     Core::EventLoop& m_main_thread_event_loop;
+    DisplayListPlayerType m_display_list_player_type;
 
     OwnPtr<Painting::DisplayListPlayerSkia> m_skia_player;
+    RefPtr<Gfx::SkiaBackendContext> m_skia_backend_context;
 
     RefPtr<Threading::Thread> m_thread;
     Atomic<bool> m_exit { false };
 
     struct Task {
         NonnullRefPtr<Painting::DisplayList> display_list;
-        NonnullRefPtr<Gfx::PaintingSurface> painting_surface;
+        NonnullRefPtr<Painting::BackingStore> backing_store;
         Function<void()> callback;
     };
     // NOTE: Queue will only contain multiple items in case tasks were scheduled by screenshot requests.
@@ -47,6 +54,9 @@ private:
     Queue<Task> m_rendering_tasks;
     Threading::Mutex m_rendering_task_mutex;
     Threading::ConditionVariable m_rendering_task_ready_wake_condition { m_rendering_task_mutex };
+
+    HashMap<Gfx::Bitmap*, NonnullRefPtr<Gfx::PaintingSurface>> m_bitmap_to_surface;
+    bool m_needs_to_clear_bitmap_to_surface_cache { false };
 };
 
 }

Libraries/LibWeb/HTML/TraversableNavigable.cpp

@@ -62,7 +62,8 @@ TraversableNavigable::TraversableNavigable(GC::Ref<Page> 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<Gfx::PaintingSurface> 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<Gfx::PaintingSurface> 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<NonnullRefPtr<Painting::BackingStore>>(backing_store)](auto& surface) { surface.read_into_bitmap(backing_store->bitmap()); };
-#endif
-#ifdef AK_OS_MACOS
-        // macOS: Wrap an IOSurface if available.
-        if (is<Painting::IOSurfaceBackingStore>(backing_store)) {
-            auto& iosurface_backing_store = static_cast<Painting::IOSurfaceBackingStore&>(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<Painting::DisplayList> TraversableNavigable::record_display_list(DevicePixelRect const& content_rect, PaintOptions paint_options)
@@ -1454,9 +1424,9 @@ RefPtr<Painting::DisplayList> TraversableNavigable::record_display_list(DevicePi
     return document->record_display_list(paint_config);
 }
 
-void TraversableNavigable::start_display_list_rendering(NonnullRefPtr<Painting::DisplayList> display_list, NonnullRefPtr<Gfx::PaintingSurface> painting_surface, Function<void()>&& callback)
+void TraversableNavigable::start_display_list_rendering(NonnullRefPtr<Painting::DisplayList> display_list, NonnullRefPtr<Painting::BackingStore> backing_store, Function<void()>&& 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));
 }
 
 }

Libraries/LibWeb/HTML/TraversableNavigable.h

@@ -98,7 +98,7 @@ public:
     [[nodiscard]] GC::Ptr<DOM::Node> currently_focused_area();
 
     RefPtr<Painting::DisplayList> record_display_list(DevicePixelRect const&, PaintOptions);
-    void start_display_list_rendering(NonnullRefPtr<Painting::DisplayList> display_list, NonnullRefPtr<Gfx::PaintingSurface> painting_surface, Function<void()>&& callback);
+    void start_display_list_rendering(NonnullRefPtr<Painting::DisplayList>, NonnullRefPtr<Painting::BackingStore>, Function<void()>&& 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<Gfx::PaintingSurface> painting_surface_for_backing_store(Painting::BackingStore&);
-
 private:
     TraversableNavigable(GC::Ref<Page>);
 
@@ -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<Gfx::SkiaBackendContext> m_skia_backend_context;
-    HashMap<Gfx::Bitmap*, NonnullRefPtr<Gfx::PaintingSurface>> m_bitmap_to_surface;
 
     bool m_needs_repaint { true };
-
-    RenderingThread m_rendering_thread;
 };
 
 struct BrowsingContextAndDocument {

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<Web::QueuedInputEvent>& PageClient::input_event_queue()