Closed
Bug 1014209
Opened 10 years ago
Closed 10 years ago
Performance impact from enabling WEBGL_draw_buffers
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
People
(Reporter: jrmuizel, Assigned: jrmuizel)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
There's currently a negative performance impact if WEBGL_draw_buffers is enabled. For example, we do additional work in Clear(). This is unexpected and can cause frameworks like emscripten to unintentionally lower the performance of applications.
Assignee | ||
Comment 1•10 years ago
|
||
I've filed https://github.com/kripken/emscripten/issues/2373 about trying to deal with this.
Comment 2•10 years ago
|
||
What is the perf impact? Glancing through the code, I don't see anything obvious. I assume we're over-clearing somehow?
Assignee | ||
Comment 3•10 years ago
|
||
The drawBuffers call in clear() causes a flush
... what drawBuffers call in clear()?
Comment 5•10 years ago
|
||
Ooh, ok. That's not in webgl.clear, that's in *ClearScreen*. This is an easy fix, we just need to cache calls to DrawBuffers.
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8428010 -
Flags: feedback?(jgilbert)
Assignee | ||
Comment 7•10 years ago
|
||
It may also be that DrawBuffers() is just causing work that the Clear() would have caused anyways...
Comment 8•10 years ago
|
||
Comment on attachment 8428010 [details] [diff] [review]
Something like this could work
Review of attachment 8428010 [details] [diff] [review]:
-----------------------------------------------------------------
We probably shouldn't be doing the state introspection that we do here, but if it's "ok" for now, we can leave it.
::: content/canvas/src/WebGLContext.cpp
@@ +994,5 @@
> bool initializeColorBuffer = 0 != (mask & LOCAL_GL_COLOR_BUFFER_BIT);
> bool initializeDepthBuffer = 0 != (mask & LOCAL_GL_DEPTH_BUFFER_BIT);
> bool initializeStencilBuffer = 0 != (mask & LOCAL_GL_STENCIL_BUFFER_BIT);
> bool drawBuffersIsEnabled = IsExtensionEnabled(WebGLExtensionID::WEBGL_draw_buffers);
> + bool saveDrawBuffers = false;
It's less that we need to save it, and more that we need to override it temporarily. `shouldOverrideDrawBuffers`?
Attachment #8428010 -
Flags: feedback?(jgilbert) → feedback+
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> Comment on attachment 8428010 [details] [diff] [review]
> Something like this could work
>
> Review of attachment 8428010 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> We probably shouldn't be doing the state introspection that we do here, but
> if it's "ok" for now, we can leave it.
What would your preferred implementation look like?
Flags: needinfo?(jgilbert)
Assignee | ||
Comment 10•10 years ago
|
||
I confirmed this fixes the bug I was looking at
Attachment #8428010 -
Attachment is obsolete: true
Attachment #8429473 -
Flags: review?(jgilbert)
Comment 11•10 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> (In reply to Jeff Gilbert [:jgilbert] from comment #8)
> > Comment on attachment 8428010 [details] [diff] [review]
> > Something like this could work
> >
> > Review of attachment 8428010 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > We probably shouldn't be doing the state introspection that we do here, but
> > if it's "ok" for now, we can leave it.
>
> What would your preferred implementation look like?
We should just cache this stuff, instead of ever risking calls to glGet* functions. We don't need to do that here, though.
Flags: needinfo?(jgilbert)
Updated•10 years ago
|
Attachment #8429473 -
Flags: review?(jgilbert) → review+
Comment 12•10 years ago
|
||
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 13•10 years ago
|
||
We are hoping this to be uplifted to FFOS 1.4, since we are targeting to improve performance on that release. Is this change possible to be applied there?
Updated•10 years ago
|
blocking-b2g: --- → 1.4?
Comment 15•10 years ago
|
||
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
status-firefox30:
--- → wontfix
status-firefox31:
--- → wontfix
status-firefox32:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•