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.

Could we be thrashing the pixel format?

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.

This is a cleaned-up version of the above patch, and it's currently going through Try.

Jeff, the part that needs the most review IMO is BeforeGLDrawCall() and BeforeGLReadCall()

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.

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

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

I am concerned that this may be related to bug 724476.

This version of the patch actually binds non-0 FBOs, and thus does not entirely break WebGL. Try results pending.

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

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 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.

Addressed review comments.

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

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

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 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 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.

Fixing comments.

Nope, did not mean to include that hunk. Removed.

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/

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9a1bbc54586 https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2ded81da6a

https://hg.mozilla.org/mozilla-central/rev/c9a1bbc54586 https://hg.mozilla.org/mozilla-central/rev/6c2ded81da6a