Bug 225304 - OffscreenCanvas should preserve context transform after transferToImageBitmap
Summary: OffscreenCanvas should preserve context transform after transferToImageBitmap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords: InRadar
Depends on:
Blocks: 183720
  Show dependency treegraph
 
Reported: 2021-05-03 07:24 PDT by Chris Lord
Modified: 2021-05-05 06:20 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.56 KB, patch)
2021-05-04 01:31 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (8.50 KB, patch)
2021-05-05 02:13 PDT, Chris Lord
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lord 2021-05-03 07:24:49 PDT
As the bug title says - currently if you call transferToImageBitmap on an OffscreenCanvas, the graphics context will be reset.
Comment 1 Chris Lord 2021-05-04 01:31:37 PDT
Created attachment 427643 [details]
Patch
Comment 2 Chris Lord 2021-05-04 02:23:17 PDT
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.
Comment 3 Darin Adler 2021-05-04 22:33:51 PDT
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().
Comment 4 Chris Lord 2021-05-05 02:13:51 PDT
Created attachment 427748 [details]
Patch
Comment 5 Chris Lord 2021-05-05 02:33:24 PDT
(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).
Comment 6 EWS 2021-05-05 02:49:53 PDT
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].
Comment 7 Radar WebKit Bug Importer 2021-05-05 02:50:25 PDT
<rdar://problem/77548954>