Bug 89700 - [chromium] Use WebGraphicsContext3D in compositor implementation
Summary: [chromium] Use WebGraphicsContext3D in compositor implementation
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-06-21 15:57 PDT by James Robinson
Modified: 2012-06-25 18:01 PDT (History)
9 users (show)

See Also:


Attachments
wip, not ready for review (127.68 KB, patch)
2012-06-21 15:57 PDT, James Robinson
no flags Details | Formatted Diff | Diff
checkpoint (180.00 KB, patch)
2012-06-21 19:17 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (186.42 KB, patch)
2012-06-24 13:41 PDT, James Robinson
enne: 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-06-21 15:57:00 PDT
[chromium] Use WebGraphicsContext3D in compositor implementation
Comment 1 James Robinson 2012-06-21 15:57:35 PDT
Created attachment 148907 [details]
wip, not ready for review
Comment 2 James Robinson 2012-06-21 19:17:46 PDT
Created attachment 148940 [details]
checkpoint
Comment 3 James Robinson 2012-06-24 13:41:29 PDT
Created attachment 149203 [details]
Patch
Comment 4 James Robinson 2012-06-24 13:42:17 PDT
From the WebCore ChangeLog:


        This converts the compositor implementation from using WebCore::GraphicsContext3D to using the Platform-provided
        WebGraphicsContext3D. This removes several unnecessary layers of indirection/wrapping and cuts down the
        compositor's implementation dependencies. GraphicsContext3D.h is still widely used to provide GL enum values.
        Most of the changes are purely mechanical - changing type names and the like.

        Ownership is changed a bit. Instead of multiple components holding references to the compositor's context, the
        context is now owned by the CCGraphicsContext, which is now owned directly by CCLayerTreeHostImpl.
        CCLayerTreeHostImpl also has ownership of its CCRenderer (LayerRendererChromium in 3D mode) and passes a
        non-owning pointer down to the CCRenderer.

        Extension checking is a bit different. The compositor does not (and never has) used extensions provided by
        WebGL's request/ensure mechanism. It simply checks for the existence of extensions it needs in the GL_EXTENSIONS
        string.

        FrameBufferSkPictureCanvasLayerTextureUpdater had to be patched as well, since it was grabbing a GrContext off
        of the compositor's GraphicsContext3D. This caused many problems. It was inefficient, since it required a full
        state flush when switching between ganesh and compositor calls. The gpu memory management was completely broken
        since the compositor clobbered ganesh's onMemoryAllocationChanged callback. This moves FBSkPCLTU over to using
        the appropriate SharedGraphicsContext3D, like filters.
Comment 5 WebKit Review Bot 2012-06-24 13:44:55 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 6 Adrienne Walker 2012-06-25 17:14:21 PDT
Comment on attachment 149203 [details]
Patch

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

I feel like I should have more substantial comments on a 186k patch, but R=me.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1365
> -    RefPtr<CCGraphicsContext> ccContext = CCGraphicsContext::create3D(m_context);
> -    texture->framebufferTexture2D(ccContext.get(), m_implTextureAllocator.get());
> +    if (!texture->textureId())
> +        texture->allocate(m_implTextureAllocator.get());
> +    GLC(m_context, m_context->framebufferTexture2D(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::COLOR_ATTACHMENT0, GraphicsContext3D::TEXTURE_2D, texture->textureId(), 0));

Can the ManagedTexture::framebufferTexture2d and bindTexture functions get removed?
Comment 7 James Robinson 2012-06-25 17:21:17 PDT
(In reply to comment #6)
> (From update of attachment 149203 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149203&action=review
> 
> I feel like I should have more substantial comments on a 186k patch, but R=me.
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1365
> > -    RefPtr<CCGraphicsContext> ccContext = CCGraphicsContext::create3D(m_context);
> > -    texture->framebufferTexture2D(ccContext.get(), m_implTextureAllocator.get());
> > +    if (!texture->textureId())
> > +        texture->allocate(m_implTextureAllocator.get());
> > +    GLC(m_context, m_context->framebufferTexture2D(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::COLOR_ATTACHMENT0, GraphicsContext3D::TEXTURE_2D, texture->textureId(), 0));
> 
> Can the ManagedTexture::framebufferTexture2d and bindTexture functions get removed?

I'll check if that can be done as a follow-up (this patch makes some of the GraphicsContext3DPrivate extension callback stuff become dead code and I want to delete that too).
Comment 8 James Robinson 2012-06-25 17:56:08 PDT
Committed r121204: <http://trac.webkit.org/changeset/121204>
Comment 9 James Robinson 2012-06-25 18:01:15 PDT
(In reply to comment #6)
> Can the ManagedTexture::framebufferTexture2d and bindTexture functions get removed?

ManagedTexture::bindTexture() is still used in ImageLayerChromium and CCHeadsUpDisplay. The first one, at least, appears to be a useful use for this abstraction. ManagedTexture::framebufferTexture2D() is now dead code, removal patch here: https://bugs.webkit.org/show_bug.cgi?id=89930