Bug 46894 - Redesign extension mechanism in GraphicsContext3D
Summary: Redesign extension mechanism in GraphicsContext3D
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-30 06:45 PDT by Chris Marrin
Modified: 2010-11-03 15:13 PDT (History)
6 users (show)

See Also:


Attachments
Patch (58.19 KB, patch)
2010-11-02 19:16 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (58.22 KB, patch)
2010-11-03 14:26 PDT, Kenneth Russell
cmarrin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2010-09-30 06:45:20 PDT
Currently, GraphicsContext3D has direct calls for determining the presence of and using extensions. One example is supportsMapSubCHROMIUM() and mapBufferSubDataCHROMIUM(). In addition to being CHROMIUM specific, this sort of explicit extension functionality will eventually turn into a huge and unmanageable API.

One possible solution is to replace all the 'supportsXXX' calls with a single supportsExtension() call. This would be passed a string with a unique extension name and would true if the extension is supported. Then there would be a getExtension() call which would return a platform specific Extension3D object. This would be statically cast into the platform specific subclass containing the API call(s).

This can be nested. If the extension gives access to existing OpenGL functionality, there can be an Extension3DOpenGL subclass which gives access to the OpenGL version of the extension. This will reduce the amount of platform specific code needed.

Since the platform specific code will have to be used to determine whether or not an extension is supported, it might be best for the supportedExtension() to be in Extension3D. So you would go:

    if (getExtension()->supportsExtension("GL_CHROMIUM_map_sub"))
        static_cast<const Extension3DChromium*>(getExtension())->mapBufferSubData(...);
Comment 1 Kenneth Russell 2010-11-02 19:16:34 PDT
Created attachment 72785 [details]
Patch

From the ChangeLog:

Upon request, factored out extension support from GraphicsContext3D into a new Extensions3D class. (The plural was chosen because the class and subclasses hold multiple extensions.)

Unlike GraphicsContext3D, Extensions3D contains only pure virtual methods. This was done because Extensions3D's inheritance diagram and usage pattern is very different from that of GraphicsContext3D, and the concrete subclasses need to decide how to implement the various entry points. Requiring them to be placed at the Extensions3D level will cause implementation details to leak into the base class, which is highly undesirable. Any virtual call overhead to these entry points will be negligible.

Changed call sites utilizing these extensions to call through the Extensions3D object or its subclasses.

Tested:
 - Chromium on Linux with accelerated 2D canvas and HTML5 video
 - Chromium on Mac OS X with WebGL and CSS 3D content
 - Safari on Mac OS X with WebGL and CSS 3D content

No new tests. Covered by existing tests.
Comment 2 James Robinson 2010-11-03 12:58:18 PDT
Comment on attachment 72785 [details]
Patch

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

> WebCore/platform/graphics/chromium/Extensions3DChromium.h:62
> +    Extensions3DChromium(GraphicsContext3DInternal*);

nit: explicit
Comment 3 James Robinson 2010-11-03 12:58:47 PDT
This looks good to me.  Do you have any thoughts about this approach, Chris?
Comment 4 Kenneth Russell 2010-11-03 14:26:07 PDT
Created attachment 72872 [details]
Patch

Fixed style error in previous patch. Addressed review feedback.
Comment 5 Chris Marrin 2010-11-03 14:35:13 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=72785&action=review

Looks good except for some small comments.

> WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:73
> +    , m_fbo(0)

Is this unrelated? If so, it should be in a separate patch

> WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:116
> +    static_cast<Extensions3DChromium*>(m_context->getExtensions())->copyTextureToParentTextureCHROMIUM(m_internal->offscreenColorTexture, parentTexture);

So it looks here like you're using the extensons mechanism as a way to implement cross-platform extensions in a platform specific way, and to add platform specific extensions to the interface. That's fine, but you should document it in Extensions3D.h and the ChangeLog. You do have a sentence that refers to this, but it should be made more clear.

> WebKit/chromium/src/Extensions3DChromium.cpp:1
> +/*

Really odd that this in not in WebCore/platform/graphics/chromium
Comment 6 Kenneth Russell 2010-11-03 14:48:25 PDT
Committed r71272: <http://trac.webkit.org/changeset/71272>
Comment 7 Kenneth Russell 2010-11-03 15:13:18 PDT
(In reply to comment #5)
> View in context: https://bugs.webkit.org/attachment.cgi?id=72785&action=review
> 
> Looks good except for some small comments.

Sorry, I looked at the wrong email indicating the review had been granted and didn't realize you wanted changes made. I've already landed this but will make changes in a follow up bug if you want.

> > WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:73
> > +    , m_fbo(0)
> 
> Is this unrelated? If so, it should be in a separate patch

It's related. The FBO is now only allocated if the extension which DrawingBufferChromium needs is available.

> > WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:116
> > +    static_cast<Extensions3DChromium*>(m_context->getExtensions())->copyTextureToParentTextureCHROMIUM(m_internal->offscreenColorTexture, parentTexture);
> 
> So it looks here like you're using the extensons mechanism as a way to implement cross-platform extensions in a platform specific way, and to add platform specific extensions to the interface. That's fine, but you should document it in Extensions3D.h and the ChangeLog. You do have a sentence that refers to this, but it should be made more clear.

I thought this was self-explanatory from your own request of how the refactoring was to work. I suspect that anyone looking at the code will be able to figure it out. If you feel strongly then file a bug and I'll update the documentation.

> > WebKit/chromium/src/Extensions3DChromium.cpp:1
> > +/*
> 
> Really odd that this in not in WebCore/platform/graphics/chromium

It can not be placed in WebCore/platform/graphics/chromium due to how it's implemented. The implementation reaches into files that live in WebKit/chromium/src/. This is part of the reason for decoupling the interface and implementation at the Extensions3D level.