561957 - Add OpenGL rendering to Mac OS X

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Mac Context provider backend and cocoa widget changes to enable it.

Comment 2

[Matt Woodrow (:mattwoodrow)]

The pixel buffer options will almost definitely need adjusting. Just setting
the accelerated flags appears to work for now.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+} sCGLLibrary;

We don't declare classes and instantiate them in the same statement ... separate them.

+        /*
+        NSOpenGLPFAColorSize, 24,
+        NSOpenGLPFAAlphaSize, 8,
+        NSOpenGLPFADepthSize, 0,
+        NSOpenGLPFASupersample, */

Why are those commented out? Just remove them if we don't want them.

+    NSOpenGLPixelFormat *pixelFormat = [[(NSOpenGLPixelFormat *)[NSOpenGLPixelFormat alloc] initWithAttributes:attributes] autorelease]; 
+    NSOpenGLContext *context = [[NSOpenGLContext alloc] initWithFormat:pixelFormat shareContext:NULL];
+

80 column warning!

+    return glContext.forget().get(); 

Don't need get() here, I think

Otherwise looks great!

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 441709 [details] [diff] [review]
Mac Context provider backend and cocoa widget changes to enable it.

>diff --git a/gfx/layers/opengl/LayerManagerOGL.h b/gfx/layers/opengl/LayerManagerOGL.h
>--- a/gfx/layers/opengl/LayerManagerOGL.h
>+++ b/gfx/layers/opengl/LayerManagerOGL.h
>@@ -44,21 +44,21 @@
> #include <windows.h>
> #endif
> 
> /**
>  * We don't include GLDefs.h here since we don't want to drag in all defines
>  * in for all our users.
>  */
> #if defined(__APPLE__)
>-typedef unsigned long GLenum;
>-typedef unsigned long GLbitfield;
>-typedef unsigned long GLuint;
>-typedef long GLint;
>-typedef long GLsizei;
>+typedef unsigned int GLenum;
>+typedef unsigned int GLbitfield;
>+typedef unsigned int GLuint;
>+typedef int GLint;
>+typedef int GLsizei;
> #else
> typedef unsigned int GLenum;
> typedef unsigned int GLbitfield;
> typedef unsigned int GLuint;
> typedef int GLint;
> typedef int GLsizei;
> #endif

You'll notice that these are identical :)  Get rid of the #ifdef, that used to say __APPLE__XXX, not sure why Bas got rid of the XXX.  But it's not needed either way.


>diff --git a/gfx/layers/opengl/LayerManagerOGLShaders.h b/gfx/layers/opengl/LayerManagerOGLShaders.h
>--- a/gfx/layers/opengl/LayerManagerOGLShaders.h
>+++ b/gfx/layers/opengl/LayerManagerOGLShaders.h
>@@ -32,11 +32,11 @@ static const GLchar *sYUVLayerPS = SHADE
>     vec4 yuv; \
>     vec4 color; \
>     yuv.r = texture2D(uCrTexture, vTextureCoordinate).r - 0.5; \
>     yuv.g = texture2D(uYTexture, vTextureCoordinate).r - 0.0625; \
>     yuv.b = texture2D(uCbTexture, vTextureCoordinate).r - 0.5; \
>     color.r = yuv.g * 1.164 + yuv.r * 1.596; \
>     color.g = yuv.g * 1.164 - 0.813 * yuv.r - 0.391 * yuv.b; \
>     color.b = yuv.g * 1.164 + yuv.b * 2.018; \
>-    color.a = 1.0f; \
>+    color.a = 1.0; \
>     gl_FragColor = color * uLayerOpacity; \
> }";

Hmm, this isn't correct; in fact I think all the numeric constants in this shader need a 'f' suffix.  Was the GLSL compiler giving you an error?

>diff --git a/gfx/thebes/public/GLContext.h b/gfx/thebes/public/GLContext.h
>--- a/gfx/thebes/public/GLContext.h
>+++ b/gfx/thebes/public/GLContext.h
>@@ -101,16 +101,17 @@ class GLContext
> public:
>     GLContext() : mInitialized(false) { }
> 
>     virtual ~GLContext() { }
> 
>     virtual PRBool MakeCurrent() = 0;
>     virtual PRBool SetupLookupFunction() = 0;
> 
>+    virtual void *GetNativeContext() { return NULL; }
> protected:
> 
>     PRBool mInitialized;
> 
>     PRBool InitWithPrefix(const char *prefix, PRBool trygl);
> 
>     //
>     // the wrapped functions
>diff --git a/gfx/thebes/public/GLDefs.h b/gfx/thebes/public/GLDefs.h
>--- a/gfx/thebes/public/GLDefs.h
>+++ b/gfx/thebes/public/GLDefs.h
>@@ -41,21 +41,21 @@
> #define LOCALGL_H_
> 
> #if !defined(__gl_h_)
> #define __gl_h_
> 
> #include <stddef.h>
> 
> #if defined(__APPLE__)
>-typedef unsigned long GLenum;
>-typedef unsigned long GLbitfield;
>-typedef unsigned long GLuint;
>-typedef long GLint;
>-typedef long GLsizei;
>+typedef unsigned int GLenum;
>+typedef unsigned int GLbitfield;
>+typedef unsigned int GLuint;
>+typedef int GLint;
>+typedef int GLsizei;
> #else
> typedef unsigned int GLenum;
> typedef unsigned int GLbitfield;
> typedef unsigned int GLuint;
> typedef int GLint;
> typedef int GLsizei;
> #endif
> typedef char realGLboolean;

Same as above; this really shouldn't be duplicated.

>diff --git a/gfx/thebes/src/GLContextProviderCGL.mm b/gfx/thebes/src/GLContextProviderCGL.mm
>new file mode 100644
>--- /dev/null
>+++ b/gfx/thebes/src/GLContextProviderCGL.mm
>@@ -0,0 +1,156 @@
>+/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Bas Schouten <[e-mail]>
>+ *   Matt Woodrow <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#include "GLContextProvider.h"
>+#include "nsDebug.h"
>+#include "nsIWidget.h"
>+#include "OpenGL/OpenGL.h"
>+#include <OpenGL/gl.h>
>+#include <AppKit/NSOpenGL.h>
>+
>+namespace mozilla {
>+namespace gl {
>+
>+GLContextProvider sGLContextProvider;
>+
>+static class CGLLibrary
>+{
>+public:
>+    CGLLibrary() : mInitialized(PR_FALSE) {}
>+
>+    PRBool EnsureInitialized()
>+    {
>+        if (mInitialized) {
>+            return PR_TRUE;
>+        }
>+        if (!mOGLLibrary) {
>+            mOGLLibrary = PR_LoadLibrary("/System/Library/Frameworks/OpenGL.framework/OpenGL");
>+            if (!mOGLLibrary) {
>+                NS_WARNING("Couldn't load OpenGL DLL.");

^ It's not a DLL; call it a Framework.

>+                return PR_FALSE;
>+            }
>+        }
>+        
>+        mInitialized = PR_TRUE;
>+        return PR_TRUE;
>+    }
>+
>+private:
>+    PRBool mInitialized;
>+    PRLibrary *mOGLLibrary;
>+} sCGLLibrary;
>+
>+class GLContextCGL : public GLContext
>+{
>+public:
>+    GLContextCGL(NSOpenGLContext *aContext)
>+        : mContext(aContext) {}
>+
>+    ~GLContextCGL()
>+    {
>+        [mContext release];
>+    }
>+
>+    PRBool Init()
>+    {
>+        MakeCurrent();
>+        return InitWithPrefix("gl", PR_TRUE);
>+    }
>+
>+    void *GetNativeContext() 
>+    { 
>+        return mContext; 
>+    }
>+
>+    PRBool MakeCurrent()
>+    {
>+        [mContext makeCurrentContext];
>+        return PR_TRUE;
>+    }
>+
>+    PRBool SetupLookupFunction()
>+    {
>+        return PR_FALSE;
>+    }
>+
>+private:
>+    NSOpenGLContext *mContext;
>+};

Everything else looks fine.  I assume this works fine on try server (without accelerated windows)?  r=me if it does, though it would be good to explain the GLSL change near the top.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

> 80 column warning!

roc, the 60's called, they want their teletype back :p

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Nits fixed

All nits fixed except:

.forget().get() - Required to compile (and stolen from the windows backend equivalent!)

Shader changes: This was silently failing for me (giving me black and white video), debugging using 'OpenGL Shader Builder' showed the 'f' character as invalid.

Comment 7

[Joe Drew (not getting mail)]

forget().get() is required because forget() returns an already_AddRefed<GLContextCGL>, but we want an already_AddRefed<GLContext>, and we can't do that type of polymorphism in C++.

+already_AddRefed<GLContext>
+GLContextProvider::CreateForWindow(nsIWidget *aWidget)

+    nsRefPtr<GLContextCGL> glContext = new GLContextCGL(context);

+    return glContext.forget().get();

Comment 8

[Matt Woodrow (:mattwoodrow)]

Looks like this built on the try server without issues.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/63d5af97ec83

Comment 10

[Stuart Morgan]

>+  nsCocoaWindow* window = GetXULWindowWidget();
>+  if (window->GetAcceleratedRendering() != mUseAcceleratedRendering) {

Not all Gecko browsers are XUL-based; this code crashes Camino on launch. The return value of GetXULWindowWidget will always be null for embedding.

I don't have enough context to know if an additional check is all that is required, or if there need to be deeper changes to make this embedding-friendly (it seems like latter though, otherwise mUseAcceleratedRendering will always be false, which is presumably not ideal).

Comment 11

[Stuart Morgan]

Bug 570314 filed for the regression.