Closed Bug 1184402 Opened 4 years ago Closed 4 years ago

WebGL 2 - renderbufferStorageMultisample fails for new sized internal formats.

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: kamidphish, Assigned: kamidphish)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(7 files, 3 obsolete files)

2.43 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
2.58 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
2.37 KB, patch
jgilbert
: review-
Details | Diff | Splinter Review
17.47 KB, patch
jgilbert
: review-
Details | Diff | Splinter Review
15.05 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
3.30 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
5.40 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
Many failures in WebGL conformance2 tests relating to renderbuffers are caused by:

`Error: WebGL: renderbufferStorage: `internalFormat`: Invalid enum: 0x8058 (RGBA8).`
Attachment #8635042 - Flags: review?(jgilbert) → review+
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).
Attachment #8635046 - Flags: feedback?(jgilbert) → feedback+
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?
(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.
Attachment #8635065 - Flags: review?(jgilbert) → review+
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.
Attachment #8635044 - Flags: review?(jgilbert) → review-
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.
Attachment #8635844 - Flags: review?(jgilbert) → review-
(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.
(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.
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-)
Attachment #8635845 - Flags: review?(jgilbert) → review-
Attached patch Part 4 review comments. (obsolete) — Splinter Review
Attachment #8637587 - Flags: review?(jgilbert)
Jeff, Is Part 2 still r- after reviewing Part 3?
Status: NEW → ASSIGNED
Attachment #8637587 - Attachment is obsolete: true
Attachment #8637587 - Flags: review?(jgilbert)
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.
Attachment #8637623 - Flags: review?(jgilbert) → review+
Comment on attachment 8640269 [details] [diff] [review]
Address review comments for part 2.

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

Great, thanks.
Attachment #8640269 - Flags: review?(jgilbert) → review+
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;
Attachment #8641428 - Flags: review?(jgilbert) → review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/49948c276210
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.