725747 - [B2G] Huge regression in WebGL panning speed on Galaxy S 2 after switch to FBOs

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Joe Drew (not getting mail)]

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

[Kelsey Gilbert [:jgilbert]]

Could we be thrashing the pixel format?

Comment 2

[Joe Drew (not getting mail)]

Attachment: 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)]

Attachment: 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)]

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

Comment 5

[Joe Drew (not getting mail)]

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]

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/[e-mail]-72d2bdc33dba

Comment 7

[Mozilla RelEng Bot]

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/[e-mail]-6704dadaa0f6

Comment 8

[Kelsey Gilbert [:jgilbert]]

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

Comment 9

[Joe Drew (not getting mail)]

Attachment: 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]

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/[e-mail]-073b2c0d5872

Comment 11

[Joe Drew (not getting mail)]

Attachment: 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

[Kelsey Gilbert [:jgilbert]]

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)]

Attachment: Translate from fb 0 to our offscreen FB at bind time, not draw time, v4

Addressed review comments.

Comment 14

[Mozilla RelEng Bot]

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/[e-mail]-60c8ea3d1012

Comment 15

[Mozilla RelEng Bot]

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/[e-mail]-99746695a5d4

Comment 16

[Joe Drew (not getting mail)]

Attachment: 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

[Kelsey Gilbert [:jgilbert]]

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)]

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)]

Attachment: Translate from fb 0 to our offscreen FB at bind time, not draw time, v5

Fixing comments.

Comment 20

[Joe Drew (not getting mail)]

Attachment: mark tests as failing v2

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

Comment 21

[Kelsey Gilbert [:jgilbert]]

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/

Comment 22

[Joe Drew (not getting mail)]

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

Comment 23

[Ed Morley [:emorley]]

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