Closed Bug 1288351 Opened 4 years ago Closed 4 years ago

[WebGL2] hit assert with gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.DEPTH_ATTACHMENT, gl.FRAMEBUFFER_ATTACHMENT_COMPONENT_TYPE) call in gl-object-get-calls.html

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jerry, Assigned: jerry)

References

Details

Attachments

(2 files, 2 obsolete files)

Hit an assert for gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.DEPTH_ATTACHMENT, gl.FRAMEBUFFER_ATTACHMENT_COMPONENT_TYPE).

call stack:
#01: Assertion failure: false (Should never happen.), at /Users/bignose/work/mozilla/gecko/git/mozilla_central_desktop/dom/canvas/WebGLFramebuffer.cpp:564
#02: mozilla::WebGLFBAttachPoint::GetParameter(char const*, mozilla::WebGLContext*, JSContext*, unsigned int, unsigned int, unsigned int, mozilla::ErrorResult*) (WebGLFramebuffer.cpp:564, in XUL)
#03: mozilla::WebGLFramebuffer::GetAttachmentParameter(char const*, JSContext*, unsigned int, unsigned int, unsigned int, mozilla::ErrorResult*) (WebGLFramebuffer.cpp:1280, in XUL)
#04: mozilla::WebGLContext::GetFramebufferAttachmentParameter(JSContext*, unsigned int, unsigned int, unsigned int, mozilla::ErrorResult&) (WebGLContextGL.cpp:753, in XUL)
#05: mozilla::WebGL2Context::GetFramebufferAttachmentParameter(JSContext*, unsigned int, unsigned int, unsigned int, mozilla::ErrorResult&) (WebGL2ContextFramebuffers.cpp:368, in XUL)
#06: mozilla::WebGLContext::GetFramebufferAttachmentParameter(JSContext*, unsigned int, unsigned int, unsigned int, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) (WebGLContext.h:481, in XUL)
#07: mozilla::dom::WebGLRenderingContextBinding::getFramebufferAttachmentParameter(JSContext*, JS::Handle<JSObject*>, mozilla::WebGLContext*, JSJitMethodCallArgs const&) (WebGLRenderingContextBinding.cpp:13030, in XUL)
#08: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (BindingUtils.cpp:2771, in XUL)
#09: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:232, in XUL)
#10: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:453, in XUL)
#11: InternalCall(JSContext*, js::AnyInvokeArgs const&) (Interpreter.cpp:498, in XUL)
#12: js::CallFromStack(JSContext*, JS::CallArgs const&) (Interpreter.cpp:504, in XUL)
#13: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2873, in XUL)
#14: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:399, in XUL)
#15: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) (Interpreter.cpp:679, in XUL)
#16: EvalKernel(JSContext*, JS::Handle<JS::Value>, EvalType, js::AbstractFramePtr, JS::Handle<JSObject*>, unsigned char*, JS::MutableHandle<JS::Value>) (Eval.cpp:333, in XUL)
#17: js::DirectEval(JSContext*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) (Eval.cpp:451, in XUL)
In gles 3.04 p.146 table 3.14
https://www.khronos.org/registry/gles/specs/3.0/es_spec_3.0.4.pdf

It says that the d bit of d24s8 format is 24.
When we query the component type for depth_attachment for d24s8 format texture, we got Special(DEPTH24_STENCIL8) component type.

enum class ComponentType : uint8_t {
    None,
    Int,          // RGBA32I
    UInt,         // RGBA32UI, STENCIL_INDEX8
    NormInt,      // RGBA8_SNORM
    NormUInt,     // RGBA8, DEPTH_COMPONENT16
    Float,        // RGBA32F
    Special,      // DEPTH24_STENCIL8
};

Which type should we return for this 24bits type?
gl.INT or gl.SIGNED_NORMALIZED?

And where is the spec for the component type with 1bit, 5bit, 3, 5, 10, 24(for some special format GL_R3_G3_B2, GL_RGB5_A1, GL_RGB10_A2.. etc)?
Flags: needinfo?(jgilbert)
Forget the 1, 2, 3, 5 and 10 bit question. We only have:
DEPTH_COMPONENT16
DEPTH_COMPONENT24
DEPTH_COMPONENT32F
DEPTH24_STENCIL8
DEPTH32F_STENCIL8
Try to handle the ComponentType::Special[1] format case for FRAMEBUFFER_ATTACHMENT_COMPONENT_TYPE query.

[1]
https://dxr.mozilla.org/mozilla-central/search?q=ComponentType%3A%3ASpecial&redirect=false
Attachment #8773346 - Flags: review?(jgilbert)
Flags: needinfo?(jgilbert)
Comment on attachment 8773346 [details] [diff] [review]
handle gl.getFramebufferAttachmentParameter() FRAMEBUFFER_ATTACHMENT_COMPONENT_TYPE query for DS format. v1

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

Please check [1].
We set ComponentType::Special for d24s8 and d32fs8, but there is no implementation for ComponentType::Special type.

[1]
https://dxr.mozilla.org/mozilla-central/search?q=%22ComponentType%3A%3ASpecial%22&redirect=false

::: dom/canvas/WebGLFramebuffer.cpp
@@ +572,5 @@
> +                case 16:
> +                    ret = LOCAL_GL_UNSIGNED_NORMALIZED;
> +                    break;
> +                case 24:
> +                    ret = LOCAL_GL_UNSIGNED_NORMALIZED;

I'm not sure the d16 and d24 should be SIGNED_NORMALIZED, UNSIGNED_NORMALIZED or uint/int.
Summary: [WebGL2] pass gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.DEPTH_ATTACHMENT, gl.FRAMEBUFFER_ATTACHMENT_COMPONENT_TYPE) in gl-object-get-calls.html → [WebGL2] hit assert with gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.DEPTH_ATTACHMENT, gl.FRAMEBUFFER_ATTACHMENT_COMPONENT_TYPE) call in gl-object-get-calls.html
Comment on attachment 8773346 [details] [diff] [review]
handle gl.getFramebufferAttachmentParameter() FRAMEBUFFER_ATTACHMENT_COMPONENT_TYPE query for DS format. v1

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

Let's reuse our format tables.

For DEPTH24_STENCIL8, query DEPTH_COMPONENT24 for depth, and STENCIL_INDEX8 for stencil.
Branch similarly for DEPTH32F_STENCIL8.
Attachment #8773346 - Flags: review?(jgilbert) → review-
Attachment #8773346 - Attachment is obsolete: true
Comment on attachment 8773373 [details] [diff] [review]
handle gl.getFramebufferAttachmentParameter() FRAMEBUFFER_ATTACHMENT_COMPONENT_TYPE query for DS format. v2

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

::: dom/canvas/WebGLFramebuffer.cpp
@@ +618,3 @@
>          }
>          break;
> +    }

I'm not sure the indent of the switch case braces pair.
There is a variable "const webgl::FormatInfo* formatInfo" in this switch case.
Comment on attachment 8773373 [details] [diff] [review]
handle gl.getFramebufferAttachmentParameter() FRAMEBUFFER_ATTACHMENT_COMPONENT_TYPE query for DS format. v2

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

::: dom/canvas/WebGLFramebuffer.cpp
@@ +618,3 @@
>          }
>          break;
> +    }

The {} block starts at the level of the non-case statements, so this whole block would need to be indented.

Just do it without the local:
if (attachment == LOCAL_GL_DEPTH_ATTACHMENT) {
    switch (format->effectiveFormat) {
    case EffectiveFormat::DEPTH24_STENCIL8:
        format = webgl::GetFormat(EffectiveFormat::DEPTH_COMPONENT24);
        break;
Attachment #8773373 - Flags: review?(jgilbert) → review+
Attachment #8773373 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a68a82726062
Handle gl.getFramebufferAttachmentParameter() FRAMEBUFFER_ATTACHMENT_COMPONENT_TYPE query for DS format. r=jgilbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a68a82726062
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.