Bug 1002281 - WebGL2 - Refactor common functions into WebGLBindableName.; r=jgilbert

WebGLBindableName represents a GL 'name' (GLuint) that can be bound to
part of the GL state machine. Similar code appeared in a number of
classes that represent GL bindable names, such as WebGLBuffer,
WebGLTexture, WebGLFramebuffer, etc.

Cleanup to reduce copy-n-paste code that's needed for creating new
objects for WebGL 2.

--HG--
extra : source : ad7803e3daf5862099f62f24fdbc83639be1ed5a

dom/canvas/WebGLBindableName.cpp

@@ -0,0 +1,27 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "WebGLBindableName.h"
+#include "GLConsts.h"
+#include "mozilla/Assertions.h"
+
+using namespace mozilla;
+
+WebGLBindableName::WebGLBindableName()
+    : mGLName(LOCAL_GL_NONE)
+    , mTarget(LOCAL_GL_NONE)
+{ }
+
+void
+WebGLBindableName::BindTo(GLenum target)
+{
+    MOZ_ASSERT(target != LOCAL_GL_NONE, "Can't bind to GL_NONE.");
+    MOZ_ASSERT(mTarget == LOCAL_GL_NONE || mTarget == target, "Rebinding is illegal.");
+
+    bool targetChanged = (target != mTarget);
+    mTarget = target;
+    if (targetChanged)
+        OnTargetChanged();
+}

dom/canvas/WebGLBindableName.h

@@ -0,0 +1,36 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef WEBGLBINDABLENAME_H_
+#define WEBGLBINDABLENAME_H_
+
+#include "WebGLTypes.h"
+
+namespace mozilla {
+
+/** Represents a GL name that can be bound to a target.
+ */
+class WebGLBindableName
+{
+public:
+    WebGLBindableName();
+    void BindTo(GLenum target);
+
+    bool HasEverBeenBound() const { return mTarget != 0; }
+    GLuint GLName() const { return mGLName; }
+    GLenum Target() const { return mTarget; }
+
+protected:
+
+    //! Called after mTarget has been changed by BindTo(target).
+    virtual void OnTargetChanged() {}
+
+    GLuint mGLName;
+    GLenum mTarget;
+};
+
+} // namespace mozilla
+
+#endif // !WEBGLBINDABLENAME_H_

dom/canvas/WebGLBuffer.cpp

@@ -13,10 +13,9 @@
 using namespace mozilla;
 
 WebGLBuffer::WebGLBuffer(WebGLContext *context)
-    : WebGLContextBoundObject(context)
-    , mHasEverBeenBound(false)
+    : WebGLBindableName()
+    , WebGLContextBoundObject(context)
     , mByteLength(0)
-    , mTarget(LOCAL_GL_NONE)
 {
     SetIsDOMBinding();
     mContext->MakeContextCurrent();
@@ -38,8 +37,7 @@ WebGLBuffer::Delete() {
 }
 
 void
-WebGLBuffer::SetTarget(GLenum target) {
-    mTarget = target;
+WebGLBuffer::OnTargetChanged() {
     if (!mCache && mTarget == LOCAL_GL_ELEMENT_ARRAY_BUFFER)
         mCache = new WebGLElementArrayCache;
 }

dom/canvas/WebGLBuffer.h

@@ -10,6 +10,7 @@
 #include "mozilla/LinkedList.h"
 #include "mozilla/MemoryReporting.h"
 #include "nsWrapperCache.h"
+#include "WebGLBindableName.h"
 #include "WebGLObjectModel.h"
 #include "WebGLTypes.h"
 
@@ -19,6 +20,7 @@ class WebGLElementArrayCache;
 
 class WebGLBuffer MOZ_FINAL
     : public nsWrapperCache
+    , public WebGLBindableName
     , public WebGLRefCountedObject<WebGLBuffer>
     , public LinkedListElement<WebGLBuffer>
     , public WebGLContextBoundObject
@@ -30,16 +32,10 @@ public:
 
     size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
 
-    bool HasEverBeenBound() { return mHasEverBeenBound; }
-    void SetHasEverBeenBound(bool x) { mHasEverBeenBound = x; }
-    GLuint GLName() const { return mGLName; }
     WebGLsizeiptr ByteLength() const { return mByteLength; }
-    GLenum Target() const { return mTarget; }
 
     void SetByteLength(WebGLsizeiptr byteLength) { mByteLength = byteLength; }
 
-    void SetTarget(GLenum target);
-
     bool ElementArrayCacheBufferData(const void* ptr, size_t buffer_size_in_bytes);
 
     void ElementArrayCacheBufferSubData(size_t pos, const void* ptr, size_t update_size_in_bytes);
@@ -61,10 +57,9 @@ public:
 protected:
     ~WebGLBuffer();
 
-    GLuint mGLName;
-    bool mHasEverBeenBound;
+    virtual void OnTargetChanged() MOZ_OVERRIDE;
+
     WebGLsizeiptr mByteLength;
-    GLenum mTarget;
 
     nsAutoPtr<WebGLElementArrayCache> mCache;
 };

dom/canvas/WebGLContextBuffers.cpp

@@ -31,9 +31,8 @@ WebGLContext::BindBuffer(GLenum target, WebGLBuffer *buffer)
     }
 
     if (buffer) {
-        if (!buffer->Target()) {
-            buffer->SetTarget(target);
-            buffer->SetHasEverBeenBound(true);
+        if (!buffer->HasEverBeenBound()) {
+            buffer->BindTo(target);
         } else if (target != buffer->Target()) {
             return ErrorInvalidOperation("bindBuffer: buffer already bound to a different target");
         }
@@ -67,12 +66,11 @@ WebGLContext::BindBufferBase(GLenum target, GLuint index, WebGLBuffer* buffer)
     }
 
     if (buffer) {
-        if (!buffer->Target()) {
-            buffer->SetTarget(target);
-            buffer->SetHasEverBeenBound(true);
-        } else if (target != buffer->Target()) {
+        if (!buffer->HasEverBeenBound())
+            buffer->BindTo(target);
+
+        if (target != buffer->Target())
             return ErrorInvalidOperation("bindBuffer: buffer already bound to a different target");
-        }
     }
 
     WebGLRefPtr<WebGLBuffer>* bufferSlot = GetBufferSlotByTarget(target, "bindBuffer");
@@ -108,12 +106,12 @@ WebGLContext::BindBufferRange(GLenum target, GLuint index, WebGLBuffer* buffer,
     }
 
     if (buffer) {
-        if (!buffer->Target()) {
-            buffer->SetTarget(target);
-            buffer->SetHasEverBeenBound(true);
-        } else if (target != buffer->Target()) {
+        if (!buffer->HasEverBeenBound())
+            buffer->BindTo(target);
+
+        if (target != buffer->Target())
             return ErrorInvalidOperation("bindBuffer: buffer already bound to a different target");
-        }
+
         CheckedInt<WebGLsizeiptr> checked_neededByteLength = CheckedInt<WebGLsizeiptr>(offset) + size;
         if (!checked_neededByteLength.isValid() ||
             checked_neededByteLength.value() > buffer->ByteLength())

dom/canvas/WebGLContextGL.cpp

@@ -174,9 +174,9 @@ WebGLContext::BindFramebuffer(GLenum target, WebGLFramebuffer *wfb)
     if (!wfb) {
         gl->fBindFramebuffer(target, 0);
     } else {
+        wfb->BindTo(target);
         GLuint framebuffername = wfb->GLName();
         gl->fBindFramebuffer(target, framebuffername);
-        wfb->SetHasEverBeenBound(true);
     }
 
     mBoundFramebuffer = wfb;
@@ -199,7 +199,7 @@ WebGLContext::BindRenderbuffer(GLenum target, WebGLRenderbuffer *wrb)
         return;
 
     if (wrb)
-        wrb->SetHasEverBeenBound(true);
+        wrb->BindTo(target);
 
     MakeContextCurrent();
 

dom/canvas/WebGLContextVertexArray.cpp

@@ -89,5 +89,3 @@ WebGLContext::IsVertexArray(WebGLVertexArray *array)
            !array->IsDeleted() &&
            array->HasEverBeenBound();
 }
-
-

dom/canvas/WebGLFramebuffer.cpp

@@ -24,9 +24,9 @@ WebGLFramebuffer::WrapObject(JSContext* cx)
 }
 
 WebGLFramebuffer::WebGLFramebuffer(WebGLContext* context)
-    : WebGLContextBoundObject(context)
+    : WebGLBindableName()
+    , WebGLContextBoundObject(context)
     , mStatus(0)
-    , mHasEverBeenBound(false)
     , mDepthAttachment(LOCAL_GL_DEPTH_ATTACHMENT)
     , mStencilAttachment(LOCAL_GL_STENCIL_ATTACHMENT)
     , mDepthStencilAttachment(LOCAL_GL_DEPTH_STENCIL_ATTACHMENT)
@@ -79,8 +79,9 @@ WebGLFramebuffer::Attachment::HasAlpha() const
 bool
 WebGLFramebuffer::Attachment::IsReadableFloat() const
 {
-    if (Texture() && Texture()->HasImageInfoAt(mTexImageTarget, mTexImageLevel)) {
-        GLenum type = Texture()->ImageInfoAt(mTexImageTarget, mTexImageLevel).WebGLType();
+    const WebGLTexture* tex = Texture();
+    if (tex && tex->HasImageInfoAt(mTexImageTarget, mTexImageLevel)) {
+        GLenum type = tex->ImageInfoAt(mTexImageTarget, mTexImageLevel).WebGLType();
         switch (type) {
         case LOCAL_GL_FLOAT:
         case LOCAL_GL_HALF_FLOAT_OES:
@@ -89,8 +90,9 @@ WebGLFramebuffer::Attachment::IsReadableFloat() const
         return false;
     }
 
-    if (Renderbuffer()) {
-        GLenum format = Renderbuffer()->InternalFormat();
+    const WebGLRenderbuffer* rb = Renderbuffer();
+    if (rb) {
+        GLenum format = rb->InternalFormat();
         switch (format) {
         case LOCAL_GL_RGB16F:
         case LOCAL_GL_RGBA16F:
@@ -101,6 +103,8 @@ WebGLFramebuffer::Attachment::IsReadableFloat() const
         return false;
     }
 
+    // If we arrive here Attachment isn't correct setup because it has
+    // no texture nor render buffer pointer.
     MOZ_ASSERT(false, "Should not get here.");
     return false;
 }

dom/canvas/WebGLFramebuffer.h

@@ -6,6 +6,7 @@
 #ifndef WEBGLFRAMEBUFFER_H_
 #define WEBGLFRAMEBUFFER_H_
 
+#include "WebGLBindableName.h"
 #include "WebGLObjectModel.h"
 
 #include "nsWrapperCache.h"
@@ -23,6 +24,7 @@ namespace gl {
 
 class WebGLFramebuffer MOZ_FINAL
     : public nsWrapperCache
+    , public WebGLBindableName
     , public WebGLRefCountedObject<WebGLFramebuffer>
     , public LinkedListElement<WebGLFramebuffer>
     , public WebGLContextBoundObject
@@ -92,10 +94,6 @@ public:
 
     void Delete();
 
-    bool HasEverBeenBound() { return mHasEverBeenBound; }
-    void SetHasEverBeenBound(bool x) { mHasEverBeenBound = x; }
-    GLuint GLName() { return mGLName; }
-
     void FramebufferRenderbuffer(GLenum target,
                                  GLenum attachment,
                                  GLenum rbtarget,
@@ -183,9 +181,6 @@ private:
 
     mutable GLenum mStatus;
 
-    GLuint mGLName;
-    bool mHasEverBeenBound;
-
     // we only store pointers to attached renderbuffers, not to attached textures, because
     // we will only need to initialize renderbuffers. Textures are already initialized.
     nsTArray<Attachment> mColorAttachments;

dom/canvas/WebGLRenderbuffer.cpp

@@ -43,12 +43,12 @@ WebGLRenderbuffer::WrapObject(JSContext *cx) {
 }
 
 WebGLRenderbuffer::WebGLRenderbuffer(WebGLContext *context)
-    : WebGLContextBoundObject(context)
+    : WebGLBindableName()
+    , WebGLContextBoundObject(context)
     , mPrimaryRB(0)
     , mSecondaryRB(0)
     , mInternalFormat(0)
     , mInternalFormatForGL(0)
-    , mHasEverBeenBound(false)
     , mImageDataStatus(WebGLImageDataStatus::NoImageData)
 {
     SetIsDOMBinding();

dom/canvas/WebGLRenderbuffer.h

@@ -6,6 +6,7 @@
 #ifndef WEBGLRENDERBUFFER_H_
 #define WEBGLRENDERBUFFER_H_
 
+#include "WebGLBindableName.h"
 #include "WebGLObjectModel.h"
 #include "WebGLFramebufferAttachable.h"
 
@@ -17,6 +18,7 @@ namespace mozilla {
 
 class WebGLRenderbuffer MOZ_FINAL
     : public nsWrapperCache
+    , public WebGLBindableName
     , public WebGLRefCountedObject<WebGLRenderbuffer>
     , public LinkedListElement<WebGLRenderbuffer>
     , public WebGLRectangleObject
@@ -28,9 +30,6 @@ public:
 
     void Delete();
 
-    bool HasEverBeenBound() { return mHasEverBeenBound; }
-    void SetHasEverBeenBound(bool x) { mHasEverBeenBound = x; }
-
     bool HasUninitializedImageData() const { return mImageDataStatus == WebGLImageDataStatus::UninitializedImageData; }
     void SetImageDataStatus(WebGLImageDataStatus x) {
         // there is no way to go from having image data to not having any
@@ -71,7 +70,6 @@ protected:
     GLuint mSecondaryRB;
     GLenum mInternalFormat;
     GLenum mInternalFormatForGL;
-    bool mHasEverBeenBound;
     WebGLImageDataStatus mImageDataStatus;
 
     friend class WebGLFramebuffer;

dom/canvas/WebGLTexture.cpp

@@ -23,9 +23,8 @@ WebGLTexture::WrapObject(JSContext *cx) {
 }
 
 WebGLTexture::WebGLTexture(WebGLContext *context)
-    : WebGLContextBoundObject(context)
-    , mHasEverBeenBound(false)
-    , mTarget(0)
+    : WebGLBindableName()
+    , WebGLContextBoundObject(context)
     , mMinFilter(LOCAL_GL_NEAREST_MIPMAP_LINEAR)
     , mMagFilter(LOCAL_GL_LINEAR)
     , mWrapS(LOCAL_GL_REPEAT)
@@ -108,18 +107,21 @@ WebGLTexture::Bind(GLenum aTarget) {
     // this function should only be called by bindTexture().
     // it assumes that the GL context is already current.
 
-    bool firstTimeThisTextureIsBound = !mHasEverBeenBound;
+    bool firstTimeThisTextureIsBound = !HasEverBeenBound();
 
-    if (!firstTimeThisTextureIsBound && aTarget != mTarget) {
+    if (firstTimeThisTextureIsBound) {
+        BindTo(aTarget);
+    } else if (aTarget != Target()) {
         mContext->ErrorInvalidOperation("bindTexture: this texture has already been bound to a different target");
         // very important to return here before modifying texture state! This was the place when I lost a whole day figuring
         // very strange 'invalid write' crashes.
         return;
     }
 
-    mTarget = aTarget;
+    GLuint name = GLName();
+    GLenum target = Target();
 
-    mContext->gl->fBindTexture(mTarget, mGLName);
+    mContext->gl->fBindTexture(target, name);
 
     if (firstTimeThisTextureIsBound) {
         mFacesCount = (mTarget == LOCAL_GL_TEXTURE_2D) ? 1 : 6;
@@ -132,8 +134,6 @@ WebGLTexture::Bind(GLenum aTarget) {
         if (mTarget == LOCAL_GL_TEXTURE_CUBE_MAP && !mContext->gl->IsGLES())
             mContext->gl->fTexParameteri(mTarget, LOCAL_GL_TEXTURE_WRAP_R, LOCAL_GL_CLAMP_TO_EDGE);
     }
-
-    mHasEverBeenBound = true;
 }
 
 void
@@ -141,8 +141,14 @@ WebGLTexture::SetImageInfo(GLenum aTarget, GLint aLevel,
                   GLsizei aWidth, GLsizei aHeight,
                   GLenum aFormat, GLenum aType, WebGLImageDataStatus aStatus)
 {
-    if ( (aTarget == LOCAL_GL_TEXTURE_2D) != (mTarget == LOCAL_GL_TEXTURE_2D) )
+    // TODO(djg): I suspected the following ASSERT and check are
+    //            trying to express more than they're saying, probably
+    //            to do with cubemap targets. We should do this
+    //            properly. https://bugzilla.mozilla.org/show_bug.cgi?id=1006908
+    MOZ_ASSERT((aTarget == LOCAL_GL_TEXTURE_2D) == (mTarget == LOCAL_GL_TEXTURE_2D));
+    if ((aTarget == LOCAL_GL_TEXTURE_2D) != (mTarget == LOCAL_GL_TEXTURE_2D)) {
         return;
+    }
 
     EnsureMaxLevelWithCustomImagesAtLeast(aLevel);
 

dom/canvas/WebGLTexture.h

@@ -6,8 +6,9 @@
 #ifndef WEBGLTEXTURE_H_
 #define WEBGLTEXTURE_H_
 
-#include "WebGLObjectModel.h"
+#include "WebGLBindableName.h"
 #include "WebGLFramebufferAttachable.h"
+#include "WebGLObjectModel.h"
 
 #include "nsWrapperCache.h"
 
@@ -27,6 +28,7 @@ inline bool is_pot_assuming_nonnegative(GLsizei x)
 // WrapObject calls in GetParameter and GetFramebufferAttachmentParameter.
 class WebGLTexture MOZ_FINAL
     : public nsWrapperCache
+    , public WebGLBindableName
     , public WebGLRefCountedObject<WebGLTexture>
     , public LinkedListElement<WebGLTexture>
     , public WebGLContextBoundObject
@@ -37,11 +39,6 @@ public:
 
     void Delete();
 
-    bool HasEverBeenBound() const { return mHasEverBeenBound; }
-    void SetHasEverBeenBound(bool x) { mHasEverBeenBound = x; }
-    GLuint GLName() const { return mGLName; }
-    GLenum Target() const { return mTarget; }
-
     WebGLContext *GetParentObject() const {
         return Context();
     }
@@ -59,9 +56,6 @@ protected:
     friend class WebGLContext;
     friend class WebGLFramebuffer;
 
-    bool mHasEverBeenBound;
-    GLuint mGLName;
-
     // we store information about the various images that are part of
     // this texture (cubemap faces, mipmap levels)
 
@@ -205,7 +199,6 @@ public:
 
 protected:
 
-    GLenum mTarget;
     GLenum mMinFilter, mMagFilter, mWrapS, mWrapT;
 
     size_t mFacesCount, mMaxLevelWithCustomImages;
@@ -278,7 +271,7 @@ public:
     bool IsMipmapCubeComplete() const;
 
     void SetFakeBlackStatus(WebGLTextureFakeBlackStatus x);
-    
+
     // Returns the current fake-black-status, except if it was Unknown,
     // in which case this function resolves it first, so it never returns Unknown.
     WebGLTextureFakeBlackStatus ResolvedFakeBlackStatus();

dom/canvas/WebGLVertexArray.cpp

@@ -20,9 +20,8 @@ WebGLVertexArray::WrapObject(JSContext *cx) {
 }
 
 WebGLVertexArray::WebGLVertexArray(WebGLContext* context)
-    : WebGLContextBoundObject(context)
-    , mGLName(0)
-    , mHasEverBeenBound(false)
+    : WebGLBindableName()
+    , WebGLContextBoundObject(context)
 {
     SetIsDOMBinding();
     context->mVertexArrays.insertBack(this);

dom/canvas/WebGLVertexArray.h

@@ -6,6 +6,7 @@
 #ifndef WEBGLVERTEXARRAY_H_
 #define WEBGLVERTEXARRAY_H_
 
+#include "WebGLBindableName.h"
 #include "WebGLObjectModel.h"
 #include "WebGLBuffer.h"
 #include "WebGLVertexAttribData.h"
@@ -20,6 +21,7 @@ class WebGLVertexArrayFake;
 
 class WebGLVertexArray
     : public nsWrapperCache
+    , public WebGLBindableName
     , public WebGLRefCountedObject<WebGLVertexArray>
     , public LinkedListElement<WebGLVertexArray>
     , public WebGLContextBoundObject
@@ -30,7 +32,9 @@ public:
     static WebGLVertexArray* Create(WebGLContext* context);
 
     void BindVertexArray() {
-        SetHasEverBeenBound(true);
+        /* Bind to dummy value to signal that this vertex array has
+           ever been bound */
+        BindTo(LOCAL_GL_VERTEX_ARRAY_BINDING);
         BindVertexArrayImpl();
     };
 
@@ -58,9 +62,6 @@ public:
     // -------------------------------------------------------------------------
     // MEMBER FUNCTIONS
 
-    bool HasEverBeenBound() { return mHasEverBeenBound; }
-    void SetHasEverBeenBound(bool x) { mHasEverBeenBound = x; }
-
     bool EnsureAttrib(GLuint index, const char *info);
     bool HasAttrib(GLuint index) {
         return index < mAttribs.Length();
@@ -82,8 +83,6 @@ protected:
     // -------------------------------------------------------------------------
     // MEMBERS
 
-    GLuint mGLName;
-    bool mHasEverBeenBound;
     nsTArray<WebGLVertexAttribData> mAttribs;
     WebGLRefPtr<WebGLBuffer> mElementArrayBuffer;
 

dom/canvas/moz.build

@@ -41,6 +41,7 @@ if CONFIG['MOZ_WEBGL']:
         'WebGL1Context.cpp',
         'WebGL2Context.cpp',
         'WebGLActiveInfo.cpp',
+        'WebGLBindableName.cpp',
         'WebGLBuffer.cpp',
         'WebGLContext.cpp',
         'WebGLContextAsyncQueries.cpp',