The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla13

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: Joe Drew (not getting mail))

Tracking

Trunk
mozilla13
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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?
(Assignee)

Comment 2

5 years ago
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.
Assignee: nobody → joe
(Assignee)

Comment 3

5 years ago
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.
Attachment #595889 - Attachment is obsolete: true
Attachment #595940 - Flags: review?(jgilbert)
(Assignee)

Comment 4

5 years ago
Jeff, the part that needs the most review IMO is BeforeGLDrawCall() and BeforeGLReadCall()
(Assignee)

Comment 5

5 years ago
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

5 years ago
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

5 years ago
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
Blocks: 715782, 726245
Depends on: 726276
Blocks: 724476
I am concerned that this may be related to bug 724476.
(Assignee)

Comment 9

5 years ago
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.
Attachment #595940 - Attachment is obsolete: true
Attachment #595940 - Flags: review?(jgilbert)
Attachment #596693 - Flags: review?(jgilbert)

Comment 10

5 years ago
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
(Assignee)

Comment 11

5 years ago
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.
Attachment #596693 - Attachment is obsolete: true
Attachment #596693 - Flags: review?(jgilbert)
Attachment #597174 - Flags: review?(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.
Attachment #597174 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 597205 [details] [diff] [review]
Translate from fb 0 to our offscreen FB at bind time, not draw time, v4

Addressed review comments.
Attachment #597174 - Attachment is obsolete: true
Attachment #597205 - Flags: review?(jgilbert)

Comment 14

5 years ago
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

5 years ago
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
(Assignee)

Comment 16

5 years ago
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.
Attachment #597981 - Flags: review?(bjacob)
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.
Attachment #597205 - Flags: review?(jgilbert) → review-
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.
Attachment #597981 - Flags: review?(bjacob) → review-
(Assignee)

Comment 19

5 years ago
Created attachment 598000 [details] [diff] [review]
Translate from fb 0 to our offscreen FB at bind time, not draw time, v5

Fixing comments.
Attachment #597205 - Attachment is obsolete: true
Attachment #598000 - Flags: review?(jgilbert)
(Assignee)

Comment 20

5 years ago
Created attachment 598001 [details] [diff] [review]
mark tests as failing v2

Nope, did not mean to include that hunk. Removed.
Attachment #597981 - Attachment is obsolete: true
Attachment #598001 - Flags: review?(bjacob)
Attachment #598001 - Flags: review?(bjacob) → review+
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/
Attachment #598000 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 22

5 years ago
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13

Updated

5 years ago
Depends on: 728210
You need to log in before you can comment on or make changes to this bug.