Closed Bug 1236787 Opened 6 years ago Closed 6 years ago

[WebGL2] pass getInternalformatParameter in gl-object-get-calls.html

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: pchang, Assigned: daoshengmu)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

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
Blocks: 1236394
Whiteboard: [gfx-noted]
Assign daosheng to work on this.
Assignee: nobody → dmu
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/
Attachment #8757845 - Flags: review?(jgilbert) → review-
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/
Attachment #8757845 - Attachment description: MozReview Request: Bug 1236787 - Pass getInternalformatParameter in gl-object-get-calls.html (2.0); r?jgilbert → Bug 1236787 - Pass getInternalformatParameter in gl-object-get-calls.html (2.0);
Attachment #8757845 - Flags: review- → review?(jgilbert)
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.
Attachment #8757845 - Flags: review?(jgilbert) → review-
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?
Flags: needinfo?(jgilbert)
(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/
Attachment #8757845 - Flags: review- → review?(jgilbert)
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
Flags: needinfo?(jgilbert)
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.
Attachment #8757845 - Flags: review?(jgilbert) → review-
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/
Attachment #8757845 - Attachment description: Bug 1236787 - Pass getInternalformatParameter in gl-object-get-calls.html (2.0); → Bug 1236787 - Check internalformat to pass getInternalformatParameter in gl-object-get-calls.html (2.0);
Attachment #8757845 - Flags: review- → review?(jgilbert)
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())
Attachment #8757845 - Flags: review?(jgilbert) → review-
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;
    }
Flags: needinfo?(jgilbert)
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);
Flags: needinfo?(jgilbert)
(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.
Flags: needinfo?(jgilbert)
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.
Flags: needinfo?(jgilbert)
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/
Attachment #8757845 - Flags: review- → review?(jgilbert)
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.
Attachment #8757845 - Flags: review?(jgilbert) → review+
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/663bb8ffe934
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.