Bug 219760 – REGRESSION (r270458): Canvas painting is broken when enabling GPU process for DOM

• Component: Canvas
• Status: RESOLVED FIXED

[Wenson Hsieh]

SSIA

[Said Abou-Hallawa]

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?

[Wenson Hsieh]

(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.

[Said Abou-Hallawa]

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?

[Wenson Hsieh]

Created attachment 415943 [details]
Patch

[Wenson Hsieh]

(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?

[Wenson Hsieh]

(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).

[Said Abou-Hallawa]

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?

[Wenson Hsieh]

(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?

[Tim Horton]

If we go that way, maybe we make it assert instead of just be empty?

[Wenson Hsieh]

(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.

[Wenson Hsieh]

Created attachment 415950 [details]
For EWS

[Wenson Hsieh]

Created attachment 415965 [details]
Patch

[Tim Horton]

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"

[Wenson Hsieh]

(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!

[Wenson Hsieh]

Created attachment 415995 [details]
Patch for landing

[Wenson Hsieh]

Comment on attachment 415995 [details]
Patch for landing

Committed r270676: <https://trac.webkit.org/changeset/270676>

[Radar WebKit Bug Importer]

<rdar://problem/72223661>