Bug 67050 - Skia's accel canvas implementation should use GrTexture, not DrawingBuffer
Summary: Skia's accel canvas implementation should use GrTexture, not DrawingBuffer
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: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-26 11:50 PDT by Stephen White
Modified: 2011-08-29 11:16 PDT (History)
7 users (show)

See Also:


Attachments
Patch (19.59 KB, patch)
2011-08-26 12:05 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (20.51 KB, patch)
2011-08-29 07:31 PDT, Stephen White
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2011-08-26 11:50:22 PDT
Skia's accel canvas implementation should use GrTexture, not DrawingBuffer
Comment 1 Stephen White 2011-08-26 12:05:59 PDT
Created attachment 105388 [details]
Patch
Comment 2 Brian Salomon 2011-08-26 13:41:42 PDT
Comment on attachment 105388 [details]
Patch

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

Looks grrrrreat to me. Can we pull the GrContext stuff out of DrawingBuffer?

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:100
> +                SkAutoTUnref<GrTexture> texture(gr->createUncachedTexture(desc, 0, 0));

Just a note: In the future we may want to change the GrContext API to hide whether textures are recycled internally. If canvases are created and destroyed we could get some benefit from recycling the backing textures.
Comment 3 Kenneth Russell 2011-08-26 18:09:20 PDT
Comment on attachment 105388 [details]
Patch

There is a conflict between this patch and the ongoing work twiz is doing in https://bugs.webkit.org/show_bug.cgi?id=53201 to make WebGL layers use DrawingBuffer rather than having GraphicsContext3D vend a platform layer. I'm of the opinion that we should standardize on DrawingBuffer as the abstraction between the compositor and accelerated content like WebGL and 2D canvas, because doing so would move a lot of framebuffer management code into WebKit, and make the only dependency between these layers and the compositor be the sharing of OpenGL resources between their contexts. For this reason it seems to me that this patch, which adds a dependency between Canvas2DLayerChromium and GraphicsContext3D, is not really the right direction to go. Am I misunderstanding the situation? Comments?
Comment 4 Stephen White 2011-08-26 18:17:47 PDT
(In reply to comment #3)
> (From update of attachment 105388 [details])
> There is a conflict between this patch and the ongoing work twiz is doing in https://bugs.webkit.org/show_bug.cgi?id=53201 to make WebGL layers use DrawingBuffer rather than having GraphicsContext3D vend a platform layer. 

Don't worry; twiz knows what I'm up to.  :)  His patch is currently on hold until the copy or share webgl/compositor interaction is worked out.  After this patch lands, it will leave DrawingBuffer free for WebGL use (no need for the canvas2d/webGL enum in twiz's patch).

> I'm of the opinion that we should standardize on DrawingBuffer as the abstraction between the compositor and accelerated content like WebGL and 2D canvas, because doing so would move a lot of framebuffer management code into WebKit, and make the only dependency between these layers and the compositor be the sharing of OpenGL resources between their contexts. For this reason it seems to me that this patch, which adds a dependency between Canvas2DLayerChromium and GraphicsContext3D, is not really the right direction to go. Am I misunderstanding the situation? Comments?

The goal here is to be able to defer creation of the stencil buffer until it's actually needed (e.g., on the first clipping call) in order to minimize VRAM usage.  That's much more easily done inside Skia (which knows when it needs the stencil buffer) than outside.  The other thing is that Ganesh already has its own classes for FBO creation, etc, and was just wrapping the FBO's created by DrawingBuffer.  Since this is platform/graphics/skia, it makes more sense to me to use the Skia (Ganesh) classes directly, IMHO.
Comment 5 Kenneth Russell 2011-08-26 18:35:08 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 105388 [details] [details])
> > There is a conflict between this patch and the ongoing work twiz is doing in https://bugs.webkit.org/show_bug.cgi?id=53201 to make WebGL layers use DrawingBuffer rather than having GraphicsContext3D vend a platform layer. 
> 
> Don't worry; twiz knows what I'm up to.  :)  His patch is currently on hold until the copy or share webgl/compositor interaction is worked out.  After this patch lands, it will leave DrawingBuffer free for WebGL use (no need for the canvas2d/webGL enum in twiz's patch).

OK, I was sure that you and twiz were on the same page, but wanted to clarify. We need to unblock him on the other patch.

> > I'm of the opinion that we should standardize on DrawingBuffer as the abstraction between the compositor and accelerated content like WebGL and 2D canvas, because doing so would move a lot of framebuffer management code into WebKit, and make the only dependency between these layers and the compositor be the sharing of OpenGL resources between their contexts. For this reason it seems to me that this patch, which adds a dependency between Canvas2DLayerChromium and GraphicsContext3D, is not really the right direction to go. Am I misunderstanding the situation? Comments?
> 
> The goal here is to be able to defer creation of the stencil buffer until it's actually needed (e.g., on the first clipping call) in order to minimize VRAM usage.  That's much more easily done inside Skia (which knows when it needs the stencil buffer) than outside.  The other thing is that Ganesh already has its own classes for FBO creation, etc, and was just wrapping the FBO's created by DrawingBuffer.  Since this is platform/graphics/skia, it makes more sense to me to use the Skia (Ganesh) classes directly, IMHO.

OK, I see the reasoning. I am still not really sure that pushing the drawing buffer management for CanvasLayer2DChromium deeper into GraphicsContext3D is the best idea, but I'm sure you guys know what you're doing and that we can revisit the issue later if necessary. r=me
Comment 6 Stephen White 2011-08-29 07:15:20 PDT
(In reply to comment #2)
> (From update of attachment 105388 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105388&action=review
> 
> Looks grrrrreat to me. Can we pull the GrContext stuff out of DrawingBuffer?

Good idea.  Will do.

> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:100
> > +                SkAutoTUnref<GrTexture> texture(gr->createUncachedTexture(desc, 0, 0));
> 
> Just a note: In the future we may want to change the GrContext API to hide whether textures are recycled internally. If canvases are created and destroyed we could get some benefit from recycling the backing textures.

Hmm.  So is there something I can change now in anticipation of that, or something we should revisit later?
Comment 7 Stephen White 2011-08-29 07:31:47 PDT
Created attachment 105488 [details]
Patch
Comment 8 Stephen White 2011-08-29 07:35:20 PDT
(In reply to comment #7)
> Created an attachment (id=105488) [details]
> Patch

Removed all Ganesh-related datatypes and calls from DrawingBuffer.

Reverted the change to GraphicsContext3D, since it was not correctly setting the context before flushing, and adding a makeContextCurrent() at that point seemed wrong.  Moved the code to Canvas2DLayerChromium::updateCompositorResources() instead.
Comment 9 Kenneth Russell 2011-08-29 11:12:17 PDT
Comment on attachment 105488 [details]
Patch

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

Revision looks good to me.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:74
> +            m_context->makeContextCurrent();

According to current semantics this could actually go into the #if USE(SKIA) block, because it only has an effect when interacting with external code (like Ganesh) using the GC3D's underlying OpenGL context. The call to m_context->flush() below will implicitly make the context current.
Comment 10 Stephen White 2011-08-29 11:16:58 PDT
Committed r93988: <http://trac.webkit.org/changeset/93988>