Untangle the mixup of formats vs internalformats in the WebGL implementation

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Other Branch
mozilla35
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

So in bug 1071340 we added STRONG_GLENUM's for texture format/type/internalformat.

The good thing about those --- that they actually assert on bad values --- unfortunately also prevents us from using them in the place where we need them the most: in entry points and helper functions implementing parameter validation, which is currently a mess of confusion between format and internalformat.

So this patch (see the WebGLStrongTypes.h part first) introduces, for each StrongGLenum, a second type prefixed with Unvalidated*. For instance, to TexFormat now corresponds UnvalidatedTexFormat. The rest of this patch then uses these new Unvalidated types.

The great thing (thanks to our new DOM bindings, too) is that we can replace GLenum by Unvalidated* type everywhere, even directly in the prototype of our entry points!

While they don't runtime-assert on bad values, Unvalidated* types still bring us type safety. For example, you can assign a UnvalidatedTexFormat to a TexFormat, but not to a TexInternalFormat.

This caught a thousand mismatches in our current validation code, so the patch ended up being bigger and more nontrivial than I wished. This is the mininum I had to do to get something that builds and pass relevant 1.0.3 conformance tests as well as trunk does on my machine.
Attachment #8494872 - Flags: review?(jgilbert)
Attachment #8494872 - Flags: review?(dglastonbury)
Note: the point of Unvalidated* types is not to replace validated types, but to replace raw GLenums in the parts of the code that run ahead of validation or inside validation code. The idea remains that the more code we can have use proper validated types, the better.
Comment on attachment 8494872 [details] [diff] [review]
unvalidated-enums

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

I don't think we should actually go this way. I think we should just use something like:
class GLenumT : public StrongInt<GLenum>
{}

I don't think it's a good match to put unvalidated stuff in with our validated stuff, especially when we will be getting something like this for other types like GLsizei.
Attachment #8494872 - Flags: review?(jgilbert) → review-
Comment on attachment 8494872 [details] [diff] [review]
unvalidated-enums

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

No no no no. Just stick with GLenum.
Attachment #8494872 - Flags: review?(dglastonbury) → review-
Could you guys elaborate more? I have put a lot of thinking into this patch would like to read more than a one-line 'no' if for some reason it's not good.

As evidenced in this patch, our current code is full of format/internalformat fields that are accidentally used instead of the other and/or abused in various ways. This prevents me from reasoning about how to cleanly proceed with implementing the WebGL2 textures state machine. I know I'm not the only one --- you guys have had a lot of trouble last year with format/internalformat confusion, too. I was trying to make that better. I could probably proceed by adding more hacks to the existing pile of hacks (for which I bear a good part of the responsibility!) and get just what I need to work, at the cost of incurring even more technical debt to work. But I was trying to figure a way to first resolve the issues that make this code so hard to reason about, and they make a return on investment by having WebGL2 textures faster to implement.

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 8494872 [details] [diff] [review]
> unvalidated-enums
> 
> Review of attachment 8494872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think we should actually go this way. I think we should just use
> something like:
> class GLenumT : public StrongInt<GLenum>

If I understand correctly your above example, you mean that you want a single type for all GLenums. The only strong-typing that we would get, then, I suppose, would be to differentiate GLenum from other GL types and plain integers?

That is explicitly not the path we've taken since bug 1063053, which added strong typing making GLenums different from each other, e.g. TexTarget vs. TexImageTarget.

My patch here was just building on this existing approach, that I thought we already agreed on. 

However, as explained in comment 0, a basic issue was preventing us from using this approach we had agreed on, in most places where we needed it. The issue is that STRONG_GLENUM's were doing two different things at the same time: 1) introduce different types for different sets of GLenum's, and 2) runtime-assert againt GLenum values incompatible with a given type. I found that while 1) was always desirable, 2) was only possible to use _after_ bad inputs had been validated away, and I had the biggest need for 1) before that point. Whence the idea of introducing an Unvalidated variant to be able to enjoy property 1) alone where appropriate i.e. before and during validation.

> {}
> 
> I don't think it's a good match to put unvalidated stuff in with our
> validated stuff

That sail already shipped in bug 1063053 which introduced some  validated values in the middle of a bunch of totally unvalidated values, i.e. validated TexImageTarget parameters surrounded by plain GLenum's (not validated) in Tex*Image2D_base.

All I'm doing here is taking some of these existing totally unvalidated GLenum values at at least making them typed so, while we can still have bad runtime values in these variables, at least we can't accidentally assign a format into an internalformat, etc, as we were accidentally doing a lot, which made it so hard to reason about how to do WebGL2 textures.

>, especially when we will be getting something like this for
> other types like GLsizei.

I hadn't considered at all that we would want the same for other GL types than GLenum --- that's news to me. Sounds like a good idea, but I don't understand the need to discuss it here.
Flags: needinfo?(jgilbert)
Flags: needinfo?(dglastonbury)
Replied via email.
Flags: needinfo?(dglastonbury)
Renamed the bug to better reflect what this is about and why it's necessary for WebGL2.

The introduction of strong typing for these GLenums is a detail of how I managed to untangle this. I thought I'd share the tool that allowed me to do this, but if you don't like the tool, after all, all I really need to land for my short-term project is the resulting untangling of formats vs internalformats.

Would you prefer, then, a patch only doing the resulting untangling but reverting to plain GLenums?
Flags: needinfo?(dglastonbury)
Summary: Add Unvalidated strong GL enum types for texture formats, and use them in validation code → Untangle the mixup of formats vs internalformats in the WebGL implementation
(In reply to Benoit Jacob [:bjacob] from comment #6)
> Would you prefer, then, a patch only doing the resulting untangling but
> reverting to plain GLenums?

I would. Can't speak for :jgilbert.
Flags: needinfo?(dglastonbury)
This should be a complete untangling, since validated by strong typing (even as the strong typing is removed from this form of the patch, it already did its thing).

One thing that this patch doesn't fix is variable/parameter names: we still have some internalformats named 'format'. I don't even know if we care a lot about getting all the variable names right in this respect, as this is very brittle --- the compiler can't force us to name internalformats 'internalformat' and it's often more natural to just write 'format'.

But at least, there should be no instance left of formats and internalformat getting mixed up and passed for one another or assigned into each other.
Attachment #8497226 - Flags: review?(jgilbert)
Attachment #8497226 - Flags: review?(dglastonbury)
Assignee: nobody → dglastonbury
Status: NEW → ASSIGNED
Comment on attachment 8497226 [details] [diff] [review]
Untangle format vs internalformat

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

I've uploaded patches to add internal format tracking in Bug 1075305.

::: dom/canvas/WebGLContext.h
@@ +570,5 @@
> +        if (!tex) {
> +            return ErrorInvalidOperation("texSubImage2D: no texture bound on active texture unit");
> +        }
> +        const WebGLTexture::ImageInfo &imageInfo = tex->ImageInfoAt(texImageTarget, level);
> +        const TexInternalFormat internalformat = imageInfo.WebGLFormat();

Isn't this wrong? Oh ...

I see from http://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLTexture.h#117 that we're tracking internalformat as format. I have a patch that adds internalformat. I'll get that uploaded for review.

::: dom/canvas/WebGLContextUtils.cpp
@@ +493,2 @@
>  bool
> +WebGLContext::IsCompressedTextureFormat(GLenum format)

IMO, This should keep taking TexInternalFormat.

@@ +514,5 @@
>  
> +
> +bool
> +WebGLContext::IsTextureFormatCompressed(TexInternalFormat format)
> +{

... and remove this one.

::: dom/canvas/WebGLContextValidate.cpp
@@ +259,1 @@
>  WebGLContext::BaseTexFormat(GLenum internalFormat) const

Should return TexFormat.

@@ +1160,5 @@
>      return validCombo;
>  }
>  
> +bool
> +WebGLContext::ValidateCompTexImageInternalFormat(GLenum format,

I think that format should be renamed 'internalformat'. (keeping with the way OpenGL writes it, too)
(In reply to Dan Glastonbury :djg :kamidphish from comment #10)
> Comment on attachment 8497226 [details] [diff] [review]
> Untangle format vs internalformat
> 
> Review of attachment 8497226 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've uploaded patches to add internal format tracking in Bug 1075305.

(/me scratches head) WebGLTexture::ImageInfo already tracks internal formats since bug 1071340 landed!

> 
> ::: dom/canvas/WebGLContext.h
> @@ +570,5 @@
> > +        if (!tex) {
> > +            return ErrorInvalidOperation("texSubImage2D: no texture bound on active texture unit");
> > +        }
> > +        const WebGLTexture::ImageInfo &imageInfo = tex->ImageInfoAt(texImageTarget, level);
> > +        const TexInternalFormat internalformat = imageInfo.WebGLFormat();
> 
> Isn't this wrong? Oh ...
> 
> I see from
> http://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLTexture.h#117
> that we're tracking internalformat as format. I have a patch that adds
> internalformat. I'll get that uploaded for review.

The existing webGLFormat field is already a TexInternalFormat, look at its declaration. I agree that the names are confusing. I would r+ a patch renaming it to add 'internal' to its name.

> 
> ::: dom/canvas/WebGLContextUtils.cpp
> @@ +493,2 @@
> >  bool
> > +WebGLContext::IsCompressedTextureFormat(GLenum format)
> 
> IMO, This should keep taking TexInternalFormat.
> 
> @@ +514,5 @@
> >  
> > +
> > +bool
> > +WebGLContext::IsTextureFormatCompressed(TexInternalFormat format)
> > +{
> 
> ... and remove this one.

I needed to introduce a function taking a GLenum becasue I needed to call it in validation code before the value was validated to be a legal compressed internalformat. TexInternalFormat asserts upon assignment that the value is legal. That's why we can't use it in validation code. I know that that's very unfortunate. That's what I addressed in the first patch above, on this bug, adding Unvalidated* variants of these strong glenum types.

> 
> ::: dom/canvas/WebGLContextValidate.cpp
> @@ +259,1 @@
> >  WebGLContext::BaseTexFormat(GLenum internalFormat) const
> 
> Should return TexFormat.

OK - I see that you already have a patch for that on bug 1075305.

> 
> @@ +1160,5 @@
> >      return validCombo;
> >  }
> >  
> > +bool
> > +WebGLContext::ValidateCompTexImageInternalFormat(GLenum format,
> 
> I think that format should be renamed 'internalformat'. (keeping with the
> way OpenGL writes it, too)

I agree, here and in many other places in existing code, we have confusion in variable names between format and internalformat. See comment 8. I could fix specifically this instance but there are plenty others in the code, so I'm not sure if it matters to fix one instance. This is very brittle: we can't have the compiler catch bad variable names for us. All we could have is the compiler catch bad types, which was the approach of the original patch here. It would not be a big deal to have this parameter named 'format' if it were declared with a type mentioning 'Internal'. We can't use TexInternalFormat here for the reason explained above, which was the rationale for UnvalidatedTexInternalFormat.
Comment on attachment 8497951 [details] [diff] [review]
WebGL2 - Fix internalformat use in CopyTexSubImage2D_base(). Remove double param checks.

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

Very good catch!
Attachment #8497951 - Flags: review?(bjacob) → review+
Comment on attachment 8497226 [details] [diff] [review]
Untangle format vs internalformat

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

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

* to left

@@ +569,5 @@
> +        WebGLTexture *tex = activeBoundTextureForTexImageTarget(texImageTarget);
> +        if (!tex) {
> +            return ErrorInvalidOperation("texSubImage2D: no texture bound on active texture unit");
> +        }
> +        const WebGLTexture::ImageInfo &imageInfo = tex->ImageInfoAt(texImageTarget, level);

& to left

::: dom/canvas/WebGLContextGL.cpp
@@ +374,5 @@
>      // patch lands.
>      if (!ValidateTexImage(2, texImageTarget, level, internalformat,
>                            xoffset, yoffset, 0,
>                            width, height, 0,
> +                          0, LOCAL_GL_NONE, LOCAL_GL_NONE,

Woah, what?

::: dom/canvas/WebGLContextValidate.cpp
@@ +1096,4 @@
>  {
> +    if (IsCompressedFunc(func) || IsCopyFunc(func))
> +    {
> +        MOZ_ASSERT(type == LOCAL_GL_NONE && format == LOCAL_GL_NONE);

This is not my favorite piece of code. This is the issue with having one-validation-function-to-rule-them-all.

@@ +1163,5 @@
> +bool
> +WebGLContext::ValidateCompTexImageInternalFormat(GLenum format,
> +                                                 WebGLTexImageFunc func)
> +{
> +    if (!IsCompressedTextureFormat(format))

if (foo) {

@@ +1230,5 @@
> +                 format == LOCAL_GL_LUMINANCE_ALPHA ||
> +                 format == LOCAL_GL_LUMINANCE ||
> +                 format == LOCAL_GL_ALPHA;
> +    if (!valid)
> +    {

if (foo) {

@@ +1370,5 @@
>      /* Check incoming image format and type */
>      if (!ValidateTexImageFormatAndType(format, type, func))
>          return false;
>  
> +    if (IsCompressedFunc(func))

if (foo) {
Attachment #8497226 - Flags: review?(jgilbert) → review+
Blocks: 1077183
https://hg.mozilla.org/mozilla-central/rev/9f6ad16ffec6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Reopen, because review of Benoit's patch is still open.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I thought that Jeff's review was enough! But am still interested in your comments on this patch.
Clearly we need r?(:foo||:bar)!
Flags: needinfo?(jgilbert)
(In reply to Benoit Jacob [:bjacob] from comment #18)
> I thought that Jeff's review was enough! But am still interested in your
> comments on this patch.

Oh my mistake.

Also my patch never landed. Looking at trying to consolidate it, I see bjacob's patch landed, so I'm going to abandon it.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Attachment #8497226 - Flags: review?(dglastonbury) → review-
You need to log in before you can comment on or make changes to this bug.