Changeset 283489 in webkit

Timestamp:

Oct 4, 2021, 12:48:10 AM (5 years ago)

Author:

commit-queue@webkit.org

Message:

Regression (r283238)[ MacOS wk1 ] fast/mediacapturefromelement/CanvasCaptureMediaStream-webgl-events.html is timing out
​https://bugs.webkit.org/show_bug.cgi?id=231022

Patch by Kimmo Kinnunen <​kkinnunen@apple.com> on 2021-10-04
Reviewed by Youenn Fablet.

Originally the implementation would always return red frame, and the test would pass.
r283238 changed the implementation to not return a sample if there is not a display buffer,
as logically such cannot be used as a sample.
This broke the test case since the CanvasCaptureMediaStreamTrack would try to capture
the canvas display buffer during next runloop iteration (0s timeout) after each modification.
This does not work, as the display buffer is composed during "prepare for display"
phase.

Add CanvasBase observers to observe that display buffer has been prepared, and capture
the media sample after that observer has fired.

The test would work for wk2 due to timing related differences, preparation would have
typically run before the canvas capture 0s timeout.

Fixes fast/mediastream/captureStream/canvas3d.html for wk1.

• Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:

(WebCore::CanvasCaptureMediaStreamTrack::Source::startProducingData):
(WebCore::CanvasCaptureMediaStreamTrack::Source::canvasChanged):
(WebCore::CanvasCaptureMediaStreamTrack::Source::canvasDisplayBufferPrepared):

• Modules/mediastream/CanvasCaptureMediaStreamTrack.h:
• html/CanvasBase.cpp:

(WebCore::CanvasBase::addDisplayBufferObserver):
(WebCore::CanvasBase::removeDisplayBufferObserver):
(WebCore::CanvasBase::notifyObserversCanvasDisplayBufferPrepared):

• html/CanvasBase.h:

(WebCore::CanvasBase::hasDisplayBufferObservers const):

• html/canvas/WebGLRenderingContextBase.cpp:

(WebCore::WebGLRenderingContextBase::prepareForDisplay):
Move the "prepare only when the owner element is in the tree" logic to
its correct place to the caller, e.g. to the element itself.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp (modified) (3 diffs)
• Modules/mediastream/CanvasCaptureMediaStreamTrack.h (modified) (4 diffs)
• html/CanvasBase.cpp (modified) (1 diff)
• html/CanvasBase.h (modified) (4 diffs)
• html/HTMLCanvasElement.cpp (modified) (1 diff)
• html/canvas/WebGLRenderingContextBase.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-10-04  Kimmo Kinnunen  <kkinnunen@apple.com>
+
+        Regression (r283238)[ MacOS wk1 ] fast/mediacapturefromelement/CanvasCaptureMediaStream-webgl-events.html is timing out
+        https://bugs.webkit.org/show_bug.cgi?id=231022
+
+        Reviewed by Youenn Fablet.
+
+        Originally the implementation would always return red frame, and the test would pass.
+        r283238 changed the implementation to not return a sample if there is not a display buffer,
+        as logically such cannot be used as a sample.
+        This broke the test case since the CanvasCaptureMediaStreamTrack would try to capture
+        the canvas display buffer during next runloop iteration (0s timeout) after each modification.
+        This does not work, as the display buffer is composed during "prepare for display"
+        phase.
+
+        Add CanvasBase observers to observe that display buffer has been prepared, and capture
+        the media sample after that observer has fired.
+
+        The test would work for wk2 due to timing related differences, preparation would have
+        typically run before the canvas capture 0s timeout.
+
+        Fixes fast/mediastream/captureStream/canvas3d.html for wk1.
+
+        * Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:
+        (WebCore::CanvasCaptureMediaStreamTrack::Source::startProducingData):
+        (WebCore::CanvasCaptureMediaStreamTrack::Source::canvasChanged):
+        (WebCore::CanvasCaptureMediaStreamTrack::Source::canvasDisplayBufferPrepared):
+        * Modules/mediastream/CanvasCaptureMediaStreamTrack.h:
+        * html/CanvasBase.cpp:
+        (WebCore::CanvasBase::addDisplayBufferObserver):
+        (WebCore::CanvasBase::removeDisplayBufferObserver):
+        (WebCore::CanvasBase::notifyObserversCanvasDisplayBufferPrepared):
+        * html/CanvasBase.h:
+        (WebCore::CanvasBase::hasDisplayBufferObservers const):
+        * html/canvas/WebGLRenderingContextBase.cpp:
+        (WebCore::WebGLRenderingContextBase::prepareForDisplay):
+        Move the "prepare only when the owner element is in the tree" logic to
+        its correct place to the caller, e.g. to the element itself.
+
 2021-10-03  Antti Koivisto  <antti@apple.com>
 

trunk/Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp

@@ -90 +90 @@
         return;
     m_canvas->addObserver(*this);
+    m_canvas->addDisplayBufferObserver(*this);
 
     if (!m_frameRequestRate)
@@ -105 +106 @@
         return;
     m_canvas->removeObserver(*this);
+    m_canvas->removeDisplayBufferObserver(*this);
 }
 
@@ -153 +155 @@
 {
     ASSERT_UNUSED(canvas, m_canvas == &canvas);
-
+    if (m_canvas->renderingContext() && m_canvas->renderingContext()->needsPreparationForDisplay())
+        return;
+    scheduleCaptureCanvas();
+}
+
+void CanvasCaptureMediaStreamTrack::Source::scheduleCaptureCanvas()
+{
     // FIXME: We should try to generate the frame at the time the screen is being updated.
     if (m_captureCanvasTimer.isActive())
         return;
     m_captureCanvasTimer.startOneShot(0_s);
+}
+
+void CanvasCaptureMediaStreamTrack::Source::canvasDisplayBufferPrepared(CanvasBase& canvas)
+{
+    ASSERT_UNUSED(canvas, m_canvas == &canvas);
+    // FIXME: Here we should capture the image instead.
+    // However, submitting the sample to the receiver might cause layout,
+    // and currently the display preparation is done after layout.
+    scheduleCaptureCanvas();
 }
 

trunk/Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.h

@@ -31 +31 @@
 #include "Timer.h"
 #include <wtf/TypeCasts.h>
+#include <wtf/WeakPtr.h>
 
 namespace WebCore {
@@ -51 +52 @@
     const char* activeDOMObjectName() const override;
 
-    class Source final : public RealtimeMediaSource, private CanvasObserver {
+    class Source final : public RealtimeMediaSource, private CanvasObserver, private CanvasDisplayBufferObserver {
     public:
         static Ref<Source> create(HTMLCanvasElement&, std::optional<double>&& frameRequestRate);
@@ -61 +62 @@
         Source(HTMLCanvasElement&, std::optional<double>&&);
 
-        // CanvasObserver API
+        // CanvasObserver overrides.
         void canvasChanged(CanvasBase&, const std::optional<FloatRect>&) final;
         void canvasResized(CanvasBase&) final;
         void canvasDestroyed(CanvasBase&) final;
 
-        // RealtimeMediaSource API
+        // CanvasDisplayBufferObserver overrides.
+        void canvasDisplayBufferPrepared(CanvasBase&) final;
+
+        // RealtimeMediaSource overrides.
         void startProducingData() final;
         void stopProducingData()  final;
@@ -72 +76 @@
         const RealtimeMediaSourceSettings& settings() final;
         void settingsDidChange(OptionSet<RealtimeMediaSourceSettings::Flag>) final;
-
+        void scheduleCaptureCanvas();
         void captureCanvas();
         void requestFrameTimerFired();

trunk/Source/WebCore/html/CanvasBase.cpp

@@ -163 +163 @@
 }
 
+void CanvasBase::addDisplayBufferObserver(CanvasDisplayBufferObserver& observer)
+{
+    m_displayBufferObservers.add(&observer);
+}
+
+void CanvasBase::removeDisplayBufferObserver(CanvasDisplayBufferObserver& observer)
+{
+    m_displayBufferObservers.remove(observer);
+}
+
+void CanvasBase::notifyObserversCanvasDisplayBufferPrepared()
+{
+    for (auto& observer : m_displayBufferObservers)
+        observer.canvasDisplayBufferPrepared(*this);
+}
+
 HashSet<Element*> CanvasBase::cssCanvasClients() const
 {

trunk/Source/WebCore/html/CanvasBase.h

@@ -29 +29 @@
 #include <wtf/HashSet.h>
 #include <wtf/TypeCasts.h>
+#include <wtf/WeakHashSet.h>
 
 namespace WebCore {
@@ -53 +54 @@
     virtual void canvasResized(CanvasBase&) = 0;
     virtual void canvasDestroyed(CanvasBase&) = 0;
+};
+
+class CanvasDisplayBufferObserver : public CanMakeWeakPtr<CanvasDisplayBufferObserver> {
+public:
+    virtual ~CanvasDisplayBufferObserver() = default;
+
+    virtual void canvasDisplayBufferPrepared(CanvasBase&) = 0;
 };
 
@@ -93 +101 @@
     void notifyObserversCanvasResized();
     void notifyObserversCanvasDestroyed(); // Must be called in destruction before clearing m_context.
+    void addDisplayBufferObserver(CanvasDisplayBufferObserver&);
+    void removeDisplayBufferObserver(CanvasDisplayBufferObserver&);
+    void notifyObserversCanvasDisplayBufferPrepared();
+    bool hasDisplayBufferObservers() const { return !m_displayBufferObservers.computesEmpty(); }
 
     HashSet<Element*> cssCanvasClients() const;
@@ -133 +145 @@
 #endif
     HashSet<CanvasObserver*> m_observers;
+    WeakHashSet<CanvasDisplayBufferObserver> m_displayBufferObservers;
 };
 

trunk/Source/WebCore/html/HTMLCanvasElement.cpp

@@ -1055 +1055 @@
     ASSERT(needsPreparationForDisplay());
 
+    bool shouldPrepare = true;
+#if ENABLE(WEBGL)
+    // FIXME: Currently the below prepare skip logic is conservative and applies only to
+    // WebGL elements.
+    if (is<WebGLRenderingContextBase>(m_context)) {
+        // If the canvas is not in the document body, then it won't be
+        // composited and thus doesn't need preparation. Unfortunately
+        // it can't tell at the time it was added to the list, since it
+        // could be inserted or removed from the document body afterwards.
+        shouldPrepare = isInTreeScope() || hasDisplayBufferObservers();
+    }
+#endif
+    if (!shouldPrepare)
+        return;
     if (m_context)
         m_context->prepareForDisplay();
+    notifyObserversCanvasDisplayBufferPrepared();
 }
 

trunk/Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp

@@ -8176 +8176 @@
         return;
 
-    // If the canvas is not in the document body, then it won't be
-    // composited and thus doesn't need preparation. Unfortunately
-    // it can't tell at the time it was added to the list, since it
-    // could be inserted or removed from the document body afterwards.
-    auto canvas = htmlCanvas();
-    if (!canvas || !canvas->isInTreeScope())
-        return;
-
     m_context->prepareForDisplay();
 }