973625 - WebGL glDrawArrays and glClear spend excessive time in validating framebuffer completeness.

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jukka Jylänki]

Attachment: Screen Shot 2014-02-17 at 5.17.46 PM.png

When making profiles of Emscripten+WebGL execution on OSX, I often see a considerable amount of time spent inside the glCheckFramebufferStatus function. See the attached image for an example.

From the profile, it looks like Firefox rechecks FBO completeness on each draw call and clear call(?). Would it be possible to avoid performing this kind of validation on these functions, and only do the validation when the FBO changes?

I have a sample application but I can't link to it here, since it's nonpublic Mozilla partner code, so please contact [e-mail] if you are interested in getting a test case.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Yeah, we can fix this. What's the priority of this look like?

Comment 2

[Jukka Jylänki]

I'm seeing this in multiple Emscripten WebGL apps, so it would be very beneficial for a lot of 3D rendering ports. Would be great if there was something for GDC, but this is not currently a blocker though.

Comment 3

[u480271]

I can look at it if it's required for GDC.

Comment 4

[Kelsey Gilbert [:jgilbert]]

In fact, I just added another check which will call CheckFramebufferStatus again.

Comment 5

[Kelsey Gilbert [:jgilbert]]

So here's a rough plan:
Framebuffer completeness status only changes under a certain set of events. We should basically shadow the status until one of these changes 'dirties' our shadowing, at which point we should ask the driver to refresh the shadow.

These events are actually enumerated in the GLES2 spec in section "4.4.5 Framebuffer Completeness":  
  Performing any of the following actions may change whether the framebuffer
  is considered complete or incomplete.

  * Binding to a different framebuffer with `BindFramebuffer`.

  * Attaching an image to the framebuffer with `FramebufferTexture2D` or
  `FramebufferRenderbuffer`.

  * Detaching an image from the framebuffer with `FramebufferTexture2D` or
  `FramebufferRenderbuffer`.

  * Changing the width, height, or internal format of a texture image that
  is attached to the framebuffer by calling `TexImage2D`, `CopyTexImage2D`
  and `CompressedTexImage2D`.

  * Changing the width, height, or internal format of a renderbuffer that
  is attached to the framebuffer by calling `RenderbufferStorage`.

  * Deleting, with `DeleteTextures` or `DeleteRenderbuffers`, an object
  containing an image that is attached to a framebuffer object that is
  bound to the framebuffer.

Comment 6

[Kelsey Gilbert [:jgilbert]]

We should track the current framebuffer status on each WebGLFramebuffer. When we do one of the operations that might change completeness, we should reset the status to `0` to indicate that we need to re-request it. (`0` isn't a valid retval from CheckFramebufferStatus)
We should then use WebGLFramebuffer->CheckFramebufferStatus() to take advantage of this shadowing.

Comment 7

[u480271]

Attachment: bug-973625-cache-framebuffer-status.patch

Jukka,

Can you try this patch to see if it helps?

thanks,
Dan

Comment 8

[u480271]

Comment on attachment 8381263 [details] [diff] [review]
bug-973625-cache-framebuffer-status.patch

Jeff, Does this look like the right direction?

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8381263 [details] [diff] [review]
bug-973625-cache-framebuffer-status.patch

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

Yeah, this is the right idea. I think it would be better if we tracked a list(set, really) of where each WebGLTexture/Renderbuffer's is bound, both FB and attachment point. That way we can iterate quickly through them, with minimal wasted iteration.

::: content/canvas/src/WebGLContextUtils.cpp
@@ +252,5 @@
> +    if (!tex)
> +        return;
> +
> +    for (WebGLFramebuffer* fb = mFramebuffers.getFirst(); fb; fb = fb->getNext()) {
> +        fb->InvalidateStatusIfBound(tex);

This is probably fine, but we should really have a reverse-lookup multi-map/map<int,set<>> to make perf reliable if we ever have a large number of framebuffers. Or perhaps we just store a set<int> of mFramebuffersBoundTo in WebGLTexture/Renderbuffer.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +793,5 @@
> +            if (height % blockHeight != 0) {
> +                ErrorInvalidOperation("%s: height of level 0 must be multipel of %d",
> +                                      InfoFrom(func), blockHeight);
> +                return false;
> +            }

These look like changes from your other patch for the compressed texture bug.

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +761,5 @@
> +WebGLFramebuffer::InvalidateStatusIfBound(WebGLTexture* tex)
> +{
> +    const size_t colorAttachmentCount = mColorAttachments.Length();
> +    for (size_t n = 0; n < colorAttachmentCount; n++) {
> +        if (ColorAttachment(n).Texture() == tex) {

Ideally we would cache which attachment point a texture was bound to.

Comment 10

[Jukka Jylänki]

Thanks Dan! I tested your patch, and after that the framebuffer completeness checks have disappeared from the profile altogether!

Comment 11

[u480271]

Attachment: Implement WebGLFramebuffer completeness status caching.

Comment 12

[u480271]

Comment on attachment 8385684 [details] [diff] [review]
Implement WebGLFramebuffer completeness status caching.

Cleaned up patch. Ran and passes all tests locally. Try run has some strange failures I don't understand but think are unrelated.

https://tbpl.mozilla.org/?tree=Try&rev=9dd3da17a99f

f? from Jukka, I hope he can apply this patch and try his testing again.

Comment 13

[Jukka Jylänki]

Tested the try build on OSX. The profiles look clean and good, I'm happy with this!

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8385684 [details] [diff] [review]
Implement WebGLFramebuffer completeness status caching.

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

I love caching.

::: content/canvas/src/WebGLContextGL.cpp
@@ +544,3 @@
>          UpdateWebGLErrorAndClearGLError();
> +        // Invalidate framebuffer status cache
> +        tex->NotifyFBsStatusChanged();

I think you forgot to do this for CompressedTexImage.

@@ +4252,5 @@
>      for(WebGLFramebuffer *framebuffer = mFramebuffers.getFirst();
>          framebuffer;
>          framebuffer = framebuffer->getNext())
>      {
> +        size_t colorAttachmentCount = framebuffer->ColorAttachmentCount();

The containing function for this change is gone on tip.

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +398,5 @@
>          return mContext->ErrorInvalidValue("framebufferTexture2D: level must be 0");
>  
> +    Attachment* a = GetAttachmentUnsafe(attachment);
> +    if (!a)
> +        return;

Leave a comment here saying why we're early-outing.

@@ +418,5 @@
> +    a->SetTexImage(wtex, textarget, level);
> +}
> +
> +WebGLFramebuffer::Attachment*
> +WebGLFramebuffer::GetAttachmentUnsafe(GLenum attachment)

GetAttachmentOrNull might be clearer. It doesn't seem unsafe per se, just that it's return value is nullable.

@@ +795,5 @@
>      }
>  }
>  
> +void
> +WebGLFramebuffer::NotifyAttachableChanged(WebGLFramebufferAttachable* /*wfa*/)

Can't we just not supply this arg, then?

::: content/canvas/src/WebGLFramebuffer.h
@@ +180,5 @@
> +    Attachment* AttachmentFor(GLenum attachment);
> +    void NotifyAttachableChanged(WebGLFramebufferAttachable* wfa);
> +
> +private:
> +    mutable GLenum mStatus;    GLuint mGLName;

Looks like you accidentally a newline here.
I'm also increasingly a fan of mutable cache vars.

::: content/canvas/src/WebGLFramebufferAttachable.cpp
@@ +25,5 @@
> +    return nullptr;
> +}
> +
> +void
> +WebGLFramebufferAttachable::AttachTo(WebGLFramebuffer* fb, GLenum attachment)

Please MOZ_ASSERT(fb).

@@ +27,5 @@
> +
> +void
> +WebGLFramebufferAttachable::AttachTo(WebGLFramebuffer* fb, GLenum attachment)
> +{
> +    printf_stderr("-> Attach %p to %p at 0x%04x\n", this, fb, attachment);

Remove this debug stuff before landing.

@@ +37,5 @@
> +    }
> +
> +    // Append new attachment
> +    mAttachmentPoints.growBy(1);
> +    point = mAttachmentPoints.end() - 1;

This is weird. I'd rather we make local copy with a ctor, and push that on to the vector as a copy. (though I haven't looked at the MFBT vector)

@@ +44,5 @@
> +}
> +
> +void
> +WebGLFramebufferAttachable::DetachFrom(WebGLFramebuffer* fb, GLenum attachment)
> +{

Please MOZ_ASSERT(fb).

::: content/canvas/src/WebGLRenderbuffer.h
@@ +20,5 @@
>      , public WebGLRefCountedObject<WebGLRenderbuffer>
>      , public LinkedListElement<WebGLRenderbuffer>
>      , public WebGLRectangleObject
>      , public WebGLContextBoundObject
> +    , public WebGLFramebufferAttachable

Mixins! :D

::: content/canvas/src/WebGLTexture.h
@@ +11,5 @@
>  
>  #include "nsWrapperCache.h"
>  
>  #include "mozilla/LinkedList.h"
> +#include "mozilla/Vector.h"

Unnecessary include.

Comment 15

[u480271]

Attachment: Implement WebGLFramebuffer completeness status caching.

Address Jeff's review comments.

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8390270 [details] [diff] [review]
Implement WebGLFramebuffer completeness status caching.

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

::: content/canvas/src/WebGLFramebuffer.h
@@ +177,5 @@
>      bool CheckColorAttachmentNumber(GLenum attachment, const char* functionName) const;
>  
> +    void EnsureColorAttachments(size_t colorAttachmentId);
> +
> +    Attachment* AttachmentFor(GLenum attachment);

Can use Attachment& if it's never going to be null.

::: content/canvas/src/WebGLFramebufferAttachable.h
@@ +19,5 @@
> +    {
> +        AttachmentPoint(const WebGLFramebuffer* fb, GLenum attachment)
> +            : mFB(fb)
> +            , mAttachment(attachment)
> +        {}

I'd actually mainly like to assert that `fb` is non-null here.

Comment 17

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> I love caching.

Mmm, caching.

> ::: content/canvas/src/WebGLContextGL.cpp
> I think you forgot to do this for CompressedTexImage.

I moved the invalidate cache to WebGLTexture::SetImageInfo. All the texture setting paths appear to hit that function.

> @@ +418,5 @@
> GetAttachmentOrNull might be clearer. It doesn't seem unsafe per se, just
> that it's return value is nullable.

Done. I had Writer's Block when trying to think of a name. The hardest part always is naming something.

> ::: content/canvas/src/WebGLFramebuffer.h
> I'm also increasingly a fan of mutable cache vars.

mutable is the only choice, really.

> @@ +37,5 @@
> > +    }
> > +
> > +    // Append new attachment
> > +    mAttachmentPoints.growBy(1);
> > +    point = mAttachmentPoints.end() - 1;
> 
> This is weird. I'd rather we make local copy with a ctor, and push that on
> to the vector as a copy. (though I haven't looked at the MFBT vector)

Replaced with append.

> ::: content/canvas/src/WebGLRenderbuffer.h
> @@ +20,5 @@
> >      , public WebGLRefCountedObject<WebGLRenderbuffer>
> >      , public LinkedListElement<WebGLRenderbuffer>
> >      , public WebGLRectangleObject
> >      , public WebGLContextBoundObject
> > +    , public WebGLFramebufferAttachable
> 
> Mixins! :D

I thought the exact same thing when I added the line. Mixin all the classes!

Comment 18

[u480271]

Significant changes requires try run:

https://tbpl.mozilla.org/?tree=Try&rev=57261b9a4f57

Comment 19

[u480271]

Tested with Sanctuary Demo

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77b90f9bc6db

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77b90f9bc6db