Last Comment Bug 725747 - [B2G] Huge regression in WebGL panning speed on Galaxy S 2 after switch to FBOs
: [B2G] Huge regression in WebGL panning speed on Galaxy S 2 after switch to FBOs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla13
Assigned To: Joe Drew (not getting mail)
:
:
Mentors:
Depends on: 726276 728210
Blocks: b2g-demo-phone 720467 724476 726245
  Show dependency treegraph
 
Reported: 2012-02-09 11:10 PST by Joe Drew (not getting mail)
Modified: 2012-02-17 06:48 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
don't continuously rebind framebuffers (9.76 KB, patch)
2012-02-09 15:40 PST, Joe Drew (not getting mail)
no flags Details | Diff | Splinter Review
Translate from fb 0 to our offscreen FB at bind time, not draw time (9.25 KB, patch)
2012-02-09 19:20 PST, Joe Drew (not getting mail)
no flags Details | Diff | Splinter Review
Translate from fb 0 to our offscreen FB at bind time, not draw time, v2 (10.18 KB, patch)
2012-02-13 08:46 PST, Joe Drew (not getting mail)
no flags Details | Diff | Splinter Review
Translate from fb 0 to our offscreen FB at bind time, not draw time, v3 (10.80 KB, patch)
2012-02-14 13:51 PST, Joe Drew (not getting mail)
jgilbert: review-
Details | Diff | Splinter Review
Translate from fb 0 to our offscreen FB at bind time, not draw time, v4 (11.19 KB, patch)
2012-02-14 15:11 PST, Joe Drew (not getting mail)
jgilbert: review-
Details | Diff | Splinter Review
mark tests as failing (1.72 KB, patch)
2012-02-16 13:47 PST, Joe Drew (not getting mail)
jacob.benoit.1: review-
Details | Diff | Splinter Review
Translate from fb 0 to our offscreen FB at bind time, not draw time, v5 (11.85 KB, patch)
2012-02-16 14:38 PST, Joe Drew (not getting mail)
jgilbert: review+
Details | Diff | Splinter Review
mark tests as failing v2 (860 bytes, patch)
2012-02-16 14:38 PST, Joe Drew (not getting mail)
jacob.benoit.1: review+
Details | Diff | Splinter Review

Description Joe Drew (not getting mail) 2012-02-09 11:10:07 PST
This is a regression from bug 720467, which is necessary to properly support WebGL on the maguro device.

It seems we're spending a huge amount of time in the kernel, apparently handling IRQs. Since PBuffers and FBOs are logically equivalent, we're probably just using FBOs in a way that causes slowness; it should be possible to recover all the performance once we find out where we're spending our time.
Comment 1 Jeff Gilbert [:jgilbert] 2012-02-09 14:58:34 PST
Could we be thrashing the pixel format?
Comment 2 Joe Drew (not getting mail) 2012-02-09 15:40:07 PST
Created attachment 595889 [details] [diff] [review]
don't continuously rebind framebuffers

This patch is rough and includes a pile of debugging code as well as the patch for bug 717442, but in essence it makes fBindFramebuffer(0) immediately bind to the offscreen framebuffer we use internally, rather than binding before a GL draw call, and rebinding fb 0 afterwards.

This makes an enormous difference to performance on the Galaxy S 2.
Comment 3 Joe Drew (not getting mail) 2012-02-09 19:20:40 PST
Created attachment 595940 [details] [diff] [review]
Translate from fb 0 to our offscreen FB at bind time, not draw time

This is a cleaned-up version of the above patch, and it's currently going through Try.
Comment 4 Joe Drew (not getting mail) 2012-02-09 19:22:15 PST
Jeff, the part that needs the most review IMO is BeforeGLDrawCall() and BeforeGLReadCall()
Comment 5 Joe Drew (not getting mail) 2012-02-09 19:26:33 PST
For future reference, the problem that we had is that we were binding to our offscreen framebuffer on every draw call, then unbinding afterwards. This caused (AFAICT) an internal flush to happen in the driver, with all the waiting that entailed, which was disastrous for performance.

This patch switches us to do the default (0) to internal (offscreen) framebuffer translation at glBindFramebuffer time, which is much faster since it only happens once. Unfortunately we also have to override glGetIntegerv because we want to lie about what framebuffer is bound to the user.

One other note is that it's not clear if other glGetIntegerv values need to be overridden, but Benoit Jacob and I think we're probably OK.
Comment 6 Mozilla RelEng Bot 2012-02-10 02:45:19 PST
Try run for 72d2bdc33dba is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=72d2bdc33dba
Results (out of 138 total builds):
    exception: 1
    success: 103
    warnings: 34
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jdrew@mozilla.com-72d2bdc33dba
Comment 7 Mozilla RelEng Bot 2012-02-10 21:01:21 PST
Try run for 6704dadaa0f6 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6704dadaa0f6
Results (out of 138 total builds):
    exception: 1
    success: 112
    warnings: 25
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jdrew@mozilla.com-6704dadaa0f6
Comment 8 Jeff Gilbert [:jgilbert] 2012-02-13 07:53:16 PST
I am concerned that this may be related to bug 724476.
Comment 9 Joe Drew (not getting mail) 2012-02-13 08:46:06 PST
Created attachment 596693 [details] [diff] [review]
Translate from fb 0 to our offscreen FB at bind time, not draw time, v2

This version of the patch actually binds non-0 FBOs, and thus does not entirely break WebGL. Try results pending.
Comment 10 Mozilla RelEng Bot 2012-02-13 15:16:13 PST
Try run for 073b2c0d5872 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=073b2c0d5872
Results (out of 138 total builds):
    success: 116
    warnings: 22
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jdrew@mozilla.com-073b2c0d5872
Comment 11 Joe Drew (not getting mail) 2012-02-14 13:51:54 PST
Created attachment 597174 [details] [diff] [review]
Translate from fb 0 to our offscreen FB at bind time, not draw time, v3

Argh. Accidentally removed a very important case in BeforeGLReadCall() which resulted in us reading from the wrong FBO when bound to a custom FBO. This broke several webgl tests.
Comment 12 Jeff Gilbert [:jgilbert] 2012-02-14 14:43:52 PST
Comment on attachment 597174 [details] [diff] [review]
Translate from fb 0 to our offscreen FB at bind time, not draw time, v3

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

::: gfx/gl/GLContext.h
@@ +915,2 @@
>              break;
>          }

Can we get some default case here which just blindly binds using the inputs? Maybe also include an NS_WARNING, because this case *shouldn't* happen. We should actually do the GL call though, so as to handle the error state.

@@ +2029,5 @@
>      }
>  
> +public:
> +    void fGetIntegerv(GLenum pname, GLint *params) {
> +        if (pname == LOCAL_GL_FRAMEBUFFER_BINDING) {

I think this should be a switch instead of an else-if chain.

@@ +2030,5 @@
>  
> +public:
> +    void fGetIntegerv(GLenum pname, GLint *params) {
> +        if (pname == LOCAL_GL_FRAMEBUFFER_BINDING) {
> +            *params = mUserBoundDrawFBO;

For these, can we use the GetBoundDraw/ReadFBO() functions? It would further isolate the mUser/InternalBoundDraw/ReadFBO variables, and would give use the DEBUG error checking of those functions.
Comment 13 Joe Drew (not getting mail) 2012-02-14 15:11:30 PST
Created attachment 597205 [details] [diff] [review]
Translate from fb 0 to our offscreen FB at bind time, not draw time, v4

Addressed review comments.
Comment 14 Mozilla RelEng Bot 2012-02-14 21:16:11 PST
Try run for 60c8ea3d1012 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=60c8ea3d1012
Results (out of 139 total builds):
    exception: 1
    success: 121
    warnings: 17
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jdrew@mozilla.com-60c8ea3d1012
Comment 15 Mozilla RelEng Bot 2012-02-14 22:01:09 PST
Try run for 99746695a5d4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=99746695a5d4
Results (out of 139 total builds):
    exception: 1
    success: 120
    warnings: 17
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jdrew@mozilla.com-99746695a5d4
Comment 16 Joe Drew (not getting mail) 2012-02-16 13:47:53 PST
Created attachment 597981 [details] [diff] [review]
mark tests as failing

Benoit and I looked at this, and it seems quite clear that this is some sort of bizzare bug in the driver. We're just going to mark it as failing for now.
Comment 17 Jeff Gilbert [:jgilbert] 2012-02-16 13:58:10 PST
Comment on attachment 597205 [details] [diff] [review]
Translate from fb 0 to our offscreen FB at bind time, not draw time, v4

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

::: gfx/gl/GLContext.h
@@ +891,5 @@
> +                                 mInternalBoundReadFBO);
> +            break;
> +
> +          default:
> +            NS_NOTREACHED("invalid framebuffer target");

We can't put this here, since glBindFramebuffer(bad,bad) is still a valid call.

@@ +892,5 @@
> +            break;
> +
> +          default:
> +            NS_NOTREACHED("invalid framebuffer target");
> +            // FALL THROUGH

We should not fall through here, since it will treat any invalid 'target' as GL_FRAMEBUFFER.

We just need to blindly run fBindFramebuffer on the input in this case.
Comment 18 Benoit Jacob [:bjacob] (mostly away) 2012-02-16 13:58:46 PST
Comment on attachment 597981 [details] [diff] [review]
mark tests as failing

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +1907,2 @@
>      gl->fGenerateMipmap(target);
> +        gl->fTexParameteri(target, LOCAL_GL_TEXTURE_MIN_FILTER, tex->mMinFilter);

So do we really want to keep this after all? I didn't remember that. r- until clarification.
Comment 19 Joe Drew (not getting mail) 2012-02-16 14:38:15 PST
Created attachment 598000 [details] [diff] [review]
Translate from fb 0 to our offscreen FB at bind time, not draw time, v5

Fixing comments.
Comment 20 Joe Drew (not getting mail) 2012-02-16 14:38:50 PST
Created attachment 598001 [details] [diff] [review]
mark tests as failing v2

Nope, did not mean to include that hunk. Removed.
Comment 21 Jeff Gilbert [:jgilbert] 2012-02-16 14:57:40 PST
Comment on attachment 598000 [details] [diff] [review]
Translate from fb 0 to our offscreen FB at bind time, not draw time, v5

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

\o/

Note You need to log in before you can comment on or make changes to this bug.