Bug 202797 – Implement Canvas.transferControlToOffscreen and OffscreenCanvasRenderingContext2D.commit

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Chris Lord]

Specs:

https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-transfercontroltooffscreen
https://html.spec.whatwg.org/multipage/canvas.html#offscreencontext2d-commit

[Chris Lord]

Created attachment 380645 [details]
Patch

[Chris Lord]

Created attachment 390644 [details]
Patch

[Chris Lord]

Created attachment 395065 [details]
Patch

[Chris Lord]

Created attachment 395157 [details]
Patch

[Darin Adler]

Comment on attachment 395157 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395157&action=review

Looks good overall. Lots of coding style comments. Generally confused about the threading strategy; seems likely there are mistakes in that. It’s tricky to correctly use a RefCounted object from multiple threads and this doesn’t seem to have a disciplined approach to that.

review- for now; maybe Chris Dumez can help us vet the threading part of a future cut at the patch

> Source/WebCore/html/HTMLCanvasElement.cpp:746
> +    RefPtr<OffscreenCanvas> canvas = OffscreenCanvas::create(context, width(), height());

This should use auto. And it should be a Ref, not RefPtr.

> Source/WebCore/html/HTMLCanvasElement.cpp:747
> +    canvas->setPlaceholderCanvas(makeWeakPtr(*this));

Can this be passed to an overload of the create function instead?

> Source/WebCore/html/HTMLCanvasElement.cpp:753
> +#if USE(TEXTURE_MAPPER_GL)
> +    // Need to make sure a RenderLayer and compositing layer get created for the Canvas.
> +    invalidateStyleAndLayerComposition();
> +#endif

This seems like oblique side effect programming. How does "invalidate" make sure something is created? Can we do better?

Should not capitalize "Canvas".

> Source/WebCore/html/HTMLCanvasElement.h:101
> +    ExceptionOr<RefPtr<OffscreenCanvas>> transferControlToOffscreen(ScriptExecutionContext&);

This should be ExceptionOr<Ref> instead. RefPtr means the result can be null even when there is no exception; this result can’t be.

> Source/WebCore/html/OffscreenCanvas.cpp:80
> +    callOnMainThread([canvas = detachedCanvas->takePlaceholderCanvas(), offscreenCanvas = clone.ptr()] () mutable {

Seems peculiar and unsafe to use .ptr() here, turning offscreenCanvas into a raw pointer; normally I would have expected copyRef to pass a Ref<> instead. But also unclear how it’s thread-safe to pass a non-threadsafe-ref-counted offscreen canvas from one thread to another. What prevents this lambda from using a stale pointer after the canvas has been destroyed?

Not clear at all to me the role of the main thread here and threading strategy.

> Source/WebCore/html/OffscreenCanvas.cpp:84
> +                PlaceholderRenderingContext& placeholderContext = downcast<PlaceholderRenderingContext>(*renderingContext);
> +                placeholderContext.setOffscreenCanvas(offscreenCanvas);

This should use auto& or be a one-liner.

    downcast<PlaceholderRenderingContext>(*renderingContext).setOffscreenCanvas(offscreenCanvas);

Is there some way to write this code without the downcast? I am unclear on where the type guarantee comes from.

> Source/WebCore/html/OffscreenCanvas.cpp:149
> +                return Optional<OffscreenRenderingContext> { WTF::nullopt };

I’ve seen some other people have success with multiple sets of braces without the type names. For example, this might work here:

    return { { WTF::nullopt } };

Maybe give it a try? The many other places here we had to write out that long type name, instead we might just be able to add one more set of parentheses. And even remove some of the other type names.

> Source/WebCore/html/OffscreenCanvas.cpp:345
> +        std::unique_ptr<ImageBuffer> buffer = ImageBuffer::create(FloatSize(m_pendingCommitData->size()), RenderingMode::Unaccelerated);

auto

Also doesn’t make sense that ImageBuffer supplies a create function that returns a unique_ptr. Normally we use create functions when something is reference counted. If we are using unique_ptr and not intrusive reference counting then we should instead have public constructors and use makeUnique at the call sites.

> Source/WebCore/html/OffscreenCanvas.cpp:357
> +    // TODO: Transfer texture over if we're using accelerated compositing

WebKit project uses FIXME, not TODO. Lets not mix them in.

> Source/WebCore/html/OffscreenCanvas.cpp:382
> +        ScriptExecutionContext* context = scriptExecutionContext();

auto& context = *scriptExecutionContext();

> Source/WebCore/html/OffscreenCanvas.h:38
> +#include <JavaScriptCore/Uint8ClampedArray.h>

Mysterious include addition. Why?

> Source/WebCore/html/OffscreenCanvas.h:119
> +    HTMLCanvasElement* placeholderCanvas() const;

What is this public function for? I can’t find any uses of it?

> Source/WebCore/html/OffscreenCanvas.h:120
> +    void commitToPlaceholderCanvas();

Why is this public?

> Source/WebCore/html/OffscreenCanvas.h:163
> +    bool m_pendingCommitTask { false };

This local variable name sounds like it is a variable that holds a "pending commit task". For best naming of boolean, think of how to explain it to someone. One example could be m_hasPendingCommitTask. But you could probably come up with an even better name.

> Source/WebCore/html/OffscreenCanvas.h:168
> +    bool m_pendingCommit { false };
> +    RefPtr<ImageData> m_pendingCommitData;

Very unclear the relationship between the boolean and the data.

Also, I never see m_pendingCommitData set to nullptr after the commit. I think it probably should be. I suspect we can get rid of the boolean and use the nullity instead.

> Source/WebCore/html/canvas/OffscreenCanvasRenderingContext2D.cpp:54
> +    OffscreenCanvas& offscreenCanvas = downcast<OffscreenCanvas>(canvasBase());
> +    offscreenCanvas.commitToPlaceholderCanvas();

Should write this as a one-liner:

    canvas().commitToPlaceholderCanvas();

Also might consider just putting this in the header as long as it doesn’t create the need for a new include.

> Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:57
> +void PlaceholderRenderingContext::setOffscreenCanvas(OffscreenCanvas* offscreenCanvas)
> +{
> +    ASSERT(offscreenCanvas);
> +    m_offscreenCanvas = offscreenCanvas;
>  }

When we take a pointer and assert it’s not null, that means we should be taking a reference instead.

> Source/WebCore/html/canvas/PlaceholderRenderingContext.h:39
> +    PlaceholderRenderingContext(CanvasBase&, OffscreenCanvas*);

This should take OffscreenCanvas&.

> Source/WebCore/html/canvas/PlaceholderRenderingContext.h:43
> +    void setOffscreenCanvas(OffscreenCanvas*);

This should take OffscreenCanvas&.

> Source/WebCore/html/canvas/PlaceholderRenderingContext.h:48
> +    OffscreenCanvas* m_offscreenCanvas;

Unclear how it’s safe to use a raw pointer here. What guarantees this object won’t outlive the OffscreenCanvas?

[Chris Lord]

Created attachment 395666 [details]
Patch

[Chris Lord]

I think I've addressed all the review comments, but whether it's adequate or not is another question;

I think the most contentious part is still setting the placeholder canvas on an OffscreenCanvas created from a DetachedOffscreenCanvas. For context, DetachedOffscreenCanvas is the object created when transferring an OffscreenCanvas from the main thread to a worker. The placeholder canvas weak reference is only valid on the main thread and should only be set there, but the OffscreenCanvas it needs to be set on is in a worker.

I've (partially?) resolved the issue of a possibly stale pointer here by changing callOnMainThread to callOnMainThreadAndWait. The OffscreenCanvas can't be destroyed on the main thread, so the alternative way to fix this would be to take an extra reference and call back into the OffscreenCanvas's execution context to release it (I've seen this done elsewhere in the codebase). I don't have strong feelings on which is more desirable and am happy to change this (or do something different entirely, if there's any guidance).

The PlaceholderRenderingContext doesn't actually need a reference to the OffscreenCanvas in this patch (this is something that crept in from bug 202798), so this can be resolved in that patch if necessary. For this one, I just removed it.

re commitToPlaceholder being public, it's used by OffscreenCanvasRenderingContext2D.

[Dean Jackson]

Comment on attachment 395666 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395666&action=review

It still would be nice if cdumez verified the threading bits.

> Source/WebCore/html/HTMLCanvasElement.cpp:941
> +    if (isControlledByOffscreen() && oldSize != size()) {
> +        setAttributeWithoutSynchronization(widthAttr, AtomString::number(width()));
> +        setAttributeWithoutSynchronization(heightAttr, AtomString::number(height()));
> +        notifyObserversCanvasResized();
> +        invalidateStyleAndRenderersForSubtree();
> +    }

Why is this only for placeholder canvas objects? Does the specification explicitly say we should update the attributes? I assume this is the end of the call to transferFromImageBitmap().

> Source/WebCore/html/OffscreenCanvas.cpp:150
>              if (!is<OffscreenCanvasRenderingContext2D>(*m_context))

I don't think we can ever get this case. Am I mistaken? If not, I wonder if we should just assert (or let downcast do that).

> Source/WebCore/html/OffscreenCanvas.cpp:164
> +            if (is<WebGLRenderingContext>(*m_context))

Same here.

> Source/WebCore/html/OffscreenCanvas.cpp:341
> +        std::unique_ptr<ImageBuffer> buffer = ImageBuffer::create(FloatSize(m_pendingCommitData->size()), RenderingMode::Unaccelerated);

Agree with Darin that we should probably update ImageBuffer::create uses to makeUnique, but not in this patch.

> Source/WebCore/html/OffscreenCanvas.cpp:379
> +        auto& context = *scriptExecutionContext();
> +        m_hasScheduledCommit = context.postTaskTo(context.contextIdentifier(), [this] (ScriptExecutionContext&) {

Suggest using a different name here for "context", which could be confused with the rendering context.

> Source/WebCore/html/OffscreenCanvas.h:69
> +    WeakPtr<HTMLCanvasElement> takePlaceholderCanvas();

I wonder if this should be givePlaceholderCanvas() because the action is being called on the DetachedOffscreenCanvas. (Or, releasePlaceholderCanvas, except it is only a weakptr so "release" isn't clear)

> Source/WebCore/html/canvas/OffscreenCanvasRenderingContext2D.cpp:54
> +    OffscreenCanvas& offscreenCanvas = downcast<OffscreenCanvas>(canvasBase());
> +    offscreenCanvas.commitToPlaceholderCanvas();

Darin mentioned doing this in a single line.

[Chris Lord]

(In reply to Dean Jackson from comment #8)
> Comment on attachment 395666 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395666&action=review
> 
> It still would be nice if cdumez verified the threading bits.

I'll ping him, thanks for your review!

> > Source/WebCore/html/HTMLCanvasElement.cpp:941
> > +    if (isControlledByOffscreen() && oldSize != size()) {
> > +        setAttributeWithoutSynchronization(widthAttr, AtomString::number(width()));
> > +        setAttributeWithoutSynchronization(heightAttr, AtomString::number(height()));
> > +        notifyObserversCanvasResized();
> > +        invalidateStyleAndRenderersForSubtree();
> > +    }
> 
> Why is this only for placeholder canvas objects? Does the specification
> explicitly say we should update the attributes? I assume this is the end of
> the call to transferFromImageBitmap().

Yes, the spec says that the width/height attributes need to be updated when the width/height of the OffscreenCanvas change - this happens when a commit happens. You can see this in the first note here: https://html.spec.whatwg.org/multipage/canvas.html#offscreencontext2d-commit

> > Source/WebCore/html/OffscreenCanvas.cpp:150
> >              if (!is<OffscreenCanvasRenderingContext2D>(*m_context))
> 
> I don't think we can ever get this case. Am I mistaken? If not, I wonder if
> we should just assert (or let downcast do that).
> 
> > Source/WebCore/html/OffscreenCanvas.cpp:164
> > +            if (is<WebGLRenderingContext>(*m_context))
> 
> Same here.

These can both happen, if you call getContext(<type>), the first time when there's no context, it will be created. If you then call it with a different type, you may reach these cases (in which, the spec says to return null). These are covered by the w3c tests that are marked as passing in this patch.

> > Source/WebCore/html/OffscreenCanvas.cpp:341
> > +        std::unique_ptr<ImageBuffer> buffer = ImageBuffer::create(FloatSize(m_pendingCommitData->size()), RenderingMode::Unaccelerated);
> 
> Agree with Darin that we should probably update ImageBuffer::create uses to
> makeUnique, but not in this patch.
> 
> > Source/WebCore/html/OffscreenCanvas.cpp:379
> > +        auto& context = *scriptExecutionContext();
> > +        m_hasScheduledCommit = context.postTaskTo(context.contextIdentifier(), [this] (ScriptExecutionContext&) {
> 
> Suggest using a different name here for "context", which could be confused
> with the rendering context.

Good call, I'll go with the full scriptExecutionContext.

> > Source/WebCore/html/OffscreenCanvas.h:69
> > +    WeakPtr<HTMLCanvasElement> takePlaceholderCanvas();
> 
> I wonder if this should be givePlaceholderCanvas() because the action is
> being called on the DetachedOffscreenCanvas. (Or, releasePlaceholderCanvas,
> except it is only a weakptr so "release" isn't clear)

I think where I've seen similar functions, its been given the 'take' verb, but I don't have strong feelings about it - I'll leave as is unless you feel more strongly it should change.

> > Source/WebCore/html/canvas/OffscreenCanvasRenderingContext2D.cpp:54
> > +    OffscreenCanvas& offscreenCanvas = downcast<OffscreenCanvas>(canvasBase());
> > +    offscreenCanvas.commitToPlaceholderCanvas();
> 
> Darin mentioned doing this in a single line.

Sorry, missed this - thanks!

[Chris Dumez]

Comment on attachment 395666 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395666&action=review

> Source/WebCore/html/OffscreenCanvas.cpp:80
> +    callOnMainThreadAndWait([canvas = detachedCanvas->takePlaceholderCanvas(), offscreenCanvas = clone.ptr()] () mutable {

What is this callOnMainThreadAndWait() for? Also, why is this factory doing the callOnMainThead () to set the placeholder canvas but not the factory below?

> Source/WebCore/html/OffscreenCanvas.cpp:332
> +    m_placeholderCanvas = canvas;

WTFMove(canvas)

> Source/WebCore/html/OffscreenCanvas.cpp:337
> +    callOnMainThread([this] () mutable {

What guarantees that |this| is still alive once we're on the main thread?

>> Source/WebCore/html/OffscreenCanvas.cpp:379
>> +        m_hasScheduledCommit = context.postTaskTo(context.contextIdentifier(), [this] (ScriptExecutionContext&) {
> 
> Suggest using a different name here for "context", which could be confused with the rendering context.

What guarantees that |this| is still alive after the task has posted?

Also, I think you want context.postTast([]). postTaskTo() is a static, which is why it takes the contextID as parameter. However, you already have the script execution context object here.

[Chris Lord]

Created attachment 397666 [details]
Patch

[Chris Lord]

Created attachment 397669 [details]
Patch

[Chris Lord]

(In reply to Chris Dumez from comment #10)
> Comment on attachment 395666 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395666&action=review
> 
> > Source/WebCore/html/OffscreenCanvas.cpp:80
> > +    callOnMainThreadAndWait([canvas = detachedCanvas->takePlaceholderCanvas(), offscreenCanvas = clone.ptr()] () mutable {
> 
> What is this callOnMainThreadAndWait() for? Also, why is this factory doing
> the callOnMainThead () to set the placeholder canvas but not the factory
> below?

We don't want the OffscreenCanvas to be destroyed on the main thread, and the alternative would be to callOnMainThread and then postTask inside that callback to release the reference on it (I suppose?), which seemed overkill vs. just waiting. Happy to change or open to alternatives though.

The call below is expected to only be called from the main thread, so it doesn't need this - I've added an assert in setPlaceholderCanvas to make this clearer/safer.

> > Source/WebCore/html/OffscreenCanvas.cpp:332
> > +    m_placeholderCanvas = canvas;
> 
> WTFMove(canvas)

Thanks, fixed.

> > Source/WebCore/html/OffscreenCanvas.cpp:337
> > +    callOnMainThread([this] () mutable {
> 
> What guarantees that |this| is still alive once we're on the main thread?

You're right and I missed this, updating now...

> >> Source/WebCore/html/OffscreenCanvas.cpp:379
> >> +        m_hasScheduledCommit = context.postTaskTo(context.contextIdentifier(), [this] (ScriptExecutionContext&) {
> > 
> > Suggest using a different name here for "context", which could be confused with the rendering context.
> 
> What guarantees that |this| is still alive after the task has posted?

You're right here too and I've fixed this.

> Also, I think you want context.postTast([]). postTaskTo() is a static, which
> is why it takes the contextID as parameter. However, you already have the
> script execution context object here.

Thanks, fixed.

[Chris Lord]

Created attachment 397677 [details]
Patch

[Chris Lord]

With this revision of the patch, I've made sure that the placeholder is only referenced on the main thread, that the weak reference gets moved over to the main thread on destruction and that all main thread callbacks take references as necessary and release them on the appropriate thread. There are no waiting calls anymore.

[Chris Dumez]

Comment on attachment 397677 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397677&action=review

> Source/WebCore/html/OffscreenCanvas.cpp:54
> +    , m_placeholderCanvas(WTFMove(placeholderCanvas))

makeWeakPtr()

> Source/WebCore/html/OffscreenCanvas.cpp:66
> +    return WTFMove(m_placeholderCanvas);

I believe people prefer to use std::exchange(m_placeholderCanvas, nullptr) nowadays rather than rely on specific behaviors of move constructors.

> Source/WebCore/html/OffscreenCanvas.cpp:84
> +        offscreenCanvas->scriptExecutionContext()->postTask([releaseOffscreenCanvas = WTFMove(offscreenCanvas)] (ScriptExecutionContext&) { });

I believe you want a CleanupTask here to guarantee that the task gets processed even if the thread is getting terminated:
{ ScriptExecutionContext::Task::CleanupTask, [releaseOffscreenCanvas = WTFMove(offscreenCanvas)] (ScriptExecutionContext&) { }}

This is particularly important given that offscreenCanvas is not ThreadSafeRefCounted.

> Source/WebCore/html/OffscreenCanvas.cpp:105
> +    callOnMainThread([placeholder = WTFMove(m_placeholderCanvas)] { });

This is not necessary. You can destroy a WeakPtr from any thread (you just cannot deref it from any thread).

> Source/WebCore/html/OffscreenCanvas.cpp:336
> +    m_placeholderCanvas = WTFMove(canvas);

makeWeakPtr()

> Source/WebCore/html/OffscreenCanvas.h:64
> +    DetachedOffscreenCanvas(std::unique_ptr<ImageBuffer>&&, const IntSize&, bool originClean, WeakPtr<HTMLCanvasElement>&& placeholderCanvas);

We usually pass regular pointers / references as parameter and let the implementation decide if they need a WeakPtr.

> Source/WebCore/html/OffscreenCanvas.h:94
> +    static Ref<OffscreenCanvas> create(ScriptExecutionContext&, WeakPtr<HTMLCanvasElement>&&);

Same comment about passing a WeakPtr.

> Source/WebCore/html/OffscreenCanvas.h:149
> +    void setPlaceholderCanvas(WeakPtr<HTMLCanvasElement>&&);

I believe our usual pattern is to pass a HTMLCanvasElement& or a HTMLCanvasElement* and let the implementation of setPlaceholderCanvas() decide what to do (here create a weak pointer using makeWeakPtr()). Any reason you cannot do the same here?

[Chris Lord]

Created attachment 397691 [details]
Patch

[Chris Lord]

I think this is all the changes suggested in comment #16 made. In addition, I've reinstated the boolean m_hasPendingCommitData - we don't want to destroy m_pendingCommitData on the main thread. This causes an assert to get hit after bug 202798.

[Chris Lord]

Created attachment 397702 [details]
Patch

[Chris Lord]

And one last change to clear commit data in the destructor.

[Chris Lord]

Created attachment 397814 [details]
Patch

[Chris Lord]

(In reply to Chris Lord from comment #20)
> And one last change to clear commit data in the destructor.

Sorry for the churn, obviously wasn't thinking clearly at the end of the day - removed this change. It's not needed as while there's a commit pending, a reference is held, so the destructor will never be triggered.

[Chris Lord]

Created attachment 398046 [details]
Patch

[Chris Lord]

From working on bug 202798, I found an incorrect assumption I made in this patch - detach can happen off the main thread, so it isn't safe to create the weak pointer again. Instead, to avoid having a weak pointer as a parameter, I made OffscreenCanvas a friend class and it sets the member itself in detach, but I'm obviously open to doing that other ways.

I also corrected the behaviour of resize - it was incorrectly calling an invalidate function instead of just operating on the renderer, as can be seen in reset(), which would normally happen on resize. This, I assume, cuts down on unnecessary layout work when the canvas is resized via an OffscreenCanvas size change.

[Chris Lord]

Created attachment 398054 [details]
Patch

[Chris Lord]

Created attachment 406369 [details]
Patch

[Chris Lord]

Created attachment 406451 [details]
Patch

[EWS]

Committed r265543: <https://trac.webkit.org/changeset/265543>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406451 [details].

[Radar WebKit Bug Importer]

<rdar://problem/66894727>