Bug 30625 – Implement WebGL on Windows

• Component: WebGL
• Status: RESOLVED DUPLICATE

[Chris Marrin]

The first implementation of this could possibly be based on Chrome's glex implementation.

[Chris Marrin]

We will implement this on top of ANGLE, which is being added here:

    https://bugs.webkit.org/show_bug.cgi?id=42789

[Paul Sawaya]

Created attachment 65767 [details]
Refactored some non Mac specific OpenGL code out of GraphicsContext3DMac.mm to make ANGLE (libGLESv2) integration easier.

[Kenneth Russell]

I think you'll find that that code is in fact Mac specific. It depends on the OpenGL 2.1 entry points being linkable, which is only true on Mac OS X. On Windows you can only rely on being able to link against the OpenGL 1.1 entry points; everything else needs to be dynamically looked up.

You might consider looking at WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp and its use of the (currently Chromium specific) app/gfx/gl/gl_bindings.{h,cpp}. This is Chromium's legacy in-process WebGL implementation and is known to work on Mac, Windows and Linux.

[Darin Adler]

Comment on attachment 65767 [details]
Refactored some non Mac specific OpenGL code out of GraphicsContext3DMac.mm to make ANGLE (libGLESv2) integration easier.

Is someone going to review this?

[Chris Marrin]

Comment on attachment 65767 [details]
Refactored some non Mac specific OpenGL code out of GraphicsContext3DMac.mm to make ANGLE (libGLESv2) integration easier.

It looks like you didn't do an svn copy to create the GraphicsContext3DOpenGL.cpp file. If you do this:

    svn cp WebCore/platform/graphics/mac/GraphicsContext3DMac.mm WebCore/platform/graphics/GraphicsContextOpenGL.cpp

then svn will remember where the file came from, which both helps with history tracking and makes svn diff show the differences between the old and new files. To avoid having to do any extra work:

1) Move your current GraphicsContext3DOpenGL.cpp to a -saved file
2) do the svn cp
3) delete the copied GraphicsContext3DOpenGL.cpp and replace it with your -saved version.

I believe that will make everything work and show us a better diff.

[Chris Marrin]

(In reply to comment #3)
> I think you'll find that that code is in fact Mac specific. It depends on the OpenGL 2.1 entry points being linkable, which is only true on Mac OS X. On Windows you can only rely on being able to link against the OpenGL 1.1 entry points; everything else needs to be dynamically looked up.

It will ultimately depend on all these symbols being linkable, it's true. But these symbols will be linkable on Mac OpenGL, iOS OpenGL ES 2.0 and ANGLE. I don't see linking with Windows OpenGL ever being a concern here. 

Eventually the plan is to add ifdefs to deal with the naming diffs (with EXT vs without). Other diffs can go into the platform specific versions (GraphicsContext3DMac, GraphicsContextIOS, GraphicsContextANGLE).

[Chris Marrin]

Comment on attachment 65767 [details]
Refactored some non Mac specific OpenGL code out of GraphicsContext3DMac.mm to make ANGLE (libGLESv2) integration easier.

I've already given this patch an r-, but I wanted to give more info for the revised patch:

...

> Index: WebCore/platform/graphics/GraphicsContext3DOpenGL.cpp
> ===================================================================
> --- WebCore/platform/graphics/GraphicsContext3DOpenGL.cpp	(revision 0)
> +++ WebCore/platform/graphics/GraphicsContext3DOpenGL.cpp	(revision 0)
> @@ -0,0 +1,1502 @@
> 
> +
> +#include "ArrayBuffer.h"
> +#include "ArrayBufferView.h"
> +#include "WebGLObject.h"
> +#include "CanvasRenderingContext.h"
> +#include "Float32Array.h"
> +#include "GraphicsContext.h"
> +#include "HTMLCanvasElement.h"
> +#include "ImageBuffer.h"
> +#include "Int32Array.h"
> +#include "NotImplemented.h"
> +#include "Uint8Array.h"
> +#include <CoreGraphics/CGBitmapContext.h>
> +#include <OpenGL/CGLRenderers.h>
> +#include <OpenGL/gl.h>

This file should not have any bare CG or OpenGL includes. These are specific to the Mac implementation. The GL includes should be wrapped in a #if PLATFORM(MAC). These should not be any CG specific code in this file (see below).

> +#include <wtf/UnusedParam.h>
> +#include <wtf/text/CString.h>
> +
> +namespace WebCore {
> +    
> +GraphicsContext3D::~GraphicsContext3D()
> +{
> +    if (m_contextObj) {
> +        CGLSetCurrentContext(m_contextObj);
> +        ::glDeleteTextures(1, &m_texture);
> +        if (m_attrs.antialias) {
> +            ::glDeleteRenderbuffersEXT(1, &m_multisampleColorBuffer);
> +            if (m_attrs.stencil || m_attrs.depth)
> +                ::glDeleteRenderbuffersEXT(1, &m_multisampleDepthStencilBuffer);
> +            ::glDeleteFramebuffersEXT(1, &m_multisampleFBO);
> +        } else {
> +            if (m_attrs.stencil || m_attrs.depth)
> +                ::glDeleteRenderbuffersEXT(1, &m_depthStencilBuffer);
> +        }
> +        ::glDeleteFramebuffersEXT(1, &m_fbo);
> +        CGLSetCurrentContext(0);
> +        CGLDestroyContext(m_contextObj);
> +    }
> +}

This function should not be here since it is CG specific. It should be in GraphicsContext3DMac.


> ...
> +
> +void GraphicsContext3D::makeContextCurrent()
> +{
> +    CGLSetCurrentContext(m_contextObj);
> +}

This function should also be in GraphicsContext3DMac.


> ...
> +
> +bool GraphicsContext3D::isGLES2Compliant() const
> +{
> +    return false;
> +}
> +
> +bool GraphicsContext3D::isGLES2NPOTStrict() const
> +{
> +    return false;
> +}
> +
> +bool GraphicsContext3D::isErrorGeneratedOnOutOfBoundsAccesses() const
> +{
> +    return false;
> +}

Some or all of these 3 functions probably need to be in the platform specific code


> +
> +void GraphicsContext3D::reshape(int width, int height)
> +{
> +    if (width == m_currentWidth && height == m_currentHeight || !m_contextObj)
> +        return;
> +
> +    m_currentWidth = width;
> +    m_currentHeight = height;
> +
> +    CGLSetCurrentContext(m_contextObj);

Need to replace this with makeContextCurrent().


> ...
> +
> +static inline void ensureContext(CGLContextObj context)
> +{
> +    if (!context)
> +        return;
> +
> +    CGLContextObj currentContext = CGLGetCurrentContext();
> +    if (currentContext != context)
> +        CGLSetCurrentContext(context);
> +}

This needs to be replaced by a member function and that function needs to be in platform specific code. The new function would not need a passed object, it can just use m_contextObj (or the equivalent on other platforms). In fact, we should probably get rid of makeContextCurrent and just use ensureContext() everywhere. The only difference is that ensureContext just checks the current context and skips the set if it's already set, as an optimization.

[Paul Sawaya]

Created attachment 66310 [details]
Made requested changes

[Chris Marrin]

Comment on attachment 66310 [details]
Made requested changes

Patch looks good, but you still need a Changelog. It can be generated using WebKitTools/Scripts/prepareChangelog.

[Paul Sawaya]

Created attachment 66380 [details]
Added ChangeLog

Oops, sorry about that.

[Chris Marrin]

Comment on attachment 66380 [details]
Added ChangeLog

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

> WebCore/ChangeLog:5
> +        Refactored out Mac specific code for platform/graphics/mac/GraphicsContext3D.mm file, leaving general OpenGL code in platform/graphics/GraphicsContext3D.cpp.


Try to avoid wrapping here (and don't use tabs when you wrap it yourself). And the OpenGL code is in GraphicsContext3DOpenGL, right?

r+ with those things fixed.

[Paul Sawaya]

Created attachment 66518 [details]
Fixed changelog

[WebKit Commit Bot]

Comment on attachment 66518 [details]
Fixed changelog

Rejecting patch 66518 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Chris Marrin', u'--force']" exit_code: 2
Parsed 7 diffs from patch file(s).
cp: platform/graphics/mac/GraphicsContext3DMac.mm: No such file or directory
Failed to copy platform/graphics/mac/GraphicsContext3DMac.mm platform/graphics/GraphicsContext3DOpenGL.cpp. at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply line 416.

Full output: http://queues.webkit.org/results/3943086

[Paul Sawaya]

Created attachment 66568 [details]
Fixed paths in patch, hopefully this one will apply

[WebKit Review Bot]

Attachment 66568 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/GraphicsContext3DOpenGL.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/GraphicsContext3DOpenGL.cpp:330:  Extra space after ( in function call  [whitespace/parens] [4]
WebCore/platform/graphics/GraphicsContext3DOpenGL.cpp:798:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/platform/graphics/GraphicsContext3DOpenGL.cpp:1192:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/platform/graphics/GraphicsContext3DOpenGL.cpp:1256:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebCore/platform/graphics/GraphicsContext3DOpenGL.cpp:1241:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/platform/graphics/GraphicsContext3DOpenGL.cpp:1258:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/platform/graphics/GraphicsContext3DOpenGL.cpp:1429:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 8 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Marrin]

Landed in http://trac.webkit.org/changeset/66781

[WebKit Review Bot]

http://trac.webkit.org/changeset/66781 might have broken Chromium Linux Release

[WebKit Review Bot]

Attachment 66568 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3976080

[James Robinson]

I like the intent of this patch a lot, but I'm going to have to revert it as it breaks the build in several ways.  First you've changed the interface of GraphicsContext3D without updating all implementations (Qt and Chromium also have implementations of GraphicsContext3D).  Second you've added a file to platform/graphics that will only compile on a mac without adding #if PLATFORM() guards.  I'm happy to help you fix all of those, as I'd like to use a common implementation of most of the GraphicsContext3D interface, but since it's very late on a Friday before a long weekend reverting seems more prudent.

[Chris Marrin]

This should have not been closed. This is only a patch to prepare for the WebGL Windows work. Other patches are coming.

[Chris Marrin]

(In reply to comment #19)
> I like the intent of this patch a lot, but I'm going to have to revert it as it breaks the build in several ways.  First you've changed the interface of GraphicsContext3D without updating all implementations (Qt and Chromium also have implementations of GraphicsContext3D).  Second you've added a file to platform/graphics that will only compile on a mac without adding #if PLATFORM() guards.  I'm happy to help you fix all of those, as I'd like to use a common implementation of most of the GraphicsContext3D interface, but since it's very late on a Friday before a long weekend reverting seems more prudent.

That is reasonable. But this is not mac specific. It is designed for any system that uses an OpenGL 2.x or OpenGL ES 2.0 API. That includes desktop OpenGL like Mac, true OpenGL ES 2.0 systems and ANGLE systems. Seems like perhaps it would be appropriate to put this into an opengl subdir. 

Sorry about the API change. Let's work together to get this back in. For now I will redo this patch and add the appropriate API to the other interfaces. It seems like if we put this into a subdir and only include it on the appropriate platforms, we shouldn't need any ifdefs, right?

[James Robinson]

(In reply to comment #21)
> (In reply to comment #19)
> > I like the intent of this patch a lot, but I'm going to have to revert it as it breaks the build in several ways.  First you've changed the interface of GraphicsContext3D without updating all implementations (Qt and Chromium also have implementations of GraphicsContext3D).  Second you've added a file to platform/graphics that will only compile on a mac without adding #if PLATFORM() guards.  I'm happy to help you fix all of those, as I'd like to use a common implementation of most of the GraphicsContext3D interface, but since it's very late on a Friday before a long weekend reverting seems more prudent.
> 
> That is reasonable. But this is not mac specific. It is designed for any system that uses an OpenGL 2.x or OpenGL ES 2.0 API. That includes desktop OpenGL like Mac, true OpenGL ES 2.0 systems and ANGLE systems. Seems like perhaps it would be appropriate to put this into an opengl subdir. 
> 
> Sorry about the API change. Let's work together to get this back in. For now I will redo this patch and add the appropriate API to the other interfaces. It seems like if we put this into a subdir and only include it on the appropriate platforms, we shouldn't need any ifdefs, right?

I noticed a few CoreGraphics calls that were not guarded by any #if()s - for example http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/GraphicsContext3DOpenGL.cpp?rev=66781#L90.  These are more likely just omissions, however.

The only change in GraphicsContext3D.h was to rename makeContextCurrent() to ensureContext().  I think that without that change this patch can land OK so long as GraphicsContext3DOpenGL.cpp only ever compiles on a mac.  I think it would be a useful step to still create that file, leave it in mac/ and then move it to a common location when all the CoreGraphics stuff is removed.

[Paul Sawaya]

Created attachment 66856 [details]
Moved into /mac, build only on Mac

Okay, I'm moving GCD3DOGL out of the gypi so that it will only build with xcode. I'm leaving the prototype for makeContextCurrent in, as I see it's used in gpu/SharedGraphicsContext3D, and #ifdef-ing ensureContext on only in mac builds.

[WebKit Review Bot]

Attachment 66856 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/mac/GraphicsContext3DOpenGL.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/mac/GraphicsContext3DOpenGL.cpp:330:  Extra space after ( in function call  [whitespace/parens] [4]
WebCore/platform/graphics/mac/GraphicsContext3DOpenGL.cpp:437:  Missing space after ,  [whitespace/comma] [3]
WebCore/platform/graphics/mac/GraphicsContext3DOpenGL.cpp:438:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/platform/graphics/mac/GraphicsContext3DOpenGL.cpp:802:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/platform/graphics/mac/GraphicsContext3DOpenGL.cpp:1196:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/platform/graphics/mac/GraphicsContext3DOpenGL.cpp:1260:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebCore/platform/graphics/mac/GraphicsContext3DOpenGL.cpp:1245:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/platform/graphics/mac/GraphicsContext3DOpenGL.cpp:1262:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/platform/graphics/mac/GraphicsContext3DOpenGL.cpp:1433:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 10 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Marrin]

Comment on attachment 66856 [details]
Moved into /mac, build only on Mac

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

> WebCore/html/canvas/WebGLObject.cpp:63
> +            m_context->graphicsContext3D()->ensureContext();
I really want to get rid of ensureContext and make everything use makeContextCurrent(), which should be a member function implemented by each platform.

> WebCore/platform/graphics/GraphicsContext3D.h:432
> +    void ensureContext();
Again, why not just use makeContextCurrent(), and have that function (in the Mac implementation) do the optimization of skipping the set if the context is already set?

> WebCore/platform/graphics/mac/GraphicsContext3DOpenGL.cpp:1
> +/*
This is not Mac specific. Please make a platform/graphics/opengl subdirectory and put it there. For now this file will only be included in Mac builds. But that will change when we do implementations for other platforms.

[Chris Marrin]

(In reply to comment #22)
> ...
> I noticed a few CoreGraphics calls that were not guarded by any #if()s - for example http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/GraphicsContext3DOpenGL.cpp?rev=66781#L90.  These are more likely just omissions, however.

Yes, that call should be replaced by makeContextCurrent().

[Paul Sawaya]

Created attachment 66912 [details]
makeContextCurrent, not ensureContext.

[Chris Marrin]

Comment on attachment 66912 [details]
makeContextCurrent, not ensureContext.

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

> WebCore/platform/graphics/ANGLEWebKitBridge.h:41
> +
Can you remove this extra line to avoid this spurious file in the patch?

> WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:273
> +    CGLSetCurrentContext(m_contextObj);
Need to replace this with makeContextCurrent()

> WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:332
> +    CGLSetCurrentContext(m_contextObj);
Need to replace this with makeContextCurrent()

> WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:45
>  #include <OpenGL/CGLRenderers.h>
You shouldn't need this here, should you? There should be no CGL specific calls left in this file.

> WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:439
> +
Should get rid of debugging statements like this.

[Kenneth Russell]

Also, could you please regenerate the ChangeLog by copying your comments to another file, svn reverting it, and rerunning prepare-ChangeLog? It's out of date (indicates that GraphicsContext3D.h is being modified when it isn't any more, etc.).

[Paul Sawaya]

Created attachment 66922 [details]
Removed CGL calls

[Kenneth Russell]

Comment on attachment 66922 [details]
Removed CGL calls

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

> WebCore/ChangeLog:11
> +        No new tests. (OOPS!)
You need to remove this OOPS.

[Paul Sawaya]

Are you asking that an additional test be designed, or that I just edit the OOPS out of the Changelog?

[Chris Marrin]

Comment on attachment 66922 [details]
Removed CGL calls

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

Looks good. r=me. I will land this by hand and remove the OOPS.

[Eric Seidel (no email)]

Was this landed?

[Chris Marrin]

Sorry, my bad. This was landed in http://trac.webkit.org/changeset/66781, with minor fixes in http://trac.webkit.org/changeset/67226, http://trac.webkit.org/changeset/67227, and http://trac.webkit.org/changeset/67228

[Won Jeon]

On Mac, we have "defaults write com.apple.Safari WebKitWebGLEnabled -bool YES" to enable WebGL. How can we enable WebGL on Windows then? Thanks.

[Alex Christensen]

This was finished in https://bugs.webkit.org/show_bug.cgi?id=133503

*** This bug has been marked as a duplicate of bug 133503 ***