WebGLContextFramebuffers.cpp contains a couple of MOZ_CRASH("Not Implemented."): void WebGL2Context::FramebufferTextureLayer(...) void WebGL2Context::GetInternalformatParameter(...)

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

Comment on attachment 8622916 [details] [diff] [review] Part 1: Sort out ARB_framebuffer_object symbol queries. Review of attachment 8622916 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContext.cpp @@ +780,2 @@ > > + bool ARB_framebuffer_object_supported = true; Please don't start local vars with caps. `hasARBFramebufferObject` should be fine. ::: gfx/gl/GLContext.h @@ +446,2 @@ > NV_half_float, > + NV_framebuffer_blit, Keep this alphabetized! ::: gfx/gl/GLContextFeatures.cpp @@ +813,5 @@ > void > +GLContext::MarkSupported(GLFeature feature) > +{ > + mAvailableFeatures[size_t(feature)] = true; > + // TODO: Should this enable the extension too? No, extensions should stay as is.

Comment on attachment 8622918 [details] [diff] [review] Part 2: Implement FramebufferTextureLayer. Review of attachment 8622918 [details] [diff] [review]: ----------------------------------------------------------------- I'm concerned about the -1 handling. Otherwise just nits. ::: dom/canvas/WebGL2ContextFramebuffers.cpp @@ +332,5 @@ > +static bool > +ValidateTextureLayerAttachment(GLenum attachment) > +{ > + if (LOCAL_GL_COLOR_ATTACHMENT0 < attachment && > + attachment <= LOCAL_GL_COLOR_ATTACHMENT15) I guess? Really it needs to be less than the queryable max color attachment limit. @@ +386,5 @@ > + break; > + > + default: > + return ErrorInvalidOperation("framebufferTextureLayer: texture must be an " > + "existing 3D texture, or a 2D texture array."); This is disappointing to me, but is what the spec says. ::: dom/canvas/WebGL2ContextRenderbuffers.cpp @@ +21,5 @@ > + return; > + > + if (target != LOCAL_GL_RENDERBUFFER) { > + return ErrorInvalidEnumInfo("getInternalfomratParameter: target must be " > + "RENDERBUFFER. Was:", target); I'd really like to stop returning void func calls into void. @@ +36,5 @@ > + > + GLint* samples = nullptr; > + GLint sampleCount = 0; > + gl->fGetInternalformativ(LOCAL_GL_RENDERBUFFER, internalformat, > + LOCAL_GL_NUM_SAMPLE_COUNTS, 1, &sampleCount); We should probably just return The Right Thing without asking the driver. ::: dom/canvas/WebGLContext.h @@ +1171,5 @@ > int32_t mGLMaxDrawBuffers; > uint32_t mGLMaxTransformFeedbackSeparateAttribs; > GLuint mGLMaxUniformBufferBindings; > GLsizei mGLMaxSamples; > + GLint mGLMax3DTextureSize; Limits should really be unsigned. ::: dom/canvas/WebGLFramebuffer.cpp @@ +20,5 @@ > : mFB(fb) > , mAttachmentPoint(attachmentPoint) > , mTexImageTarget(LOCAL_GL_NONE) > + , mTexImageLayer(-1) > + , mTexImageLevel(-1) Just leave these as undefined. We should only consider them defined when mTexturePtr is truthy, and completely ignore them otherwise. @@ +133,5 @@ > mTexturePtr = tex; > mRenderbufferPtr = nullptr; > mTexImageTarget = target; > mTexImageLevel = level; > + mTexImageLayer = -1; Shouldn't this default to 0? And shouldn't this func really just call into SetTexImageLayer below, being a subset of it? @@ +166,5 @@ > UnmarkAttachment(*this); > > mTexturePtr = nullptr; > mRenderbufferPtr = rb; > + mTexImageTarget = LOCAL_GL_NONE; Don't bother clearing these here.

Comment on attachment 8624002 [details] [diff] [review] Part 1: Sort out ARB_framebuffer_object symbol queries. Review of attachment 8624002 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContext.cpp @@ +57,5 @@ > > > #define MAX_SYMBOL_LENGTH 128 > #define MAX_SYMBOL_NAMES 5 > +#define _STR(x) #x Not really necessary. @@ +839,5 @@ > + if (IsExtensionSupported(GLContext::ARB_geometry_shader4) || > + IsExtensionSupported(GLContext::NV_geometry_program4)) > + { > + SymLoadStruct extSymbols[] = { > + EXT_SYMBOL2(FramebufferTextureLayer, ARB, EXT), Huh, the NV ext exposes EXT suffixed funcs? ::: gfx/gl/GLContextFeatures.cpp @@ +825,5 @@ > void > +GLContext::MarkSupported(GLFeature feature) > +{ > + mAvailableFeatures[size_t(feature)] = true; > + // TODO: Should this enable the extension too? I don't think it should.

Comment on attachment 8624003 [details] [diff] [review] Part 2: Implement FramebufferTextureLayer. Review of attachment 8624003 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGL2ContextFramebuffers.cpp @@ +378,5 @@ > + } > + break; > + > + case LOCAL_GL_TEXTURE_2D_ARRAY: > + if ((GLuint) layer >= mGLMaxArrayTextureLayers) { Do we delay checking against the actual TexImage until the FB completeness check? @@ +397,5 @@ > + WebGLFramebuffer* fb; > + switch (target) { > + case LOCAL_GL_FRAMEBUFFER: > + case LOCAL_GL_DRAW_FRAMEBUFFER: > + fb = mBoundDrawFramebuffer; We should really make this stuff an out-var from the target validation function. ::: dom/canvas/WebGL2ContextRenderbuffers.cpp @@ +48,5 @@ > + if (!obj) { > + rv = NS_ERROR_OUT_OF_MEMORY; > + } > + > + delete[] samples; Just use a UniquePtr.

Comment on attachment 8624003 [details] [diff] [review] Part 2: Implement FramebufferTextureLayer. Review of attachment 8624003 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, we need to update our FB attachment completeness.

(In reply to Jeff Gilbert [:jgilbert] from comment #12) > Comment on attachment 8624002 [details] [diff] [review] > ::: gfx/gl/GLContext.cpp > > +#define _STR(x) #x > > Not really necessary. All the macros aren't, but I'd like to clean up all the struct setting. Is there a standard "stringify" macro? Otherwise, it's needed to get the macro turned into a string to then concatenate... Unless... I use string pasting, like #glname #suffix. Not sure if that works.

(In reply to Jeff Gilbert [:jgilbert] from comment #12) > Huh, the NV ext exposes EXT suffixed funcs? Yeeeeep. https://www.opengl.org/registry/specs/NV/geometry_program4.txt

(In reply to Jeff Gilbert [:jgilbert] from comment #14) > Comment on attachment 8624003 [details] [diff] [review] > Part 2: Implement FramebufferTextureLayer. > > Review of attachment 8624003 [details] [diff] [review]: > ----------------------------------------------------------------- > > Yeah, we need to update our FB attachment completeness. OK, we need talk about this. I've been staring that the completeness check code and I'm not sure exactly what needs to be updated.

Spoke with :jgilbert. Going to land this so we can then get Jeff's texture refactoring landed, which can then be used to make texture layers more robust. Green try run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1674d61a0de

Comment on attachment 8622889 [details] [diff] [review] Part 4: Implement GetInternalformatParameter. Olli, I moved some webidl function and added [throw] to getInternalformatParameter because the function allocates an Int32Array to return the result, which can fail with NS_ERROR_OUT_OF_MEMORY.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a3a00830a33 https://hg.mozilla.org/integration/mozilla-inbound/rev/521350724620 https://hg.mozilla.org/integration/mozilla-inbound/rev/0310a2141d95 https://hg.mozilla.org/integration/mozilla-inbound/rev/4a61055fc72d

https://hg.mozilla.org/mozilla-central/rev/0a3a00830a33 https://hg.mozilla.org/mozilla-central/rev/521350724620 https://hg.mozilla.org/mozilla-central/rev/0310a2141d95 https://hg.mozilla.org/mozilla-central/rev/4a61055fc72d