Remove GraphicsContext3D::paintsIntoCanvasBuffer and unify WebGL and canvas 2d logic
Created attachment 128367 [details] Patch
Comment on attachment 128367 [details] Patch Thumbs up. r=me
Committed r108597: <http://trac.webkit.org/changeset/108597>
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)
(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.
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!
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.
> 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.
(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
Reopening to attach new patch.
Created attachment 128402 [details] Patch
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 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
(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).
Created attachment 128522 [details] Patch
(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 on attachment 128522 [details] Patch Won't this break WebGL?
Committed r108657: <http://trac.webkit.org/changeset/108657>
(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...
(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.
(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 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.)
(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 :)