Bug 225304 – OffscreenCanvas should preserve context transform after transferToImageBitmap

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

[Chris Lord]

As the bug title says - currently if you call transferToImageBitmap on an OffscreenCanvas, the graphics context will be reset.

[Chris Lord]

Created attachment 427643 [details]
Patch

[Chris Lord]

Just a note, I guess there were 3 'obvious' options to fix this;

1- Figure out how to decouple GraphicsContext from ImageBuffer
2- Replay the state stack on a new GraphicsContext
3- Return a copy of the buffer instead of the original buffer

I went with option 3 as it's by far the easiest, smallest and least invasive change. I played around with option 2 and it's very feasible, but it feels a bit hacky. I didn't even attempt option 1 as that seems like a pretty huge departure from how things are setup right now.

The downside of option 3 is that you end up with a copy and a clear rather than just an allocate and clear, but given transferToImageBitmap isn't really expected to be fast, this doesn't seem unreasonable to me (we have similar work-arounds for similar problems elsewhere in canvas).

This is one of those areas where I feel dogged adherence to the spec is actually just harming both browsers and users, but it's tested in WPT and it's pretty low effort to fix this.

[Darin Adler]

Comment on attachment 427643 [details]
Patch

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

> Source/WebCore/html/OffscreenCanvas.cpp:271
> +        // As the canvas context state is stored in GraphicsContext, which is tightly bound
> +        // to buffer(), to avoid resetting the context state, we have to make a copy and
> +        // clear the original buffer rather than returning the original buffer.

The comment "tightly bound to buffer()" seems to say that we can’t possibly get a GraphicsContext to release its current buffer and allocate a new one.

Is that true, maybe due to limitations to CGGraphicsContext on Apple’s platforms, or is this simply new work we’d need to do in GraphicsContext?

> Source/WebCore/html/OffscreenCanvas.cpp:272
> +        ASSERT(buffer() && drawingContext());

Maybe assert these separately?

But also, we don’t normally have to assert pointers are non-null; dereferencing them usually hits a null dereference quickly without us writing any additional code.

And also, if we are using m_context below, and want to assert it’s not null, then we should assert that rather than drawingContext().

[Chris Lord]

Created attachment 427748 [details]
Patch

[Chris Lord]

(In reply to Darin Adler from comment #3)
> Comment on attachment 427643 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427643&action=review
> 
> > Source/WebCore/html/OffscreenCanvas.cpp:271
> > +        // As the canvas context state is stored in GraphicsContext, which is tightly bound
> > +        // to buffer(), to avoid resetting the context state, we have to make a copy and
> > +        // clear the original buffer rather than returning the original buffer.
> 
> The comment "tightly bound to buffer()" seems to say that we can’t possibly
> get a GraphicsContext to release its current buffer and allocate a new one.
> 
> Is that true, maybe due to limitations to CGGraphicsContext on Apple’s
> platforms, or is this simply new work we’d need to do in GraphicsContext?

I've changed this comment, I don't think I picked my words well - the GraphicsContext is created and owned by the ImageBuffer and CanvasBase always accesses it through the image buffer. I think being able to release the associated surface and recreate one would involve some platform-specific plumbing that isn't currently there and may be non-trivial.

It would be a lot easier to do option 2 in comment #2 than this.

> > Source/WebCore/html/OffscreenCanvas.cpp:272
> > +        ASSERT(buffer() && drawingContext());
> 
> Maybe assert these separately?
> 
> But also, we don’t normally have to assert pointers are non-null;
> dereferencing them usually hits a null dereference quickly without us
> writing any additional code.
> 
> And also, if we are using m_context below, and want to assert it’s not null,
> then we should assert that rather than drawingContext().

I've just removed this assert, none of those pointers can be null at that point unless something went catastrophically wrong.

m_context is checked at the start of the function and nothing is called that can destroy it, buffer() is checked just above with the !m_hasCreatedImageBuffer check and drawingContext() can only be null if m_context isn't a 2d context (it is and is checked within this block) and buffer() returns null (which shouldn't happen because m_hasCreatedImageBuffer is true).

[EWS]

Committed r277011 (237327@main): <https://commits.webkit.org/237327@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/77548954>