Bug 79317 - Remove GraphicsContext3D::paintsIntoCanvasBuffer and unify WebGL and canvas 2d logic
Summary: Remove GraphicsContext3D::paintsIntoCanvasBuffer and unify WebGL and canvas 2...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-22 18:47 PST by James Robinson
Modified: 2012-02-24 10:58 PST (History)
5 users (show)

See Also:


Attachments
Patch (10.79 KB, patch)
2012-02-22 18:53 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (1.44 KB, patch)
2012-02-22 23:10 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (1.21 KB, patch)
2012-02-23 11:55 PST, Matthew Delaney
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-02-22 18:47:34 PST
Remove GraphicsContext3D::paintsIntoCanvasBuffer and unify WebGL and canvas 2d logic
Comment 1 James Robinson 2012-02-22 18:53:22 PST
Created attachment 128367 [details]
Patch
Comment 2 Kenneth Russell 2012-02-22 19:04:14 PST
Comment on attachment 128367 [details]
Patch

Thumbs up. r=me
Comment 3 James Robinson 2012-02-22 19:06:50 PST
Committed r108597: <http://trac.webkit.org/changeset/108597>
Comment 4 Matthew Delaney 2012-02-22 19:54:49 PST
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)
Comment 5 James Robinson 2012-02-22 19:59:34 PST
(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.
Comment 6 Matthew Delaney 2012-02-22 20:09:17 PST
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!
Comment 7 James Robinson 2012-02-22 20:17:39 PST
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.
Comment 8 Simon Fraser (smfr) 2012-02-22 22:49:34 PST
> 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.
Comment 9 James Robinson 2012-02-22 23:09:54 PST
(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
Comment 10 James Robinson 2012-02-22 23:10:47 PST
Reopening to attach new patch.
Comment 11 James Robinson 2012-02-22 23:10:49 PST
Created attachment 128402 [details]
Patch
Comment 12 James Robinson 2012-02-22 23:14:40 PST
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?
Comment 13 Matthew Delaney 2012-02-23 11:46:22 PST
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
Comment 14 James Robinson 2012-02-23 11:55:29 PST
(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).
Comment 15 Matthew Delaney 2012-02-23 11:55:44 PST
Created attachment 128522 [details]
Patch
Comment 16 Matthew Delaney 2012-02-23 12:00:56 PST
(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.
Comment 17 James Robinson 2012-02-23 12:15:48 PST
Comment on attachment 128522 [details]
Patch

Won't this break WebGL?
Comment 18 Matthew Delaney 2012-02-23 12:16:51 PST
Committed r108657: <http://trac.webkit.org/changeset/108657>
Comment 19 Matthew Delaney 2012-02-23 12:18:18 PST
(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...
Comment 20 Matthew Delaney 2012-02-23 12:25:37 PST
(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.
Comment 21 Matthew Delaney 2012-02-23 12:29:07 PST
(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
Comment 22 Stephen White 2012-02-23 22:43:22 PST
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.)
Comment 23 James Robinson 2012-02-24 10:58:56 PST
(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

:)