Bug 67050 – Skia's accel canvas implementation should use GrTexture, not DrawingBuffer

• Component: New Bugs
• Status: RESOLVED FIXED

[Stephen White]

Skia's accel canvas implementation should use GrTexture, not DrawingBuffer

[Stephen White]

Created attachment 105388 [details]
Patch

[Brian Salomon]

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.

[Kenneth Russell]

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?

[Stephen White]

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

[Kenneth Russell]

(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

[Stephen White]

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

[Stephen White]

Created attachment 105488 [details]
Patch

[Stephen White]

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

[Kenneth Russell]

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.

[Stephen White]

Committed r93988: <http://trac.webkit.org/changeset/93988>