801986 - WebGL test failures in framebuffer-object-attachment.html

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

This is one of the WebGL 1.0.2-pre tests that's failing, almost certainly due to a bug on our side.

https://www.khronos.org/registry/webgl/sdk/tests/conformance/renderbuffers/framebuffer-object-attachment.html

Your mission is to understand exactly what's going on here (show how we are failing to be conformant here) and make a patch.

The failures seem to pertain to attaching and detaching certain renderbuffers from framebuffer object (using the WebGL function framebufferRenderbuffer).

The C++ implementation of this function is WebGLContext::FramebufferRenderbuffer in WebGLContextGL.cpp.

However, most of the work is done inside another function that it calls into: WebGLFramebuffer::FramebufferRenderbuffer, in WebGLContext.h.

Specifically, the code at the end of this function is where I would start looking for a bug:

http://hg.mozilla.org/mozilla-central/file/c909c5d3c339/content/canvas/src/WebGLContext.h#l2830

This is where we are actually doing the corresponding OpenGL call.  See how DEPTH_STENCIL renderbuffers are treated as a special case here. Indeed, WebGL differs from OpenGL in that it offers a special framebuffer attachment point, DEPTH_STENCIL, with the property that a renderbuffer attached to that point will provide both the depth and the stencil buffers.

It seems to be that the current code has a bug that is revealed by this test doing a particular sequence of framebufferRenderbuffer calls.

For this bug you will need to understand very well both the WebGL and the OpenGL specs on framebuffer objects.

WebGL:
http://www.khronos.org/registry/webgl/specs/latest/#6.5
http://www.khronos.org/registry/webgl/specs/latest/#5.14.6

OpenGL:
http://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf , section 4.4 page 107 and onwards

Good luck!

Comment 1

[Saurabh Anand [:sawrubh]]

Unfortunately, I've been a little busy in the past few days and think I won't be able to look at it in the near future. Just don't want to block anybody if anybody wants to take a shot at it.

Sorry Benoit.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Saurabh, thanks for unassigning from yourself then. You did the right thing there.

Comment 3

[Erick Dransch [:edransch]]

Attachment: Preliminary Patch reavealing bug

Disclaimer: I'm new to the firefox codebase; I'll likely need guidance understanding the codebase, coding style, and testing changes.

The cause of this issue is related to the way that we handle the DEPTH_STENCIL_ATTACHMENT attachment point.
One of the sets of steps that reveals the issue is:

1) Attach a renderbuffer to DEPTH_ATTACHMENT -> We call the corresponding OpenGL attachment function.
2) Attach a renderbuffer to DEPTH_STENCIL_ATTACHMENT -> We attach to both DEPTH and STENCIL attachments
3) We are now in an error state according to http://www.khronos.org/registry/webgl/specs/latest/#6.5 -> We handle this correctly
4) Detach DEPTH_STENCIL_ATTACHMENT -> We detach both DEPTH and STENCIL from OpenGL.

We are now in a broken state:  
According to WebGL specs, we should still have an intact and valid attachment at DEPTH_ATTACHMENT.
However, we have detached both DEPTH and STENCIL buffers using OpenGL calls.

( For the conformance test that reveals the issue using these steps, see: https://github.com/KhronosGroup/WebGL/blob/master/sdk/tests/conformance/renderbuffers/framebuffer-object-attachment.html#L186 )

I've addressed the issue in this preliminary patch, but it may not be the best way to approach the problem. 
There may be a better place to put the logic that handles this case. 
If you have any guidance on this matter, I'd appreciate it.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

This analysis makes perfect sense, thanks. WebGLFramebuffer::FramebufferRenderbuffer is indeed the right place to fix this. But I would like to suggest a different approach to fixing this bug.

This bug is specifically about how we map WebGL attachment points to OpenGL attachment points. This mapping is trivial for most attachment points, and only is nontrivial for WebGL's DEPTH_STENCIL attachment point. The bug is in how we map WebGL's DEPTH_STENCIL to OpenGL's DEPTH and STENCIL attachment points, which is done here:

http://hg.mozilla.org/mozilla-central/file/c909c5d3c339/content/canvas/src/WebGLContext.h#l2883

This is where we are making (wrong) assumptions that we can naively map WebGL calls on DEPTH_STENCIL to a pair of OpenGL calls on DEPTH and STENCIL.

As long as we are only _attaching_ new attachments, we can get away with it, because in case of incompatible WebGL attachments (e.g. if the user attaches to DEPTH and to DEPTH_STENCIL) we would generate an error anyways.

But when we _detach_ a WebGL attachment, this breaks down because we are forgetting to restore possible other WebGL attachments that we may still have, that would map to the same OpenGL attachment.

So I think that you can fix this bug with a smaller patch that would add logic like the following, to the above existing code:

  if (we are detaching a WebGL attachment) {
    for (each OpenGL attachment point corresponding to this WebGL attach point) {
      if (we still have another WebGL attachment mapping to this OpenGL attach point) {
        restore its OpenGL attachment
      } else {
        actually detach the OpenGL attachment like we currently do
      }
    }
  }

That should give a smaller patch, right?

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Also, there is the question of what to do in FramebufferTexture2D. It's not clear to me right now whether we can simply say that since DEPTH_STENCIL textures don't currently exist, we don't need to worry about this in FramebufferTexture2D. If we can, we should then go further and remove the existing buggy code in this function (see link in comment 0).

Comment 6

[Erick Dransch [:edransch]]

Attachment: Two different solutions

Thanks for the feedback Benoit! 

I've got two different adjustments to the patch here. Since our implementation of FramebufferTexture2D is identical to FramebufferRenderbuffer, both fixes are demonstrated in this patch.

The fix shown in FramebufferRenderbuffer builds the necessary logic into the assignment of the buffername variables, and leaves the opengl calls largely unchanged. This is the smaller patch. I slightly prefer this approach.

The fix shown in FramebufferTexture2D corresponds more directly to your suggestion. We check first if we're adding or removing an attachment, and handle each case separately. It's a larger patch, but it may be more legible. 

Which approach seems best for this bug? Feel free to offer other suggestions as well.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 676065 [details] [diff] [review]
Two different solutions

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

OK, please go ahead with the FramebufferRenderbuffer approach. It's simpler.

Please simplify the code a bit: depthbuffername and stencilbuffername are only used in the !(attachment == LOCAL_GL_DEPTH_STENCIL_ATTACHMENT) case, so you could move them inside of that if() case, which would allow to simplify them a bit. There might be a few further simplifications around there.

Comment 8

[Erick Dransch [:edransch]]

Attachment: Implementation

Thank you for the feedback!
I've made adjustments according to your suggestions. I think the logic is easier to follow now.

Comment 9

[Erick Dransch [:edransch]]

Attachment: Implementation

My apologies, the previous patch contains unnecessary whitespace changes. I've removed them here.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 676478 [details] [diff] [review]
Implementation

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

::: content/canvas/src/WebGLContext.h
@@ +2871,5 @@
> +            WebGLuint depthbuffername = wrb ? renderbuffername : 
> +                mDepthAttachment.Renderbuffer() ? mDepthAttachment.Renderbuffer()->GLName() : 0;
> +
> +            WebGLuint stencilbuffername = wrb ? renderbuffername :
> +                mStencilAttachment.Renderbuffer() ?  mStencilAttachment.Renderbuffer()->GLName() : 0;

Since the part |wrb ? renderbuffername : | appears twice above, it seems that it could be easier to follow the logic here with a single if(wrb).

If you prefer to keep the two big nested ternary operator, then please follow a coding style more like

    Type result
        = condition1 ? result1
        : condition2 ? result2
        : 0;

Like in WebGLFramebufferAttachment::IsDeleteRequested.

@@ +2879,4 @@
>          } else {
> +            if(!wrb && (attachment == LOCAL_GL_DEPTH_ATTACHMENT || attachment == LOCAL_GL_STENCIL_ATTACHMENT)){
> +                renderbuffer = mDepthStencilAttachment.Renderbuffer() ? 
> +                    mDepthStencilAttachment.Renderbuffer()->GLName() : 0;

Is seems that you wanted |renderbuffername| here instead of |renderbuffer|.

For symmetry with the above case and out of a general principle that it's confusing to modify the meaning of existing variables, I would prefer if you declared a new |depthstencilname| here.

Comment 11

[Erick Dransch [:edransch]]

(In reply to Benoit Jacob [:bjacob] from comment #10)
> Comment on attachment 676478 [details] [diff] [review]
> Implementation
> 
> Review of attachment 676478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.h
> @@ +2871,5 @@
> > +            WebGLuint depthbuffername = wrb ? renderbuffername : 
> > +                mDepthAttachment.Renderbuffer() ? mDepthAttachment.Renderbuffer()->GLName() : 0;
> > +
> > +            WebGLuint stencilbuffername = wrb ? renderbuffername :
> > +                mStencilAttachment.Renderbuffer() ?  mStencilAttachment.Renderbuffer()->GLName() : 0;
> 
> Since the part |wrb ? renderbuffername : | appears twice above, it seems
> that it could be easier to follow the logic here with a single if(wrb).
> 
> If you prefer to keep the two big nested ternary operator, then please
> follow a coding style more like
> 
>     Type result
>         = condition1 ? result1
>         : condition2 ? result2
>         : 0;
> 
> Like in WebGLFramebufferAttachment::IsDeleteRequested.

I agree, I'll move these assignments into an |if (!wrb)| block.

> 
> @@ +2879,4 @@
> >          } else {
> > +            if(!wrb && (attachment == LOCAL_GL_DEPTH_ATTACHMENT || attachment == LOCAL_GL_STENCIL_ATTACHMENT)){
> > +                renderbuffer = mDepthStencilAttachment.Renderbuffer() ? 
> > +                    mDepthStencilAttachment.Renderbuffer()->GLName() : 0;
> 
> Is seems that you wanted |renderbuffername| here instead of |renderbuffer|.
> 
> For symmetry with the above case and out of a general principle that it's
> confusing to modify the meaning of existing variables, I would prefer if you
> declared a new |depthstencilname| here.

I'm not sure |depthstencilname| is appropriate in this case, as this code path is executed for all 3 of (DEPTH_ATTACHMENT, STENCIL_ATTACHMENT, and COLOR_ATTACHMENT0). Perhaps we can rename the variable defined before the if-statement to be something like |parameterbuffername|, and keep the |renderbuffername| in this block.

Comment 12

[Erick Dransch [:edransch]]

Attachment: Implementation

Addresses feedback on previous patch, according to previous comment.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 676636 [details] [diff] [review]
Implementation

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

Thanks! Some nits:

::: content/canvas/src/WebGLContext.h
@@ +2870,2 @@
>          if (attachment == LOCAL_GL_DEPTH_STENCIL_ATTACHMENT) {
> +

Useless empty line?

@@ +2870,5 @@
>          if (attachment == LOCAL_GL_DEPTH_STENCIL_ATTACHMENT) {
> +
> +            WebGLuint depthbuffername = parambuffername;
> +            WebGLuint stencilbuffername = parambuffername;
> +            if (!wrb){

I'd find it easier to read if the condition here was if (!parambuffername). Fewer identifiers around.

Comment 14

[Erick Dransch [:edransch]]

Attachment: Implementation

Thanks for the feedback, I've addressed those nits with this patch.

As I mentioned, I'm new to the firefox codebase. Would you be able to provide some guidance as to how I can go about running builds & tests across multiple platforms for this patch?

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Do you have level 1 access?

If yes, you'll want to push to tryserver. Use this tool to generate your try-options commit message:
http://trychooser.pub.build.mozilla.org/

For this patch, you'll want: debug build only, all platforms, mochitest-1 only, no Talos. Mochitest-1 is where the WebGL mochitest lives.

Generic Tryserver instructions there:
https://wiki.mozilla.org/ReleaseEngineering/TryServer

Comment 16

[Erick Dransch [:edransch]]

It turns out my hg access has expired due to inactivity. I'm waiting in https://bugzilla.mozilla.org/show_bug.cgi?id=807057 for it to be re-enabled. I will push to try once I have regained access.

Comment 17

[Erick Dransch [:edransch]]

tbpl entry for try run is available at https://tbpl.mozilla.org/?tree=Try&rev=37178e0fd3e0

Comment 18

[Erick Dransch [:edransch]]

The try run is complete and all green. I'm not sure how to interpret the results for any further analysis. 

Should we be seeing some tests that now pass but previously failed?

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Nothing more to analyze: the green 1's and B's are enough to tell that your patch can land.

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Erick Dransch [:edransch] from comment #18)
> Should we be seeing some tests that now pass but previously failed?

Oh I see the question now. No, because the version of the WebGL conformance tests that we have in our tree is 1.0.1 and it does not have the specific subtest that was showing the error.

Comment 21

[Benoit Jacob [:bjacob] (mostly away)]

Please make a mercurial patch with your user info and a commit message for landing.

Comment 22

[Erick Dransch [:edransch]]

Attachment: Mercurial patch

If there are any errors with the patch formatting, please let me know and I will be happy to re-generate it.

Thanks!

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75fa1f74f174

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75fa1f74f174