Closed Bug 1233353 Opened 5 years ago Closed 5 years ago

Pass WebGL2 conformance test multisampled-renderbuffer-initialization

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: ethlin, Assigned: ethlin)

References

()

Details

Attachments

(1 file, 2 obsolete files)

We should pass the conformance2 test 'multisampled-renderbuffer-initialization'.
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
Attached patch Initialize the renderbuffer (obsolete) — Splinter Review
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
Attachment #8699908 - Flags: review?(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?
Attachment #8699908 - Flags: review?(jgilbert) → review-
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 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.
Blocks: 1236785
No longer blocks: 1236785
Blocks: 1236784
Attachment #8699908 - Attachment is obsolete: true
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.
Attachment #8709318 - Flags: review?(jgilbert) → review-
Attachment #8709318 - Attachment is obsolete: true
Attachment #8709804 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/b5cfe7882973
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.