1193070 - WebGL 2 - Implement getFramebufferAttachmentParameter

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Comment 1

[u480271]

Attachment: Override GetFramebufferAttachmentParameter for WebGL2.

Preparation for implementation.

Comment 2

[u480271]

Attachment: Add color sizes to FormatInfo.

This makes implementing GetFramebufferAttachmentParameter easier.

If needed, hasColor, hasDepth, etc can be implemented as helper
functions that check the responsible field for > 0.

Comment 3

[u480271]

Attachment: Implement getFramebufferAttachmentParamter.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8646026 [details] [diff] [review]
Implement getFramebufferAttachmentParamter.

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

::: dom/canvas/WebGL2ContextFramebuffers.cpp
@@ +28,5 @@
> +    if (fba) {
> +        format = fba->EffectiveInternalFormat().get();
> +    } else {
> +        const GLFormats& formats = gl->GetGLFormats();
> +        switch (component) {

MOZ_CRASH if we don't recognize `component`.

@@ +47,5 @@
> +        }
> +    }
> +
> +    MOZ_ASSERT(format != LOCAL_GL_NONE);
> +    return webgl::GetInfoBySizedFormat(format);

I don't think this will work in the general case here. I don't have confidence in our EffectiveInternalFormats.

@@ +469,5 @@
> +
> +    /* OpenGL ES 3.0.4 (August 27, 2014) 6.1. QUERYING GL STATE 240
> +       getFramebufferAttachmentParamter returns information about attachments of a bound
> +       framebuffer object. target must be DRAW_FRAMEBUFFER, READ_FRAMEBUFFER, or
> +       FRAMEBUFFER. */

/* */ is useful for long comments, but I think we should use `//` anyways. /**/ comments make it excruciating to comment large blocks of code out.

Also, if it's a quote from the spec, put it in double-quotes!

@@ +480,5 @@
> +
> +    WebGLFramebuffer* boundFB = nullptr;
> +    switch (target) {
> +    case LOCAL_GL_DRAW_FRAMEBUFFER: boundFB = mBoundDrawFramebuffer; break;
> +    case LOCAL_GL_READ_FRAMEBUFFER: boundFB = mBoundReadFramebuffer; break;

About here IMO you should forward the call to WebGLFramebuffer::GetFramebufferAttachmentParameter, if we're not referencing the default FB.

The default FB can be handled in this function fairly simply.

@@ +518,5 @@
> +        case LOCAL_GL_DEPTH_ATTACHMENT:
> +        case LOCAL_GL_DEPTH_STENCIL_ATTACHMENT:
> +            break;
> +
> +        case LOCAL_GL_STENCIL_ATTACHMENT:

This should switch places with LOCAL_GL_DEPTH_STENCIL_ATTACHMENT above.

@@ +523,5 @@
> +                /* If attachment is DEPTH_STENCIL_ATTACHMENT, and different objects are bound to
> +                 the depth and stencil attachment points of target, the query will fail and
> +                 generate an INVALID_OPERATION error. If the same object is bound to both
> +                 attachment points, information about that object will be returned. */
> +                if (boundFB->DepthAttachment() != boundFB->StencilAttachment()) {

If this is the only user of operator!=, I would just dump the logic in here.

@@ +561,5 @@
> +    /* If the value of FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE is NONE, no framebuffer is bound
> +       to target. In this case querying pname FRAMEBUFFER_ATTACHMENT_OBJECT_NAME will
> +       return zero, and all other queries will generate an INVALID_OPERATION error. */
> +    if (objectType == LOCAL_GL_NONE) {
> +        // Here I convert name zero to mean JS null.

s/I convert/WebGL converts/

@@ +577,5 @@
> +    /* If the value of FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE is not NONE, these queries apply
> +       to all other framebuffer types: */
> +    if (objectType != LOCAL_GL_NONE) {
> +        switch (pname) {
> +        /* • If pname is FRAMEBUFFER_ATTACHMENT_RED_SIZE,

Please stick to * instead of non-trivially-typeable chars.

@@ +583,5 @@
> +             FRAMEBUFFER_ATTACHMENT_ALPHA_SIZE, FRAMEBUFFER_ATTACHMENT_DEPTH_SIZE, or
> +             FRAMEBUFFER_ATTACHMENT_STENCIL_SIZE, then params will contain the number of
> +             bits in the corresponding red, green, blue, alpha, depth, or stencil
> +             component of the specified attachment. Zero is returned if the requested
> +             component is not present in attachment. */

Pretty sure we don't need this obvious bit of spec.

@@ +615,5 @@
> +                    ErrorInvalidOperation("getFramebufferAttachmentParameter:");
> +                    return JS::NullValue();
> +                }
> +
> +                int result = 0;

Move result's decl closer to where it's set.

@@ +616,5 @@
> +                    return JS::NullValue();
> +                }
> +
> +                int result = 0;
> +                const FormatInfo* info = FormatInfoForAttachment(gl, fba, pname);

FormatInfoForAttachment is not equiped to handle a pname of LOCAL_GL_FRAMEBUFFER_ATTACHMENT_COMPONENT_TYPE.

@@ +638,5 @@
> +             the attachment, params will contain the value LINEAR. */
> +        case LOCAL_GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING:
> +            {
> +                // TODO: This implementation reports default framebuffer linear. Is that
> +                // true?

Yes it is.

@@ +645,5 @@
> +                    const WebGLFBAttachPoint& fba = boundFB->GetAttachPoint(attachment);
> +                    const FormatInfo* info = webgl::GetInfoBySizedFormat(
> +                        fba.EffectiveInternalFormat().get());
> +
> +                    if (fba.IsDefined() && info) {

If !fba.IsDefined(), fba.EffectiveInternalFormat() isn't safe to call.

@@ +671,5 @@
> +            const WebGLFBAttachPoint& fba = boundFB->GetAttachPoint(attachment);
> +            const WebGLTexture* tex = fba.Texture();
> +
> +            /* • If pname is FRAMEBUFFER_ATTACHMENT_OBJECT_NAME, then params will contain
> +                 the name of the texture object which contains the attached image. */

This level of spec-quoting seems unnecessary. Just use a switch, and leave a reference to where in the spec the exact text is.

It's hard to read the code through all the comments.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +31,5 @@
>  bool
> +WebGLFBAttachPoint::operator==(const WebGLFBAttachPoint& rhs) const
> +{
> +    if (Texture())
> +        return Texture() == rhs.Texture();

This isn't true. It has to match levels too.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8646025 [details] [diff] [review]
Add color sizes to FormatInfo.

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

::: dom/canvas/WebGLFormats.cpp
@@ +14,5 @@
>  //////////////////////////////////////////////////////////////////////////////////////////
>  
> +
> +GLint
> +ComponentSize(const FormatInfo* info, GLenum comp)

I wouldn't bother with a function for this, and would just put it in the calling code.

@@ +16,5 @@
> +
> +GLint
> +ComponentSize(const FormatInfo* info, GLenum comp)
> +{
> +    MOZ_ASSERT(info);

MOZ_RELEASE_ASSERT

@@ +19,5 @@
> +{
> +    MOZ_ASSERT(info);
> +    GLint size = 0;
> +
> +    if (info) {

Don't bother with this.

@@ +60,5 @@
> +ColorEncoding(const FormatInfo* info)
> +{
> +    MOZ_ASSERT(info);
> +    GLenum encoding = LOCAL_GL_LINEAR;
> +    if (info) {

Don't tolerate null infos.

@@ +66,5 @@
> +        case EffectiveFormat::SRGB8:
> +        case EffectiveFormat::SRGB8_ALPHA8:
> +        case EffectiveFormat::COMPRESSED_SRGB8_ETC2:
> +        case EffectiveFormat::COMPRESSED_SRGB8_ALPHA8_ETC2_EAC:
> +        case EffectiveFormat::COMPRESSED_SRGB8_PUNCHTHROUGH_ALPHA1_ETC2:

The main thing I don't like about this is that it breaks the "It's all in the tables" idea. I like this idea, and think we should keep it.

Even embedding this logic in AddFormatInfo() would be better, I think.

@@ -114,5 @@
> -    bool hasAlpha = false;
> -    bool hasDepth = false;
> -    bool hasStencil = false;
> -
> -    switch (unsizedFormat) {

Keep this switch, and assert that it matches the presence of bits for the components.
Such as:
case UnsizedFormat::D:
  MOZ_ASSERT(!isColorFormat&& !stencilSize);
  MOZ_ASSERT(depthSize);

And have above:
const bool isColorFormat = (redSize || greenSize || blueSize || alphaSize || luminanceSize);

@@ +180,5 @@
>      MOZ_ASSERT(!bytesPerPixel == bool(compressedFormatInfo));
>  
> +    const FormatInfo info = { format, name, baseFormat, dataType, bytesPerPixel,
> +                              redSize, greenSize, blueSize, alphaSize,
> +                              luminanceSize, depthSize, stencilSize,

We want to keep an `isColorFormat` which = (redSize || greenSize || blueSize || alphaSize || luminanceSize).

@@ +212,3 @@
>  
>      // GLES 3.0.4, p130-132, table 3.13
> +    AddFormatInfo(FOO(R8            ), _(RED ), _(UNSIGNED_NORMALIZED),  1, _R    (8));

It's tidier to keep `bytesPerTexel` before RED and UNSIGNED_NORMALIZED.
Otherwise, it may also be tempting to assume that bytesPerTexel was related to the unsized format and componentType, which it's looser than that.

@@ +239,3 @@
>  
> +    AddFormatInfo(FOO(R11F_G11F_B10F), _(RGB ), _(FLOAT),  4, _R_G_B(11,11,10));
> +    AddFormatInfo(FOO(RGB9_E5       ), _(RGB ), _(FLOAT),  4, _RGB(9));

RGB9E5 is closest to _RGB(14), though the exact bit depths can't be queried via the WebGL API.

@@ +272,5 @@
> +    AddFormatInfo(FOO(DEPTH_COMPONENT16 ), _(DEPTH_COMPONENT), _(UNSIGNED_NORMALIZED), 2, _D (16));
> +    AddFormatInfo(FOO(DEPTH_COMPONENT24 ), _(DEPTH_COMPONENT), _(UNSIGNED_NORMALIZED), 3, _D (24));
> +    AddFormatInfo(FOO(DEPTH_COMPONENT32F), _(DEPTH_COMPONENT), _(FLOAT),               4, _D (32));
> +    AddFormatInfo(FOO(DEPTH24_STENCIL8  ), _(DEPTH_STENCIL  ), _(UNSIGNED_NORMALIZED), 4, _DS(24,8));
> +    AddFormatInfo(FOO(DEPTH32F_STENCIL8 ), _(DEPTH_STENCIL  ), _(FLOAT),               8, _DS(32,8));

DEPTH_STENCIL formats have a component-type of NONE.

::: dom/canvas/WebGLFormats.h
@@ +170,5 @@
> +    /**
> +     * Base format is one of GL_RED, GL_RG, GL_RGB, GL_RGBA, GL_ALPHA, GL_LUMINANCE,
> +     * GL_LUMINANCE_ALPHA, GL_DEPTH_COMPONENT, GL_STENCIL_INDEX, GL_DEPTH_STENCIL.
> +     */
> +    GLenum baseFormat;

Don't use a GLenum for this, since this never needs to be converted back to an enum.

@@ +176,5 @@
> +    /**
> +     * Logical data type: one of GL_UNSIGNED_NORMALIZED, GL_SIGNED_NORMALIZED,
> +     * GL_UNSIGNED_INT, GL_INT, GL_FLOAT.
> +     */
> +    GLenum dataType;

`componentType`

@@ +185,5 @@
>      const uint8_t bytesPerPixel; // 0 iff `!!compression`.
> +
> +    /**
> +     * Size of components, in bits.
> +     */

Don't use three lines for a one-line comment.

@@ +262,5 @@
> +
> +/**
> + * Return the size in bits for comp.
> + */
> +GLint ComponentSize(const FormatInfo* info, GLenum comp);

What is `comp`? There's not enough info here to have an idea without reading the implementation.

Comment 6

[u480271]

I cut'n'paste the spec from preview to aid in checking the logic is correct.  I'll clean up the function and split default from non-default framebuffers.

Comment 7

[u480271]

Attachment: Implement GetFramebufferAttachmentParameter.

Comment 8

[u480271]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=580d3f8b1070

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81c18a4b672f

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/81c18a4b672f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/81c18a4b672f