Bug 30625

Summary: Implement WebGL on Windows
Product: WebKit Reporter: Chris Marrin <cmarrin>
Component: WebGLAssignee: Chris Marrin <cmarrin>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: abarth, achristensen, adele, bikoz.r, cmarrin, commit-queue, dglazkov, eric, jamesr, kbr, oliver, prideout, psawaya, webkit.review.bot, wjjeon, xanthor+bz, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Bug Depends on: 45220    
Bug Blocks:    
Attachments:
Description Flags
Refactored some non Mac specific OpenGL code out of GraphicsContext3DMac.mm to make ANGLE (libGLESv2) integration easier.
cmarrin: review-
Made requested changes
cmarrin: review-
Added ChangeLog
none
Fixed changelog
none
Fixed paths in patch, hopefully this one will apply
none
Moved into /mac, build only on Mac
cmarrin: review-
makeContextCurrent, not ensureContext.
cmarrin: review-
Removed CGL calls none

Description Chris Marrin 2009-10-21 06:34:17 PDT
The first implementation of this could possibly be based on Chrome's glex implementation.
Comment 1 Chris Marrin 2010-07-26 10:49:24 PDT
We will implement this on top of ANGLE, which is being added here:

    https://bugs.webkit.org/show_bug.cgi?id=42789
Comment 2 Paul Sawaya 2010-08-27 14:22:49 PDT
Created attachment 65767 [details]
Refactored some non Mac specific OpenGL code out of GraphicsContext3DMac.mm to make ANGLE (libGLESv2) integration easier.
Comment 3 Kenneth Russell 2010-08-27 16:44:51 PDT
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.
Comment 4 Darin Adler 2010-08-29 13:11:17 PDT
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?
Comment 5 Chris Marrin 2010-08-30 12:41:20 PDT
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.
Comment 6 Chris Marrin 2010-08-30 12:46:48 PDT
(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).
Comment 7 Chris Marrin 2010-08-30 13:02:00 PDT
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.
Comment 8 Paul Sawaya 2010-09-01 18:36:47 PDT
Created attachment 66310 [details]
Made requested changes
Comment 9 Chris Marrin 2010-09-02 08:17:56 PDT
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.
Comment 10 Paul Sawaya 2010-09-02 09:42:20 PDT
Created attachment 66380 [details]
Added ChangeLog

Oops, sorry about that.
Comment 11 Chris Marrin 2010-09-02 10:06:13 PDT
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.
Comment 12 Paul Sawaya 2010-09-03 10:48:59 PDT
Created attachment 66518 [details]
Fixed changelog
Comment 13 WebKit Commit Bot 2010-09-03 13:53:16 PDT
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
Comment 14 Paul Sawaya 2010-09-03 17:17:00 PDT
Created attachment 66568 [details]
Fixed paths in patch, hopefully this one will apply
Comment 15 WebKit Review Bot 2010-09-03 17:19:19 PDT
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.
Comment 16 Chris Marrin 2010-09-03 17:50:39 PDT
Landed in http://trac.webkit.org/changeset/66781
Comment 17 WebKit Review Bot 2010-09-03 18:00:56 PDT
http://trac.webkit.org/changeset/66781 might have broken Chromium Linux Release
Comment 18 WebKit Review Bot 2010-09-03 18:10:51 PDT
Attachment 66568 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3976080
Comment 19 James Robinson 2010-09-03 18:12:53 PDT
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.
Comment 20 Chris Marrin 2010-09-04 06:17:06 PDT
This should have not been closed. This is only a patch to prepare for the WebGL Windows work. Other patches are coming.
Comment 21 Chris Marrin 2010-09-06 07:40:31 PDT
(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?
Comment 22 James Robinson 2010-09-07 14:18:30 PDT
(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.
Comment 23 Paul Sawaya 2010-09-08 01:33:50 PDT
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.
Comment 24 WebKit Review Bot 2010-09-08 01:38:45 PDT
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.
Comment 25 Chris Marrin 2010-09-08 07:45:33 PDT
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.
Comment 26 Chris Marrin 2010-09-08 07:47:25 PDT
(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().
Comment 27 Paul Sawaya 2010-09-08 10:38:12 PDT
Created attachment 66912 [details]
makeContextCurrent, not ensureContext.
Comment 28 Chris Marrin 2010-09-08 11:05:02 PDT
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.
Comment 29 Kenneth Russell 2010-09-08 11:38:15 PDT
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.).
Comment 30 Paul Sawaya 2010-09-08 11:59:45 PDT
Created attachment 66922 [details]
Removed CGL calls
Comment 31 Kenneth Russell 2010-09-08 12:06:47 PDT
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.
Comment 32 Paul Sawaya 2010-09-08 12:49:10 PDT
Are you asking that an additional test be designed, or that I just edit the OOPS out of the Changelog?
Comment 33 Chris Marrin 2010-09-08 13:50:33 PDT
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.
Comment 34 Eric Seidel (no email) 2010-12-20 22:37:53 PST
Was this landed?
Comment 36 Won Jeon 2011-06-14 14:45:23 PDT
On Mac, we have "defaults write com.apple.Safari WebKitWebGLEnabled -bool YES" to enable WebGL. How can we enable WebGL on Windows then? Thanks.
Comment 37 Alex Christensen 2014-06-04 17:15:22 PDT
This was finished in https://bugs.webkit.org/show_bug.cgi?id=133503

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