RESOLVED FIXED 46894
Redesign extension mechanism in GraphicsContext3D
https://bugs.webkit.org/show_bug.cgi?id=46894
Summary Redesign extension mechanism in GraphicsContext3D
Chris Marrin
Reported 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(...);
Attachments
Patch (58.19 KB, patch)
2010-11-02 19:16 PDT, Kenneth Russell
no flags
Patch (58.22 KB, patch)
2010-11-03 14:26 PDT, Kenneth Russell
cmarrin: review+
Kenneth Russell
Comment 1 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.
James Robinson
Comment 2 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
James Robinson
Comment 3 2010-11-03 12:58:47 PDT
This looks good to me. Do you have any thoughts about this approach, Chris?
Kenneth Russell
Comment 4 2010-11-03 14:26:07 PDT
Created attachment 72872 [details] Patch Fixed style error in previous patch. Addressed review feedback.
Chris Marrin
Comment 5 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
Kenneth Russell
Comment 6 2010-11-03 14:48:25 PDT
Kenneth Russell
Comment 7 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.
Note You need to log in before you can comment on or make changes to this bug.