u480271 Assignee
	Description • 9 years ago

Many failures in WebGL conformance2 tests relating to renderbuffers are caused by:

`Error: WebGL: renderbufferStorage: `internalFormat`: Invalid enum: 0x8058 (RGBA8).`

	Kelsey Gilbert [:jgilbert]
	Comment 4 • 9 years ago

Comment on attachment 8635046 [details] [diff] [review]
Part 3: Add Texture formats via extension enabling. f=jgilbert

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

This is the right idea, though not correct for this case.

::: dom/canvas/WebGLExtensionTextureFloat.cpp
@@ +10,4 @@
>  
>  namespace mozilla {
>  
> +using namespace mozilla::webgl;

Try `using mozilla::webgl::EffectiveFormat;` explicitly?

@@ +16,5 @@
>      : WebGLExtensionBase(webgl)
>  {
> +    webgl::FormatUsageAuthority* authority = webgl->mFormatUsage.get();
> +
> +    // This is a mess. This table folds in WEBGL_color_buffer_float

Right, but this is only texture_float, so RGBA32F should not be renderable (nor RB-able).

	Kelsey Gilbert [:jgilbert]
	Comment 5 • 9 years ago

Comment on attachment 8635044 [details] [diff] [review]
Part 2: Rename LuminanceAlpha, Luminance, Alpha to LA, L, A to match RGBA.

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

::: dom/canvas/WebGLFormats.h
@@ -95,5 @@
>  
>      // GLES 3.0.4, p128, table 3.12.
> -    Luminance8Alpha8,
> -    Luminance8,
> -    Alpha8,

These are named differently because they don't have equivalent sizedFormats. I kept the naming from the spec, but we can discuss going our own path, I suppose. (I do enjoy strictly following the spec here)

@@ +133,5 @@
>  
> +    // OES_texture_float
> +    LA32F,
> +    L32F,
> +    A32F,

I don't see you including the format tables for these formats. Is this a TODO?

	u480271 Assignee
	Comment 6 • 9 years ago

(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Comment on attachment 8635044 [details] [diff] [review]
> Part 2: Rename LuminanceAlpha, Luminance, Alpha to LA, L, A to match RGBA.
> 
> Review of attachment 8635044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLFormats.h
> @@ -95,5 @@
> >  
> >      // GLES 3.0.4, p128, table 3.12.
> > -    Luminance8Alpha8,
> > -    Luminance8,
> > -    Alpha8,
> 
> These are named differently because they don't have equivalent sizedFormats.
> I kept the naming from the spec, but we can discuss going our own path, I
> suppose. (I do enjoy strictly following the spec here)

I'm open to discussion on this.

> @@ +133,5 @@
> >  
> > +    // OES_texture_float
> > +    LA32F,
> > +    L32F,
> > +    A32F,
> 
> I don't see you including the format tables for these formats. Is this a
> TODO?

Those should be added via the extension mechanism in WebGLExtensionTextureFloat.cpp.

	Kelsey Gilbert [:jgilbert]
	Comment 8 • 9 years ago

Comment on attachment 8635044 [details] [diff] [review]
Part 2: Rename LuminanceAlpha, Luminance, Alpha to LA, L, A to match RGBA.

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

I would rather keep these exactly like from the GLES3 spec:
* It makes it more clear than effectiveFormats is a superset of sizedFormats. (Luminance8 is not a sized format)
* Even though this is from GLES3, GL3+ or GL4+ at some point removed LUMINANCE8 and friends from core, so moving forward, Luminance8 (etc.) will never be a valid sizedFormat, or have an enum.
* The name is long, but since it's rarely used, I don't think it's problematic.
* The weird name cues readers into realizing that it's not like the other enum-looking things around it.

Luminance32FAlpha32F is a really a relic already, since it's not in WebGL 2.

	Kelsey Gilbert [:jgilbert]
	Comment 11 • 9 years ago

Comment on attachment 8635844 [details] [diff] [review]
Part 2: Added luminance float effective formats. r=jgilbert

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

::: dom/canvas/WebGLFormats.cpp
@@ +275,5 @@
> +
> +    // OES_texture_half_float
> +    AddFormatInfo(FOO(Luminance16FAlpha16F), 2, UnsizedFormat::LA, ComponentType::Float);
> +    AddFormatInfo(FOO(Luminance16F        ), 1, UnsizedFormat::L , ComponentType::Float);
> +    AddFormatInfo(FOO(Alpha16F            ), 1, UnsizedFormat::A , ComponentType::Float);

You'll also need to add unpack pair info and FormatInfo for these.

	u480271 Assignee
	Comment 12 • 9 years ago

(In reply to Jeff Gilbert [:jgilbert] from comment #11)
> ::: dom/canvas/WebGLFormats.cpp
> @@ +275,5 @@
> > +
> > +    // OES_texture_half_float
> > +    AddFormatInfo(FOO(Luminance16FAlpha16F), 2, UnsizedFormat::LA, ComponentType::Float);
> > +    AddFormatInfo(FOO(Luminance16F        ), 1, UnsizedFormat::L , ComponentType::Float);
> > +    AddFormatInfo(FOO(Alpha16F            ), 1, UnsizedFormat::A , ComponentType::Float);
> 
> You'll also need to add unpack pair info and FormatInfo for these.

WebGL 2 doesn't support these as valid unpack formats. At least not in core. Maybe via an extension.

I add the unpack pair info and FormatInfo for WebGL1 in patch part 4.

	u480271 Assignee
	Comment 13 • 9 years ago

(In reply to Dan Glastonbury :djg :kamidphish from comment #12)
> I add the unpack pair info and FormatInfo for WebGL1 in patch part 4.

Sorry in patch part 3. Add WebGL1 formats when enabling extensions.

	Kelsey Gilbert [:jgilbert]
	Comment 14 • 9 years ago

Comment on attachment 8635845 [details] [diff] [review]
Part 3: Add WebGL1 formats when enabling extensions.

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

::: dom/canvas/WebGL2Context.cpp
@@ -57,5 @@
>      WebGLExtensionID::EXT_blend_minmax,
>      WebGLExtensionID::EXT_sRGB,
>      WebGLExtensionID::OES_element_index_uint,
>      WebGLExtensionID::OES_standard_derivatives,
> -    WebGLExtensionID::OES_texture_float,

Why are these removed, but not the _linear ones?

::: dom/canvas/WebGLExtensionColorBufferFloat.cpp
@@ +25,5 @@
> +
> +    webgl::FormatUsageAuthority* authority = webgl->mFormatUsage.get();
> +
> +    // Ensure require formats are initialized.
> +    WebGLExtensionTextureFloat::InitWebGLFormats(authority);

You can't use this as-is, since right now it inits the formats with asTexture:true. Changes InitWebGLFormats to have asTexture:false, and make TextureFloat.cpp `updateUsage` to mark asTexture:true. (r-)

::: dom/canvas/WebGLExtensionSRGB.cpp
@@ +33,5 @@
>          gl->fEnable(LOCAL_GL_FRAMEBUFFER_SRGB_EXT);
>      }
> +
> +    webgl::FormatUsageAuthority* authority = webgl->mFormatUsage.get();
> +    webgl::AddFormatsIfMissing(*authority, &gSRGBFormatData[0]);

I would rather have `gSRGBFormatData` just passed directly. I don't see what &foo[0] adds here. It's clearly an array, and even if it weren't, &foo[0] is valid C++ anyway.

::: dom/canvas/WebGLExtensionTextureHalfFloat.cpp
@@ +17,5 @@
> +    { EffectiveFormat::RGB16F              , LOCAL_GL_RGB            , LOCAL_GL_HALF_FLOAT_OES, false, false, true, false },
> +    { EffectiveFormat::Luminance16FAlpha16F, LOCAL_GL_LUMINANCE_ALPHA, LOCAL_GL_HALF_FLOAT_OES, false, false, true, false },
> +    { EffectiveFormat::Luminance16F        , LOCAL_GL_LUMINANCE      , LOCAL_GL_HALF_FLOAT_OES, false, false, true, false },
> +    { EffectiveFormat::Alpha16F            , LOCAL_GL_ALPHA          , LOCAL_GL_HALF_FLOAT_OES, false, false, true, false },
> +    { EffectiveFormat::MAX, }

Not my favorite abuse, but ok.

::: dom/canvas/WebGLFormats.h
@@ +264,5 @@
> +
> +struct SimpleFormatUsageInfo {
> +    EffectiveFormat effectiveFormat;
> +    GLenum format;
> +    GLenum type;

`unpackFormat` and `unpackType` please.

@@ +273,5 @@
> +};
> +
> +// Accepts a table of SimpleFormatUsageInfos to add. The table is
> +// terminated by an entry with effectiveFormat == EffectiveFormat::MAX
> +void AddFormatsIfMissing(FormatUsageAuthority& authority, SimpleFormatUsageInfo* infos);

This should be a member on FormatUsageAuthority. (r-)

	u480271 Assignee
	Comment 16 • 9 years ago

Jeff, Is Part 2 still r- after reviewing Part 3?

	Kelsey Gilbert [:jgilbert]
	Comment 18 • 9 years ago

Comment on attachment 8637623 [details] [diff] [review]
Part 4 review comments.

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

::: dom/canvas/WebGLExtensionTextureHalfFloat.cpp
@@ +31,5 @@
>  }
>  
> +#undef ADD_FORMAT_IF_MISSING
> +
> +#define UPDATE_USAGE(fmt, uf, ut)                                       \

Most of these macros don't seem that advantageous compared to using static functions.

	Kelsey Gilbert [:jgilbert]
	Comment 20 • 9 years ago

Comment on attachment 8640269 [details] [diff] [review]
Address review comments for part 2.

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

Great, thanks.

	u480271 Assignee
	Comment 21 • 9 years ago

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=95091e93e8cf

	Pulsebot
	Comment 22 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/631e78da4e3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fe2bb2c7656
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a61f0b040fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd68bfb4041

	Carsten Book [:Tomcat]
	Comment 23 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/631e78da4e3d
https://hg.mozilla.org/mozilla-central/rev/5fe2bb2c7656
https://hg.mozilla.org/mozilla-central/rev/5a61f0b040fc
https://hg.mozilla.org/mozilla-central/rev/1dd68bfb4041

	Kelsey Gilbert [:jgilbert]
	Comment 25 • 9 years ago

Comment on attachment 8641428 [details] [diff] [review]
Reset format usage authority on context lose/restore.

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

This is fine, but I think CreateFormatUsageAuthority would be cleaner.

::: dom/canvas/WebGLContextValidate.cpp
@@ +1686,5 @@
>  {
>      if (!gl)
>          return false;
>  
> +    EnsureFormatUsageAuthority();

Just unconditionally recreate the authority here, instead of using an 'ensure'. 

It seems cleaner and more functional to have:
virtual UniquePtr<webgl::FormatUsageAuthority> CreateFormatUsage() const = 0;

	Pulsebot
	Comment 26 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/49948c276210

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/49948c276210