Bug 219760 - REGRESSION (r270458): Canvas painting is broken when enabling GPU process for DOM
Summary: REGRESSION (r270458): Canvas painting is broken when enabling GPU process for...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-10 15:56 PST by Wenson Hsieh
Modified: 2020-12-11 08:08 PST (History)
10 users (show)

See Also:


Attachments
Patch (3.00 KB, patch)
2020-12-10 16:32 PST, Wenson Hsieh
sabouhallawa: review-
Details | Formatted Diff | Diff
For EWS (1.47 KB, patch)
2020-12-10 17:26 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (1.89 KB, patch)
2020-12-10 21:25 PST, Wenson Hsieh
thorton: review+
Details | Formatted Diff | Diff
Patch for landing (1.88 KB, patch)
2020-12-11 07:27 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2020-12-10 15:56:21 PST
SSIA
Comment 1 Said Abou-Hallawa 2020-12-10 16:06:25 PST
The canvas to canvas painting has been broken since bug 218773 was introduced. And the flushing was disabled in Recorder::drawImageBuffer() till r270645 was landed. So what dud exactly r270458 break in the canvas to canvas painting scenario?
Comment 2 Wenson Hsieh 2020-12-10 16:24:53 PST
(In reply to Said Abou-Hallawa from comment #1)
> The canvas to canvas painting has been broken since bug 218773 was
> introduced. And the flushing was disabled in Recorder::drawImageBuffer()
> till r270645 was landed. So what dud exactly r270458 break in the canvas to
> canvas painting scenario?

Good point — retitled the bug to be more accurate.
Comment 3 Said Abou-Hallawa 2020-12-10 16:28:22 PST
I am still not clear on what is wrong here. Is the drawing inside the canvas wrong? Or is drawing the canvas itself as a NativeImage to a canvas to a layer wrong?

Can't you attach a test case to look at?
Comment 4 Wenson Hsieh 2020-12-10 16:32:27 PST
Created attachment 415943 [details]
Patch
Comment 5 Wenson Hsieh 2020-12-10 16:32:58 PST Comment hidden (obsolete)
Comment 6 Wenson Hsieh 2020-12-10 16:35:10 PST
(In reply to Said Abou-Hallawa from comment #3)
> I am still not clear on what is wrong here. Is the drawing inside the canvas
> wrong? Or is drawing the canvas itself as a NativeImage to a canvas to a
> layer wrong?
> 
> Can't you attach a test case to look at?

The issue is that ImageBufferShareableIOSurfaceBackend::backendSize() now always returns (0, 0), which breaks various parts of painting code running in the web process (the canvas => canvas case is just one instance).
Comment 7 Said Abou-Hallawa 2020-12-10 16:52:49 PST
Comment on attachment 415943 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:61
> +IntSize ImageBufferBackend::backendSize() const
> +{
> +    return calculateBackendSize(logicalSize(), resolutionScale());
> +}

I do not think this is the right approach.

If you want to add this function, what is the point of having these functions?

ImageBufferCairoSurfaceBackend::backendSize()
ImageBufferCGBitmapBackend::backendSize()
ImageBufferIOSurfaceBackend::backendSize()

They can all be deleted if you make this change. When I did r270458, I thought because ImageBufferShareableIOSurfaceBackend does not have an IOSurface, it makes sense to return a backendSize = { 0, 0 }. In fact I thought we should ASSERT_NOT_REACHED if ImageBufferBackend::backendSize() is ever called. The goal of this change was to have a single place to get the backendSize which is the backend itself (IOSurface or CGContext) not from a calculated value.

Are you sure you want the backendSize not the logicalSize?
Comment 8 Wenson Hsieh 2020-12-10 16:59:30 PST
(In reply to Said Abou-Hallawa from comment #7)
> Comment on attachment 415943 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415943&action=review
> 
> > Source/WebCore/platform/graphics/ImageBufferBackend.cpp:61
> > +IntSize ImageBufferBackend::backendSize() const
> > +{
> > +    return calculateBackendSize(logicalSize(), resolutionScale());
> > +}
> 
> I do not think this is the right approach.
> 
> If you want to add this function, what is the point of having these
> functions?
> 
> ImageBufferCairoSurfaceBackend::backendSize()
> ImageBufferCGBitmapBackend::backendSize()
> ImageBufferIOSurfaceBackend::backendSize()
> 
> They can all be deleted if you make this change. When I did r270458, I
> thought because ImageBufferShareableIOSurfaceBackend does not have an
> IOSurface, it makes sense to return a backendSize = { 0, 0 }. In fact I
> thought we should ASSERT_NOT_REACHED if ImageBufferBackend::backendSize() is
> ever called. The goal of this change was to have a single place to get the
> backendSize which is the backend itself (IOSurface or CGContext) not from a
> calculated value.

I see — it wasn't clear to me that that was the intent. In that case, it sounds like we'll want to audit the call sites of backendSize() and use logicalSize() instead, if possible.

> 
> Are you sure you want the backendSize not the logicalSize?
Comment 9 Tim Horton 2020-12-10 17:01:57 PST
If we go that way, maybe we make it assert instead of just be empty?
Comment 10 Wenson Hsieh 2020-12-10 17:03:40 PST
(In reply to Tim Horton from comment #9)
> If we go that way, maybe we make it assert instead of just be empty?

Yeah, it should really be an assertion (and maybe even override ImageBufferShareableIOSurfaceBackend::backendSize() to explicitly return an empty size). This would've made the intention clearer.
Comment 11 Wenson Hsieh 2020-12-10 17:26:08 PST
Created attachment 415950 [details]
For EWS
Comment 12 Wenson Hsieh 2020-12-10 21:25:20 PST
Created attachment 415965 [details]
Patch
Comment 13 Tim Horton 2020-12-10 22:52:21 PST
Comment on attachment 415965 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        changing image buffers. When enabling GPU process for canvas, `ImageBufferShareableIOSurfaceBackend::backendSize`

I think this should say "DOM" not "canvas"
Comment 14 Wenson Hsieh 2020-12-11 07:26:06 PST
(In reply to Tim Horton from comment #13)
> Comment on attachment 415965 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415965&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        changing image buffers. When enabling GPU process for canvas, `ImageBufferShareableIOSurfaceBackend::backendSize`
> 
> I think this should say "DOM" not "canvas"

Oops, that's right. Fixed!
Comment 15 Wenson Hsieh 2020-12-11 07:27:44 PST
Created attachment 415995 [details]
Patch for landing
Comment 16 Wenson Hsieh 2020-12-11 08:07:20 PST
Comment on attachment 415995 [details]
Patch for landing

Committed r270676: <https://trac.webkit.org/changeset/270676>
Comment 17 Radar WebKit Bug Importer 2020-12-11 08:08:17 PST
<rdar://problem/72223661>