Bug 219760

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?

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

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?

Created attachment 415943 [details] Patch

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

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

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

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

Created attachment 415950 [details] For EWS

Created attachment 415965 [details] Patch

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"

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

Created attachment 415995 [details] Patch for landing

Comment on attachment 415995 [details] Patch for landing Committed r270676: <https://trac.webkit.org/changeset/270676>

<rdar://problem/72223661>