571831 - Use FBOs for WebGL

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Oleg Romashin (:romaxa)]

On Maemo platform CreatePBuffer is not supported. and GL Canvas currently requesting PBuffer surface, and if that failed, then it stop work
http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp#177

It would be nice to check for PBuffer support, and if that is not supported, then use something else... like FBO, or create offscreen window surface like X-Surface..

Comment 1

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

Attachment: 1 - do proper resource cleanup on context destruction

With shared contexts, we need to be sure we clean up after ourselves and after WebGL.  Do so whenever we destroy a webgl context.

Comment 2

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

Attachment: 2 - Use FBOs for WebGL

Use FBOs for rendering, via new CreateOffscreen GLContextProvider API.  We set up the FBO ourselves, and provide it to the Canvas layers for rendering.

Comment 3

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

Comment on attachment 454576 [details] [diff] [review]
1 - do proper resource cleanup on context destruction

+static PLDHashOperator
+DeleteTextureFunction(const PRUint32& aKey, WebGLTexture *aValue, void *aData)
+{

The aKey parameter here is unused, so it should be unnamed to avoid compiler warnings.

DeleteTextureFunction(const PRUint32& /*aKey*/, WebGLTexture *aValue, void *aData)

Comment 4

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

Comment on attachment 454577 [details] [diff] [review]
2 - Use FBOs for WebGL

I just have 1 question and 1 comment.

 WebGLContext::SetDimensions(PRInt32 width, PRInt32 height)
 {
+    if (mWidth == width && mHeight == height)
+        return NS_OK;

Question: what if for some reason the canvas size was actually 0x0 pixels? Since 0x0 is the size the WebGLContext constructor initializes with, SetDimentions(0,0) is a no-operation. Is that OK?

+    bool alpha = mContextFormat.alpha > 0;

Is it possible to rename bool alpha to bool hasAlpha ? Otherwise it's a bit confusing that one alpha is bool and not the other.

Comment 5

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

Actually, I have one more small comment:

+#ifdef USE_GLES2
+    const bool isMobile = true;
+#else
+    const bool isMobile = false;
+#endif

is it really a good idea to equate GLES==mobile? Why not call that isGLES ?

Comment 6

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

Attachment: combined & updated for new offscreen API

Comment 7

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

Attachment: updated again for new API

Comment 8

[Michael Monreal [:micmon]]

bjacob asked me to give this patch a try during the summit because webgl does not work for me at all (on intel i945 graphics). Sadly, the patch doesn't apply to the current code in central anymore.

Comment 9

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

intel 945 doesn't have programmable shaders, so it can't be used for WebGL. The earliest Intel chipsets with programmable shaders are the GMA X3000 generation from 2006,

http://en.wikipedia.org/wiki/Intel_GMA#History

Sorry!

Comment 10

[Michael Monreal [:micmon]]

(In reply to comment #8)
> [...] on intel i945 graphics) [...]

...or so I was thinking. I never really understood intel's naming scheme and seeing the driver module being called "i915" certainly didn't help. Anyway, turns out I actually have one of the very common X3100 (GM965). But I didn't mean to hijack this bug, just wanted to say that the patch needs updating.

Comment 11

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

Sure. Don't worry, Vlad's going to do that and push it... when he has time :-)

Comment 13

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

Attachment: updated for offscreen glcontext api

Ok, final change.  This uses the offscreen glcontext API introduced in bug 575469, and pushes the decision of pbuffers vs FBOs into the specific glcontext.  The linux GLX provider should be using FBOs and context sharing exclusively.

Comment 14

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

Comment on attachment 458062 [details] [diff] [review]
updated for offscreen glcontext api

Just one remark:

     // increment the generation number
     ++mGeneration;
 
+#if 0
+    if (mGeneration > 0) {
+        // XXX dispatch context lost event
+    }
+#endif
+


I understand this is just a placeholder, but this needs to go before the ++mGeneration, or it will dispatch a lost event on the first call.

Comment 15

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

(In reply to comment #13)
> The linux GLX provider should be using FBOs and context sharing
> exclusively.

Why special-case GLX here? I understand that many GLX systems don't support Pbuffers, but if they don't, the Pbuffer creation just fails so you know it. So why bother special-case-ing GLX?

Comment 16

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

Mainly because pbuffer set up on linux is somewhat more complex than on other systems, and it seemed like a pain to maintain it; we can always put it back, though.

Comment 17

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

http://hg.mozilla.org/mozilla-central/rev/e6b9c0f1c051