1071233 - WebGL - Use strong types for framebuffers and renderbuffers

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Walter Litwinczyk [:walter]]

Since the strong GL enums patch has landed: Bug 1063053 it's worth going through and adding the functionality where it can be added. This bug will add strong GL enum support for:

Framebuffer targets
Framebuffer attachments
Framebuffer status

Renderbuffer targets
Renderbuffer parameters

Comment 1

[Walter Litwinczyk [:walter]]

Attachment: fb_rb_strong_types_patch

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8493371 [details] [diff] [review]
fb_rb_strong_types_patch

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

::: dom/canvas/WebGLContextValidate.cpp
@@ +537,5 @@
> +        // Check if the user was trying to use multiple color
> +        // buffers and provide a helpful error message.
> +        if (attachment > LOCAL_GL_COLOR_ATTACHMENT0 &&
> +            attachment <= LOCAL_GL_COLOR_ATTACHMENT15 &&
> +            !IsExtensionEnabled(WebGLExtensionID::WEBGL_draw_buffers))

It seems like this whole function could be simpler if we use early-returns.

if it's a depth and/or stencil buffer:
  return true

GLenum colorAttachCount = 1
if draw buffers enabled:
  colorAttachCount = mGLMaxColorAttachments

if attachment within [color0, color0+colorAttachCount):
  return true

invalid enum!
return false

::: dom/canvas/WebGLFramebuffer.cpp
@@ +335,5 @@
>          }
>  
>          if (mAttachmentPoint >= LOCAL_GL_COLOR_ATTACHMENT0 &&
> +            mAttachmentPoint <= FBAttachment(LOCAL_GL_COLOR_ATTACHMENT0 - 1 +
> +                                             WebGLContext::kMaxColorAttachments))

I think the old way was better.
`< max` is more common than `<= last`.

@@ +765,5 @@
>  {
>      if (mStatus != 0)
>          return mStatus;
>  
> +    mStatus = PrecheckFramebufferStatus().get();

Shouldn't mStatus be a strong type?

::: dom/canvas/WebGLFramebufferAttachable.h
@@ +12,3 @@
>  #include "GLDefs.h"
>  #include "nsTArray.h"
>  #include "mozilla/WeakPtr.h"

Try to keep the includes alphabetical.
Here, it would be:
GLDefs.h
mozilla/WeakPtr.h
nsTArray.h
WebGLFramebuffer.h
WebGLStrongTypes.h

Comment 3

[Walter Litwinczyk [:walter]]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 8493371 [details] [diff] [review]
> fb_rb_strong_types_patch
> 
> Review of attachment 8493371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLContextValidate.cpp
> @@ +537,5 @@
> > +        // Check if the user was trying to use multiple color
> > +        // buffers and provide a helpful error message.
> > +        if (attachment > LOCAL_GL_COLOR_ATTACHMENT0 &&
> > +            attachment <= LOCAL_GL_COLOR_ATTACHMENT15 &&
> > +            !IsExtensionEnabled(WebGLExtensionID::WEBGL_draw_buffers))
> 
> It seems like this whole function could be simpler if we use early-returns.
> 

Done

> ::: dom/canvas/WebGLFramebuffer.cpp
> @@ +335,5 @@
> >          }
> >  
> >          if (mAttachmentPoint >= LOCAL_GL_COLOR_ATTACHMENT0 &&
> > +            mAttachmentPoint <= FBAttachment(LOCAL_GL_COLOR_ATTACHMENT0 - 1 +
> > +                                             WebGLContext::kMaxColorAttachments))
> 
> I think the old way was better.
> `< max` is more common than `<= last`.
> 

Well, I would have left it, but because of the validation, it will assert if you do plain LOCAL_GL_COLOR_ATTACHMENT0 + kMax... The alternative is to add a LOCAL_GL_COLOR_ATTACHMENT16 to FBAttachment and then I could use the old way. I wasn't sure which would be better. As adding the extra one might let off-by-one errors slip by.

> @@ +765,5 @@
> >  {
> >      if (mStatus != 0)
> >          return mStatus;
> >  
> > +    mStatus = PrecheckFramebufferStatus().get();
> 
> Shouldn't mStatus be a strong type?
> 

I had done that originally, and ran into the problem with the sentinel value of 0 being used. I really didn't want to add LOCAL_GL_NONE to the list, but I suppose I could. Or maybe instead of using LOCAL_GL_NONE we can add another non-zero constant? Something less likely to happen? A STRONG_GLENUM_INVALID or something.

> ::: dom/canvas/WebGLFramebufferAttachable.h
> @@ +12,3 @@
> >  #include "GLDefs.h"
> >  #include "nsTArray.h"
> >  #include "mozilla/WeakPtr.h"
> 
> Try to keep the includes alphabetical.
> Here, it would be:
> GLDefs.h
> mozilla/WeakPtr.h
> nsTArray.h
> WebGLFramebuffer.h
> WebGLStrongTypes.h

Done

Comment 4

[Walter Litwinczyk [:walter]]

Attachment: fb_rb_strong_types_patch

Rebased onto bjacob's patches, which are already in inbound and made most of the requested changes (minus the two comments above).

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8494090 [details] [diff] [review]
fb_rb_strong_types_patch

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

::: dom/canvas/WebGLContextValidate.cpp
@@ +544,5 @@
> +        return true;
> +    }
> +
> +    ErrorInvalidEnum("%s: attachment: invalid enum value 0x%x.", funcName, attachment);
> +    return true;

false, surely?

Comment 6

[Walter Litwinczyk [:walter]]

Whoops! You are correct. At least the StrongGL types caught that in try!

Comment 7

[Walter Litwinczyk [:walter]]

Attachment: fb_rb_strong_types_patch

Comment 8

[Walter Litwinczyk [:walter]]

Attachment: fb_rb_strong_types_patch

r=jgilbert carried forward. Minor try fixes due to Bug 1071339 now requiring the enums to be in ascending order.

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b2bd6111568

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6b2bd6111568