1233353 - Pass WebGL2 conformance test multisampled-renderbuffer-initialization

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Ethan Lin[:ethlin]]

We should pass the conformance2 test 'multisampled-renderbuffer-initialization'.

Comment 1

[Ethan Lin[:ethlin]]

Test result:    
    failed: at (0, 12) expected: 0,0,0,0 was 0,0,0,255
    failed: at (0, 12) expected: 0,0,0,0 was 0,0,0,255
    failed: at (0, 12) expected: 0,0,0,0 was 0,0,0,255
    failed: at (0, 12) expected: 0,0,0,0 was 0,0,0,255

Comment 2

[Ethan Lin[:ethlin]]

Attachment: Initialize the renderbuffer

The test failed due to the lack of RenderBuffer initialization. According to the spec, the WebGL implementation must initialize their contents to 0 when the initial data is not provided. So I clear the buffer value to zero in the RenderbufferStorage, and do a little fix for the ScopedFramebufferForRenderbuffer to keep the binding of framebuffer.

[1] https://www.khronos.org/registry/webgl/specs/latest/1.0/#4.1

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8699908 [details] [diff] [review]
Initialize the renderbuffer

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

::: dom/canvas/WebGLContextGL.cpp
@@ +1677,5 @@
>                          ErrorName(error));
>          return;
>      }
> +
> +    // Initialize Renderbuffer to zero

Don't eagerly do this. It should be lazily initialized like everything else.

::: dom/canvas/WebGLFramebufferAttachable.h
@@ +19,5 @@
>      // Track FBO/Attachment combinations
>      void MarkAttachment(const WebGLFBAttachPoint& attachment);
>      void UnmarkAttachment(const WebGLFBAttachPoint& attachment);
>      void InvalidateStatusOfAttachedFBs() const;
> +    bool HasAttachment();

IsAttached()

::: gfx/gl/ScopedGLHelpers.h
@@ +235,5 @@
>  
>  protected:
>      bool mComplete; // True if the framebuffer we create is complete.
>      GLuint mFB;
> +    ScopedBindFramebuffer mAutoFB;

Why are you adding this?

Comment 4

[Ethan Lin[:ethlin]]

Comment on attachment 8699908 [details] [diff] [review]
Initialize the renderbuffer

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

::: dom/canvas/WebGLContextGL.cpp
@@ +1677,5 @@
>                          ErrorName(error));
>          return;
>      }
> +
> +    // Initialize Renderbuffer to zero

okay, I will try to lazily initialize the Renderbuffer.

::: gfx/gl/ScopedGLHelpers.h
@@ +235,5 @@
>  
>  protected:
>      bool mComplete; // True if the framebuffer we create is complete.
>      GLuint mFB;
> +    ScopedBindFramebuffer mAutoFB;

The autoFB was a local variable in the constructor, so it will bind old framebuffer after constructor. I want to keep the binding for initialization until destruct ScopedFramebufferForRenderbuffer.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8699908 [details] [diff] [review]
Initialize the renderbuffer

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

::: gfx/gl/ScopedGLHelpers.h
@@ +235,5 @@
>  
>  protected:
>      bool mComplete; // True if the framebuffer we create is complete.
>      GLuint mFB;
> +    ScopedBindFramebuffer mAutoFB;

We don't necessarily want this, as this changes the behavior of the function.

We'd have to audit the call-sites to make sure this is a reasonable change, and also we should change this name to ScopedBindFramebufferForRenderbuffer.

We should avoid doing this in this bug though. Open another bug if you'd like to work on this.

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Initialize rb correctly if rb bind to READ_FRAMEBUFFER.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8709318 [details] [diff] [review]
Initialize rb correctly if rb bind to READ_FRAMEBUFFER.

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

::: dom/canvas/WebGL2ContextRenderbuffers.cpp
@@ +62,5 @@
>    const char funcName[] = "renderbufferStorageMultisample";
>    if (IsContextLost())
>      return;
>  
> +  RenderbufferStorage_base(funcName, target, samples, internalFormat, width, height);

This is bug 1094458. We can't just re-enable this yet.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +992,5 @@
> +    {
> +        // This FB maybe bind to GL_READ_FRAMEBUFFER and glClear only
> +        // clear GL_DRAW_FRAMEBUFFER. So bind FB to GL_DRAW_FRAMEBUFFER
> +        // here.
> +        gl::ScopedBindFramebuffer autoFB(mContext->gl, mGLName);

While I would normally object to rebinding unconditionally, this is an implicit clear, so we don't care whether it's fast. Therefore this is fine.

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Initialize rb correctly if rb bind to READ_FRAMEBUFFER v2.

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a901050ab8c

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5cfe7882973

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b5cfe7882973