1221822 - (webgl-tex-refactor) Finish the texture refactor

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Let's finish this.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Bug-1189917-Refactor-ImageInfo-handling-mipmaps-fake.patch

Here's the github branch:
https://github.com/jdashg/gecko-dev/tree/tex-formats

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Pull-in-aggressive-refactor-from-tex-format-tables.patch

Here's the big one.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: 0003-Optimize-good-FB-status-instead-of-bad-and-mark-dept.patch

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: 0004-Add-2D_ARRAY-textures-and-fix-FormatAuthority-startu.patch

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: 0005-Misc-from-initial-review-vidyo.patch

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: 0006-WebGL2-Get-TexParam-and-more-TEX_2D_ARRAY.-Also-no-r.patch

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: 0007-Misc-fixes.patch

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: 0008-Fix-local-conformance-tests.patch

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: 0009-Compile-on-Linux-Clang-3.6.2-and-GCC-4.8.patch

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: 0010-Check-that-target-is-valid-for-formats.patch

Comment 11

[Kelsey Gilbert [:jgilbert]]

Try is doing ok:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b9e2b5081ae

The only mysterious thing is the IsContextCurrent failure on OSX. Really not sure how that's happening.

Windows, linux, android all seem green.

Modulo tests that are wrong, we're also at parity with 1.0.4 conformance tests, at least on my Windows machine.

Comment 12

[Kelsey Gilbert [:jgilbert]]

Attachment: 0011-Fixes-from-Try.patch

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: 0014-Implement-swizzling-for-L-A-LA.patch

Comment 14

[Kelsey Gilbert [:jgilbert]]

I did conclude that doing swizzle setting at TexImage time isn't enough, because:
TexImage(level=0, format=L8)
TexImage(level=1, format=RGBA8)
TexParameter(MINFILTER, NEAREST) // disable mipmapping
// Texture is now complete, because level=1 isn't active, and isn't checked for format compatibiliy.
// This means if we disabled swizzle with the TexImage(level=1), then we won't have the swizzling we need
// for L8.

Comment 15

[u480271]

Comment on attachment 8683395 [details] [diff] [review]
0002-Pull-in-aggressive-refactor-from-tex-format-tables.patch

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

I tried to be as thorough as possible, but I really couldn't check all this code for correctness. (I spent 2 days looking over the files.)

We're going in have to rely upon conformance tests to assure that there aren't any regressions.

::: dom/canvas/TexUnpackBlob.cpp
@@ +24,5 @@
> +                GLsizei width, GLsizei height, GLsizei depth, const void* data)
> +{
> +    auto internalFormat = dui->internalFormat;
> +    auto unpackFormat = dui->unpackFormat;
> +    auto unpackType = dui->unpackType;

why not just modify DoTexSubImage/DoTexImage to take the DriverUnpackInfo by pointer?

@@ +74,5 @@
> +
> +    CheckedUint32 strideBytesPerRow = bytesPerPixel * stridePixelsPerRow;
> +    strideBytesPerRow = RoundUpToMultipleOf(strideBytesPerRow, rowByteAlignment);
> +
> +    CheckedUint32 strideBytesPerImage = strideBytesPerRow * strideRowsPerImage;

bytesPerImage is sufficient, right?

@@ +285,5 @@
> +            return;
> +        }
> +
> +        uploadBytes = tempBuffer.get();
> +        break;

superfluous?

@@ +316,5 @@
> +    return false;
> +}
> +
> +static bool
> +SupportsBGRA(gl::GLContext* gl)

This function doesn't appear to be used.

::: dom/canvas/WebGL2ContextFramebuffers.cpp
@@ +34,4 @@
>  
>      if (fb->ColorAttachment(0).IsDefined()) {
>          const auto& attachment = fb->ColorAttachment(0);
> +        *out_colorFormat = attachment.Format()->format;

Format()->format looks `funny` to me. I guess FormatInfo()->format is too long?

::: dom/canvas/WebGLContext.cpp
@@ +1792,4 @@
>  }
>  
>  CheckedUint32
> +RoundedToNextMultipleOf(CheckedUint32 value, CheckedUint32 multiple)

RoundUpToMultipleOf() and RoundedToNextMultipleOf() have the exact same source code, except for types. Turn into a template'd function?

::: dom/canvas/WebGLContextDraw.cpp
@@ +114,4 @@
>          return false;
>      }
>  
> +    if (!BindFakeBlackTextures(info))

Surely, return BindFakeBlackTextures(info); or do we need to be really explicit?

::: dom/canvas/WebGLContextValidate.cpp
@@ +905,5 @@
> +    const auto fnFloor = [](uint32_t& val) {
> +        if (val) {
> +            val = FloorPOT(val);
> +        }
> +    };

Isn't this a little gratuitous? Make FloorPOT handle zero. There must be one of the variants in the gfx code that handles it.

::: dom/canvas/WebGLExtensionColorBufferFloat.cpp
@@ +31,5 @@
>  
> +#define FOO(x) fnUpdateUsage(LOCAL_GL_ ## x, webgl::EffectiveFormat::x)
> +
> +    FOO(RGBA32F);
> +    FOO(RGB32F);

I'm confused by the spec. The spec seems to say that only RGBA32F is accepted. *Note: this is different to what WEBGL_color_buffer_half_float says!*

https://www.khronos.org/registry/webgl/extensions/WEBGL_color_buffer_float/

"The 32-bit floating-point type RGBA32F becomes available as a color-renderable format. Renderbuffers can be created in this format. These and textures created with format = RGBA and type = FLOAT as specified in OES_texture_float, can be attached to framebuffer object color attachments for rendering."

::: dom/canvas/WebGLExtensionSRGB.cpp
@@ +25,4 @@
>          gl->fEnable(LOCAL_GL_FRAMEBUFFER_SRGB_EXT);
>      }
>  
> +    auto& fua = webgl->mFormatUsage;

Why not make this one look like the other extension setups, where you use a lambda to do the heavy lifting?

::: dom/canvas/WebGLFormats.cpp
@@ +175,2 @@
>                                hasStencil, compressedFormatInfo };
> +    const auto itr = AlwaysInsert(gFormatInfoMap, format, info);

Unused variable `itr`?

@@ +299,5 @@
>  
> +    // OES_texture_half_float
> +    AddFormatInfo(FOO(Luminance16FAlpha16F), 2, UnsizedFormat::LA, false, ComponentType::Float);
> +    AddFormatInfo(FOO(Luminance16F        ), 1, UnsizedFormat::L , false, ComponentType::Float);
> +    AddFormatInfo(FOO(Alpha16F            ), 1, UnsizedFormat::A , false, ComponentType::Float);

"bytes per pixel" for these 6 formats is completely wrong. I've fixed it in a patch that's on the texture conversion bug.

@@ +549,5 @@
> +    FOO(RGB5_A1          );
> +    FOO(RGB565           );
> +    FOO(DEPTH_COMPONENT16);
> +    FOO(STENCIL_INDEX8   );
> +    //FOO(DEPTH24_STENCIL8 );

Please add a comment as to why DEPTH24_STENCIL8 is commented-out.

::: dom/canvas/WebGLFormats.h
@@ +165,2 @@
>  enum class ComponentType : uint8_t {
> +    None,         // DEPTH24_STENCIL8

When I looked into this for framebuffer attachment parameter queries, I ran tests against drivers (desktop, NV). If I recall correctly, they only return information about the depth part and ignore stencil. So DEPTH24_STENCIL8 is some int type.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +911,2 @@
>      if (!attachPoint.IsDefined()) {
> +        mContext->ErrorInvalidOperation("%s: THe attachment specified for reading is"

Typo: THe

::: dom/canvas/WebGLShader.cpp
@@ +183,5 @@
> +    if (PR_GetEnv("MOZ_WEBGL_DUMP_SHADERS")) {
> +        printf_stderr("////////////////////////////////////////\n");
> +        printf_stderr("// MOZ_WEBGL_DUMP_SHADERS:\n");
> +
> +        // Wow - Roll Your Own Foreach-Lines because printf_stderr has a hard-coded

`Wow` can be removed. I was being a over-the-top dramatic when I typed that.

::: dom/canvas/WebGLStrongTypes.h
@@ +147,4 @@
>          return mValue;
>      }
>  
> +

Do we need an extra blank here?

::: dom/webidl/WebGL2RenderingContext.webidl
@@ +344,5 @@
>      void texStorage2D(GLenum target, GLsizei levels, GLenum internalformat, GLsizei width, GLsizei height);
>      void texStorage3D(GLenum target, GLsizei levels, GLenum internalformat, GLsizei width, GLsizei height,
>                        GLsizei depth);
> +
> +    void texImage3D(GLenum target, GLint level, GLenum internalformat, GLsizei width,

Are these just formatting changes? You'll need review from DOM peer before you can land, so may, or may not, be worth it.

::: gfx/gl/ScopedGLHelpers.cpp
@@ +539,5 @@
> +
> +    if (scopedVal != mOldVal) {
> +        gl->fPixelStorei(LOCAL_GL_UNPACK_ALIGNMENT, scopedVal);
> +    } else {
> +      // Don't try to re-set it during unwrap.

indent

Comment 16

[u480271]

Comment on attachment 8683978 [details] [diff] [review]
0005-Misc-from-initial-review-vidyo.patch

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

::: dom/canvas/WebGLTextureUpload.cpp
@@ +1107,5 @@
> +    const webgl::DriverUnpackInfo* driverUnpackInfo;
> +    if (!dstUsage->IsUnpackValid(srcPacking, &driverUnpackInfo)) {
> +        mContext->ErrorInvalidOperation("%s: Mismatched internalFormat and format/type:"
> +                                        " 0x%04x and 0x%04x/0x%04x",
> +                                        funcName, internalFormat, unpackFormat,

Instead of 0x%04x, you can use mContext->EnumName() to get a string representation or hex code, if function doesn't know about enum.

Comment 17

[u480271]

Comment on attachment 8683979 [details] [diff] [review]
0006-WebGL2-Get-TexParam-and-more-TEX_2D_ARRAY.-Also-no-r.patch

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

::: dom/canvas/WebGLTexture.cpp
@@ -400,4 @@
>          if (!baseImageInfo.IsPowerOfTwo()) {
>              // "either the texture wrap mode is not CLAMP_TO_EDGE"
>              if (mWrapS != LOCAL_GL_CLAMP_TO_EDGE ||
> -                mWrapS != LOCAL_GL_CLAMP_TO_EDGE)

Oooh, nice spot.

Comment 18

[u480271]

Comment on attachment 8683981 [details] [diff] [review]
0008-Fix-local-conformance-tests.patch

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

::: dom/canvas/test/webgl-conformance/conformance/more/functions/texSubImage2DBadArgs.html
@@ +52,4 @@
>      gl.deleteTexture(tex);
>  }
>  
> +function assertGLErrorIn(gl, expectedList, desc, func) {

Shouldn't you use assertGLErrorIn from util.js?

::: dom/canvas/test/webgl-conformance/conformance/textures/tex-input-validation.html
@@ +66,2 @@
>      gl.texParameteri(testCase.target, testCase.pname, testCase.param);
> +    glErrorShouldBe(gl, errors, msg);

glErrorShouldBeIn?

@@ +76,2 @@
>      gl.getTexParameter(testCase.target, testCase.pname);
> +    glErrorShouldBe(gl, errors, msg);

glErrorShouldBeIn?

Comment 19

[u480271]

Comment on attachment 8684526 [details] [diff] [review]
0014-Implement-swizzling-for-L-A-LA.patch

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

::: dom/canvas/WebGLTexture.cpp
@@ +576,4 @@
>      if (!imageInfo.IsDefined())
>          return true; // The driver should handle this for us?
>                       // (happens because ResolveFakeBlack plus incomplete mipchains)
> +    */

Remove if determined that this code isn't needed.

::: dom/canvas/WebGLTextureUpload.cpp
@@ +1639,5 @@
> +                                 LOCAL_GL_RENDERBUFFER, rgbaRB);
> +
> +    const GLenum status = gl->fCheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER);
> +    if (status != LOCAL_GL_FRAMEBUFFER_COMPLETE) {
> +        MOZ_CRASH("Temp framebuffer is not Complete.");

complete.

Comment 20

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #15)
> Comment on attachment 8683395 [details] [diff] [review]
> 0002-Pull-in-aggressive-refactor-from-tex-format-tables.patch
> 
> Review of attachment 8683395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I tried to be as thorough as possible, but I really couldn't check all this
> code for correctness. (I spent 2 days looking over the files.)
> 
> We're going in have to rely upon conformance tests to assure that there
> aren't any regressions.

First off, thanks for soldiering through! It's a solid ton of code.

And yep, I've been running conformance tests and will ensure that we're at at least
parity for 1.0.4 and WebGL 2 tests.

> 
> ::: dom/canvas/TexUnpackBlob.cpp
> @@ +24,5 @@
> > +                GLsizei width, GLsizei height, GLsizei depth, const void* data)
> > +{
> > +    auto internalFormat = dui->internalFormat;
> > +    auto unpackFormat = dui->unpackFormat;
> > +    auto unpackType = dui->unpackType;
> 
> why not just modify DoTexSubImage/DoTexImage to take the DriverUnpackInfo by
> pointer?

Ok, I piped DriverUnpackInfo and PackingInfo deeper into DoTex(Sub)Image. I think it is
better this way. Fewer variables being passed around, at least!

> 
> @@ +74,5 @@
> > +
> > +    CheckedUint32 strideBytesPerRow = bytesPerPixel * stridePixelsPerRow;
> > +    strideBytesPerRow = RoundUpToMultipleOf(strideBytesPerRow, rowByteAlignment);
> > +
> > +    CheckedUint32 strideBytesPerImage = strideBytesPerRow * strideRowsPerImage;
> 
> bytesPerImage is sufficient, right?

The terminology here is tricky. I tried to clean this up a little more.

I explicitly call it 'strideBytesPerImage', because we don't need all of the bytes of
the final image:
If height is 10 and depth is 1, but UnpackImageHeight is 1000, we still only need the
first 10 rows worth of data, even if strideBytesPerImage would be 1000 * strideBytesPerRow.

> 
> @@ +285,5 @@
> > +            return;
> > +        }
> > +
> > +        uploadBytes = tempBuffer.get();
> > +        break;
> 
> superfluous?

Yep, removed.

> 
> @@ +316,5 @@
> > +    return false;
> > +}
> > +
> > +static bool
> > +SupportsBGRA(gl::GLContext* gl)
> 
> This function doesn't appear to be used.

This was a mistake, and is corrected in a later patch.

> 
> ::: dom/canvas/WebGL2ContextFramebuffers.cpp
> @@ +34,4 @@
> >  
> >      if (fb->ColorAttachment(0).IsDefined()) {
> >          const auto& attachment = fb->ColorAttachment(0);
> > +        *out_colorFormat = attachment.Format()->format;
> 
> Format()->format looks `funny` to me. I guess FormatInfo()->format is too
> long?

Format() is really FormatUsage(), but it sounds really funny to ask for the 'format usage'
of anything. I would say this is best continuing to look a little funny, but ultimately
is fairly clear.

> 
> ::: dom/canvas/WebGLContext.cpp
> @@ +1792,4 @@
> >  }
> >  
> >  CheckedUint32
> > +RoundedToNextMultipleOf(CheckedUint32 value, CheckedUint32 multiple)
> 
> RoundUpToMultipleOf() and RoundedToNextMultipleOf() have the exact same
> source code, except for types. Turn into a template'd function?

It's already a templated function in WebGLContext.h. I will delete these type-specific
versions.

> 
> ::: dom/canvas/WebGLContextDraw.cpp
> @@ +114,4 @@
> >          return false;
> >      }
> >  
> > +    if (!BindFakeBlackTextures(info))
> 
> Surely, return BindFakeBlackTextures(info); or do we need to be really
> explicit?

This is changed significantly in the patch that fixes L/A/LA.

> 
> ::: dom/canvas/WebGLContextValidate.cpp
> @@ +905,5 @@
> > +    const auto fnFloor = [](uint32_t& val) {
> > +        if (val) {
> > +            val = FloorPOT(val);
> > +        }
> > +    };
> 
> Isn't this a little gratuitous? Make FloorPOT handle zero. There must be one
> of the variants in the gfx code that handles it.

The issue is that zero isn't a defined integer power-of-two. (log2(0) is -Inf)
This lambda is so we can use one function to also handle 'max sizes' of '0', which is what we
use if we don't support the relevant tex image target. I've changed it to this:

    // If we don't support a target, its max size is 0. We should only floor-to-POT if the
    // value if it's non-zero. (NB log2(0) is -Inf, so zero isn't an integer power-of-two)
    const auto fnFloorPOTIfSupported = [](uint32_t& val) {
        if (val) {
            val = FloorPOT(val);
        }
    };

    fnFloorPOTIfSupported(mImplMaxTextureSize);
    fnFloorPOTIfSupported(mImplMaxCubeMapTextureSize);
    fnFloorPOTIfSupported(mImplMaxRenderbufferSize);

    fnFloorPOTIfSupported(mImplMax3DTextureSize);
    fnFloorPOTIfSupported(mImplMaxArrayTextureLayers);

> 
> ::: dom/canvas/WebGLExtensionColorBufferFloat.cpp
> @@ +31,5 @@
> >  
> > +#define FOO(x) fnUpdateUsage(LOCAL_GL_ ## x, webgl::EffectiveFormat::x)
> > +
> > +    FOO(RGBA32F);
> > +    FOO(RGB32F);
> 
> I'm confused by the spec. The spec seems to say that only RGBA32F is
> accepted. *Note: this is different to what WEBGL_color_buffer_half_float
> says!*
> 
> https://www.khronos.org/registry/webgl/extensions/WEBGL_color_buffer_float/
> 
> "The 32-bit floating-point type RGBA32F becomes available as a
> color-renderable format. Renderbuffers can be created in this format. These
> and textures created with format = RGBA and type = FLOAT as specified in
> OES_texture_float, can be attached to framebuffer object color attachments
> for rendering."

You are absolutely correct. Fixed.

> 
> ::: dom/canvas/WebGLExtensionSRGB.cpp
> @@ +25,4 @@
> >          gl->fEnable(LOCAL_GL_FRAMEBUFFER_SRGB_EXT);
> >      }
> >  
> > +    auto& fua = webgl->mFormatUsage;
> 
> Why not make this one look like the other extension setups, where you use a
> lambda to do the heavy lifting?

No reason. I can do this.

> 
> ::: dom/canvas/WebGLFormats.cpp
> @@ +175,2 @@
> >                                hasStencil, compressedFormatInfo };
> > +    const auto itr = AlwaysInsert(gFormatInfoMap, format, info);
> 
> Unused variable `itr`?

Yep, removed.

> 
> @@ +299,5 @@
> >  
> > +    // OES_texture_half_float
> > +    AddFormatInfo(FOO(Luminance16FAlpha16F), 2, UnsizedFormat::LA, false, ComponentType::Float);
> > +    AddFormatInfo(FOO(Luminance16F        ), 1, UnsizedFormat::L , false, ComponentType::Float);
> > +    AddFormatInfo(FOO(Alpha16F            ), 1, UnsizedFormat::A , false, ComponentType::Float);
> 
> "bytes per pixel" for these 6 formats is completely wrong. I've fixed it in
> a patch that's on the texture conversion bug.

Alright, fixed it here anyway.

> 
> @@ +549,5 @@
> > +    FOO(RGB5_A1          );
> > +    FOO(RGB565           );
> > +    FOO(DEPTH_COMPONENT16);
> > +    FOO(STENCIL_INDEX8   );
> > +    //FOO(DEPTH24_STENCIL8 );
> 
> Please add a comment as to why DEPTH24_STENCIL8 is commented-out.
It now looks like:

    //FOO(DEPTH24_STENCIL8 ); // WebGL 1 uses DEPTH_STENCIL instead of DEPTH24_STENCIL8.

> 
> ::: dom/canvas/WebGLFormats.h
> @@ +165,2 @@
> >  enum class ComponentType : uint8_t {
> > +    None,         // DEPTH24_STENCIL8
> 
> When I looked into this for framebuffer attachment parameter queries, I ran
> tests against drivers (desktop, NV). If I recall correctly, they only return
> information about the depth part and ignore stencil. So DEPTH24_STENCIL8 is
> some int type.

The answer for GetFramebufferAttachmentParameter from GLES 3.0.4 p240:
"If attachment is DEPTH_STENCIL_ATTACHMENT the query will fail and generate an INVALID_OPERATION error."

Also, our GetFramebufferAttachmentParameter is a mess, so I fixed it.

> ::: dom/canvas/WebGLFramebuffer.cpp
> @@ +911,2 @@
> >      if (!attachPoint.IsDefined()) {
> > +        mContext->ErrorInvalidOperation("%s: THe attachment specified for reading is"
> 
> Typo: THe

Fixed.

> 
> ::: dom/canvas/WebGLShader.cpp
> @@ +183,5 @@
> > +    if (PR_GetEnv("MOZ_WEBGL_DUMP_SHADERS")) {
> > +        printf_stderr("////////////////////////////////////////\n");
> > +        printf_stderr("// MOZ_WEBGL_DUMP_SHADERS:\n");
> > +
> > +        // Wow - Roll Your Own Foreach-Lines because printf_stderr has a hard-coded
> 
> `Wow` can be removed. I was being a over-the-top dramatic when I typed that.

I like it. It's not necessary, but I think it's fine.

> 
> ::: dom/canvas/WebGLStrongTypes.h
> @@ +147,4 @@
> >          return mValue;
> >      }
> >  
> > +
> 
> Do we need an extra blank here?

No, this is a relic from churn.

> 
> ::: dom/webidl/WebGL2RenderingContext.webidl
> @@ +344,5 @@
> >      void texStorage2D(GLenum target, GLsizei levels, GLenum internalformat, GLsizei width, GLsizei height);
> >      void texStorage3D(GLenum target, GLsizei levels, GLenum internalformat, GLsizei width, GLsizei height,
> >                        GLsizei depth);
> > +
> > +    void texImage3D(GLenum target, GLint level, GLenum internalformat, GLsizei width,
> 
> Are these just formatting changes? You'll need review from DOM peer before
> you can land, so may, or may not, be worth it.

It'll be easy enough to get.

> 
> ::: gfx/gl/ScopedGLHelpers.cpp
> @@ +539,5 @@
> > +
> > +    if (scopedVal != mOldVal) {
> > +        gl->fPixelStorei(LOCAL_GL_UNPACK_ALIGNMENT, scopedVal);
> > +    } else {
> > +      // Don't try to re-set it during unwrap.
> 
> indent

Thanks. I fixed this and other indent issues in the file.

Comment 21

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #18)
> Comment on attachment 8683981 [details] [diff] [review]
> 0008-Fix-local-conformance-tests.patch
> 
> Review of attachment 8683981 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> dom/canvas/test/webgl-conformance/conformance/more/functions/
> texSubImage2DBadArgs.html
> @@ +52,4 @@
> >      gl.deleteTexture(tex);
> >  }
> >  
> > +function assertGLErrorIn(gl, expectedList, desc, func) {
> 
> Shouldn't you use assertGLErrorIn from util.js?

Yep, fixed. I also don't need to switch all the one-error asserts over.

> 
> :::
> dom/canvas/test/webgl-conformance/conformance/textures/tex-input-validation.
> html
> @@ +66,2 @@
> >      gl.texParameteri(testCase.target, testCase.pname, testCase.param);
> > +    glErrorShouldBe(gl, errors, msg);
> 
> glErrorShouldBeIn?
Fixed.

> 
> @@ +76,2 @@
> >      gl.getTexParameter(testCase.target, testCase.pname);
> > +    glErrorShouldBe(gl, errors, msg);
> 
> glErrorShouldBeIn?
Fixed.

Comment 22

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #19)
> Comment on attachment 8684526 [details] [diff] [review]
> 0014-Implement-swizzling-for-L-A-LA.patch
> 
> Review of attachment 8684526 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLTexture.cpp
> @@ +576,4 @@
> >      if (!imageInfo.IsDefined())
> >          return true; // The driver should handle this for us?
> >                       // (happens because ResolveFakeBlack plus incomplete mipchains)
> > +    */
> 
> Remove if determined that this code isn't needed.
Yep, I forget to remove it.
> 
> ::: dom/canvas/WebGLTextureUpload.cpp
> @@ +1639,5 @@
> > +                                 LOCAL_GL_RENDERBUFFER, rgbaRB);
> > +
> > +    const GLenum status = gl->fCheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER);
> > +    if (status != LOCAL_GL_FRAMEBUFFER_COMPLETE) {
> > +        MOZ_CRASH("Temp framebuffer is not Complete.");
> 
> complete.
Ok.

Comment 23

[Kelsey Gilbert [:jgilbert]]

Attachment: 0015-Fix-ScopedCopyTex-and-fix-build-error-on-Mac.patch

Comment 24

[Kelsey Gilbert [:jgilbert]]

Attachment: 0016-1.0.4-parity.patch

This one went a little wild on me, sorry!

Comment 25

[Kelsey Gilbert [:jgilbert]]

Attachment: 0017-More-fixes-from-review.patch

Comment 26

[Kelsey Gilbert [:jgilbert]]

Attachment: 0018-Don-t-require-draw_buffers-as-a-native-ext-for-WebGL.patch

We should slowly wean these away, and support them natively, rather than as pseudo-exts.

Comment 27

[u480271]

Comment on attachment 8685745 [details] [diff] [review]
0016-1.0.4-parity.patch

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

There seemed to be a couple of logical changes mixed up here. Would appreciate those separated out in future.

::: dom/canvas/WebGLContext.cpp
@@ +1273,5 @@
>  
> +    const bool changeDrawBuffers = (mDefaultFB_DrawBuffer0 != LOCAL_GL_BACK);
> +    if (changeDrawBuffers) {
> +        const GLenum back = LOCAL_GL_BACK;
> +        gl->fDrawBuffers(1, &back);

To keep the theme: ScopedBindDrawBuffer RAII helper?

::: dom/canvas/WebGLContextGL.cpp
@@ +767,5 @@
> +    case LOCAL_GL_FRAMEBUFFER_ATTACHMENT_RED_SIZE:
> +    case LOCAL_GL_FRAMEBUFFER_ATTACHMENT_GREEN_SIZE:
> +    case LOCAL_GL_FRAMEBUFFER_ATTACHMENT_BLUE_SIZE:
> +        if (attachment == LOCAL_GL_COLOR)
> +            return JS::Int32Value(8);

Are these always true for default framebuffer? What about on a 16bit GLES device? (Do we support such a thing anymore?)

Comment 28

[u480271]

Comment on attachment 8685746 [details] [diff] [review]
0017-More-fixes-from-review.patch

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

::: dom/canvas/test/webgl-conformance/conformance/more/util.js
@@ +1006,3 @@
>    if (glErr !== err) {
>      if (glErr === undefined) {
>        testFailed("assertGLError: UNEXPCETED EXCEPTION", name, f);

UNEXPECTED

@@ +1026,4 @@
>      if ('glError' in e) {
>        actualError = e.glError;
>      } else {
> +      testFailed("assertGLErrorIn: UNEXPCETED EXCEPTION", name, f);

UNEXPECTED

Comment 29

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #27)
> Comment on attachment 8685745 [details] [diff] [review]
> 0016-1.0.4-parity.patch
> 
> Review of attachment 8685745 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There seemed to be a couple of logical changes mixed up here. Would
> appreciate those separated out in future.
> 
> ::: dom/canvas/WebGLContext.cpp
> @@ +1273,5 @@
> >  
> > +    const bool changeDrawBuffers = (mDefaultFB_DrawBuffer0 != LOCAL_GL_BACK);
> > +    if (changeDrawBuffers) {
> > +        const GLenum back = LOCAL_GL_BACK;
> > +        gl->fDrawBuffers(1, &back);
> 
> To keep the theme: ScopedBindDrawBuffer RAII helper?

I think it would be more complexity than it's worth to do an RAII helper here.

> 
> ::: dom/canvas/WebGLContextGL.cpp
> @@ +767,5 @@
> > +    case LOCAL_GL_FRAMEBUFFER_ATTACHMENT_RED_SIZE:
> > +    case LOCAL_GL_FRAMEBUFFER_ATTACHMENT_GREEN_SIZE:
> > +    case LOCAL_GL_FRAMEBUFFER_ATTACHMENT_BLUE_SIZE:
> > +        if (attachment == LOCAL_GL_COLOR)
> > +            return JS::Int32Value(8);
> 
> Are these always true for default framebuffer? What about on a 16bit GLES
> device? (Do we support such a thing anymore?)

In theory, but let's cross that bridge later.

Comment 30

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #28)
> Comment on attachment 8685746 [details] [diff] [review]
> 0017-More-fixes-from-review.patch
> 
> Review of attachment 8685746 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/test/webgl-conformance/conformance/more/util.js
> @@ +1006,3 @@
> >    if (glErr !== err) {
> >      if (glErr === undefined) {
> >        testFailed("assertGLError: UNEXPCETED EXCEPTION", name, f);
> 
> UNEXPECTED
> 
> @@ +1026,4 @@
> >      if ('glError' in e) {
> >        actualError = e.glError;
> >      } else {
> > +      testFailed("assertGLErrorIn: UNEXPCETED EXCEPTION", name, f);
> 
> UNEXPECTED

Fixed.

Comment 31

[Kelsey Gilbert [:jgilbert]]

Attachment: 0019-No-INCOMPLETE_DIMENSIONS-in-WebGL-2.patch

In the spirit of moving fast, I fixed a bunch of tests. It's hard to tell what tests are safe to leave broken before landing on nightly. It's easiest to coalesce these fixes into this already-monolithic cset.

Comment 32

[Kelsey Gilbert [:jgilbert]]

Attachment: 0020-Fix-conformance2-renderbuffers-invalidate-framebuffe.patch

Comment 33

[Kelsey Gilbert [:jgilbert]]

Attachment: 0021-Remove-native-extensions-from-WebGL-2.patch

Comment 34

[Kelsey Gilbert [:jgilbert]]

Attachment: 0022-Fix-build-issues.patch

Comment 35

[Kelsey Gilbert [:jgilbert]]

Attachment: 0023-Try-fixes.patch

Comment 36

[Kelsey Gilbert [:jgilbert]]

Attachment: 0024-Fix-conformance2-textures-tex-mipmap-levels.patch

Comment 37

[Kelsey Gilbert [:jgilbert]]

Attachment: 0025-Try-fixes.patch

Comment 38

[Kelsey Gilbert [:jgilbert]]

Attachment: 0026-Fix-conformance-textures-misc-tex-storage-2d.patch

Comment 39

[Kelsey Gilbert [:jgilbert]]

Attachment: 0027-Fix-conformance2-textures-misc-tex-storage-compresse.patch

Comment 40

[Kelsey Gilbert [:jgilbert]]

Attachment: 0028-Fix-conformance-textures-misc-texture-npot.patch

Comment 41

[Kelsey Gilbert [:jgilbert]]

Attachment: 0029-Mark-L-A-LA-as-filterable-for-WebGL-2.patch

Comment 42

[Kelsey Gilbert [:jgilbert]]

Attachment: 0030-Support-webgl2-tex-format-tests.patch

Comment 43

[Kelsey Gilbert [:jgilbert]]

Attachment: 0031-Review-nits.patch

Comment 44

[Kelsey Gilbert [:jgilbert]]

Try is clean:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a9a1130b370

I'll post the remaining few patches shortly. I'm trying to get fast video uploads working again.

Comment 45

[Kelsey Gilbert [:jgilbert]]

Attachment: 0032-Fix-GetFBAttach-behavior-for-null-attachments.patch

Bad news: More patches.

These are all pretty simple except for the "Fix fast video uploads" stuff. Even that's not bad.

Comment 46

[Kelsey Gilbert [:jgilbert]]

Attachment: 0033-Fix-sRGB.patch

Comment 47

[Kelsey Gilbert [:jgilbert]]

Attachment: 0034-Fix-tex-half-float-exts-for-WebGL1-on-GLES.patch

Comment 48

[Kelsey Gilbert [:jgilbert]]

Attachment: 0035-Isolate-OSX-assert.patch

These were intended to be temporary sanity checks, but I think we should take them permanently. They're really handy in case something surprises us MakeCurrent-wise.

Comment 49

[Kelsey Gilbert [:jgilbert]]

Attachment: 0036-Fix-OSX-IsCurrent-issue.patch

This is actually only half the fix. The next patch has the fuller solution.

Comment 50

[Kelsey Gilbert [:jgilbert]]

Attachment: 0037-Fix-OSX-MakeCurrent-and-fix-obj-del-behav-test.patch

Comment 51

[Kelsey Gilbert [:jgilbert]]

Attachment: 0038-Fix-refcounted-lambda-captures-and-fix-implicit_colo.patch

Static analysis errors if you capture a pointer to a refcounted object. It's over-sensitive, but I'm not going to stop these patches to fix it.

I've commented in the corresponding bug, and none of our proposed usecases are unsafe.

Comment 52

[Kelsey Gilbert [:jgilbert]]

Attachment: 0039-Add-more-spew-and-try-to-fix-implicit-color-buffer-h.patch

More handy tools to add to spew. (features)

Comment 53

[Kelsey Gilbert [:jgilbert]]

Attachment: 0040-Fix-FB-completeness-for-WebGL2.patch

Comment 54

[Kelsey Gilbert [:jgilbert]]

Attachment: 0041-Allow-SurfaceFromElement-to-return-the-Image.patch

One for Mett!

Comment 55

[Kelsey Gilbert [:jgilbert]]

Attachment: 0042-Fix-fast-video-uploads.patch

Comment 56

[Kelsey Gilbert [:jgilbert]]

Attachment: 0043-Compressed-texture-formats-should-be-filterable.patch

Comment 57

[Kelsey Gilbert [:jgilbert]]

Attachment: 0044-Add-pref-for-using-unsized-float-half-float-formats-.patch

The now-venerable Teleporter demo uses RGBA/RGBA/HALF_FLOAT_OES, where it should be using RGBA16F/RGBA/HALF_FLOAT.

Toggling this pref doesn't seem to change rendering, but we can at least use it to demonstrate that it's not the problem.

After this patch, the only errors regard the demo's use of texParameter(SWIZZLE_) params, which are currently not planned for core WebGL 2. We should draft an extension to enable these. It isn't hard to write, but I don't really want to do it in this branch. :)

Comment 58

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8688813 [details] [diff] [review]
0041-Allow-SurfaceFromElement-to-return-the-Image.patch

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

::: layout/base/nsLayoutUtils.h
@@ +2091,1 @@
>      /* mSourceSurface will contain the resulting surface, or will be nullptr on error */

This comment needs to be updated to account for mLayersImage.

Comment 59

[Kelsey Gilbert [:jgilbert]]

Attachment: 0045-Build-on-Linux.patch

Comment 60

[Kelsey Gilbert [:jgilbert]]

Attachment: 0046-Update-comment-for-SFEResult.patch

How's this?

Comment 61

[Kelsey Gilbert [:jgilbert]]

Attachment: 0047-Fix-ordering-of-SFEResult-checking.-CORS-write-only.patch

Comment 62

[Kelsey Gilbert [:jgilbert]]

Attachment: 0048-Update-tests-based-on-feedback-from-WG.patch

I misread some specs!

Comment 63

[Kelsey Gilbert [:jgilbert]]

Attachment: 0049-Clarify-some-WebGLFormat-funcs-and-support-INVALID_E.patch

I couldn't find a way to weasel out of having to deal with INVALID_ENUM errors for WebGL 1 for texture internalFormat/unpackFormat/unpackTypes.

So now we collect the valid enums, and check against them. (but only when we fail validation!)

Comment 64

[Kelsey Gilbert [:jgilbert]]

Most of the above patch is just moving and renaming.

Comment 65

[Kelsey Gilbert [:jgilbert]]

Attachment: 0050-DOM-imptest-for-cmpTexSubImage-is-wrong-and-not-wort.patch

Imagine my surprise when this test started failing in m-2 on Try. :|

Here's the PR to remove it from upstream:
https://github.com/w3c/web-platform-tests/pull/2359

For the record:
The errors here can be either INVALID_ENUM (explicitly noted in the WebGL spec) or INVALID_OPERATION. (as it's a SubImage operation on an undefined texture (GLES2+ specs))

Comment 66

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8689313 [details] [diff] [review]
0050-DOM-imptest-for-cmpTexSubImage-is-wrong-and-not-wort.patch

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

This caught a bug in Gecko at the time of writing, which wasn't found by the webgl test suite.

Comment 67

[Kelsey Gilbert [:jgilbert]]

(In reply to :Ms2ger from comment #66)
> Comment on attachment 8689313 [details] [diff] [review]
> 0050-DOM-imptest-for-cmpTexSubImage-is-wrong-and-not-wort.patch
> 
> Review of attachment 8689313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This caught a bug in Gecko at the time of writing, which wasn't found by the
> webgl test suite.

This test is incorrect, and I do not think it's worth maintaining as a separate test suite. I have filed bug 1226336 for you for this case. I have also filed bug 1226349, to address stemming the split work that has already been done.

Since this test is malformed, and you do not want to remove it, I will disable it for the time being.

Comment 68

[Kelsey Gilbert [:jgilbert]]

Attachment: 0050-DOM-imptest-for-cmpTexSubImage-is-wrong-and-not-wort.patch

Alright, let's skip it to unblock this.
I can merge the web-platform tests after this.

Comment 69

[Kelsey Gilbert [:jgilbert]]

Attachment: 0050-Skip-web-platform-test-compressedTexSubImage2D-until.patch

So this test is actually in the tree (and run) twice.

Comment 70

[Kelsey Gilbert [:jgilbert]]

Attachment: 0051-Fix-format-code-and-add-more-error-checking-to-the-s.patch

Extra error checking in the sRGB test is because we pass along GetFramebufferAttachmentParameter to the driver, which doesn't appear to be spec-compliant on Win10+NV+GL. Maybe it's a desktop GL issue, and we should probably use our FormatInfo, but getting correct behavior for GetFramebufferAttachmentParameter is low priority.

Comment 71

[u480271]

Comment on attachment 8686946 [details] [diff] [review]
0022-Fix-build-issues.patch

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

::: dom/canvas/WebGLFramebuffer.h
@@ +111,5 @@
>      void OnBackingStoreRespecified() const;
>  };
>  
> +template<typename T>
> +class PlacementArray

Why create Yet Another Array Type? Won't one of the multitude in the source code suffice?

Comment 72

[u480271]

Comment on attachment 8686948 [details] [diff] [review]
0024-Fix-conformance2-textures-tex-mipmap-levels.patch

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

::: dom/canvas/WebGLTexture.cpp
@@ +202,4 @@
>      MOZ_ASSERT(DoesMinFilterRequireMipmap());
>      // GLES 3.0.4, p161
>  
> +    const auto maxLevel = MaxEffectiveMipmapLevel();

Wasn't this one of the examples of when not to use auto?

Comment 73

[u480271]

Comment on attachment 8686951 [details] [diff] [review]
0026-Fix-conformance-textures-misc-tex-storage-2d.patch

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

::: dom/canvas/WebGLTextureUpload.cpp
@@ +955,5 @@
> +    const uint32_t lastLevelWidth = uint32_t(width) >> lastLevel;
> +    const uint32_t lastLevelHeight = uint32_t(height) >> lastLevel;
> +    const uint32_t lastLevelDepth = uint32_t(depth) >> lastLevel;
> +
> +    if (!lastLevelWidth && !lastLevelHeight && !lastLevelDepth) {

Should these be ||?

ie. if (lastLevelWidth == 0 || lastLevelHeight == 0 || lastlevelDepth == 0)
instead of
    if (lastlevelWidth == 0 && lastLevelHeight == 0 && lastLevelDepth == 0)
???

Comment 74

[u480271]

Comment on attachment 8688814 [details] [diff] [review]
0042-Fix-fast-video-uploads.patch

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

::: dom/canvas/TexUnpackBlob.cpp
@@ +302,5 @@
> +        {
> +            break;
> +        }
> +
> +        return;

Please comment this return.

::: dom/canvas/WebGLContextState.cpp
@@ -77,4 @@
>  {
>      *out_stencilBits = 0;
>      if (mBoundDrawFramebuffer) {
> -        if (mBoundDrawFramebuffer->HasDepthStencilConflict()) {

Personally, I feel this makes more sense than the new version.

::: dom/canvas/WebGLTextureUpload.cpp
@@ +1226,4 @@
>      }
>  
>      const bool isSubImage = true;
> +    const bool needsRespect = false;

https://www.youtube.com/watch?v=6FOUqQt3Kg0

Comment 75

[u480271]

Comment on attachment 8688816 [details] [diff] [review]
0044-Add-pref-for-using-unsized-float-half-float-formats-.patch

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

I don't understand why we have a pref for this? Can you explain?

Comment 76

[u480271]

Comment on attachment 8689757 [details] [diff] [review]
0051-Fix-format-code-and-add-more-error-checking-to-the-s.patch

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

::: dom/canvas/test/webgl-conformance/conformance/extensions/ext-sRGB.html
@@ +78,4 @@
>    }
>  }
>  
> +function createGreySRGBATexture(gl, color) {

But it's sRGBA, not SRGBA. I did consider this when I first named the function.

Probably better than it reading as Greys RGBA, instead of Grey sRBGA.

Comment 77

[u480271]

Comment on attachment 8688811 [details] [diff] [review]
0039-Add-more-spew-and-try-to-fix-implicit-color-buffer-h.patch

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

This seems like mostly debug prints, that I'm guessing will be removed later, so I'm not going to review it.

Comment 78

[u480271]

Comment on attachment 8688805 [details] [diff] [review]
0035-Isolate-OSX-assert.patch

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

This patch appear to just be debug prints that I, presume, are going to be removed, so I'm not going to review it.

Comment 79

[u480271]

Comment on attachment 8688816 [details] [diff] [review]
0044-Add-pref-for-using-unsized-float-half-float-formats-.patch

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

There's no rpending, so I'll mark r- until explanation.

Comment 80

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #76)
> Comment on attachment 8689757 [details] [diff] [review]
> 0051-Fix-format-code-and-add-more-error-checking-to-the-s.patch
> 
> Review of attachment 8689757 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/test/webgl-conformance/conformance/extensions/ext-sRGB.html
> @@ +78,4 @@
> >    }
> >  }
> >  
> > +function createGreySRGBATexture(gl, color) {
> 
> But it's sRGBA, not SRGBA. I did consider this when I first named the
> function.
> 
> Probably better than it reading as Greys RGBA, instead of Grey sRBGA.

I think it's better if we communicate it well, even if it's technically wrong.

Comment 81

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8688811 [details] [diff] [review]
0039-Add-more-spew-and-try-to-fix-implicit-color-buffer-h.patch

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

I'll remove the ShouldDumpExts override, but please review the rest of this.

::: dom/canvas/WebGLExtensionTextureHalfFloat.cpp
@@ +52,4 @@
>      //////
>  
>      pi = {LOCAL_GL_RGB, LOCAL_GL_HALF_FLOAT_OES};
> +    dui = {pi.format, pi.format, driverUnpackType};

These changes are essential.

::: dom/canvas/WebGLTextureUpload.cpp
@@ +1162,5 @@
>      if (glError) {
>          mContext->ErrorInvalidOperation("%s: Unexpected error during upload: 0x%04x",
>                                          funcName, glError);
> +        printf_stderr("%s: dui: %x/%x/%x\n", funcName, driverUnpackInfo->internalFormat,
> +                      driverUnpackInfo->unpackFormat, driverUnpackInfo->unpackType);

I believe we want this, as it shortcuts some Try debugging for us. We expect to never hit this In The Wild. (But it would also be useful there, if someone does run into this!)

::: gfx/gl/GLContext.cpp
@@ +2583,4 @@
>  /*static*/ bool
>  GLContext::ShouldDumpExts()
>  {
> +    return true; //gfxEnv::GlDumpExtensions();

This is the only part we don't want from this patch.

::: gfx/gl/GLContextFeatures.cpp
@@ +840,5 @@
> +        for (size_t featureId = 0; featureId < size_t(GLFeature::EnumMax); featureId++) {
> +            GLFeature feature = GLFeature(featureId);
> +            printf_stderr("[%s] Feature::%s\n",
> +                          IsSupported(feature) ? "enabled" : "disabled",
> +                          GetFeatureName(feature));

This is useful and we want to keep it.

Comment 82

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8686951 [details] [diff] [review]
0026-Fix-conformance-textures-misc-tex-storage-2d.patch

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

Please re-review!

::: dom/canvas/WebGLTextureUpload.cpp
@@ +955,5 @@
> +    const uint32_t lastLevelWidth = uint32_t(width) >> lastLevel;
> +    const uint32_t lastLevelHeight = uint32_t(height) >> lastLevel;
> +    const uint32_t lastLevelDepth = uint32_t(depth) >> lastLevel;
> +
> +    if (!lastLevelWidth && !lastLevelHeight && !lastLevelDepth) {

Actually no!
Consider width = 4, height = 1, levels = 2:

lastLevel = 2 - 1 = 2
lastLevelWidth = 4 >> 1 = 2
lastLevelHeight = 1 >> 1 = 0

This is because (4, 1) reduces to (2,1).

However, with levels = 4:
lastLevel = 4 - 1 = 3
lastLevelWidth = 4 >> 3 = 0
lastLevelHeight = 1 >> 3 = 0
// Reduces (4,1) to (2,1)@level1, (1,1)@level2, <invalid>@level3

If all of these are zero, it means that some earlier level must have been the 1x1x1 minimal level, thus `levels` must be too high.

Comment 83

[u480271]

Comment on attachment 8688811 [details] [diff] [review]
0039-Add-more-spew-and-try-to-fix-implicit-color-buffer-h.patch

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

This seems like mostly debug prints, that I'm guessing will be removed later, so I'm not going to review it.

Comment 84

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8688816 [details] [diff] [review]
0044-Add-pref-for-using-unsized-float-half-float-formats-.patch

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

This is a hedge against any existing WebGL 2 demos (particularly Teleporter) that use the old-style RGBA/RGBA/FLOAT, which is not actually allowed in WebGL 2.

For instance, the Teleporter demo hits this error currently. Toggling this pref shows us that it's not harming anything. We should basically remove this once Teleporter is fixed. I would like to keep this until it is fixed, however.

Comment 85

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8683981 [details] [diff] [review]
0008-Fix-local-conformance-tests.patch

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

::: dom/canvas/test/webgl-conformance/conformance/more/functions/texSubImage2DBadArgs.html
@@ +52,4 @@
>      gl.deleteTexture(tex);
>  }
>  
> +function assertGLErrorIn(gl, expectedList, desc, func) {

Yes, but this change here was reverted in a later patch, and is not in the final diff.

::: dom/canvas/test/webgl-conformance/conformance/textures/tex-input-validation.html
@@ +66,2 @@
>      gl.texParameteri(testCase.target, testCase.pname, testCase.param);
> +    glErrorShouldBe(gl, errors, msg);

Fixed in a later patch.

@@ +76,2 @@
>      gl.getTexParameter(testCase.target, testCase.pname);
> +    glErrorShouldBe(gl, errors, msg);

Fixed in a later patch.

Comment 86

[u480271]

Comment on attachment 8688816 [details] [diff] [review]
0044-Add-pref-for-using-unsized-float-half-float-formats-.patch

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

r+ after explanation.

Comment 87

[Kelsey Gilbert [:jgilbert]]

Doesn't look like anything new so far:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=133ddbba3f01

Let's launch.

Comment 88

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8683395 [details] [diff] [review]
0002-Pull-in-aggressive-refactor-from-tex-format-tables.patch

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

Just the .webidls, please!

Comment 89

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8683395 [details] [diff] [review]
0002-Pull-in-aggressive-refactor-from-tex-format-tables.patch

r=me if I'm correct that all the IDL changes are basically whitespace/comment changes.

If there's a substantive change to the IDL here, please let me know what it is?

Comment 90

[Kelsey Gilbert [:jgilbert]]

(In reply to Boris Zbarsky [:bz] from comment #89)
> Comment on attachment 8683395 [details] [diff] [review]
> 0002-Pull-in-aggressive-refactor-from-tex-format-tables.patch
> 
> r=me if I'm correct that all the IDL changes are basically
> whitespace/comment changes.
> 
> If there's a substantive change to the IDL here, please let me know what it
> is?

texImage3D no longer [Throws].

Comment 91

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f614bcd9405

Comment 92

[Boris Zbarsky [:bzbarsky]]

> texImage3D no longer [Throws].

Looks fine, if the underlying implementation doesn't want to throw.

Comment 93

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e82fd82346cb - at least OS X 10.10 opt wasn't happy, https://treeherder.mozilla.org/logviewer.html#?job_id=17737519&repo=mozilla-inbound

Comment 94

[Kelsey Gilbert [:jgilbert]]

Attachment: 0055-Check-for-swizzle-if-we-need-it.patch

Comment 95

[Kelsey Gilbert [:jgilbert]]

Attachment: 0056-Fix-WebGLFormats-and-simplify-L-A-LA-test.patch

Comment 96

[Kelsey Gilbert [:jgilbert]]

Attachment: 0057-Don-t-ask-the-driver-about-FB-attachment-color-encod.patch

Comment 97

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d1c223f397c

Comment 98

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d1c223f397c

Comment 99

[Kelsey Gilbert [:jgilbert]]

57 explicit patches (even ignoring patches that were flattened!) and 816KB of diff later... :)

Explicit thanks to :kamidphish for the mountain of reviews!