598143 - Aquarium demo flickers madly if MOZ_ACCELERATED=1 on Mac

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Build: 2010-09-20 Mac nightly (but I tried some other local builds, etc, and the problem is there too).

STEPS TO REPRODUCE:
1)  env MOZ_ACCELERATED=1 firefox
2)  Load the url in the url field (which is a WebGL demo)

EXPECTED RESULTS: Super-fast rendering of swimming fish

ACTUAL RESULTS: Super-fast rendering of solid black

Comment 2

[Boris Zbarsky [:bzbarsky]]

OK, good news and bad news.  The fix for bug 596050 made the black go away.  Now the aquarium backdrop flickers like mad, though, and the fish flicker some too.

Comment 3

[Joe Drew (not getting mail)]

Certain elements flicker in and out of drawing - different things with each paint. Very odd.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Weird; likely a non-power-of-two texture issue, with default filters using mipmaps.  Also, SetTextureFilter would have been a better name for the function, but that's a little bikesheddy.

Comment 5

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

That would be a great explanation, but looking at the patch in bug 596050, it's not using mipmaps:


+void LayerOGL::ApplyFilter(gfxPattern::GraphicsFilter aFilter)
+{
+  if (aFilter == gfxPattern::FILTER_NEAREST) {
+    gl()->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MIN_FILTER, LOCAL_GL_NEAREST);
+    gl()->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MAG_FILTER, LOCAL_GL_NEAREST);
+  } else {
+    if (aFilter != gfxPattern::FILTER_GOOD) {
+      NS_WARNING("Unsupported filter type!");
+    }
+    gl()->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MIN_FILTER, LOCAL_GL_LINEAR);
+    gl()->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MAG_FILTER, LOCAL_GL_LINEAR);
+  }
+}


While we are discussing black textures, if we are ever going to be running this on OpenGL ES (e.g. Android...) we absolutely must set the WRAP_S and WRAP_T modes to GL_CLAMP_TO_EDGE. Otherwise we will for sure get solid black. See GL ES spec section 3.8.2 for details.

Comment 6

[Joe Drew (not getting mail)]

Benoit, feel free to reassign as necessary.

Comment 7

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

Err, I don't have a Mac to test this. But the first thing I would do is apply the GL debug mode patch from bug 597881, you can remove the THREAD_LOCAL stuff as it's not needed for now and doesn't compile on Mac, and run with

   MOZ_GL_DEBUG_MODE=1

which will abort if a GL function is called on the wrong GL context, or perhaps even

   MOZ_GL_DEBUG_MODE_ABORT_ON_ERROR=1
which will in addition abort if any GL error occurs.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I just tried that; I get flicker, but no aborts (with both those env vars set).

Comment 9

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

So, Boris ran this in OpenGL debug mode (bug 597881) and noted that there was no flickering.

The only relevant that the OpenGL debug mode makes here is that it calls glFinish() after every GL call. So the conclusion is that we were missing a glFinish() call!

5 minutes and a discussion with Joe and Jeff later, this is confirmed and Joe's made a patch that fixes it...

Comment 10

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

For the record, doing glFlush() over glFinish() resulted in a +25% speed increase with 1000 fish, because it makes the graphics not block JS execution...

Comment 11

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

doing glFlush() *instead of* glFinish()

Comment 12

[Joe Drew (not getting mail)]

Attachment: flush the canvas context before reading from it

We're reading from the shared WebGL framebuffer, but not flushing it. This doesn't work because WebGL is rendering to a different context from the one we're rendering to.

Comment 13

[Joe Drew (not getting mail)]

Realistically, this should have blocked beta 7 all along - WebGL's a major feature of it, and so is OpenGL composition.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 479587 [details] [diff] [review]
flush the canvas context before reading from it

yuck.  OK for now, there are some spec changes in WebGL that should hopefully make this not needed.  However -- I thought that re-binding the context as a texture should have done an implicit flush, but maybe not?  It sucks that we have to switch contexts here too...

Comment 15

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/93bdbcb2833f

Comment 16

[Boris Zbarsky [:bzbarsky]]

Benoit, thanks!

Confirmed that the problem is fixed; I now get my 70fps with no blinking at 50 fish, and a beautiful 40fps with 1000 fish (compared to 25fps and 12fps without MOZ_ACCELERATED=1).

Comment 17

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

(In reply to comment #16)
> Benoit, thanks!

That was an amazing example of collaborative bug fixing! There are 4 people to thank equally: Joe, Jeff, you and me.

Joyous!!! (as Joe uses to say)