Closed Bug 1080137 Opened 5 years ago Closed 5 years ago

WebGL2: misc fixes to make new tex formats and sized internalformats actually work


(Core :: Canvas: WebGL, defect)

Other Branch
Not set





(Reporter: bjacob, Assigned: bjacob)




(2 files)

This is the collection of all the misc stuff that I had to fix to make along with making these new conformance tests,

What we have here ranges from the expected (adjusting validation logic to accept things that are legal in WebGL2) to the silly (typos in the big formats table).

There is some code removal: BasTexFormat goes away, it had become redundant, and had only one caller left, and that caller was not useful.

The only nontrivial part here was how to deal with new formats without having to write all the code to support texture format conversions for them, or alpha premultiplication, before we even know what the WG will settle on on this topic. This is resolved by introducing a dummy WebGLTexelFormat (which is an enum used solely around tex conversions, so maybe it should be renamed to reflect that) saying "I'm a format for which no conversion is supported" and generate INVALID_OPERATION when trying to actually convert those.
Attachment #8502030 - Flags: review?(jgilbert)
Comment on attachment 8502030 [details] [diff] [review]

Review of attachment 8502030 [details] [diff] [review]:

::: dom/canvas/WebGLContextGL.cpp
@@ +3729,5 @@
> +    WebGLTexelFormat dstFormat = GetWebGLTexelFormat(effectiveinternalformat);
> +    if (dstFormat == WebGLTexelFormat::FormatNotSupportingAnyConversion) {
> +        if (srcFormat != WebGLTexelFormat::FormatNotSupportingAnyConversion &&
> +            srcFormat != WebGLTexelFormat::Auto)

I'm being silly here. This if condition should be simply: if (srcFormat != WebGLTexelFormat::Auto)
Comment on attachment 8502030 [details] [diff] [review]

Review of attachment 8502030 [details] [diff] [review]:

::: dom/canvas/WebGLContextGL.cpp
@@ +3689,5 @@
> +    // EffectiveInternalFormatFromInternalFormatAndType for that purpose. Really, an unsized source format
> +    // is the same thing as an unsized internalformat.
> +    TexInternalFormat effectivesourceformat =
> +        EffectiveInternalFormatFromInternalFormatAndType(format, type);
> +    MOZ_ASSERT(effectivesourceformat != LOCAL_GL_NONE); // should have validated format/type combo earlier

Camel-case, please.

@@ +3896,5 @@
>      }
> +    const size_t bitspertexel = GetBitsPerTexel(existingEffectiveInternalFormat);
> +    MOZ_ASSERT((bitspertexel % 8) == 0); // should not have compressed formats here.
> +    const size_t srcTexelSize = bitspertexel / 8;

I like this new stuff.
Attachment #8502030 - Flags: review?(jgilbert) → review+
Comment on attachment 8502195 [details] [diff] [review]
address review comment, make fixes needed to still pass WebGL1 conformance tests, remove depth formats from WebGLTexelFormat as they don't support conversions

Review of attachment 8502195 [details] [diff] [review]:

::: dom/canvas/WebGLContextGL.cpp
@@ +3736,4 @@
>      WebGLTexelFormat actualSrcFormat = srcFormat == WebGLTexelFormat::Auto ? dstFormat : srcFormat;
>      if (byteLength) {
> +        size_t   bitspertexel = GetBitsPerTexel(effectiveInternalFormat);

Attachment #8502195 - Flags: review?(jgilbert) → review+
Surprisingly tricky to land: this patch was trying to always pass sized internalformats to the driver, but that turned out to make our test slaves unhappy with old 16bit formats. Now passing sized internalformats only for the new formats introduced in WebGL2 for which unsized internalformat is illegal.
Blocks: 1071335
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.