Skip to content

Commit

Permalink
Cherry-pick da97441. rdar://117708049
Browse files Browse the repository at this point in the history
    Draws to unused 2D contexts may consume excessive amount of memory on Cocoa
    https://bugs.webkit.org/show_bug.cgi?id=268608
    rdar://117708049

    Reviewed by Simon Fraser.

    Accelerated CG applies a drawn operation only once a specific draw
    operation limit is hit, if the result is needed as a source to other
    draw or on explicit flush. If the operations are not implicitly or
    explicitly flushed, the draws are retained in the draw queue.

    This causes memory leaks in cases where modifiable surfaces are used as
    sources for draws and and then subsequently the the surfaces are
    modified. The modifications force copy-on-write for the surfaces that
    have pending references in other draw queues.

    This can be triggered by drawing a canvas A to a 2D context B
    and then never using the result, subsequently modifying A.

    Fix by adding a list of flushed CanvasRenderingContexts to Document.
    Any 2D context modified during any JS callstack will be put to the
    flush list.

    Flush the contexts during rendering update, PrepareCanvases phase.
    Rename the rendering update PrepareCanvasesForDisplay phase
    PrepareCanvases phase, and do both.

    PrepareCanvases phase of Document now does two things:
      - flushes deferred operations
      - prepares for display

    Currently only Document manages the canvas preparation.
    This works for OffscreenCanvas and HTMLCanvasElement in Web main run
    loop.

    WorkerGlobalContext is not implemented in this patch.
    In future, PrepareCanvases phase of WorkerGlobalContext will do
    the flush operations, but not the prepares for display. OffscreenCanvas
    does not have the prepare for display, as it's not displaying.

    * Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:
    (WebCore::CanvasCaptureMediaStreamTrack::Source::canvasChanged):
    * Source/WebCore/dom/Document.cpp:
    (WebCore::Document::prepareCanvasesIfNeeded):
    (WebCore::Document::updateCanvasPreparationForDisplayOrFlush):
    (WebCore::Document::removeCanvasPreparationForDisplayOrFlush):
    (WebCore::Document::prepareCanvasesForDisplayIfNeeded): Deleted.
    (WebCore::Document::clearCanvasPreparation): Deleted.
    (WebCore::Document::canvasChanged): Deleted.
    (WebCore::Document::canvasDestroyed): Deleted.
    * Source/WebCore/dom/Document.h:

    Remove Document CanvasObserver interface. The interface was making
    the actual logic harder than needed and did not abstract anything, as
    the clearCanvasPreparation was a non-interface method anyway.

    The object which does the prepare (Document, WorkerGlobalContext), is
    always statically known at call site. Thus the call site does not need
    to jump thorough the canvasChanged hoop.

    * Source/WebCore/html/HTMLCanvasElement.cpp:
    (WebCore::HTMLCanvasElement::~HTMLCanvasElement):
    (WebCore::HTMLCanvasElement::createContext2d):
    (WebCore::HTMLCanvasElement::createContextWebGL):
    (WebCore::HTMLCanvasElement::createContextWebGPU):
    (WebCore::HTMLCanvasElement::didDraw):
    (WebCore::HTMLCanvasElement::setSurfaceSize):
    (WebCore::HTMLCanvasElement::createImageBuffer const):
    (WebCore::HTMLCanvasElement::didMoveToNewDocument):
    (WebCore::HTMLCanvasElement::insertedIntoAncestor): Deleted.
    (WebCore::HTMLCanvasElement::removedFromAncestor): Deleted.

    Remove redundant insertedIntoAncestor, removedFromAncestor handlers.
    These are called when the element is still in the same document, but
    detached from the tree. This does not cause any changes to the
    prepare list, as canvases need to be prepared regardless whether
    they're attached or detached.

    WebGL still will not prepareForDisplay if it's not in tree, unless
    it has been captured by media stream.

    * Source/WebCore/html/HTMLCanvasElement.h:
    * Source/WebCore/html/OffscreenCanvas.cpp:
    (WebCore::OffscreenCanvas::~OffscreenCanvas):
    (WebCore::OffscreenCanvas::didDraw):
    (WebCore::OffscreenCanvas::updateCanvasPreparation):
    (WebCore::OffscreenCanvas::removeCanvasPreparation):
    * Source/WebCore/html/OffscreenCanvas.h:
    * Source/WebCore/html/canvas/CanvasRenderingContext.h:
    (WebCore::CanvasRenderingContext::hasDeferredOperations const):
    (WebCore::CanvasRenderingContext::flushDeferredOperations):
    * Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:
    (WebCore::CanvasRenderingContext2DBase::hasDeferredOperations const):
    (WebCore::CanvasRenderingContext2DBase::flushDeferredOperations):
    (WebCore::CanvasRenderingContext2DBase::didDraw):
    (WebCore::CanvasRenderingContext2DBase::needsPreparationForDisplay const):
    * Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:
    * Source/WebCore/page/Page.cpp:
    (WebCore::Page::doAfterUpdateRendering):
    (WebCore::operator<<):
    * Source/WebCore/page/Page.h:
    * Source/WebCore/platform/graphics/ImageBuffer.cpp:
    (WebCore::ImageBuffer::flushDrawingContextAsync):
    * Source/WebCore/platform/graphics/ImageBuffer.h:
    (WebCore::ImageBuffer::prefersPreparationForDisplay): Deleted.

    Remove unused ImageBuffer::prefersPreparationForDisplay.

    Canonical link: https://commits.webkit.org/274164@main

Identifier: 272448.532@safari-7618.1.15.10-branch
  • Loading branch information
kkinnunen-apple authored and rjepstein committed Feb 8, 2024
1 parent 249ee2c commit 74f8798
Show file tree
Hide file tree
Showing 27 changed files with 158 additions and 206 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -152,13 +152,11 @@ void CanvasCaptureMediaStreamTrack::Source::canvasResized(CanvasBase& canvas)
setSize(IntSize(m_canvas->width(), m_canvas->height()));
}

void CanvasCaptureMediaStreamTrack::Source::canvasChanged(CanvasBase& canvas, const std::optional<FloatRect>&)
void CanvasCaptureMediaStreamTrack::Source::canvasChanged(CanvasBase&, const FloatRect&)
{
ASSERT_UNUSED(canvas, m_canvas == &canvas);
#if ENABLE(WEBGL)
if (m_canvas->renderingContext() && m_canvas->renderingContext()->needsPreparationForDisplay() && m_canvas->hasObserver(m_canvas->document()))
// If canvas needs preparation, the capture will be scheduled once document prepares the canvas.
if (m_canvas->needsPreparationForDisplay())
return;
#endif
scheduleCaptureCanvas();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class CanvasCaptureMediaStreamTrack final : public MediaStreamTrack {
Source(HTMLCanvasElement&, std::optional<double>&&);

// CanvasObserver overrides.
void canvasChanged(CanvasBase&, const std::optional<FloatRect>&) final;
void canvasChanged(CanvasBase&, const FloatRect&) final;
void canvasResized(CanvasBase&) final;
void canvasDestroyed(CanvasBase&) final;

Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/Modules/webxr/WebXRWebGLLayer.h
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ class WebXRWebGLLayer : public WebXRLayer, private CanvasObserver {
static IntSize computeNativeWebGLFramebufferResolution();
static IntSize computeRecommendedWebGLFramebufferResolution();

void canvasChanged(CanvasBase&, const std::optional<FloatRect>&) final { };
void canvasChanged(CanvasBase&, const FloatRect&) final { };
void canvasResized(CanvasBase&) final;
void canvasDestroyed(CanvasBase&) final { };
Ref<WebXRSession> m_session;
Expand Down
63 changes: 32 additions & 31 deletions Source/WebCore/dom/Document.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9948,38 +9948,45 @@ const CrossOriginOpenerPolicy& Document::crossOriginOpenerPolicy() const
return SecurityContext::crossOriginOpenerPolicy();
}

void Document::prepareCanvasesForDisplayIfNeeded()
void Document::prepareCanvasesForDisplayOrFlushIfNeeded()
{
// Some canvas contexts need to do work when rendering has finished but
// before their content is composited.

// FIXME: Calling prepareForDisplay should not call back into a method
// that would mutate our m_canvasesNeedingDisplayPreparation list. It
// would be nice if this could be enforced to remove the copyToVector.

auto canvases = copyToVectorOf<Ref<HTMLCanvasElement>>(m_canvasesNeedingDisplayPreparation);
m_canvasesNeedingDisplayPreparation.clear();
for (auto& canvas : canvases)
canvas->prepareForDisplay();
auto contexts = copyToVectorOf<WeakPtr<CanvasRenderingContext>>(m_canvasContextsToPrepare);
m_canvasContextsToPrepare.clear();
for (auto& weakContext : contexts) {
auto* context = weakContext.get();
if (!context)
continue;
// Some canvas contexts hold memory that should be periodically freed.
if (context->hasDeferredOperations())
context->flushDeferredOperations();

// Some canvases need to do work when rendering has finished but before their content is composited.
if (auto* htmlCanvas = dynamicDowncast<HTMLCanvasElement>(context->canvasBase())) {
if (htmlCanvas->needsPreparationForDisplay())
htmlCanvas->prepareForDisplay();
}
}
}

void Document::clearCanvasPreparation(HTMLCanvasElement& canvas)
void Document::addCanvasNeedingPreparationForDisplayOrFlush(CanvasBase& canvas)
{
m_canvasesNeedingDisplayPreparation.remove(canvas);
auto* context = canvas.renderingContext();
if (!context)
return;
if (context->hasDeferredOperations() || context->needsPreparationForDisplay()) {
bool shouldSchedule = m_canvasContextsToPrepare.isEmptyIgnoringNullReferences();
m_canvasContextsToPrepare.add(*context);
if (shouldSchedule)
scheduleRenderingUpdate(RenderingUpdateStep::PrepareCanvasesForDisplayOrFlush);
}
}

void Document::canvasChanged(CanvasBase& canvasBase, const std::optional<FloatRect>& changedRect)
void Document::removeCanvasNeedingPreparationForDisplayOrFlush(CanvasBase& canvas)
{
auto* canvas = dynamicDowncast<HTMLCanvasElement>(canvasBase);
if (canvas && canvas->needsPreparationForDisplay()) {
m_canvasesNeedingDisplayPreparation.add(*canvas);
// Schedule a rendering update to force handling of prepareForDisplay
// for any queued canvases. This is especially important for any canvas
// that is not in the DOM, as those don't have a rect to invalidate to
// trigger an update. <http://bugs.webkit.org/show_bug.cgi?id=240380>.
if (!changedRect)
scheduleRenderingUpdate(RenderingUpdateStep::PrepareCanvasesForDisplay);
}
auto* context = canvas.renderingContext();
if (!context)
return;
m_canvasContextsToPrepare.remove(*context);
}

void Document::updateSleepDisablerIfNeeded()
Expand All @@ -9992,12 +9999,6 @@ void Document::updateSleepDisablerIfNeeded()
m_sleepDisabler = nullptr;
}

void Document::canvasDestroyed(CanvasBase& canvasBase)
{
if (auto* canvasElement = dynamicDowncast<HTMLCanvasElement>(canvasBase))
m_canvasesNeedingDisplayPreparation.remove(*canvasElement);
}

JSC::VM& Document::vm()
{
return commonVM();
Expand Down
19 changes: 8 additions & 11 deletions Source/WebCore/dom/Document.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@

#pragma once

#include "CanvasObserver.h"
#include "Color.h"
#include "ContainerNode.h"
#include "ContextDestructionObserverInlines.h"
Expand Down Expand Up @@ -99,6 +98,7 @@ class CachedCSSStyleSheet;
class CachedFrameBase;
class CachedResourceLoader;
class CachedScript;
class CanvasRenderingContext;
class CanvasRenderingContext2D;
class CharacterData;
class Comment;
Expand Down Expand Up @@ -398,7 +398,6 @@ class Document
, public FrameDestructionObserver
, public Supplementable<Document>
, public Logger::Observer
, public CanvasObserver
, public ReportingClient {
WTF_MAKE_ISO_ALLOCATED_EXPORT(Document, WEBCORE_EXPORT);
public:
Expand Down Expand Up @@ -1848,11 +1847,9 @@ class Document
void setFragmentDirective(const String& fragmentDirective) { m_fragmentDirective = fragmentDirective; }
const String& fragmentDirective() const { return m_fragmentDirective; }

void prepareCanvasesForDisplayIfNeeded();
void clearCanvasPreparation(HTMLCanvasElement&);
void canvasChanged(CanvasBase&, const std::optional<FloatRect>&) final;
void canvasResized(CanvasBase&) final { };
void canvasDestroyed(CanvasBase&) final;
void prepareCanvasesForDisplayOrFlushIfNeeded();
void addCanvasNeedingPreparationForDisplayOrFlush(CanvasBase&);
void removeCanvasNeedingPreparationForDisplayOrFlush(CanvasBase&);

bool contains(const Node& node) const { return this == &node.treeScope() && node.isConnected(); }
bool contains(const Node* node) const { return node && contains(*node); }
Expand Down Expand Up @@ -2170,10 +2167,10 @@ class Document

std::unique_ptr<SVGDocumentExtensions> m_svgExtensions;

// Collection of canvas objects that need to do work after they've
// rendered but before compositing, for the next frame. The set is
// cleared after they've been called.
WeakHashSet<HTMLCanvasElement, WeakPtrImplWithEventTargetData> m_canvasesNeedingDisplayPreparation;
// Collection of canvas contexts that need periodic work in "PrepareCanvasesForDisplayOrFlush" phase of
// render update. Hold canvases via rendering context, since there is no common base class that
// would be managed.
WeakHashSet<CanvasRenderingContext> m_canvasContextsToPrepare;

HashMap<String, RefPtr<HTMLCanvasElement>> m_cssCanvasElements;

Expand Down
27 changes: 22 additions & 5 deletions Source/WebCore/html/CanvasBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -171,21 +171,24 @@ bool CanvasBase::hasObserver(CanvasObserver& observer) const
return m_observers.contains(observer);
}

void CanvasBase::notifyObserversCanvasChanged(const std::optional<FloatRect>& rect)
void CanvasBase::notifyObserversCanvasChanged(const FloatRect& rect)
{
for (auto& observer : m_observers)
observer.canvasChanged(*this, rect);
}

void CanvasBase::didDraw(const std::optional<FloatRect>& rect, ShouldApplyPostProcessingToDirtyRect shouldApplyPostProcessingToDirtyRect)
{
addCanvasNeedingPreparationForDisplayOrFlush();
IntRect dirtyRect { { }, size() };
if (rect)
dirtyRect.intersect(enclosingIntRect(*rect));
notifyObserversCanvasChanged(dirtyRect);

// FIXME: We should exclude rects with ShouldApplyPostProcessingToDirtyRect::No
if (shouldInjectNoiseBeforeReadback()) {
if (shouldApplyPostProcessingToDirtyRect == ShouldApplyPostProcessingToDirtyRect::Yes) {
if (rect)
m_canvasNoiseInjection.updateDirtyRect(intersection(enclosingIntRect(*rect), { { }, size() }));
else
m_canvasNoiseInjection.updateDirtyRect({ { }, size() });
m_canvasNoiseInjection.updateDirtyRect(dirtyRect);
} else if (!rect)
m_canvasNoiseInjection.clearDirtyRect();
}
Expand Down Expand Up @@ -356,6 +359,20 @@ void CanvasBase::recordLastFillText(const String& text)
m_lastFillText = text;
}

void CanvasBase::addCanvasNeedingPreparationForDisplayOrFlush()
{
if (auto* document = dynamicDowncast<Document>(scriptExecutionContext()))
document->addCanvasNeedingPreparationForDisplayOrFlush(*this);
// FIXME: WorkerGlobalContext does not have prepare phase yet.
}

void CanvasBase::removeCanvasNeedingPreparationForDisplayOrFlush()
{
if (auto* document = dynamicDowncast<Document>(scriptExecutionContext()))
document->removeCanvasNeedingPreparationForDisplayOrFlush(*this);
// FIXME: WorkerGlobalContext does not have prepare phase yet.
}

bool CanvasBase::postProcessPixelBufferResults(Ref<PixelBuffer>&& pixelBuffer) const
{
if (m_canvasNoiseHashSalt)
Expand Down
6 changes: 5 additions & 1 deletion Source/WebCore/html/CanvasBase.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class CanvasBase {
void addObserver(CanvasObserver&);
void removeObserver(CanvasObserver&);
bool hasObserver(CanvasObserver&) const;
void notifyObserversCanvasChanged(const std::optional<FloatRect>&);
void notifyObserversCanvasChanged(const FloatRect&);
void notifyObserversCanvasResized();
void notifyObserversCanvasDestroyed(); // Must be called in destruction before clearing m_context.
void addDisplayBufferObserver(CanvasDisplayBufferObserver&);
Expand All @@ -113,6 +113,7 @@ class CanvasBase {
virtual GraphicsContext* drawingContext() const;
virtual GraphicsContext* existingDrawingContext() const;

// !rect means caller knows the full canvas is invalidated previously.
void didDraw(const std::optional<FloatRect>& rect) { return didDraw(rect, ShouldApplyPostProcessingToDirtyRect::Yes); }
virtual void didDraw(const std::optional<FloatRect>&, ShouldApplyPostProcessingToDirtyRect);

Expand Down Expand Up @@ -149,12 +150,15 @@ class CanvasBase {

RefPtr<ImageBuffer> allocateImageBuffer() const;
String lastFillText() const { return m_lastFillText; }
void addCanvasNeedingPreparationForDisplayOrFlush();
void removeCanvasNeedingPreparationForDisplayOrFlush();

private:
bool shouldInjectNoiseBeforeReadback() const;
virtual void createImageBuffer() const { }
bool shouldAccelerate(uint64_t area) const;


mutable IntSize m_size;
mutable Lock m_imageBufferAssignmentLock;
mutable RefPtr<ImageBuffer> m_imageBuffer;
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/html/CanvasObserver.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class CanvasObserver : public CanMakeWeakPtr<CanvasObserver> {

virtual bool isStyleCanvasImage() const { return false; }

virtual void canvasChanged(CanvasBase&, const std::optional<FloatRect>& changedRect) = 0;
virtual void canvasChanged(CanvasBase&, const FloatRect& changedRect) = 0;
virtual void canvasResized(CanvasBase&) = 0;
virtual void canvasDestroyed(CanvasBase&) = 0;
};
Expand Down
Loading

0 comments on commit 74f8798

Please sign in to comment.