Closed Bug 973625 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Canvas: WebGL, defect)

30 Branch
x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jujjyl, Assigned: djg)

References

Details

(Whiteboard: [games] webgl-perf)

Attachments

(2 files, 2 obsolete files)

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 jjylanki@mozilla.com if you are interested in getting a test case.
Whiteboard: [games]
Yeah, we can fix this. What's the priority of this look like?
Whiteboard: [games] → [games] webgl-perf
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.
I can look at it if it's required for GDC.
In fact, I just added another check which will call CheckFramebufferStatus again.
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.
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.
Jukka,

Can you try this patch to see if it helps?

thanks,
Dan
Flags: needinfo?(jujjyl)
Comment on attachment 8381263 [details] [diff] [review]
bug-973625-cache-framebuffer-status.patch

Jeff, Does this look like the right direction?
Attachment #8381263 - Flags: feedback?(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.
Attachment #8381263 - Flags: feedback?(jgilbert) → feedback+
Thanks Dan! I tested your patch, and after that the framebuffer completeness checks have disappeared from the profile altogether!
Flags: needinfo?(jujjyl)
Assignee: nobody → dglastonbury
Status: NEW → ASSIGNED
Attachment #8381263 - Attachment is obsolete: true
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.
Attachment #8385684 - Flags: review?(jgilbert)
Attachment #8385684 - Flags: feedback?(jujjyl)
Tested the try build on OSX. The profiles look clean and good, I'm happy with this!
Attachment #8385684 - Flags: feedback?(jujjyl) → feedback+
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.
Attachment #8385684 - Flags: review?(jgilbert) → review-
Address Jeff's review comments.
Attachment #8390270 - Flags: review?(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.
Attachment #8390270 - Flags: review?(jgilbert) → review+
(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!
Attachment #8385684 - Attachment is obsolete: true
Tested with Sanctuary Demo
https://hg.mozilla.org/mozilla-central/rev/77b90f9bc6db
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: CVE-2014-1556
You need to log in before you can comment on or make changes to this bug.