Bug 79317 – Remove GraphicsContext3D::paintsIntoCanvasBuffer and unify WebGL and canvas 2d logic

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

Remove GraphicsContext3D::paintsIntoCanvasBuffer and unify WebGL and canvas 2d logic

[James Robinson]

Created attachment 128367 [details]
Patch

[Kenneth Russell]

Comment on attachment 128367 [details]
Patch

Thumbs up. r=me

[James Robinson]

Committed r108597: <http://trac.webkit.org/changeset/108597>

[Matthew Delaney]

I believe you left something out. Last I checked, we aren't using the ACCELERATED_2D_CANVAS flag that was used here: http://trac.webkit.org/changeset/108597/trunk/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp

So, CRC2D.cpp I think should have this full line unless I missed something else in your patch (I'm on a bumpy bus at the moment):
#if ENABLE(ACCELERATED_2D_CANVAS) && USE(ACCELERATED_COMPOSITING)

[James Robinson]

(In reply to comment #4)
> I believe you left something out. Last I checked, we aren't using the ACCELERATED_2D_CANVAS flag that was used here: http://trac.webkit.org/changeset/108597/trunk/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> 
> So, CRC2D.cpp I think should have this full line unless I missed something else in your patch (I'm on a bumpy bus at the moment):
> #if ENABLE(ACCELERATED_2D_CANVAS) && USE(ACCELERATED_COMPOSITING)

I believe the existing checks in CanvasRenderingContext2D::isAccelerated() are sufficient for the mac build, which doesn't set ENABLE(ACCELERATED_2D_CANVAS).  Let me know if that's not the case.

[Matthew Delaney]

Ugh, all these poorly named variables. I can hardly view it on here, but if that m_context->paintsIntoBuffer check was a 2d context, then in our case we want it to return true whether or not it's accelerated. In the 2D case, we're always rendering into an intermediary buffer which is what I think that check was for, right? I'll have to check when I get home though it doesn't look right at first glance.

I'm all for cleanup there though!

[James Robinson]

I see, so you want paintsIntoCanvasBuffer() to return true for accelerated 2d canvases? That's easy enough to accomplish.  Would you mind expanding a bit on why? I was under the (perhaps mistaken) impression that the Mac accelerated canvas 2d implementation was also constructing a composited layer for the canvas content and thus there is no need to do anything at paint time.

Perhaps the better thing to key this off of is asking the CanvasRenderingContext if it is currently compositing into a layer or not.

[Simon Fraser (smfr)]

> I was under the (perhaps mistaken) impression that the Mac accelerated canvas 2d implementation was also constructing a composited layer for the canvas content and thus there is no need to do anything at paint time.

No. We do that for WebGL, but not for normal canvases. For those, we use an IOSurface-backed ImageBuffer, but we still paint that buffer into a CALayer to get the canvas displayed. We do that for synchronization reasons; we have to ensure that the painting into the canvas buffer is flushed somehow, and painting the buffer is equivalent to doing some kind of context flush.

[James Robinson]

(In reply to comment #8)
> > I was under the (perhaps mistaken) impression that the Mac accelerated canvas 2d implementation was also constructing a composited layer for the canvas content and thus there is no need to do anything at paint time.
> 
> No. We do that for WebGL, but not for normal canvases. For those, we use an IOSurface-backed ImageBuffer, but we still paint that buffer into a CALayer to get the canvas displayed. We do that for synchronization reasons; we have to ensure that the painting into the canvas buffer is flushed somehow, and painting the buffer is equivalent to doing some kind of context flush.

Gotcha.  I think

[James Robinson]

Reopening to attach new patch.

[James Robinson]

Created attachment 128402 [details]
Patch

[James Robinson]

Whoops, hit commit too soon.  This completely untested patch should restore the previous behavior for !ENABLE(ACCELERATED_2D_CANVAS) builds - would one of you like to confirm whether this helps?  If not I can test it tomorrow on a Mac Pro with Lion.  The interesting case is a 2d canvas that happens to be composited for other reasons (a 3d transform or the like).

I'm a bit academically curious about how the synchronization is different between canvas and WebGL in CoreAnimation. For instance, I know that some WebGL sites move an DOM element on top of the WebGL content in script and expect them to stay in sync - is that different for webgl and canvas content?

[Matthew Delaney]

Comment on attachment 128402 [details]
Patch

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

I just built ToT and it's definitely broken, so please commit this asap.

> Source/WebCore/html/HTMLCanvasElement.cpp:277
> +#if !ENABLE(ACCELERATED_2D_CANVAS)

I think USE(IOSURFACE_CANVAS_BACKING_STORE) is more precise

[James Robinson]

(In reply to comment #13)
> (From update of attachment 128402 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128402&action=review
> 
> I just built ToT and it's definitely broken, so please commit this asap.
> 

Find me a review and sure :)

> > Source/WebCore/html/HTMLCanvasElement.cpp:277
> > +#if !ENABLE(ACCELERATED_2D_CANVAS)
> 
> I think USE(IOSURFACE_CANVAS_BACKING_STORE) is more precise

I think that makes a lot less sense in the context of this function.  The question this is asking is "if the accelerated 2d canvas feature is off, then all 2d contexts paint into the canvas buffer".  This is the case for IOSURFACE_CANVAS_BACKING_STORE and any other possible canvas 2d rendering model that isn't ENABLE(ACCELERATED_2D_CANVAS).

[Matthew Delaney]

Created attachment 128522 [details]
Patch

[Matthew Delaney]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 128402 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=128402&action=review
> > 
> > I just built ToT and it's definitely broken, so please commit this asap.
> > 
> 
> Find me a review and sure :)

Wanna just review mine instead? :-)

> 
> > > Source/WebCore/html/HTMLCanvasElement.cpp:277
> > > +#if !ENABLE(ACCELERATED_2D_CANVAS)
> > 
> > I think USE(IOSURFACE_CANVAS_BACKING_STORE) is more precise
> 
> I think that makes a lot less sense in the context of this function.  The question this is asking is "if the accelerated 2d canvas feature is off, then all 2d contexts paint into the canvas buffer".  This is the case for IOSURFACE_CANVAS_BACKING_STORE and any other possible canvas 2d rendering model that isn't ENABLE(ACCELERATED_2D_CANVAS).

I think this logic is a bit misplaced in general. The confusing part is that we have multiple flags that say "accelerated canvas" of which the iosurface backing store one is probably the most confusing. I'll just clean this up in a separate patch, but it's still true that for now my having the iosurface flag is more precise here.

[James Robinson]

Comment on attachment 128522 [details]
Patch

Won't this break WebGL?

[Matthew Delaney]

Committed r108657: <http://trac.webkit.org/changeset/108657>

[Matthew Delaney]

(In reply to comment #17)
> (From update of attachment 128522 [details])
> Won't this break WebGL?

Damn, you're right. Not sure your original path is going to work then...

[Matthew Delaney]

(In reply to comment #19)
> (In reply to comment #17)
> > (From update of attachment 128522 [details] [details])
> > Won't this break WebGL?
> 
> Damn, you're right. Not sure your original path is going to work then...

I see, I forgot the is2d() here. Adding that in now.

[Matthew Delaney]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #17)
> > > (From update of attachment 128522 [details] [details] [details])
> > > Won't this break WebGL?
> > 
> > Damn, you're right. Not sure your original path is going to work then...
> 
> I see, I forgot the is2d() here. Adding that in now.

Committed that quick fix here: http://trac.webkit.org/changeset/108658

[Stephen White]

Comment on attachment 128367 [details]
Patch

Post-hoc drive-by:  this is great to see.  Now if we could get rid of its use in GraphicsContext3DPrivate::platformTexture(), we could get rid of m_webViewImpl altogether.  (Kind of weird the way  it's grabbing the onscreen context at that point, actually.)

[James Robinson]

(In reply to comment #22)
> (From update of attachment 128367 [details])
> Post-hoc drive-by:  this is great to see.  Now if we could get rid of its use in GraphicsContext3DPrivate::platformTexture(), we could get rid of m_webViewImpl altogether.  (Kind of weird the way  it's grabbing the onscreen context at that point, actually.)

http://trac.webkit.org/changeset/108706/trunk/Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp

:)