Closed Bug 1221822 (webgl-tex-refactor) Opened 4 years ago Closed 4 years ago

Finish the texture refactor

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(52 files, 2 obsolete files)

191.28 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
545.90 KB, patch
kamidphish
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
3.62 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
10.47 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
4.97 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
12.15 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
5.61 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
36.21 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
12.44 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
6.79 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
2.99 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
63.30 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
1.30 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
99.96 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
9.39 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
1.50 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
928 bytes, patch
kamidphish
: review+
Details | Diff | Splinter Review
13.65 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
20.60 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
9.68 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
3.40 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
4.72 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
3.50 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
3.31 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
6.69 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
3.40 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
861 bytes, patch
kamidphish
: review+
Details | Diff | Splinter Review
3.14 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
1.37 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
2.11 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
945 bytes, patch
kamidphish
: review+
Details | Diff | Splinter Review
11.18 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
3.10 KB, patch
Details | Diff | Splinter Review
800 bytes, patch
kamidphish
: review+
Details | Diff | Splinter Review
8.74 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
11.95 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
3.25 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
4.61 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
17.90 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
21.18 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
4.17 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
2.62 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
3.71 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
1.33 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.61 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
4.16 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
35.13 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
1.98 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
10.35 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
3.62 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
8.11 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
980 bytes, patch
kamidphish
: review+
Details | Diff | Splinter Review
Let's finish this.
Blocks: 1189917
No longer blocks: 1189917
Depends on: 1189917
Here's the big one.
Attachment #8683395 - Flags: review?(dglastonbury)
Attachment #8683978 - Flags: review?(dglastonbury)
Attachment #8683980 - Flags: review?(dglastonbury)
Attachment #8683981 - Flags: review?(dglastonbury)
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.
Attachment #8684525 - Flags: review?(dglastonbury)
Attachment #8684526 - Flags: review?(dglastonbury)
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 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
Attachment #8683395 - Flags: review?(dglastonbury) → review+
Attachment #8683396 - Flags: review?(dglastonbury) → review+
Attachment #8683397 - Flags: review?(dglastonbury) → review+
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.
Attachment #8683978 - Flags: review?(dglastonbury) → review+
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.
Attachment #8683979 - Flags: review?(dglastonbury) → review+
Attachment #8683980 - Flags: review?(dglastonbury) → review+
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?
Attachment #8683981 - Flags: review?(dglastonbury) → review-
Attachment #8684016 - Flags: review?(dglastonbury) → review+
Attachment #8684017 - Flags: review?(dglastonbury) → review+
Attachment #8684525 - Flags: review?(dglastonbury) → review+
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.
Attachment #8684526 - Flags: review?(dglastonbury) → review+
Blocks: 1213689
(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.
(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.
(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.
This one went a little wild on me, sorry!
Attachment #8685745 - Flags: review?(dglastonbury)
Attachment #8685746 - Flags: review?(dglastonbury)
We should slowly wean these away, and support them natively, rather than as pseudo-exts.
Attachment #8685759 - Flags: review?(dglastonbury)
Attachment #8685744 - Flags: review?(dglastonbury) → review+
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?)
Attachment #8685745 - Flags: review?(dglastonbury) → review+
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
Attachment #8685746 - Flags: review?(dglastonbury) → review+
Attachment #8685759 - Flags: review?(dglastonbury) → review+
(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.
(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.
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.
Attachment #8686943 - Flags: review?(dglastonbury)
Attachment #8686946 - Flags: review?(dglastonbury)
Attachment #8686947 - Flags: review?(dglastonbury)
Attachment #8686950 - Flags: review?(dglastonbury)
Attachment #8686955 - Flags: review?(dglastonbury)
Attachment #8686956 - Flags: review?(dglastonbury)
Attachment #8686943 - Flags: review?(dglastonbury) → review+
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.
Bad news: More patches.

These are all pretty simple except for the "Fix fast video uploads" stuff. Even that's not bad.
Attachment #8688802 - Flags: review?(dglastonbury)
Attachment #8688803 - Flags: review?(dglastonbury)
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.
Attachment #8688805 - Flags: review?(dglastonbury)
This is actually only half the fix. The next patch has the fuller solution.
Attachment #8688806 - Flags: review?(dglastonbury)
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.
Attachment #8688809 - Flags: review?(dglastonbury)
More handy tools to add to spew. (features)
Attachment #8688811 - Flags: review?(dglastonbury)
Attachment #8688812 - Flags: review?(dglastonbury)
One for Mett!
Attachment #8688813 - Flags: review?(matt.woodrow)
Attachment #8688814 - Flags: review?(dglastonbury)
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. :)
Attachment #8688816 - Flags: review?(dglastonbury)
See Also: → 1191521
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.
Attachment #8688813 - Flags: review?(matt.woodrow) → review+
Attachment #8686944 - Flags: review?(dglastonbury) → review+
Attachment #8689242 - Flags: review?(dglastonbury)
How's this?
Attachment #8689243 - Flags: review?(matt.woodrow)
Attachment #8689243 - Flags: review?(matt.woodrow) → review+
I misread some specs!
Attachment #8689311 - Flags: review?(dglastonbury)
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!)
Attachment #8689312 - Flags: review?(dglastonbury)
Most of the above patch is just moving and renaming.
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))
Attachment #8689313 - Flags: review?(dglastonbury)
Attachment #8689313 - Flags: review?(Ms2ger)
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.
Attachment #8689313 - Flags: review?(Ms2ger) → review-
(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.
Alright, let's skip it to unblock this.
I can merge the web-platform tests after this.
Attachment #8689713 - Flags: review?(dglastonbury)
Attachment #8689313 - Attachment is obsolete: true
Attachment #8689313 - Flags: review?(dglastonbury)
So this test is actually in the tree (and run) twice.
Attachment #8689713 - Attachment is obsolete: true
Attachment #8689713 - Flags: review?(dglastonbury)
Attachment #8689756 - Flags: review?(dglastonbury)
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.
Attachment #8689757 - Flags: review?(dglastonbury)
Attachment #8686945 - Flags: review?(dglastonbury) → review+
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?
Attachment #8686946 - Flags: review?(dglastonbury) → review+
Attachment #8686947 - Flags: review?(dglastonbury) → review+
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?
Attachment #8686948 - Flags: review?(dglastonbury) → review+
Attachment #8686950 - Flags: review?(dglastonbury) → review+
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)
???
Attachment #8686951 - Flags: review?(dglastonbury) → review-
Attachment #8686952 - Flags: review?(dglastonbury) → review+
Attachment #8686953 - Flags: review?(dglastonbury) → review+
Attachment #8686954 - Flags: review?(dglastonbury) → review+
Attachment #8686955 - Flags: review?(dglastonbury) → review+
Attachment #8686956 - Flags: review?(dglastonbury) → review+
Attachment #8688802 - Flags: review?(dglastonbury) → review+
Attachment #8688803 - Flags: review?(dglastonbury) → review+
Attachment #8688804 - Flags: review?(dglastonbury) → review+
Attachment #8688806 - Flags: review?(dglastonbury) → review+
Attachment #8688808 - Flags: review?(dglastonbury) → review+
Attachment #8688809 - Flags: review?(dglastonbury) → review+
Attachment #8688812 - Flags: review?(dglastonbury) → review+
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
Attachment #8688814 - Flags: review?(dglastonbury) → review+
Attachment #8688815 - Flags: review?(dglastonbury) → review+
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?
Attachment #8689242 - Flags: review?(dglastonbury) → review+
Attachment #8689245 - Flags: review?(dglastonbury) → review+
Attachment #8689311 - Flags: review?(dglastonbury) → review+
Attachment #8689312 - Flags: review?(dglastonbury) → review+
Attachment #8689756 - Flags: review?(dglastonbury) → review+
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.
Attachment #8689757 - Flags: review?(dglastonbury) → review+
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.
Attachment #8688811 - Flags: review?(dglastonbury)
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.
Attachment #8688805 - Flags: review?(dglastonbury)
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.
Attachment #8688816 - Flags: review?(dglastonbury) → review-
(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 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.
Attachment #8688811 - Flags: review?(dglastonbury)
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.
Attachment #8686951 - Flags: review- → review?(dglastonbury)
Attachment #8686951 - Flags: review?(dglastonbury) → review+
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.
Attachment #8688811 - Flags: review?(dglastonbury) → review+
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.
Attachment #8688816 - Flags: review- → review?(dglastonbury)
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.
Attachment #8683981 - Flags: review- → review?(dglastonbury)
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.
Attachment #8688816 - Flags: review?(dglastonbury) → review+
Attachment #8683981 - Flags: review?(dglastonbury) → review+
Doesn't look like anything new so far:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=133ddbba3f01

Let's launch.
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!
Attachment #8683395 - Flags: review?(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?
Attachment #8683395 - Flags: review?(bzbarsky) → review+
(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].
> texImage3D no longer [Throws].

Looks fine, if the underlying implementation doesn't want to throw.
Attachment #8691669 - Flags: review?(dglastonbury)
Attachment #8691669 - Flags: review?(dglastonbury) → review+
Attachment #8691670 - Flags: review?(dglastonbury) → review+
Attachment #8691671 - Flags: review?(dglastonbury) → review+
https://hg.mozilla.org/mozilla-central/rev/7d1c223f397c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
57 explicit patches (even ignoring patches that were flattened!) and 816KB of diff later... :)

Explicit thanks to :kamidphish for the mountain of reviews!
Depends on: 1228683
Depends on: 1228687
Depends on: 1228885
Depends on: 1228886
Blocks: 1228885
No longer depends on: 1228885
Blocks: 1229944
Blocks: 1232346
Blocks: 1229585
Alias: webgl-tex-refactor
Blocks: 1233047
Depends on: 1233046
Blocks: 1233557
Depends on: 1247977
Depends on: 1261320
Depends on: 1376399
You need to log in before you can comment on or make changes to this bug.