Add stronger types to WebGL texture code for better debug/compile time checking

RESOLVED FIXED in mozilla35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: wlitwinczyk, Assigned: wlitwinczyk)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8484383 [details] [diff] [review]
strong_types_1

WebGL uses GLenum internally for most of its parameters, which has recently led to a lot of bugs in the texture code. Because of this I talked with Jeff and it was decided that it would be a good idea if parameters and values were made more explicit in their type (and purpose). A small wrapper class for GLenum has been created as a first step in cleaning up the use of plain GLenums. The main file of interest is WebGLStrongTypes.h.

This is the first installment of hopefully many more patches. This first bug deals only with the different texture targets: texture binding locations vs. texture image targets.

There are a couple of benefits to this approach:
 1.) We can eliminate a lot of redundant validation. Ideally all parameters are validated once at an entry point (one of the JS->C++ functions) and then internally almost zero validation needs to be done. It also simplifies internal code because it can have a guarantee, for example, that a TexImageTarget type only ever contains valid values.

 2.) Checks for silly mistakes, for example
> if (target == LOCAL_GL_TEXTURE_2D)
will now make sure that LOCAL_GL_TEXTURE_2D is a value that is valid for the target variable (checked during debug runtime).

 3.) Easier to refactor, the compiler will let you know if you try to pass the wrong types around.

 Plus some others

================================

Already it's pretty obvious where a lot of redundant code exists. For example the TexImage2D functions in WebGLContext.h have to validate the target enum and then have to pass the raw enum to TexImage2D_base, which then revalidates the enum. But now that these types exist texImage2D_base can be changed to take a validated type and then fix all the code that calls it!

As a note for the reviewers: the Get() call should be suspicious, only called when needed, and definitely not inside if statements comparing constants (as in the example above).

================================

bjacob: I added you for feedback because Jeff said you have done a lot of C++ meta programming. Any advice you have on what has been done (improvements or alternatives) is appreciated.

================================

There are a few pain points at the moment. I added a default constructor to get the code to compile, but ideally the wrapper class should make sure that Get() is never called on a default constructed object. This can probably be handled with a flag of some sort, but it'd be preferred if two versions of the class didn't need to be written to make sure this check is gone in release mode. There may have been others, but they escape me at the moment.
Attachment #8484383 - Flags: review?(jgilbert)
Attachment #8484383 - Flags: review?(dglastonbury)
Attachment #8484383 - Flags: feedback?(bjacob)
(Assignee)

Comment 1

4 years ago
Apparently the macro isn't quite right for release builds, but should be an easy fix:

https://tbpl.mozilla.org/?tree=Try&rev=da20aa658108
(Assignee)

Comment 2

4 years ago
Adding depends because this patch is built on top of bug 1006908
Depends on: 1006908
Comment on attachment 8484383 [details] [diff] [review]
strong_types_1

Review of attachment 8484383 [details] [diff] [review]:
-----------------------------------------------------------------

Great, but having to explicitly construct for #defined enums is unnecessary. Let's drop that requirement.

::: dom/canvas/WebGLContext.h
@@ +554,5 @@
> +            return ErrorInvalidValue("texSubImage2D: level is negative");
> +
> +        const int32_t maxLevel = MaxTextureLevelForTexImageTarget(texImageTarget);
> +        if (level > maxLevel)
> +            return ErrorInvalidValue("texSubImage2D: level %d is too large, max is %d", level, maxLevel);

It looks like this is validation that wasn't here previously. Let's handle this in a different bug, so we can backport this.

@@ +571,5 @@
>              return;
>  
>          gfx::IntSize size = data->GetSize();
>          uint32_t byteLength = data->Stride() * size.height;
> +        return TexSubImage2D_base(texImageTarget.Get(), level, xoffset, yoffset,

Please push the strong types down into this function.

@@ +1199,5 @@
>      template<class ObjectType>
>      bool ValidateObjectAssumeNonNull(const char* info, ObjectType *aObject);
>  
>  protected:
> +    int32_t MaxTextureSizeForTarget(TexTarget target) const {

The previous function, based on its assert, actually takes a TexImageTarget.

::: dom/canvas/WebGLContextGL.cpp
@@ +246,3 @@
>      }
>  
> +    const TexTarget target = TexTarget(rawTarget);

Cool.

::: dom/canvas/WebGLContextUtils.cpp
@@ +63,2 @@
>      case LOCAL_GL_TEXTURE_2D:
> +        return TexTarget(LOCAL_GL_TEXTURE_2D);

You shouldn't need to explicitly construct this.

@@ +573,5 @@
>      // Textures
>      GLenum activeTexture = mActiveTexture + LOCAL_GL_TEXTURE0;
>      AssertUintParamCorrect(gl, LOCAL_GL_ACTIVE_TEXTURE, activeTexture);
>  
> +    WebGLTexture* curTex = activeBoundTextureForTarget(TexTarget(LOCAL_GL_TEXTURE_2D));

You shouldn't need to explicitly construct this.

::: dom/canvas/WebGLContextValidate.cpp
@@ +902,5 @@
>  
>      if (level > 31)
>          level = 31;
>  
> +    const GLuint maxTexImageSize = MaxTextureSizeForTarget(TexImageTargetToTexTarget(texImageTarget)) >> level;

You shouldn't need to explicitly construct this.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +88,2 @@
>  
> +        // TODO - Override WebGLTexture::Target() to return the proper type!

Why not now! :D

::: dom/canvas/WebGLFramebuffer.h
@@ +72,5 @@
>          }
>          WebGLRenderbuffer* Renderbuffer() {
>              return mRenderbufferPtr;
>          }
> +        TexImageTarget GetTexImageTarget() const {

The others getters here aren't prefixed with Get, so just leave it as it was.

::: dom/canvas/WebGLStrongTypes.h
@@ +87,5 @@
> +}
> +
> +#endif
> +
> +#define MAKE_TYPE_BEGIN(name) \

s/name/NAME/ to make it obvious what we're replacing.

Also, STRONG_ENUM_TYPE{_BEGIN,_END} is probably a better name for these.

Further, it would be ideal to keep the `\` all in the same column. Just put them out in the column one past the end of the longest line.

@@ +122,5 @@
> +        bool operator!=(const name& other) const { \
> +            return mValue != other.mValue; \
> +        } \
> +        \
> +        explicit name(GLenum val) \

Leave this as non-explicit, so we don't need to explicitly construct these for each GLenum we test against.
As long as we don't have an `operator int`, we're protected against creating Foos from Bars.

::: dom/canvas/WebGLTexture.cpp
@@ +110,5 @@
>      bool firstTimeThisTextureIsBound = !HasEverBeenBound();
>  
>      if (firstTimeThisTextureIsBound) {
> +        BindTo(aTexTarget.Get());
> +    } else if (aTexTarget != TexTarget(Target())) {

Is it too much to ask for you to dig in here and make these functions (BindTo() and Target()) use the new stronger types?

@@ +118,5 @@
>          return;
>      }
>  
>      GLuint name = GLName();
> +    TexTarget target = TexTarget(Target());

`target` should be provably `aTexTarget` at this point.

@@ +123,2 @@
>  
> +    mContext->gl->fBindTexture(target.Get(), name);

Great! Ideally, glFunc calls should be the only place we use `.Get()`.
Attachment #8484383 - Flags: review?(jgilbert) → review-
Just a preliminary comment at this point: huge macros should only be used as a last resort. Please elaborate on why this huge macro can't be replaced by usage of appropriate c++ constructs. Sometimes one needs a macro, but can minimize its size by offloading its body to a non-macro c++ helper.
(Assignee)

Comment 5

4 years ago
Comment on attachment 8484383 [details] [diff] [review]
strong_types_1

Dan, I'm canceling your review so you don't waste time on this version. The updated version will be a bit different, which I won't get to until tomorrow.
Attachment #8484383 - Flags: review?(dglastonbury)
Comment on attachment 8484383 [details] [diff] [review]
strong_types_1

Review of attachment 8484383 [details] [diff] [review]:
-----------------------------------------------------------------

Overall I like this concept but I'd like to see more thought about the execution of it.

Instead of having a class wrap an enum-sized value, can we make an enum out of the values? My C++11-fu is weak, but we have MOZ_BEGIN_ENUM_CLASS (http://dxr.mozilla.org/mozilla-central/source/mfbt/TypedEnum.h#43) for inspiration.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +88,2 @@
>  
> +        // TODO - Override WebGLTexture::Target() to return the proper type!

Yeah. "Finish your derivations, please"

@@ +397,5 @@
>          MOZ_ASSERT(gl == Texture()->Context()->gl);
>  
>          if (attachmentLoc == LOCAL_GL_DEPTH_STENCIL_ATTACHMENT) {
>              gl->fFramebufferTexture2D(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_DEPTH_ATTACHMENT,
> +                                      GetTexImageTarget().Get(), Texture()->GLName(), GetTexImageLevel());

Should we do manual common sub-expression hoisting here?

const GLenum texImageTarget = GetTexImageTarget().Get();
if (attachmentLoc == LOCAL_GL_DEPTH_STENCIL_ATTACHMENT) {
    gl->fFrameBufferTexture2D(LOCAL_GL_FRAMEBUFFER, ..., texImageTarget, ...);
    gl->fFrameBufferTexture2D(LOCAL_GL_FRAMEBUFFER, ..., texImageTarget, ...);
} else {
    gl->fFrameBufferTexture2D(LOCAL_GL_FRAMEBUFFER, ..., texImageTarget, ...);
}

@@ +615,2 @@
>      if (mDepthStencilAttachment.Texture() == tex)
> +        FramebufferTexture2D(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_DEPTH_STENCIL_ATTACHMENT, TexImageTarget(LOCAL_GL_TEXTURE_2D), nullptr, 0);

See this sets off my tingly spider-senses: LOCAL_GL_TEXTURE_2D goes from constant to expensive-ish constructor, which we rely on compiler to extract. I think we should either:
a) Extract the constructions of TexImageTarget(LOCAL_GL_TEXTURE_2D) into a const local, or
b) Provide static TexImageTarget accessor for values.

::: dom/canvas/WebGLStrongTypes.h
@@ +125,5 @@
> +        \
> +        explicit name(GLenum val) \
> +            : mValue(val) \
> +        { \
> +            const GLenum validValues[] = { \

Does this mean that a GLenum array will be created on the stack for every GLenum -> StrongType construction? This should be const static some how.

Given the cut'n'paste nature of the comparison operators and that they can conceivably just check enum values, because the constructor acts as a "validity" gate, this macro could become a template class + base class idiom, where the template is used to customize the array of valid values. I think this would address :bjacob's comment.

::: dom/canvas/WebGLTexture.cpp
@@ +110,5 @@
>      bool firstTimeThisTextureIsBound = !HasEverBeenBound();
>  
>      if (firstTimeThisTextureIsBound) {
> +        BindTo(aTexTarget.Get());
> +    } else if (aTexTarget != TexTarget(Target())) {

BindTo can be made into a template function in WebGLBindableName.

@@ +209,5 @@
>  bool
>  WebGLTexture::IsMipmapTexture2DComplete() const {
>      if (mTarget != LOCAL_GL_TEXTURE_2D)
>          return false;
> +    if (!ImageInfoAt(TexImageTarget(LOCAL_GL_TEXTURE_2D), 0).IsPositive())

This is where I'm concerned about the constructor building an array on the stack for this local check. Jeff already reported our WebGL functions are slow on ARM. Perhaps we just accept this and then look at it again when profiling the ARM perf. issues?
(Assignee)

Comment 7

4 years ago
(In reply to Dan Glastonbury :djg :kamidphish from comment #6)
> Comment on attachment 8484383 [details] [diff] [review]
> strong_types_1
> 
> Review of attachment 8484383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall I like this concept but I'd like to see more thought about the
> execution of it.
> 
> Instead of having a class wrap an enum-sized value, can we make an enum out
> of the values? My C++11-fu is weak, but we have MOZ_BEGIN_ENUM_CLASS
> (http://dxr.mozilla.org/mozilla-central/source/mfbt/TypedEnum.h#43) for
> inspiration.

Lots to talk about! This is a large change. Jeff and I were talking about using the existing enum class, but we determined that it was a little further than we should go for now. With the enums we'd need to change more code and marshall between the GLenums and the new enum values a lot (until everything got changed). We figured this debug only validation would get us most of the way and then we could determine a better strategy. We also talked about making the WebGL enums stronger (not coercive to int) because I recently found a bug where we passed the target as the level parameter. :(

> ::: dom/canvas/WebGLFramebuffer.cpp
> @@ +88,2 @@
> >  
> > +        // TODO - Override WebGLTexture::Target() to return the proper type!
> 
> Yeah. "Finish your derivations, please"

Done

> @@ +397,5 @@
> >          MOZ_ASSERT(gl == Texture()->Context()->gl);
> >  
> >          if (attachmentLoc == LOCAL_GL_DEPTH_STENCIL_ATTACHMENT) {
> >              gl->fFramebufferTexture2D(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_DEPTH_ATTACHMENT,
> > +                                      GetTexImageTarget().Get(), Texture()->GLName(), GetTexImageLevel());
> 
> Should we do manual common sub-expression hoisting here?
> 

Maybe, might be clearer

> const GLenum texImageTarget = GetTexImageTarget().Get();
> if (attachmentLoc == LOCAL_GL_DEPTH_STENCIL_ATTACHMENT) {
>     gl->fFrameBufferTexture2D(LOCAL_GL_FRAMEBUFFER, ..., texImageTarget,
> ...);
>     gl->fFrameBufferTexture2D(LOCAL_GL_FRAMEBUFFER, ..., texImageTarget,
> ...);
> } else {
>     gl->fFrameBufferTexture2D(LOCAL_GL_FRAMEBUFFER, ..., texImageTarget,
> ...);
> }
> 
> @@ +615,2 @@
> >      if (mDepthStencilAttachment.Texture() == tex)
> > +        FramebufferTexture2D(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_DEPTH_STENCIL_ATTACHMENT, TexImageTarget(LOCAL_GL_TEXTURE_2D), nullptr, 0);
> 
> See this sets off my tingly spider-senses: LOCAL_GL_TEXTURE_2D goes from
> constant to expensive-ish constructor, which we rely on compiler to extract.
> I think we should either:
> a) Extract the constructions of TexImageTarget(LOCAL_GL_TEXTURE_2D) into a
> const local, or
> b) Provide static TexImageTarget accessor for values.

I'm pretty confident in the compiler's ability to erase the constructor calls. I did some (minor) testing and it was good about using the constant directly and not calling the constructor.

> ::: dom/canvas/WebGLStrongTypes.h
> @@ +125,5 @@
> > +        \
> > +        explicit name(GLenum val) \
> > +            : mValue(val) \
> > +        { \
> > +            const GLenum validValues[] = { \
> 
> Does this mean that a GLenum array will be created on the stack for every
> GLenum -> StrongType construction? This should be const static some how.
> 
> Given the cut'n'paste nature of the comparison operators and that they can
> conceivably just check enum values, because the constructor acts as a
> "validity" gate, this macro could become a template class + base class
> idiom, where the template is used to customize the array of valid values. I
> think this would address :bjacob's comment.

This array will only exist at debug time. The goal of this class is to perform validation on internal code to make sure we don't mess anything up. During runtime we're relying on the compiler to basically remove these classes from the code, or at least all meaningful uses of it. Although this can still probably be a static array. The macro is also about 2x as large as it will be now with the change to having the GLenum constructor be implicit. So the macro really will be hardly anything other than a class with a single integer.

> ::: dom/canvas/WebGLTexture.cpp
> @@ +110,5 @@
> >      bool firstTimeThisTextureIsBound = !HasEverBeenBound();
> >  
> >      if (firstTimeThisTextureIsBound) {
> > +        BindTo(aTexTarget.Get());
> > +    } else if (aTexTarget != TexTarget(Target())) {
> 
> BindTo can be made into a template function in WebGLBindableName.

Done

> @@ +209,5 @@
> >  bool
> >  WebGLTexture::IsMipmapTexture2DComplete() const {
> >      if (mTarget != LOCAL_GL_TEXTURE_2D)
> >          return false;
> > +    if (!ImageInfoAt(TexImageTarget(LOCAL_GL_TEXTURE_2D), 0).IsPositive())
> 
> This is where I'm concerned about the constructor building an array on the
> stack for this local check. Jeff already reported our WebGL functions are
> slow on ARM. Perhaps we just accept this and then look at it again when
> profiling the ARM perf. issues?

I think looking at the disassembly is really the only way to be sure. I'll make a release build later today and see what kind of damage this code generates.
(Assignee)

Comment 8

4 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #3)
> Comment on attachment 8484383 [details] [diff] [review]
> strong_types_1
> 
> Review of attachment 8484383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, but having to explicitly construct for #defined enums is unnecessary.
> Let's drop that requirement.
> 
> ::: dom/canvas/WebGLContext.h
> @@ +554,5 @@
> > +            return ErrorInvalidValue("texSubImage2D: level is negative");
> > +
> > +        const int32_t maxLevel = MaxTextureLevelForTexImageTarget(texImageTarget);
> > +        if (level > maxLevel)
> > +            return ErrorInvalidValue("texSubImage2D: level %d is too large, max is %d", level, maxLevel);
> 
> It looks like this is validation that wasn't here previously. Let's handle
> this in a different bug, so we can backport this.

Done

> @@ +571,5 @@
> >              return;
> >  
> >          gfx::IntSize size = data->GetSize();
> >          uint32_t byteLength = data->Stride() * size.height;
> > +        return TexSubImage2D_base(texImageTarget.Get(), level, xoffset, yoffset,
> 
> Please push the strong types down into this function.

Alright

> @@ +1199,5 @@
> >      template<class ObjectType>
> >      bool ValidateObjectAssumeNonNull(const char* info, ObjectType *aObject);
> >  
> >  protected:
> > +    int32_t MaxTextureSizeForTarget(TexTarget target) const {
> 
> The previous function, based on its assert, actually takes a TexImageTarget.

Well, it checked for those, but it really wanted to know if it was a TexTarget if you look at the ternary statement.

> ::: dom/canvas/WebGLContextUtils.cpp
> @@ +63,2 @@
> >      case LOCAL_GL_TEXTURE_2D:
> > +        return TexTarget(LOCAL_GL_TEXTURE_2D);
> 
> You shouldn't need to explicitly construct this.

Done + the other comments like this

> ::: dom/canvas/WebGLFramebuffer.cpp
> @@ +88,2 @@
> >  
> > +        // TODO - Override WebGLTexture::Target() to return the proper type!
> 
> Why not now! :D

Fixed by Dan's suggestion of a template class.

> ::: dom/canvas/WebGLFramebuffer.h
> @@ +72,5 @@
> >          }
> >          WebGLRenderbuffer* Renderbuffer() {
> >              return mRenderbufferPtr;
> >          }
> > +        TexImageTarget GetTexImageTarget() const {
> 
> The others getters here aren't prefixed with Get, so just leave it as it was.

Naming conflict :\ so new names are now ImageTarget() and MipLevel().

> ::: dom/canvas/WebGLStrongTypes.h
> @@ +87,5 @@
> > +}
> > +
> > +#endif
> > +
> > +#define MAKE_TYPE_BEGIN(name) \
> 
> s/name/NAME/ to make it obvious what we're replacing.
> 
> Also, STRONG_ENUM_TYPE{_BEGIN,_END} is probably a better name for these.
> 
> Further, it would be ideal to keep the `\` all in the same column. Just put
> them out in the column one past the end of the longest line.

Done

> @@ +122,5 @@
> > +        bool operator!=(const name& other) const { \
> > +            return mValue != other.mValue; \
> > +        } \
> > +        \
> > +        explicit name(GLenum val) \
> 
> Leave this as non-explicit, so we don't need to explicitly construct these
> for each GLenum we test against.
> As long as we don't have an `operator int`, we're protected against creating
> Foos from Bars.

It is nicer, and "hiding" these constructions should be okay, as the compiler should remove them. I should say that it will make for a fun time debugging if you do forget about these implicit conversions :) (although it'd become obvious pretty quick I suppose)

Although I believe it will push more 'checks' to runtime instead of compile time. When it's explicit it's a compile error and you have to manually create the conversion, at which point you'd hopefully implement any needed validation. With implicit constructors things get thrown around until someone complains.

> @@ +118,5 @@
> >          return;
> >      }
> >  
> >      GLuint name = GLName();
> > +    TexTarget target = TexTarget(Target());
> 
> `target` should be provably `aTexTarget` at this point.
> 
> @@ +123,2 @@
> >  
> > +    mContext->gl->fBindTexture(target.Get(), name);
> 
> Great! Ideally, glFunc calls should be the only place we use `.Get()`.

Done
(Assignee)

Comment 9

4 years ago
Created attachment 8485160 [details] [diff] [review]
strong_types_1

(In reply to Benoit Jacob [:bjacob] from comment #4)
> Just a preliminary comment at this point: huge macros should only be used as
> a last resort. Please elaborate on why this huge macro can't be replaced by
> usage of appropriate c++ constructs. Sometimes one needs a macro, but can
> minimize its size by offloading its body to a non-macro c++ helper.

The macro is now a lot smaller and, to be honest, its only purpose is to eliminate a lot of boilerplate code. We're using it as an extremely thin wrapper around the GLenums so we can separate them into distinct 'classes' (as in equivalence classes) so we don't misuse them where they shouldn't be in the code. It seems like other solutions would reduce the clarity by having too much noise. The second hope (to be verified) is that these classes are more or less eliminated in the release version of the code. I don't believe this is a final solution, only an intermediary one to make the code easier to refactor towards a stronger solution.
Attachment #8484383 - Attachment is obsolete: true
Attachment #8484383 - Flags: feedback?(bjacob)
Attachment #8485160 - Flags: review?(jgilbert)
Attachment #8485160 - Flags: review?(dglastonbury)
Attachment #8485160 - Flags: feedback?(bjacob)
(Assignee)

Comment 10

4 years ago
As a follow up for the release mode code. It appears from my sampling of a couple functions and grepping the symbols that the code for these validation classes does not make it into the release build (libxul.so). So no constructors or get() calls or anything to worry about.
Created attachment 8485261 [details]
proof of concept

FWIW, MSVC optimizes out scopes like:
{
  T arr[] = {1,2,3};
  (void)arr;
}

In local testing on MSVC2013, there are no instructions emitted for the ctors when optimized.
Attachment #8485261 - Attachment is patch: false
(In reply to Walter Litwinczyk [:walter] from comment #10)
> As a follow up for the release mode code. It appears from my sampling of a
> couple functions and grepping the symbols that the code for these validation
> classes does not make it into the release build (libxul.so). So no
> constructors or get() calls or anything to worry about.

\o/
Comment on attachment 8485160 [details] [diff] [review]
strong_types_1

Review of attachment 8485160 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLBindableName.h
@@ +52,5 @@
>  
>      //! Called after mTarget has been changed by BindTo(target).
>      virtual void OnTargetChanged() {}
>  
> +    bool mHasBeenBound;

Shouldn't really put small types like this before bigger ones.

Actually, I'd prefer if there was a way to still use the fact that mGLName is not LOCAL_GL_NONE instead of storing state. (ie. Can we cast T back to a GLenum?)
Attachment #8485160 - Flags: review?(dglastonbury) → review-
(Assignee)

Comment 14

4 years ago
(In reply to Dan Glastonbury :djg :kamidphish from comment #13)
> Comment on attachment 8485160 [details] [diff] [review]
> strong_types_1
> 
> Review of attachment 8485160 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLBindableName.h
> @@ +52,5 @@
> >  
> >      //! Called after mTarget has been changed by BindTo(target).
> >      virtual void OnTargetChanged() {}
> >  
> > +    bool mHasBeenBound;
> 
> Shouldn't really put small types like this before bigger ones.
> 
> Actually, I'd prefer if there was a way to still use the fact that mGLName
> is not LOCAL_GL_NONE instead of storing state. (ie. Can we cast T back to a
> GLenum?)

Well, if we use T = TexTarget/TexImageTarget or any of the 'validation' classes then they will never be GL_NONE (as the constructor would assert in debug mode). Jeff and I talked a bit about adding LOCAL_GL_NONE as an acceptable value, but then determined it would undermine what the validation classes were trying to do. Plus, if more enums get moved over then we wouldn't need a special case for GLenums as BindableName wouldn't be instantiated with a GL_NONE-able type.

Moving member variable: Done
Comment on attachment 8485160 [details] [diff] [review]
strong_types_1

Review of attachment 8485160 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLStrongTypes.h
@@ +9,5 @@
> +// ===========
> +//
> +// To create a new type from a set of GLenums do the following:
> +//
> +//   MAKE_TYPE_BEGIN(TypeName)

Update the docs to reflect the current name of the macro.

@@ +30,5 @@
> +//   if (myNewType == LOCAL_GL_SOME_ENUM)
> +//       ...
> +//
> +// The operators will perform a check (in debug mode) that LOCAL_GL_SOME_ENUM is
> +// a value that myNewType can have.

"The operators will perform a check (in debug mode)" --> "The operators will assert" 

(more precise and concise).

@@ +73,5 @@
> +//
> +#ifdef DEBUG
> +
> +static bool
> +IsValueInArr(GLenum value, const GLenum* arr, size_t count)

Rather than having this function taking a low-level (pointer, size) pair, just make it template<size_t N> taking a reference to an array of length N.

template<size_t N>
static bool
IsValueInArr(GLenum value, const (GLenum&)[N])

@@ +85,5 @@
> +}
> +
> +#endif
> +
> +#define STRONG_ENUM_TYPE_BEGIN(NAME)               \

Since this is explicitly about GLenum's, how about s/ENUM/GLENUM/ here ? Will also make this macro less likely to collide with unrelated stuff.

And maybe drop the _TYPE_ part?

So:

STRONG_GLENUM_BEGIN ?

@@ +86,5 @@
> +
> +#endif
> +
> +#define STRONG_ENUM_TYPE_BEGIN(NAME)               \
> +    class NAME {                                   \

It would be extra nice if this class were a "literal type" in the C++11 sense
See http://www.cplusplus.com/reference/type_traits/is_literal_type/
You can use MOZ_CONSTEXPR which will expand to constexpr if available.

@@ +106,5 @@
> +                                                   \
> +        NAME(GLenum val)                           \
> +            : mValue(val)                          \
> +        {                                          \
> +            const GLenum validValues[] = {         \

It would be extra nice if we didn't have this array at all in non-DEBUG builds. Is there a way to achieve that? Is DebugOnly<T> from MFBT useful here?
Attachment #8485160 - Flags: feedback?(bjacob) → feedback+
Comment on attachment 8485160 [details] [diff] [review]
strong_types_1

Review of attachment 8485160 [details] [diff] [review]:
-----------------------------------------------------------------

Almost. Let's get it fixed up. I'll be better about review turnaround next time.

::: dom/canvas/WebGLBindableName.h
@@ +23,2 @@
>  
> +    WebGLBindableName(T initialValue)

Let's remove this arg, and make it unconditionally init mTarget to GL_NONE.

@@ +23,4 @@
>  
> +    WebGLBindableName(T initialValue)
> +        : mHasBeenBound(false)
> +        , mGLName(LOCAL_GL_NONE)

0 for names, not NONE

@@ +26,5 @@
> +        , mGLName(LOCAL_GL_NONE)
> +        , mTarget(initialValue)
> +    { }
> +
> +    void BindTo(T target)

At first, I though this would be a bunch of code bloat, but it looks like it shouldn't very be bad.

@@ +52,5 @@
>  
>      //! Called after mTarget has been changed by BindTo(target).
>      virtual void OnTargetChanged() {}
>  
> +    bool mHasBeenBound;

I talked with Dan about this. I think it really is easiest if our strong enums allow GL_NONE, which allows us to more easily encapsulate doesn't-have-a-target as target=GL_NONE.

We should try to limit GL_NONE to this class, by having:
bool HasEverBeenBound() const { return mTarget != LOCAL_GL_NONE; }
T Target() const {
  MOZ_ASSERT(HasEverBeenBound());
  return mTarget;
}

::: dom/canvas/WebGLStrongTypes.h
@@ +106,5 @@
> +                                                   \
> +        NAME(GLenum val)                           \
> +            : mValue(val)                          \
> +        {                                          \
> +            const GLenum validValues[] = {         \

Trailing `\` here.
Attachment #8485160 - Flags: review?(jgilbert)
(Assignee)

Comment 17

4 years ago
Created attachment 8488861 [details] [diff] [review]
strong_types_1

Alright, I think I addressed all the concerns.
Attachment #8485160 - Attachment is obsolete: true
Attachment #8488861 - Flags: review?(jgilbert)
Attachment #8488861 - Flags: review?(dglastonbury)
Comment on attachment 8488861 [details] [diff] [review]
strong_types_1

Review of attachment 8488861 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLContextGL.cpp
@@ +245,5 @@
> +        if (newTex->HasEverBeenBound() && newTex->Target() != rawTarget)
> +            return ErrorInvalidOperation("bindTexture: this texture has already been bound to a different target");
> +    }
> +
> +    const TexTarget target = TexTarget(rawTarget);

Here, since TexTarget has a non-explicit constructor taking an integer, this line can be simplified to:

const TexTarget target = rawTarget;

I assume that it is fully intentional that this constructor is non-explicit; otherwise, add an explicit keyword on it!
Alternatively, this syntax would not even rely on having non-explicit conversion constructors:

const TexTarget target(rawTarget);

Up to you!
Comment on attachment 8488861 [details] [diff] [review]
strong_types_1

Review of attachment 8488861 [details] [diff] [review]:
-----------------------------------------------------------------

Close enough, for a patch like this.

::: dom/canvas/WebGLContext.h
@@ +476,5 @@
>      {
>          if (IsContextLost())
>              return;
>  
> +        if (!ValidateTexImageTarget(2, rawTexImgTarget, WebGLTexImageFunc::TexImage))

...2?
Oh, `dims`. `auto dims = 2` might help here.

::: dom/canvas/WebGLContextGL.cpp
@@ +890,5 @@
>          return ErrorInvalidOperation("generateMipmap: No texture is bound to this target.");
>  
> +    const TexImageTarget imageTarget = (target == LOCAL_GL_TEXTURE_2D)
> +                                                  ? TexImageTarget(LOCAL_GL_TEXTURE_2D)
> +                                                  : TexImageTarget(LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_X);

The explicit constructors here shouldn't be necessary.

::: dom/canvas/WebGLContextUtils.cpp
@@ +69,5 @@
>      case LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_Z:
>      case LOCAL_GL_TEXTURE_CUBE_MAP_NEGATIVE_Z:
>          return LOCAL_GL_TEXTURE_CUBE_MAP;
>      default:
> +        // Should be caught by the constructor for TexTarget

Always assert these things.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +41,5 @@
>  }
>  
>  WebGLFramebuffer::Attachment::Attachment(GLenum aAttachmentPoint)
>      : mAttachmentPoint(aAttachmentPoint)
> +    , mTexImageTarget(LOCAL_GL_TEXTURE_2D)

We would want GL_NONE, surely?

::: dom/canvas/WebGLTexture.cpp
@@ +275,5 @@
>                  mContext->GenerateWarning
>                      ("%s is a 2D texture, with a minification filter requiring a mipmap, "
>                        "and is not mipmap complete (as defined in section 3.7.10).", msg_rendering_as_black);
>                  mFakeBlackStatus = WebGLTextureFakeBlackStatus::IncompleteTexture;
> +            } else if (!ImageInfoAt(LOCAL_GL_TEXTURE_2D, 0).IsPowerOfTwo()) {

It looks like you want ImageInfoBase().

@@ +284,5 @@
>              }
>          }
>          else // no mipmap required
>          {
> +            if (!ImageInfoAt(LOCAL_GL_TEXTURE_2D, 0).IsPositive()) {

ImageInfoBase()

@@ +289,5 @@
>                  mContext->GenerateWarning
>                      ("%s is a 2D texture and its width or height is equal to zero.",
>                        msg_rendering_as_black);
>                  mFakeBlackStatus = WebGLTextureFakeBlackStatus::IncompleteTexture;
> +            } else if (!AreBothWrapModesClampToEdge() && !ImageInfoAt(LOCAL_GL_TEXTURE_2D, 0).IsPowerOfTwo()) {

ImageInfoBase()

::: dom/canvas/WebGLTexture.h
@@ +130,5 @@
>      };
>  
>  private:
> +    static size_t FaceForTarget(TexImageTarget texImageTarget) {
> +        return texImageTarget == LOCAL_GL_TEXTURE_2D ? 0 : texImageTarget.get() - LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_X;

Keep lines under 80chars.

Consider:
if (texImageTarget == TEXTURE_2D)
  return 0;

return texImageTarget.get() - LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_X;
Attachment #8488861 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 21

4 years ago
Well now it's angry about a JS array not being rooted, but I'm not sure why it would think this, there were no rooting calls before, and not a lot changed.

Try: https://tbpl.mozilla.org/?tree=Try&rev=96cc1a9d34b2

Hazard: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlitwinczyk@mozilla.com-96cc1a9d34b2/try-linux64-br-haz/hazards.txt.gz
Attachment #8488861 - Flags: review?(dglastonbury) → review+
(In reply to Walter Litwinczyk [:walter] from comment #21)
> Well now it's angry about a JS array not being rooted, but I'm not sure why
> it would think this, there were no rooting calls before, and not a lot
> changed.
> 
> Try: https://tbpl.mozilla.org/?tree=Try&rev=96cc1a9d34b2
> 
> Hazard:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/
> wlitwinczyk@mozilla.com-96cc1a9d34b2/try-linux64-br-haz/hazards.txt.gz

Ugh. Ask :waldo if he has any ideas.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 23

4 years ago
I believe I figured it out. We do have an unrooted array being used, the Uint8ClampedArray. I think the fix is easy, so I'll run another try test.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 24

4 years ago
Okay it appears to be fixed: https://tbpl.mozilla.org/?tree=Try&rev=a8fc9563ec50

I'll do another try run to make sure everything is all good.
(Assignee)

Comment 25

4 years ago
Created attachment 8491891 [details] [diff] [review]
strong_types_1

r=jgilbert,kamidphish carried forward

This is the updated patch (with requested changes) going through try at the moment
Attachment #8488861 - Attachment is obsolete: true
Attachment #8491891 - Flags: review+
(Assignee)

Comment 27

4 years ago
Try passes!
Keywords: checkin-needed
(Assignee)

Comment 30

4 years ago
Ah, it looks like WebGLSampler.cpp wasn't updated to have WebGLBindableName<GLenum>(), same with TransformFeedback
Sorry about that, and not sure how it compiled locally for me. This should be good now:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7283eb583341
https://hg.mozilla.org/mozilla-central/rev/7283eb583341
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.