726396 - Huge performance regression on WebGL with ANGLE+D3D10

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/d71dab82fff4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120211 Firefox/13.0a1 ID:20120211031145

Huge performance regression on WebGL.

Reproducible: Always

Steps to Reproduce:
1. Start Firefox with new profile
2. Go http://people.mozilla.org/~jmuizelaar/fishie/fishie-fast.html

Actual Results:
  20fps / 20fish

Expected Results:
  60fps / 20fish

Regression window(m-c)
Last good:
http://hg.mozilla.org/mozilla-central/rev/3c1cdbbea964
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120207 Firefox/13.0a1 ID:20120207010203
First bad:
http://hg.mozilla.org/mozilla-central/rev/2b61af9d18ee
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120207 Firefox/13.0a1 ID:20120207023403
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3c1cdbbea964&tochange=2b61af9d18ee


Regression window(m-i)
Last good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e2db006298c2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120206 Firefox/13.0a1 ID:20120206185103
First bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/fa92500f4ad2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120206 Firefox/13.0a1 ID:20120206191607
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e2db006298c2&tochange=fa92500f4ad2


Triggered by: Bug 720467

Comment 1

[Alice0775 White]

Maybe duplication of bug 724476

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Probably a dup of bug 725747, which has a patch.

Comment 3

[Alice0775 White]

The try server build( https://bugzilla.mozilla.org/show_bug.cgi?id=725747#c10 ) does not help.

Comment 4

[Alice0775 White]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> Probably a dup of bug 725747, which has a patch.

The try server build on https://bugzilla.mozilla.org/show_bug.cgi?id=725747#c14 and https://bugzilla.mozilla.org/show_bug.cgi?id=725747#c15 ,
both nothing changed.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Jeff, can you reproduce this on Windows, or is this again a driver-specific issue?

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Alice, what doess about:support -> Graphics say?

Comment 7

[Alice0775 White]

Graphics
   Adapter Description : ATI Radeon HD 4300/4500 Series
   Vendor ID : 0x1002
   Device ID : 0x954f
   Adapter RAM : 512
   Adapter Drivers : aticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64
   Driver Version : 8.930.0.0
   Driver Date : 12-5-2011
   Direct2D Enabled : true
   DirectWrite Enabled : true (6.1.7601.17563)
   ClearType Parameters : Gamma: 2200 Pixel Structure: RGB ClearType Level: 50 Enhanced Contrast: 200 
   WebGL Renderer : Google Inc. -- ANGLE (ATI Radeon HD 4300/4500 Series) -- OpenGL ES 2.0 (ANGLE 1.0.0.963)
   GPU Accelerated Windows : 1/1 Direct3D 10
   AzureBackend : direct2d

Comment 8

[Kelsey Gilbert [:jgilbert]]

I believe this is actually bug 724476, but the fix for bug 720467 triggered similar performance problems as XP has in bug 720467.

Comment 9

[Alice0775 White]

(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> I believe this is actually bug 724476, but the fix for bug 720467 triggered
> similar performance problems as XP has in bug 720467.

The try server build( https://bugzilla.mozilla.org/show_bug.cgi?id=724476#c18 )
35fps / 20fish 
still slower than prior landing of Bug 720467 60fps / 20fish

Comment 10

[Kelsey Gilbert [:jgilbert]]

Is there any regression when rendering with native gl? (webgl.prefer-native-gl=true)

Comment 11

[Alice0775 White]

(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> Is there any regression when rendering with native gl?
> (webgl.prefer-native-gl=true)

under condition of webgl.prefer-native-gl=true

prior landing of Bug 720467 : 45-47fps / 20fish 
after landing of Bug 720467 : 45-47fps / 20fish 
The try server build( https://bugzilla.mozilla.org/show_bug.cgi?id=724476#c18 ): 44-45fps / 20fish

Comment 12

[Alice0775 White]

In addition to Comment #0
In local build:
Last Good Changeset : e2db006298c2
First Bad Changeset : 7f6db301b290

This is regressed by 
7f6db301b290	Joe Drew — Bug 720467 - Switch to using FBOs for WebGL, and support sharing textures between EGL contexts. r=jgilbert

Comment 13

[Alice0775 White]

FYI,The performance is back when I simply backed 7f6db301b290 out from tip.

Comment 14

[Joe Drew (not getting mail)]

How is performance on trunk now that I've fixed bug 725747?

Comment 15

[Alice0775 White]

(In reply to Joe Drew (:JOEDREW!) from comment #14)
> How is performance on trunk now that I've fixed bug 725747?

http://hg.mozilla.org/mozilla-central/rev/f88a05e00f47
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120217 Firefox/13.0a1 ID:20120217034049
Graphics section(see same as Comment#7)
18-20fps/20fish  --- Nothing changed.

Comment 16

[Kelsey Gilbert [:jgilbert]]

I am investigating if FBOs are 'being slow' for some reason on ANGLE.

Comment 17

[Kelsey Gilbert [:jgilbert]]

I think I know what's up, pending tests results from a build.

I think ANGLE is no longer supplying a d3d handle to d3d layers, and the fallback (which should never happen) is /slow/.

Comment 18

[Kelsey Gilbert [:jgilbert]]

I was correct, we were not using d3d share handles, resulting in a read-back-and-forth. Patch forthcoming.

Comment 19

[Kelsey Gilbert [:jgilbert]]

Attachment: Fix logic for d3d share handles and EGL PBuffer creation

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 599319 [details] [diff] [review]
Fix logic for d3d share handles and EGL PBuffer creation

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

R+, but the code paths between pbuffers and not pbuffers keep making this code very difficult to understand. Are we going to completely get rid of the pbuffer paths? if yes please file a followup bug for that.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Try running at:
https://tbpl.mozilla.org/?tree=Try&rev=436a5bc4aa91

Follow up bug for removing PBuffer support at Bug 729285.

Comment 22

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29ee49bff6a1

Comment 23

[Daniel Holbert [:dholbert]]

Backed out, on suspicion of causing webgl conformance test failures:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/29ee49bff6a1

Comment 24

[Kelsey Gilbert [:jgilbert]]

Attachment: Fix logic for d3d share handles and EGL PBuffer creation

There was a crippling bug in the logic for blitting between the draw and read FBOs: With PBuffers, the read FBO is 0, and we naively use our BindDrawFBO when we swap our draw/read buffers for the blit. BUT, our BindDrawFBO call emulates '0' as 'mOffscreenDrawFBO', which is not what we want at all.

Comment 25

[Kelsey Gilbert [:jgilbert]]

I should note that this issue caused the blitting to fail, resulting in an empty canvas, triggering a huge number of mochitest fails on both win7 (EGL PBuffers) and winxp (WGL PBuffers still, surprisingly).

Comment 26

[Kelsey Gilbert [:jgilbert]]

Try build running at:
https://tbpl.mozilla.org/?tree=Try&rev=58a248914434

Comment 27

[Alice0775 White]

(In reply to Jeff Gilbert [:jgilbert] from comment #26)
> Try build running at:
> https://tbpl.mozilla.org/?tree=Try&rev=58a248914434

The try build works well.

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 599864 [details] [diff] [review]
Fix logic for d3d share handles and EGL PBuffer creation

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

I think that all user-facing parts of GLContext, that map 0 to the actual buffers, should be renamed to mention 'User'. So one wouldn't accidentally use one when the other is meant. Makes sense?

Comment 29

[Jim Jeffery not reading bug-mail 1/2/11]

Just downloaded the latest hourly build, and its worse here for me: 

2fps, 20 fish 1680x922

Adapter DescriptionATI Radeon HD 3200 GraphicsVendor ID0x1002Device ID0x9610Adapter RAM256Adapter Driversaticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64Driver Version8.881.0.0Driver Date7-28-2011Direct2D EnabledtrueDirectWrite Enabledtrue (6.1.7601.17563)ClearType ParametersDISPLAY1 [ Gamma: 2200 Pixel Structure: RGB ClearType Level: 100 Enhanced Contrast: 300 ] DISPLAY4 [ Gamma: 2200 Pixel Structure: RGB ClearType Level: 100 Enhanced Contrast: 50 ] WebGL RendererGoogle Inc. -- ANGLE (ATI Radeon HD 3200 Graphics) -- OpenGL ES 2.0 (ANGLE 1.0.0.963)GPU Accelerated Windows1/1 Direct3D 10

        
AzureBackenddirect2d

tested cset: https://hg.mozilla.org/mozilla-central/rev/15d7708672c1

Comment 30

[Alice0775 White]

Today'nightly is Landed Bug 724476,and this is slightly improved, 
But still slower than prior landing of Bug 720467(60fps / 20fish)(986x895)

45fps / 20fish 
http://hg.mozilla.org/mozilla-central/rev/ce20e9b47e9c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120225 Firefox/13.0a1 ID:20120225031723

Comment 31

[Kelsey Gilbert [:jgilbert]]

This patch will be needed to bring perf back up to what it was before 720467, though it's great that 724476 improved it markedly. I am trying to fix this properly, but I may have to do a minimal fix (at first) to hit a release window. The more proper fix is hitting issues, still.

Comment 32

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 599864 [details] [diff] [review]
Fix logic for d3d share handles and EGL PBuffer creation

Still working on the try fails. We're getting FBO binding prediction mismatches.

Comment 33

[Kelsey Gilbert [:jgilbert]]

Attachment: Fix logic for d3d share handles and EGL PBuffer creation

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=59c2c9933ac8

Looks like some bad-slaves, but clean otherwise.
This also cleans up our User/Internal FBO separation, which was causing FBO mispredictions on linux.

Comment 34

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 603227 [details] [diff] [review]
Fix logic for d3d share handles and EGL PBuffer creation

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

Self r- with notes for tomorrow.
bjacob, thoughts on my notes? There are a couple that are questions.

::: gfx/gl/GLContext.h
@@ +953,5 @@
>  
> +        bool abort = false;
> +
> +        if (mInternalsBindingDrawFBO) {
> +            printf_stderr("!!! Draw FBO bound internally! (current internal: %d, user: %d)\n", ret, mUserBoundDrawFBO);

These should maybe use a more standard error format? Also possibly NS_ERROR with printf_stderr for extra info.

@@ +1068,3 @@
>          return prev;
>      }
> +/*

Forgot to delete these unused functions.
BindUserDraw/ReadFBO(0) is a better solution.

@@ +1150,5 @@
>          // flip read/draw for blitting
> +        GLuint prevDraw = GetUserDrawFBO();
> +        GLuint prevRead = GetUserReadFBO();
> +
> +        NS_ASSERTION(SupportsOffscreenSplit(), "Doesn't support offscreen split?");

We should probably assert this during init and assume it thereafter.

@@ +1152,5 @@
> +        GLuint prevRead = GetUserReadFBO();
> +
> +        NS_ASSERTION(SupportsOffscreenSplit(), "Doesn't support offscreen split?");
> +
> +        BindInternalDrawFBO(mOffscreenReadFBO);

Possibly add a note about these two bindings are 'internal', and how it puts the bind points into 'internal' mode.

@@ +1162,5 @@
>                               0, 0, width, height,
>                               LOCAL_GL_COLOR_BUFFER_BIT,
>                               LOCAL_GL_NEAREST);
>  
> +        BindUserDrawFBO(prevDraw);

Add comment regarding how binding a 'user' FBO brings us out of 'internal' mode.

::: gfx/gl/GLContextProviderGLX.cpp
@@ +781,1 @@
>          return IsExtensionSupported("GL_EXT_framebuffer_object");

The order of these lines seems a little backwards.

Comment 35

[Alice0775 White]

(In reply to Jeff Gilbert [:jgilbert] from comment #33)
> Created attachment 603227 [details] [diff] [review]
> Fix logic for d3d share handles and EGL PBuffer creation
> 
> Try run:

This try build fails to initialize WebGL.
http://hg.mozilla.org/try/rev/59c2c9933ac8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120305 Firefox/13.0a1

Graphics       
  Adapter Description : ATI Radeon HD 4300/4500 Series
  Vendor ID : 0x1002
  Device ID : 0x954f
  Adapter RAM : 512
  Adapter Drivers : aticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64
  Driver Version : 8.930.0.0
  Driver Date : 12-5-2011
  Direct2D Enabled : true
  DirectWrite Enabled : true (6.1.7601.17563)
  ClearType Parameters : Gamma: 2200 Pixel Structure: RGB ClearType Level: 50 Enhanced Contrast: 200 
  WebGL Renderer : false
  GPU Accelerated Windows : 1/1 Direct3D 10
  AzureBackend : direct2d

Comment 36

[Kelsey Gilbert [:jgilbert]]

Is there any DEBUG spew around where it fails initialization?

Comment 37

[Alice0775 White]

(In reply to Jeff Gilbert [:jgilbert] from comment #36)
> Is there any DEBUG spew around where it fails initialization?
Sorry,I do not know how to DEBUG WebGL.

Error console:
Timestamp: 2012/03/06 21:41:32
Warning: WebGL: Can't get a usable WebGL context
Source File: http://people.mozilla.org/~jmuizelaar/fishie/fishie-fast.html
Line: 47

Comment 38

[Alice0775 White]

Attachment: console log

Console log of DEBUG build attached
( download from /pub/mozilla.org/firefox/try-builds/[e-mail]-59c2c9933ac8/try-win32-debug/firefox-13.0a1.en-US.win32.zip )

Comment 39

[Kelsey Gilbert [:jgilbert]]

Attachment: Fix logic for d3d share handles and EGL PBuffer creation

Updated as per self-review, and added more NS_WARNINGs for various failure situations.

Comment 40

[Kelsey Gilbert [:jgilbert]]

New try build running at:
https://tbpl.mozilla.org/?tree=Try&rev=21894a7a1ff6

Comment 41

[Kelsey Gilbert [:jgilbert]]

Attachment: Fix logic for d3d share handles and EGL PBuffer creation

Got a logic backwards in for GLX.

Comment 42

[Kelsey Gilbert [:jgilbert]]

Attachment: Fix logic for d3d share handles and EGL PBuffer creation

Forgot to qfold. ><

Comment 43

[Kelsey Gilbert [:jgilbert]]

New try:
https://tbpl.mozilla.org/?tree=Try&rev=d7fc1cf501d2

Comment 44

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 603537 [details] [diff] [review]
Fix logic for d3d share handles and EGL PBuffer creation

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

Please write a quick explanation of what was wrong and how your patch fixes it.

Comment 45

[Alice0775 White]

(In reply to Jeff Gilbert [:jgilbert] from comment #43)
> New try:
> https://tbpl.mozilla.org/?tree=Try&rev=d7fc1cf501d2

This try build works very well.
And I got 60fps / 20fish(986x895px) ;)

Comment 46

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #44)
> Comment on attachment 603537 [details] [diff] [review]
> Fix logic for d3d share handles and EGL PBuffer creation
> 
> Review of attachment 603537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please write a quick explanation of what was wrong and how your patch fixes
> it.

There are a few things going on in this patch. The main one is that the logic behind bug 720467 is incorrect: EGL *must* use PBuffers on windows to provide an EGLSurface, which is what the ANGLE interop works with. No PBuffers, no interop. Further, some of the logic involved in EGL with and without PBuffers was not quite correct. Bug 720467 also added some incorrect logic to GetD3DShareHandle(), which has been removed.

With that fixed, I was hitting errors on Linux Debug builds, because we were hitting internal bound FBO mispredictions. One of the major things this patch adds is a distinction between working with the semi-emulated 'user' bindings (where binding 0 actually binds to the offscreen buffer) and manual 'internal' bindings, which is needed for blitting between the offscreen buffers. I included checks (on debug) to makes sure we never fail to rebind to 'user' bindings after going into what I call 'internal mode'. We should never call raw_fBindFramebuffer in the general case, as it will kill our bind shadowing.

For extra care, I initialize GLContexts to be /in internal mode/, so we must be sure we 'user'-bind to 0 with InitFramebuffers(). This assures that we are sure our 'user' bindings are always correct.

Comment 47

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 603537 [details] [diff] [review]
Fix logic for d3d share handles and EGL PBuffer creation

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

::: gfx/gl/GLContext.cpp
@@ +1161,5 @@
>      const bool useDepthStencil =
>              !mIsGLES2 || IsExtensionSupported(OES_packed_depth_stencil);
>  
>      // save a few things for later restoring
> +    curBoundFramebufferDraw = GetUserDrawFBO();

As discussed on IRC, I would prefer to keep 'Bound' in the name, as it makes it clear that what it returns is a current binding, as opposed to just something you happen to have in your sleeve.

::: gfx/gl/GLContext.h
@@ +939,5 @@
>      }
>  
> +#ifdef DEBUG
> +    bool mInternalsBindingDrawFBO;
> +    bool mInternalsBindingReadFBO;

I could have used a comment to understand this stuff and a better name for these variables. Especially the fact that binding is what switches mode.

@@ +967,4 @@
>          }
> +
> +        if (abort)
> +            NS_ABORT();

Clever, so we get to see both error messages.

@@ +1135,5 @@
> +        // Store current bindings for restoring later
> +        GLuint prevDraw = GetUserDrawFBO();
> +        GLuint prevRead = GetUserReadFBO();
> +
> +        NS_ASSERTION(SupportsOffscreenSplit(), "Doesn't support offscreen split?");

What happens if this assertion fails? should this be a hard assertion like NS_ABORT_IF_FALSE?

@@ +1206,5 @@
>          AfterGLReadCall();
>      }
>  
>      void ForceDirtyFBOs() {
> +        GLuint draw = SwapUserReadFBO(0);

There seems to be a bug here, that preexists your patch:  this code looks like it wanted SwapUserDrawFBO.

@@ +1217,4 @@
>      }
>  
>      void BlitDirtyFBOs() {
> +        GLuint read = SwapUserReadFBO(0);

This looks OK, but given what we just found in ForceDirtyFBO, please proof-read this function again.

@@ +1624,5 @@
>              printf_stderr("Requested level of multisampling is unavailable, continuing without multisampling\n");
>          }
>  
> +        if (ResizeOffscreenFBO(aSize, aUseReadFBO, true))
> +            return true;

It's a bit confusing that this is a early-success return, while above we have early-fail returns.

@@ +1626,5 @@
>  
> +        if (ResizeOffscreenFBO(aSize, aUseReadFBO, true))
> +            return true;
> +
> +        NS_WARNING("ResizeOffscreenFBO failed to resize AA context with AA disabled!");

"with AA disabled" is a bit confusing, it seems to refer to a preference or blacklisting or something. Maybe "even after falling back to non-AA"?

Comment 48

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #47)
> Comment on attachment 603537 [details] [diff] [review]
> Fix logic for d3d share handles and EGL PBuffer creation
> 
> Review of attachment 603537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +1624,5 @@
> >              printf_stderr("Requested level of multisampling is unavailable, continuing without multisampling\n");
> >          }
> >  
> > +        if (ResizeOffscreenFBO(aSize, aUseReadFBO, true))
> > +            return true;
> 
> It's a bit confusing that this is a early-success return, while above we
> have early-fail returns.
> 
Here we actually have early-succeed for the previous ResizeOffscreenFBO(), but in the case of a non-AA canvas, there's no use retrying without AA, so we fail out. I would prefer to keep it as is.

Comment 49

[Kelsey Gilbert [:jgilbert]]

Attachment: Fix logic for d3d share handles, EGL PBuffer creation, and user/internal FBO bindings

New try run:
https://tbpl.mozilla.org/?tree=Try&rev=fed50a60932c

Comment 50

[Kelsey Gilbert [:jgilbert]]

Attachment: Fix logic for d3d share handles, EGL PBuffer creation, and user/internal FBO bindings

Previous patch was bad.
New patch, new try:
https://tbpl.mozilla.org/?tree=Try&rev=1d99b9d7cc4d

Comment 51

[Alice0775 White]

(In reply to Jeff Gilbert [:jgilbert] from comment #50)
> Created attachment 603948 [details] [diff] [review]
> Fix logic for d3d share handles, EGL PBuffer creation, and user/internal FBO
> bindings
> 
> Previous patch was bad.
> New patch, new try:
> https://tbpl.mozilla.org/?tree=Try&rev=1d99b9d7cc4d

This try build also works very well.

Comment 52

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6610af68b24c

Comment 53

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/6610af68b24c