Last Comment Bug 726396 - Huge performance regression on WebGL with ANGLE+D3D10
: Huge performance regression on WebGL with ANGLE+D3D10
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: 13 Branch
: x86 Windows 7
: -- normal with 2 votes (vote)
: mozilla13
Assigned To: Jeff Gilbert [:jgilbert]
:
Mentors:
http://people.mozilla.org/~jmuizelaar...
Depends on: 724476 769949
Blocks: 720467 723444 729272
  Show dependency treegraph
 
Reported: 2012-02-11 20:30 PST by Alice0775 White
Modified: 2012-07-05 15:29 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix logic for d3d share handles and EGL PBuffer creation (3.44 KB, patch)
2012-02-21 13:11 PST, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Fix logic for d3d share handles and EGL PBuffer creation (5.45 KB, patch)
2012-02-22 21:17 PST, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
jgilbert: review-
Details | Diff | Splinter Review
Fix logic for d3d share handles and EGL PBuffer creation (23.84 KB, patch)
2012-03-06 04:08 PST, Jeff Gilbert [:jgilbert]
jgilbert: review-
Details | Diff | Splinter Review
console log (12.69 KB, text/plain)
2012-03-06 05:21 PST, Alice0775 White
no flags Details
Fix logic for d3d share handles and EGL PBuffer creation (29.89 KB, patch)
2012-03-06 16:06 PST, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Fix logic for d3d share handles and EGL PBuffer creation (29.89 KB, patch)
2012-03-06 17:45 PST, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Fix logic for d3d share handles and EGL PBuffer creation (29.89 KB, patch)
2012-03-06 17:50 PST, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review-
Details | Diff | Splinter Review
Fix logic for d3d share handles, EGL PBuffer creation, and user/internal FBO bindings (30.38 KB, patch)
2012-03-07 16:05 PST, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Fix logic for d3d share handles, EGL PBuffer creation, and user/internal FBO bindings (30.39 KB, patch)
2012-03-07 18:41 PST, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review

Description Alice0775 White 2012-02-11 20:30:48 PST
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 2012-02-11 20:38:11 PST
Maybe duplication of bug 724476
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-11 22:28:59 PST
Probably a dup of bug 725747, which has a patch.
Comment 3 Alice0775 White 2012-02-13 15:43:05 PST
The try server build( https://bugzilla.mozilla.org/show_bug.cgi?id=725747#c10 ) does not help.
Comment 4 Alice0775 White 2012-02-14 22:41:42 PST
(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) 2012-02-15 04:23:32 PST
Jeff, can you reproduce this on Windows, or is this again a driver-specific issue?
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-02-15 04:24:10 PST
Alice, what doess about:support -> Graphics say?
Comment 7 Alice0775 White 2012-02-15 04:44:48 PST
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 Jeff Gilbert [:jgilbert] 2012-02-16 12:11:20 PST
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 2012-02-16 14:42:11 PST
(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 Jeff Gilbert [:jgilbert] 2012-02-16 15:07:08 PST
Is there any regression when rendering with native gl? (webgl.prefer-native-gl=true)
Comment 11 Alice0775 White 2012-02-16 15:21:36 PST
(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 2012-02-16 20:38:58 PST
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 2012-02-17 00:28:49 PST
FYI,The performance is back when I simply backed 7f6db301b290 out from tip.
Comment 14 Joe Drew (not getting mail) 2012-02-17 05:57:36 PST
How is performance on trunk now that I've fixed bug 725747?
Comment 15 Alice0775 White 2012-02-17 06:22:36 PST
(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 Jeff Gilbert [:jgilbert] 2012-02-17 07:08:37 PST
I am investigating if FBOs are 'being slow' for some reason on ANGLE.
Comment 17 Jeff Gilbert [:jgilbert] 2012-02-17 13:03:32 PST
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 Jeff Gilbert [:jgilbert] 2012-02-21 12:49:21 PST
I was correct, we were not using d3d share handles, resulting in a read-back-and-forth. Patch forthcoming.
Comment 19 Jeff Gilbert [:jgilbert] 2012-02-21 13:11:49 PST
Created attachment 599319 [details] [diff] [review]
Fix logic for d3d share handles and EGL PBuffer creation
Comment 20 Benoit Jacob [:bjacob] (mostly away) 2012-02-21 13:16:37 PST
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 Jeff Gilbert [:jgilbert] 2012-02-21 13:37:33 PST
Try running at:
https://tbpl.mozilla.org/?tree=Try&rev=436a5bc4aa91

Follow up bug for removing PBuffer support at Bug 729285.
Comment 23 Daniel Holbert [:dholbert] 2012-02-22 16:41:41 PST
Backed out, on suspicion of causing webgl conformance test failures:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/29ee49bff6a1
Comment 24 Jeff Gilbert [:jgilbert] 2012-02-22 21:17:38 PST
Created attachment 599864 [details] [diff] [review]
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 Jeff Gilbert [:jgilbert] 2012-02-22 21:20:19 PST
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 Jeff Gilbert [:jgilbert] 2012-02-22 21:24:24 PST
Try build running at:
https://tbpl.mozilla.org/?tree=Try&rev=58a248914434
Comment 27 Alice0775 White 2012-02-22 21:39:57 PST
(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) 2012-02-23 05:28:57 PST
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 2012-02-23 07:59:49 PST
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 2012-02-25 07:24:39 PST
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 Jeff Gilbert [:jgilbert] 2012-02-27 03:32:27 PST
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 Jeff Gilbert [:jgilbert] 2012-02-28 16:42:30 PST
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 Jeff Gilbert [:jgilbert] 2012-03-06 04:08:25 PST
Created attachment 603227 [details] [diff] [review]
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 Jeff Gilbert [:jgilbert] 2012-03-06 04:29:11 PST
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 2012-03-06 04:31:02 PST
(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 Jeff Gilbert [:jgilbert] 2012-03-06 04:39:10 PST
Is there any DEBUG spew around where it fails initialization?
Comment 37 Alice0775 White 2012-03-06 04:45:04 PST
(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 2012-03-06 05:21:37 PST
Created attachment 603239 [details]
console log

Console log of DEBUG build attached
( download from /pub/mozilla.org/firefox/try-builds/jgilbert@mozilla.com-59c2c9933ac8/try-win32-debug/firefox-13.0a1.en-US.win32.zip )
Comment 39 Jeff Gilbert [:jgilbert] 2012-03-06 16:06:20 PST
Created attachment 603508 [details] [diff] [review]
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 Jeff Gilbert [:jgilbert] 2012-03-06 16:07:51 PST
New try build running at:
https://tbpl.mozilla.org/?tree=Try&rev=21894a7a1ff6
Comment 41 Jeff Gilbert [:jgilbert] 2012-03-06 17:45:06 PST
Created attachment 603534 [details] [diff] [review]
Fix logic for d3d share handles and EGL PBuffer creation

Got a logic backwards in for GLX.
Comment 42 Jeff Gilbert [:jgilbert] 2012-03-06 17:50:02 PST
Created attachment 603537 [details] [diff] [review]
Fix logic for d3d share handles and EGL PBuffer creation

Forgot to qfold. ><
Comment 43 Jeff Gilbert [:jgilbert] 2012-03-06 17:51:49 PST
New try:
https://tbpl.mozilla.org/?tree=Try&rev=d7fc1cf501d2
Comment 44 Benoit Jacob [:bjacob] (mostly away) 2012-03-06 21:47:23 PST
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 2012-03-06 22:21:59 PST
(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 Jeff Gilbert [:jgilbert] 2012-03-07 02:37:35 PST
(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) 2012-03-07 15:33:51 PST
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 Jeff Gilbert [:jgilbert] 2012-03-07 16:00:46 PST
(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 Jeff Gilbert [:jgilbert] 2012-03-07 16:05:45 PST
Created attachment 603905 [details] [diff] [review]
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 Jeff Gilbert [:jgilbert] 2012-03-07 18:41:19 PST
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
Comment 51 Alice0775 White 2012-03-09 07:26:34 PST
(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 53 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-03-13 04:59:11 PDT
https://hg.mozilla.org/mozilla-central/rev/6610af68b24c

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