Test getInternalformatParameter PASS getError was expected value: NO_ERROR : PASS gl.getInternalformatParameter(gl.RENDERBUFFER, gl.R32I, gl.SAMPLES) is non-null. PASS getError was expected value: NO_ERROR : FAIL getInternalformatParameter returned undefined instead of null for invalid target enum: NO_ERROR FAIL getInternalformatParameter returned undefined instead of null for invalid pname enum: NO_ERROR FAIL getInternalformatParameter returned [object Int32Array] instead of null for invalid internalformat enum: NO_ERROR

Assign daosheng to work on this.

Review commit: https://reviewboard.mozilla.org/r/56234/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56234/

Comment on attachment 8757845 [details] Bug 1236787 - Check internalformat to pass getInternalformatParameter in gl-object-get-calls.html (2.0); Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56234/diff/1-2/

Comment on attachment 8757845 [details] Bug 1236787 - Check internalformat to pass getInternalformatParameter in gl-object-get-calls.html (2.0); https://reviewboard.mozilla.org/r/56234/#review56838 ::: dom/canvas/WebGL2ContextRenderbuffers.cpp:15 (Diff revision 2) > #include "WebGLContextUtils.h" > > namespace mozilla { > > +static bool > +ValidateInternalFormat(GLenum internalformat) { webgl::FormatUsageInfo::isRenderable should cover this.

Comment on attachment 8757845 [details] Bug 1236787 - Check internalformat to pass getInternalformatParameter in gl-object-get-calls.html (2.0); Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56234/diff/2-3/

https://reviewboard.mozilla.org/r/56234/#review56838 > webgl::FormatUsageInfo::isRenderable should cover this. I replace it with checking SizedTexUsage and UnsizedTexUsage FileUsageInfo. Please help me review again.

Comment on attachment 8757845 [details] Bug 1236787 - Check internalformat to pass getInternalformatParameter in gl-object-get-calls.html (2.0); https://reviewboard.mozilla.org/r/56234/#review57750 ::: dom/canvas/WebGL2ContextRenderbuffers.cpp:27 (Diff revision 3) > return; > > if (target != LOCAL_GL_RENDERBUFFER) { > ErrorInvalidEnum("%s: `target` must be RENDERBUFFER, was: 0x%04x.", funcName, > target); > + retval.setObjectOrNull(nullptr); Just move this to the top of the function, after funcName, but before IsContextLost(). ::: dom/canvas/WebGL2ContextRenderbuffers.cpp:36 (Diff revision 3) > - // GL_INVALID_ENUM is generated if internalformat is not color-, depth-, or > - // stencil-renderable. > - // TODO: When format table queries lands. > + const webgl::FormatUsageInfo* sizedFormatInfo = mFormatUsage->GetSizedTexUsage(internalformat); > + webgl::PackingInfo pi = {internalformat, 0}; > + const webgl::FormatUsageInfo* unsizedFormatInfo = mFormatUsage->GetUnsizedTexUsage(pi); > + if ((!sizedFormatInfo && !unsizedFormatInfo) || > + (sizedFormatInfo && !sizedFormatInfo->isRenderable) || > + (unsizedFormatInfo && !unsizedFormatInfo->isRenderable)) { const auto* usage = mFormatUsage->GetRBUsage(internalformat); if (!usage) { ErrorInvalidEnum(...

Jeff, I have tried to use mFormatUsage->GetRBUsage(internalformat) to check the internalformat and it can cover most cases but LOCAL_GL_DEPTH_STENCIL. I know LOCAL_GL_DEPTH_STENCIL is only used in WebGL1. In WebGL2, it should be LOCAL_GL_DEPTH24_STENCIL8. Therefore, I modify LOCAL_GL_DEPTH_STENCIL to LOCAL_GL_DEPTH24_STENCIL8 at https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLFormats.cpp#846. It is good in Windows, but in Mac OS X, it will affect the mochitest of test_fuzzing_bugs.html and crash at ptr->AllowRBFormat() because LOCAL_GL_DEPTH24_STENCIL8 is not inserted before. Do you think I should write like this at WebGL2Context::GetInternalformatParameter: const auto* usage = mFormatUsage->GetRBUsage(internalformat); if (!usage || internalformat==LOCAL_GL_DEPTH_STENCIL) Or, you have any better suggestion?

(In reply to Daosheng Mu[:daoshengmu] from comment #8) > Jeff, I have tried to use mFormatUsage->GetRBUsage(internalformat) to check > the internalformat and it can cover most cases but LOCAL_GL_DEPTH_STENCIL. I > know LOCAL_GL_DEPTH_STENCIL is only used in WebGL1. In WebGL2, it should be > LOCAL_GL_DEPTH24_STENCIL8. Therefore, I modify LOCAL_GL_DEPTH_STENCIL to > LOCAL_GL_DEPTH24_STENCIL8 at > https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLFormats. > cpp#846. It is good in Windows, but in Mac OS X, it will affect the > mochitest of test_fuzzing_bugs.html and crash at ptr->AllowRBFormat() > because LOCAL_GL_DEPTH24_STENCIL8 is not inserted before. Do you think I > should write like this at WebGL2Context::GetInternalformatParameter: > const auto* usage = mFormatUsage->GetRBUsage(internalformat); > if (!usage || internalformat==LOCAL_GL_DEPTH_STENCIL) > > Or, you have any better suggestion? Correcting my comment. It will trigger assert on https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLFormats.cpp#846 in Windows as well. Because LOCAL_GL_DEPTH24_STENCIL8 has been inserted to mRBFormatMap before. So, the real question is why we add LOCAL_GL_DEPTH_STENCIL to mRBFormatMap? It is not a valid internalformat in WebGL2. We should remove it at https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLFormats.cpp?q=webglformats.cpp&redirect_type=direct#846

Comment on attachment 8757845 [details] Bug 1236787 - Check internalformat to pass getInternalformatParameter in gl-object-get-calls.html (2.0); Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56234/diff/3-4/

https://reviewboard.mozilla.org/r/56234/#review57750 > Just move this to the top of the function, after funcName, but before IsContextLost(). Done. > const auto* usage = mFormatUsage->GetRBUsage(internalformat); > if (!usage) { > ErrorInvalidEnum(... LOCAL_GL_DEPTH_STENCIL should not be added into mRBFormatMap, and it would happen internalformat checking error. Therefore, I remove it from https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLFormats.cpp?q=%2Bfunction%3A%22mozilla%3A%3Awebgl%3A%3AAlwaysInsert%28std%3A%3Amap%3CK%2C+V%3E+%26%2C+const+K2+%26%2C+const+V2+%26%29%22&redirect_type=single#846.

Try looks good as well, https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ec360200d5c

Comment on attachment 8757845 [details] Bug 1236787 - Check internalformat to pass getInternalformatParameter in gl-object-get-calls.html (2.0); https://reviewboard.mozilla.org/r/56234/#review60498 ::: dom/canvas/WebGL2ContextRenderbuffers.cpp:41 (Diff revision 4) > if (pname != LOCAL_GL_SAMPLES) { > ErrorInvalidEnumInfo("%s: `pname` must be SAMPLES, was 0x%04x.", funcName, pname); > return; > } > > + const auto* usage = mFormatUsage->GetRBUsage(internalformat); GetSizedTexUsage ::: dom/canvas/WebGL2ContextRenderbuffers.cpp:43 (Diff revision 4) > return; > } > > + const auto* usage = mFormatUsage->GetRBUsage(internalformat); > + if (!usage) { > + ErrorInvalidEnumInfo("%s: `pname` must be SAMPLES, was 0x%04x.", funcName, pname); "%s: Unrecognized `internalformat`: 0x%04x" ::: dom/canvas/WebGLFormats.cpp (Diff revision 4) > > if (!AddUnsizedFormats(ptr, gl)) > return nullptr; > > - ptr->AllowRBFormat(LOCAL_GL_DEPTH_STENCIL, > - ptr->GetUsage(EffectiveFormat::DEPTH24_STENCIL8)); I believe we need to keep this.

Comment on attachment 8757845 [details] Bug 1236787 - Check internalformat to pass getInternalformatParameter in gl-object-get-calls.html (2.0); Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56234/diff/4-5/

Try result is good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5b872d5ea42

Comment on attachment 8757845 [details] Bug 1236787 - Check internalformat to pass getInternalformatParameter in gl-object-get-calls.html (2.0); https://reviewboard.mozilla.org/r/56234/#review60770 Almost! ::: dom/canvas/WebGL2ContextRenderbuffers.cpp:41 (Diff revision 5) > if (pname != LOCAL_GL_SAMPLES) { > ErrorInvalidEnumInfo("%s: `pname` must be SAMPLES, was 0x%04x.", funcName, pname); > return; > } > > + const auto* usage = mFormatUsage->GetSizedTexUsage(internalformat); `const auto usage` is actually preferable. ::: dom/canvas/WebGL2ContextRenderbuffers.cpp:42 (Diff revision 5) > ErrorInvalidEnumInfo("%s: `pname` must be SAMPLES, was 0x%04x.", funcName, pname); > return; > } > > + const auto* usage = mFormatUsage->GetSizedTexUsage(internalformat); > + if (!usage) { if (!usage || !usage->IsRenderable())

Hi, Jeff. I think we just check unsized internal Formats is not enough. Because renderable texture is not only sized format but also unsized texture. Please take a look at here, https://www.khronos.org/opengles/sdk/docs/man3/html/glTexImage2D.xhtml. Therefore, we can find this conformance test give the validate formats, https://github.com/KhronosGroup/WebGL/blob/bf1df1fb9192e0a1045ca4c05577dc8ec0962891/sdk/tests/js/tests/gl-object-get-calls.js#L753. Also, Chromium checks internalformat like this way, https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp?q=getInternalformatParameter&sq=package:chromium&dr=CSs&l=345. This is what my revision 1 and revision 3 implement. So, my idea is checking internalformat like below, and I have confirmed it will resolve this issue in Windows and OSX. const auto sizedFormatInfo = mFormatUsage->GetSizedTexUsage(internalformat); webgl::PackingInfo pi = { internalformat, 0 }; const auto unsizedFormatInfo = mFormatUsage->GetUnsizedTexUsage(pi); if ((!sizedFormatInfo && !unsizedFormatInfo) || (sizedFormatInfo && !sizedFormatInfo->IsRenderable()) || (unsizedFormatInfo && !unsizedFormatInfo->IsRenderable())) { ErrorInvalidEnum("%s: `internalformat` must be color-, depth-, or stencil-renderable, was: 0x%04x.", funcName, internalformat); return; }

Ahhh, I was right the first time. We should be using GetRBUsage, because currently the function only accepts RENDERBUFFER for `target`. We should probably include the spec text, since this has already been confusing: GLES 3.0.4 p243: "`internalformat` must be color-renderable, depth-renderable or stencil-renderable (as defined in section 4.4.4)." "`target` indicates the usage of the `internalformat`, and must be RENDERBUFFER" I recommend: // GLES 3.0.4 $4.4.4 p212: // "An internal format is color-renderable if it is one of the formats from table 3.13 // noted as color-renderable or if it is unsized format RGBA or RGB." GLenum sizedFormat; switch (internalformat) { case LOCAL_GL_RGB: sizedFormat = LOCAL_GL_RGB8; break; case LOCAL_GL_RGBA: sizedFormat = LOCAL_GL_RGBA8; break; default: sizedFormat = internalformat; break; } const auto usage = mFormatUsage->GetRBUsage(sizedFormat);

(In reply to Jeff Gilbert [:jgilbert] from comment #18) > Ahhh, I was right the first time. We should be using GetRBUsage, because > currently the function only accepts RENDERBUFFER for `target`. > We should probably include the spec text, since this has already been > confusing: > GLES 3.0.4 p243: > "`internalformat` must be color-renderable, depth-renderable or > stencil-renderable (as defined in section 4.4.4)." > "`target` indicates the usage of the `internalformat`, and must be > RENDERBUFFER" > > I recommend: > > // GLES 3.0.4 $4.4.4 p212: > // "An internal format is color-renderable if it is one of the formats from > table 3.13 > // noted as color-renderable or if it is unsized format RGBA or RGB." > > GLenum sizedFormat; > switch (internalformat) { > case LOCAL_GL_RGB: > sizedFormat = LOCAL_GL_RGB8; > break; > case LOCAL_GL_RGBA: > sizedFormat = LOCAL_GL_RGBA8; > break; > default: > sizedFormat = internalformat; > break; > } > > const auto usage = mFormatUsage->GetRBUsage(sizedFormat); I think using GetRBUsage is almost right. The last thing we need to consider is > > if (!AddUnsizedFormats(ptr, gl)) > return nullptr; > > - ptr->AllowRBFormat(LOCAL_GL_DEPTH_STENCIL, > - ptr->GetUsage(EffectiveFormat::DEPTH24_STENCIL8)); Should we remove 'LOCAL_GL_DEPTH_STENCIL' or move it to "if (gfxPrefs::WebGL2CompatMode()) { }" ? Because 'DEPTH_STENCIL' is a legacy format for WebGL2 and is neither sized nor unsized internal format.

Leave that AllowRBFormat there, if there's no reason to remove it, as it covers us in this case. I believe that since we allow DEPTH_STENCIL for RenderbufferStorage, we should also accept it for GetInternalformatParameter. If the test forbids this, we should propose a change to the test.

Comment on attachment 8757845 [details] Bug 1236787 - Check internalformat to pass getInternalformatParameter in gl-object-get-calls.html (2.0); Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56234/diff/5-6/

https://reviewboard.mozilla.org/r/56234/#review60770 > if (!usage || !usage->IsRenderable()) If we already decide to use mFormatUsage->GetRBUsage(), !usage->IsRenderable() should be redundant?

Comment on attachment 8757845 [details] Bug 1236787 - Check internalformat to pass getInternalformatParameter in gl-object-get-calls.html (2.0); https://reviewboard.mozilla.org/r/56234/#review61196 Yep, you're right. Being an RB format implies it's renderable.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03a93afb02a9. Try result is good. Please land to m-c

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/663bb8ffe934 Check internalformat to pass getInternalformatParameter in gl-object-get-calls.html (2.0); r=jgilbert