Last Comment Bug 615976 - Implement WebGL antialiasing
: Implement WebGL antialiasing
Status: RESOLVED FIXED
[tilt]
: dev-doc-complete
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: x86 All
: -- normal with 5 votes (vote)
: mozilla10
Assigned To: Jeff Gilbert [:jgilbert]
:
Mentors:
: 697490 (view as bug list)
Depends on: 681791 686083 688112 696093 696590 697490
Blocks: 695912
  Show dependency treegraph
 
Reported: 2010-12-01 13:28 PST by Alvaro
Modified: 2016-04-22 12:15 PDT (History)
32 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP for adding AA to WebGL (49.13 KB, patch)
2011-07-13 18:53 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
WIP fix for bug 615976 (59.38 KB, patch)
2011-07-20 12:22 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
WIP fix (74.47 KB, patch)
2011-07-29 20:27 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
WIP fix (96.59 KB, patch)
2011-08-11 03:04 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
WIP fix for bug 615976 (119.61 KB, patch)
2011-09-01 15:56 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 1: Fixes WebGL to default to attempting AA (856 bytes, patch)
2011-09-27 18:54 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 2: Adds support for split Draw/Read buffers to GLContext (42.25 KB, patch)
2011-09-27 18:55 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples' (7.49 KB, patch)
2011-09-27 18:59 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 4: Adds glRenderBufferStorageMultisample, extension detection for framebuffer_multisample (4.26 KB, patch)
2011-09-27 19:00 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths (36.30 KB, patch)
2011-09-27 19:02 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts (3.74 KB, patch)
2011-09-27 19:04 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5.2: Fixes for support for multisampled Linux/GLX GLContexts (1.28 KB, patch)
2011-09-27 19:05 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5.3: Force-disables multisampling on Mac/CGL due to severe display corruption (1.67 KB, patch)
2011-09-27 19:08 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5.3: Fixes layer compositing issue resulting in garbage WebGL canvases on Mac (1.57 KB, patch)
2011-09-29 17:55 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Fixes layer compositing with AA on Mac (1.61 KB, patch)
2011-09-29 18:14 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 1: Fixes WebGL to default to attempting AA (856 bytes, patch)
2011-10-05 06:31 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples' (45.44 KB, patch)
2011-10-05 06:32 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 2: Adds support for split Draw/Read buffers to GLContext (45.44 KB, patch)
2011-10-05 06:33 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples' (8.29 KB, patch)
2011-10-05 06:34 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 4: Adds glRenderBufferStorageMultisample, extension detection for framebuffer_multisample (4.26 KB, patch)
2011-10-05 06:35 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths (35.07 KB, patch)
2011-10-05 06:36 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts (3.98 KB, patch)
2011-10-05 06:36 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5.1.1: Fixes layer compositing with AA on Mac (2.42 KB, patch)
2011-10-05 06:38 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5.2: Fixes for support for multisampled Linux/GLX GLContexts (1.32 KB, patch)
2011-10-05 06:38 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed (13.25 KB, patch)
2011-10-05 06:39 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Prepatch 0.1: Printf of the resulting implementation of GLContext (4.36 KB, patch)
2011-10-05 06:51 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review-
Details | Diff | Splinter Review
Prepatch 0.2: Fixes GLContext::ResizeOffscreen leaving a mess on failure (13.33 KB, patch)
2011-10-05 06:53 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Prepatch 0.3: Adds method to disable GL implementations (3.26 KB, patch)
2011-10-05 06:55 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Prepatch 0.2: Fixes GLContext::ResizeOffscreen leaving a mess on failure (17.35 KB, patch)
2011-10-11 14:17 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review-
Details | Diff | Splinter Review
Prepatch 0.3: Adds method to disable GL implementations (3.25 KB, patch)
2011-10-11 14:18 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch 1: Fixes WebGL to default to attempting AA (856 bytes, patch)
2011-10-11 14:18 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch 2: Adds support for split Draw/Read buffers to GLContext (44.75 KB, patch)
2011-10-11 14:19 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples' (9.01 KB, patch)
2011-10-11 14:20 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch 4: Adds glRenderBufferStorageMultisample, extension detection for framebuffer_multisample (4.25 KB, patch)
2011-10-11 14:21 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths (33.75 KB, patch)
2011-10-11 14:22 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts (3.18 KB, patch)
2011-10-11 14:23 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5.1.1: Fixes layer compositing with AA on Mac (2.42 KB, patch)
2011-10-11 14:24 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5.2: Fixes for support for multisampled Linux/GLX GLContexts (1.32 KB, patch)
2011-10-11 14:25 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed (13.33 KB, patch)
2011-10-11 14:26 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 2: Adds support for split Draw/Read buffers to GLContext (33.75 KB, patch)
2011-10-13 08:13 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths (28.86 KB, patch)
2011-10-13 08:14 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts (3.92 KB, patch)
2011-10-13 08:15 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review-
Details | Diff | Splinter Review
Patch 5.1.1: Fixes layer compositing with AA on Mac (831 bytes, patch)
2011-10-13 08:16 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed (8.57 KB, patch)
2011-10-13 08:17 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch 2: Adds support for split Draw/Read buffers to GLContext (33.81 KB, patch)
2011-10-13 08:32 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review-
Details | Diff | Splinter Review
Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths (28.21 KB, patch)
2011-10-13 08:33 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review-
Details | Diff | Splinter Review
Patch 2: Adds support for split Draw/Read buffers to GLContext (26.62 KB, patch)
2011-10-18 12:11 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples' (9.19 KB, patch)
2011-10-18 12:16 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.msaa-level' (9.19 KB, patch)
2011-10-18 12:18 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch 4: Adds glRenderBufferStorageMultisample, extension detection for framebuffer_multisample (5.60 KB, patch)
2011-10-18 12:21 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths (27.89 KB, patch)
2011-10-18 13:58 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts (3.74 KB, patch)
2011-10-18 14:01 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Patch 5.1.1: Fixes layer compositing with AA on Mac (829 bytes, patch)
2011-10-18 14:03 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review+
Details | Diff | Splinter Review
Patch 5.2: Fixes for support for multisampled Linux/GLX GLContexts (1.32 KB, patch)
2011-10-18 14:04 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review+
Details | Diff | Splinter Review
Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed (4.26 KB, patch)
2011-10-18 14:05 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review+
Details | Diff | Splinter Review
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts (4.72 KB, patch)
2011-10-18 15:19 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Post-Patch Cleanup (5.11 KB, patch)
2011-10-21 16:47 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Post-Patch Cleanup (7.18 KB, patch)
2011-10-24 11:05 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review+
Details | Diff | Splinter Review

Description Alvaro 2010-12-01 13:28:13 PST
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b7) Gecko/20100101 Firefox/4.0b7
Build Identifier: 4.0b7

I reopen this issue (previously discussed in Bug 601629, Bug 579138 and Bug 539771).

In those bugs, it was said that the problem was Firefox not supporting canvas context creation attributes. This was said to be solved and the code at http://hg.mozilla.org/mozilla-central/rev/941c3d14e14d takes into account attributes like "antialiasHint".

Now, looking at the current WebGL spec (2010-11-22), https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/doc/spec/WebGL-spec.html#5.2.1 it says:

- the antialising hint attribute is "antialias"
- its default value is true. So, even without attributes, WebGL should render with antialiasing enabled. (Chrome does it AFAIK)

These are maybe recent changes in the spec, not sure...


Reproducible: Always

Steps to Reproduce:
1. Open any page with WebGL content
2. Look at the edges of shapes

Actual Results:  
No antialiasing (even with context attributes antialias:true or antialiasHint:true
(tried 2010-11-30 minefield)

Expected Results:  
smooth antialiased edges
Comment 1 Csaba Kozák [:WonderCsabo] 2010-12-16 23:56:56 PST
I'm experiencing this problem here:
http://bodybrowser.googlelabs.com/body.html#

It's very annoying. Chrome uses AA, check it out:
http://img651.imageshack.us/i/bodybrowser.jpg/
Comment 2 Alvaro 2010-12-19 10:17:37 PST
I was about to add a side-by-side screenshot like that, thanks.

BTW, it's not only that example, it's any WebGL. And even having the Nvidia driver configured to always use antialiasing overriding application settings, Firefox does no antialiasing.
Comment 3 Csaba Kozák [:WonderCsabo] 2010-12-19 10:43:36 PST
Yes, the problem affects every WebGL content.
Comment 4 Csaba Kozák [:WonderCsabo] 2011-01-14 05:04:13 PST
Benoit, Joe, Vladimir, any comment on this?
Comment 5 Vladimir Vukicevic [:vlad] [:vladv] 2011-01-14 09:43:56 PST
No real comment - antialiasing is not currently supported.  The context creation attributes are a hint, and you'll note that when you query the context's attributes, it will always be false.

We'd like to support it, but it's not in the critical path right now.
Comment 6 Rob Campbell [:rc] (:robcee) 2011-06-24 09:15:33 PDT
we'd love to have AA support in webgl for the Tilt visualization project.
Comment 7 Jeff Gilbert [:jgilbert] 2011-07-12 18:15:44 PDT
I have AA working with FBOs on Win7. (actually runs as fast as non-AA PBuffers on my machine)
Working on getting AA on PBuffers.

As an aside, it seems strange that we should be trying to use the older (apparently deprecated) PBuffers before trying to use the newer FBOs, especially since the last hardware to not support FBOs seems to be the Radeon 9700 series. Are we falling back form the older to the newer mechanism for compatibility reasons?
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2011-07-13 12:49:50 PDT
(In reply to comment #7)
> I have AA working with FBOs on Win7.

Excellent!
So there weren't any particular problems with interoperation with accelerated layers on Windows?

> (actually runs as fast as non-AA
> PBuffers on my machine)
> Working on getting AA on PBuffers.

As far as I know we don't use PBuffers anymore, we always use a FBO... I could be wrong? Anyway I'd focus 100% on FBOs unless you see a case where we use PBuffers (?)

> 
> As an aside, it seems strange that we should be trying to use the older
> (apparently deprecated) PBuffers before trying to use the newer FBOs,
> especially since the last hardware to not support FBOs seems to be the
> Radeon 9700 series. Are we falling back form the older to the newer
> mechanism for compatibility reasons?

Who said we should be trying to use PBuffers here? I agree that they're deprecated and there are other good reasons why FBOs work much better for WebGL. For example, a WebGL context may have to get resized without losing all the GL state, which FBOs allow easily but PBuffers AFAICS don't.
Comment 9 Jeff Gilbert [:jgilbert] 2011-07-13 13:03:10 PDT
True, but the current implementation through WGL tries PBuffers first, then falls back to FBOs if it fails. To get it to use FBOs, I had to force it to skip PBuffer generation.

Multisampled PBuffers aren't that much work (almost done), so I think it's probably a good idea to keep it for now, but as a fallback if FBOs fail, not the other way around.

The relevant comment for this is:
// Always try to create a pbuffer context first, because we
// want the context isolation.

I would imagine this is to prevent a bad WebGL state change from propagating to the browser?
Comment 10 Jeff Gilbert [:jgilbert] 2011-07-13 13:08:15 PDT
Also, no, there didn't seem to be any problems with integration, though I haven't gone over all the paths yet. As long as everything goes through ReadPixelsIntoImageSurface() it works, though.
Comment 11 Jeff Gilbert [:jgilbert] 2011-07-13 16:31:46 PDT
PBuffers now also have antialiasing.
Comment 12 Joe Drew (not getting mail) 2011-07-13 17:53:52 PDT
Can you upload your works in progress? A user repo of your patch queue is fine.
Comment 13 Jeff Gilbert [:jgilbert] 2011-07-13 18:53:07 PDT
Created attachment 545810 [details] [diff] [review]
WIP for adding AA to WebGL

I attached my WIP as-is, so it still has all my printfs for feeling out the code flow. I'm not sure I'm doing the PBuffer stuff properly. Also there is an issue with going back-and-forth between a PBuffer WebGL context and one using FBOs. (The FBO context stops updating to the screen. Looks like the blit just isn't happening properly)
Comment 14 Jeff Gilbert [:jgilbert] 2011-07-13 18:55:17 PDT
Also, with that patch you can add 'webgl.disable-pbuffers' to about:config to force it to skip PBuffer creation and use FBOs. (it will printf what it's using and whether it has aa)
This is all only targeted for the WGL path, so far.
Comment 15 Jeff Gilbert [:jgilbert] 2011-07-14 16:47:12 PDT
It also works for me on linux with no changes. Not that surprising since GLX uses the GLContext FBOs, but nice.
Comment 16 Jeff Gilbert [:jgilbert] 2011-07-15 18:06:35 PDT
Seems like there is a bug with the PBuffer path and this demo: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/demos/mozilla/spore/index.html
Textures disappear after rotating.
Comment 17 Jeff Gilbert [:jgilbert] 2011-07-18 12:27:13 PDT
Problem was that the demo was expecting the bound texture to persist across frames. Recording the current texture and rebinding to it after blitting the AA PBuffer to the offscreen texture fixes this.
Comment 18 Jeff Gilbert [:jgilbert] 2011-07-20 12:22:15 PDT
Created attachment 547203 [details] [diff] [review]
WIP fix for bug 615976

WIP works great on windows, though it hasn't been completely optimized yet. The logic is solid, but there may be paths that can be sped up.

Checking linux again next, then checking android.
Comment 19 Joe Drew (not getting mail) 2011-07-20 18:10:25 PDT
Comment on attachment 547203 [details] [diff] [review]
WIP fix for bug 615976

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

One thing that isn't 100% clear to me is where the actual blitting from the multisampled FB to the back buffer is happening.

::: gfx/thebes/GLContext.cpp
@@ +973,5 @@
> +
> +    bool alpha = mCreationFormat.alpha > 0;
> +    GLint internalFormat = alpha ? LOCAL_GL_RGBA : LOCAL_GL_RGB;
> +    //mOffscreenTextureFormat = alpha ? LOCAL_GL_RGBA : LOCAL_GL_RGB;
> +    GLenum format = alpha ? LOCAL_GL_BGRA : LOCAL_GL_BGR;

format and internalFormat need to match in GLES. Also, I think you'll need to test for whether the machine supports GL_EXT_texture_format_BGRA8888 on GLES.

@@ +1569,5 @@
>      }
>  
> +    Preferences::GetRootBranch();
> +    PRBool useBGRA = Preferences::GetBool("webgl.use-bgra", PR_TRUE);
> +    printf("webgl.use-bgra: %s\n", useBGRA ? "true" : "false");

Since this is in GLContext.cpp, which is used for layers and everything else, you won't want to use a webgl.* pref here.
Comment 20 Jeff Gilbert [:jgilbert] 2011-07-20 20:53:45 PDT
1) Blitting happens (if needed) in GLContext:: or GLContextWGL::ReadPixels(). 
GLContextWGL::ReadPixels is the PBuffer implementation, otherwise it defaults back to the GLContext implementation, which is FBOs.

2) Yep, I didn't realize GLES had that requirement (thus doesn't support BGRA).

3) Just gl.* then?
Comment 21 Benoit Jacob [:bjacob] (mostly away) 2011-07-21 00:19:28 PDT
(In reply to comment #9)
> True, but the current implementation through WGL tries PBuffers first, then
> falls back to FBOs if it fails. To get it to use FBOs, I had to force it to
> skip PBuffer generation.

Oh, WGL. It's almost never used by default, since on windows we normally default to using ANGLE with EGL. Currently, WGL only gets used by default on Optimus systems due to a blacklist rule. It could become used by default on more systems in the future though e.g. if we eventually use the OpenGL driver on NVIDIA cards by default. Anyway, currently it's significantly less well maintained than the other GL context providers. That it still tries PBuffers first is pretty bad, for the reasons explained above. I'm still not aware of any reason why we would still bother with PBuffers but I might be missing something.

> Multisampled PBuffers aren't that much work (almost done), so I think it's
> probably a good idea to keep it for now, but as a fallback if FBOs fail, not
> the other way around.

If FBOs are unsupported then we can't implement WebGL anyways (since WebGL exposes FBOs). If framebuffer_multisample is unsupported I'd just disable WebGL antialiasing, as AFAICS the costs of using PBuffers instead of FBOs would still offset the benefit of antialiasing.

> 
> The relevant comment for this is:
> // Always try to create a pbuffer context first, because we
> // want the context isolation.
> 
> I would imagine this is to prevent a bad WebGL state change from propagating
> to the browser?

Maybe (I didn't write this comment) but the code to carefully prevent this kind of bug is already written and was needed anyways, so there's no need to worry about this anymore. I suppose this comment is no longer relevant.
Comment 22 Benoit Jacob [:bjacob] (mostly away) 2011-07-21 00:24:48 PDT
(In reply to comment #10)
> Also, no, there didn't seem to be any problems with integration, though I
> haven't gone over all the paths yet. As long as everything goes through
> ReadPixelsIntoImageSurface() it works, though.

That works indeed, but is slow because it forces all GPU commands to finish at this point and causes the image to do a round trip to main memory. When using accelerated layers it is much better (really, necessary for high performance) to keep images on the GPU side i.e. as textures. This means that the texture that is the color part of the WebGL framebuffer must be used directly by the compositor ('layers'). Joe or Jeff could give you pointers about that.
Comment 23 Benoit Jacob [:bjacob] (mostly away) 2011-07-21 00:28:59 PDT
(In reply to comment #17)
> Problem was that the demo was expecting the bound texture to persist across
> frames. Recording the current texture and rebinding to it after blitting the
> AA PBuffer to the offscreen texture fixes this.

This kind of bug and many others are also caught by the WebGL conformance test suite which we have integrated as a mochitest. From your build directory, you can do:

TEST_PATH=content/canvas/test/webgl/test_webgl_conformance_test_suite.html make mochitest-plain

to run this test.
Comment 24 Jeff Gilbert [:jgilbert] 2011-07-29 20:27:13 PDT
Created attachment 549543 [details] [diff] [review]
WIP fix

Updated WIP patch. Works with AA/no-AA on Win7 for both WGL (PBuffers and FBOs) and EGL (ReadPixels and ANGLE shared texture). It might still work on linux, and I should be getting a mac soon to test there too.

Note there are a bunch of webgl regressions I still need to address, but the basic anti-aliasing is working.
Comment 25 Jeff Gilbert [:jgilbert] 2011-07-29 20:29:36 PDT
Also the about:config vars:
gl.disable-egl (bool: True forces fallback to WGL)
layers.disable-d3d-sharing (bool: True forces glReadPixels instead of shared texture)
gl.disable-pbuffers (bool: True forces fallback to FBOs on WGL)
webgl.disable-aa (bool)
Comment 26 Jeff Gilbert [:jgilbert] 2011-08-11 03:04:05 PDT
Created attachment 552333 [details] [diff] [review]
WIP fix

Updated WIP which is much closer to meeting the webgl conformance tests.
It still has regressions in a couple tests, including some soft crashes with EGL, and a test with a hard BSOD crash in WGL.

Soft crashes on EGL:
drawingbuffer-static-canvas-test
drawingbuffer-test

Hard crash on WGL:
read-pixels-test

Regressions:
read-pixels-test
tex-image-and-sub-image-2d-with-array-buffer-view
viewpoint-unchanged-upon-resize
Comment 27 Jeff Gilbert [:jgilbert] 2011-09-01 15:56:07 PDT
Created attachment 557679 [details] [diff] [review]
WIP fix for bug 615976

Updated WIP:

Failing conf. tests on windows 7:
oes-texture-float
uninitialized-test

AA is disabled by default, enable with 'webgl.disable-aa false'.
Comment 28 Benoit Jacob [:bjacob] (mostly away) 2011-09-02 09:38:11 PDT
The patch doesn't apply cleanly anymore, and I wasn't comfortable enough with the GLContext changes to rebase it myself. Please rebase.

120 K is the biggest patch I've seen in Mozilla yet, I don't think we can review something so big. Please split it into many small patches. The hg qcrecord tool is useful for that:

https://developer.mozilla.org/en/Mercurial_Queues#Splitting_a_patch.2C_the_general_case.2C_including_per-hunk_and_per-line_splitting

Sorry to have let you start right away on something so long and difficult :-)

The toughest part to get in is the GLContext changes. This part is already too complex, and is shared with lots of other code, so adding more complexity in there will require lots of explanations (either in comments in the code, or comments on this bug). For example, here is something I don't understand:

 void
+GLContext::Finish()
+{
+    printf("Finish()\n");
+
+    if (mOffscreenInFBO) {
+        // Need to blit from In to Out
+        BlitOffscreenFramebuffer();
+    } else {
+        fFinish();
+    }
+
+    //fFinish();
+}

I don't get why we need this here. If new BlitOffscreenFramebuffer calls are needed in some places, then add calls where needed, but I don't understand why a BlitOffscreenFramebuffer call would be needed everywhere we want to call glFinish, which is what putting it there suggests.

As it's shared between so many different parts of the codebase, the GLContext class is better kept as minimalistic as possible. When you have a choice between implementing stuff on WebGLContext or on GLContext, the former is typically better.
Comment 29 Jeff Gilbert [:jgilbert] 2011-09-27 18:54:10 PDT
Created attachment 562940 [details] [diff] [review]
Patch 1: Fixes WebGL to default to attempting AA
Comment 30 Jeff Gilbert [:jgilbert] 2011-09-27 18:55:36 PDT
Created attachment 562941 [details] [diff] [review]
Patch 2: Adds support for split Draw/Read buffers to GLContext
Comment 31 Jeff Gilbert [:jgilbert] 2011-09-27 18:59:00 PDT
Created attachment 562942 [details] [diff] [review]
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples'
Comment 32 Jeff Gilbert [:jgilbert] 2011-09-27 19:00:11 PDT
Created attachment 562943 [details] [diff] [review]
Patch 4: Adds glRenderBufferStorageMultisample, extension detection for framebuffer_multisample
Comment 33 Jeff Gilbert [:jgilbert] 2011-09-27 19:02:58 PDT
Created attachment 562944 [details] [diff] [review]
Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths

Patch 5 alone will almost certainly cause issues for non-windows platforms. Supplement with 5.* sub-patches until they get merged.
Comment 34 Jeff Gilbert [:jgilbert] 2011-09-27 19:04:27 PDT
Created attachment 562945 [details] [diff] [review]
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts
Comment 35 Jeff Gilbert [:jgilbert] 2011-09-27 19:05:37 PDT
Created attachment 562947 [details] [diff] [review]
Patch 5.2: Fixes for support for multisampled Linux/GLX GLContexts
Comment 36 Jeff Gilbert [:jgilbert] 2011-09-27 19:08:51 PDT
Created attachment 562948 [details] [diff] [review]
Patch 5.3: Force-disables multisampling on Mac/CGL due to severe display corruption

The corruption avoided by patch 5.3 may be because my test Mac has an AMD card.
I anticipate being able to re-enable AA on Mac with a little extra work.
Comment 37 Jeff Gilbert [:jgilbert] 2011-09-27 19:11:05 PDT
Try build with the current changes (but before I gave the patches nice titles; they're in the same order as presented here, though): https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=1d2ec80ce54e

Not quite there, but close.
Comment 38 Jeff Gilbert [:jgilbert] 2011-09-27 19:25:44 PDT
Also, be advised that this iteration still has a huge amount of console printf noise.
Comment 39 Jeff Gilbert [:jgilbert] 2011-09-29 17:40:47 PDT
Note: Patch from bug 688112 is required, and 686083 is recommended.
Comment 40 Jeff Gilbert [:jgilbert] 2011-09-29 17:46:56 PDT
Fixed the layering issue on Mac, so AA can be re-enabled.
Comment 41 Jeff Gilbert [:jgilbert] 2011-09-29 17:55:54 PDT
Created attachment 563613 [details] [diff] [review]
Patch 5.3: Fixes layer compositing issue resulting in garbage WebGL canvases on Mac
Comment 42 Jeff Gilbert [:jgilbert] 2011-09-29 18:14:36 PDT
Created attachment 563618 [details] [diff] [review]
Fixes layer compositing with AA on Mac

Will submit a new try build for this shortly.
Comment 43 Jeff Gilbert [:jgilbert] 2011-10-04 04:44:51 PDT
I have an updated patch list with some changes brought on by the fix for bug 688112. It also includes a couple fixes that I ran into while unbitrotting the changes caused by 688112.
Comment 44 Benoit Jacob [:bjacob] (mostly away) 2011-10-04 05:04:13 PDT
Then please update it here, as I'm about to start reviewing.
Comment 45 Jeff Gilbert [:jgilbert] 2011-10-05 06:31:54 PDT
Created attachment 564816 [details] [diff] [review]
Patch 1: Fixes WebGL to default to attempting AA
Comment 46 Jeff Gilbert [:jgilbert] 2011-10-05 06:32:45 PDT
Created attachment 564817 [details] [diff] [review]
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples'
Comment 47 Jeff Gilbert [:jgilbert] 2011-10-05 06:33:40 PDT
Created attachment 564818 [details] [diff] [review]
Patch 2: Adds support for split Draw/Read buffers to GLContext
Comment 48 Jeff Gilbert [:jgilbert] 2011-10-05 06:34:33 PDT
Created attachment 564819 [details] [diff] [review]
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples'
Comment 49 Jeff Gilbert [:jgilbert] 2011-10-05 06:35:28 PDT
Created attachment 564820 [details] [diff] [review]
Patch 4: Adds glRenderBufferStorageMultisample, extension detection for framebuffer_multisample
Comment 50 Jeff Gilbert [:jgilbert] 2011-10-05 06:36:18 PDT
Created attachment 564821 [details] [diff] [review]
Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths
Comment 51 Jeff Gilbert [:jgilbert] 2011-10-05 06:36:53 PDT
Created attachment 564822 [details] [diff] [review]
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts
Comment 52 Jeff Gilbert [:jgilbert] 2011-10-05 06:38:06 PDT
Created attachment 564823 [details] [diff] [review]
Patch 5.1.1: Fixes layer compositing with AA on Mac
Comment 53 Jeff Gilbert [:jgilbert] 2011-10-05 06:38:57 PDT
Created attachment 564824 [details] [diff] [review]
Patch 5.2: Fixes for support for multisampled Linux/GLX GLContexts
Comment 54 Jeff Gilbert [:jgilbert] 2011-10-05 06:39:57 PDT
Created attachment 564825 [details] [diff] [review]
Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed
Comment 55 Jeff Gilbert [:jgilbert] 2011-10-05 06:51:28 PDT
Created attachment 564828 [details] [diff] [review]
Prepatch 0.1: Printf of the resulting implementation of GLContext
Comment 56 Jeff Gilbert [:jgilbert] 2011-10-05 06:53:13 PDT
Created attachment 564830 [details] [diff] [review]
Prepatch 0.2: Fixes GLContext::ResizeOffscreen leaving a mess on failure
Comment 57 Jeff Gilbert [:jgilbert] 2011-10-05 06:55:08 PDT
Created attachment 564832 [details] [diff] [review]
Prepatch 0.3: Adds method to disable GL implementations
Comment 58 Jeff Gilbert [:jgilbert] 2011-10-05 07:04:24 PDT
Now fully cleans up after itself.
At parity with pre-patch mochi-1 on local Win7. (EGL,WGL+PB,WGL+FB)
Pre-revision had parity with OS X and Linux.

Current try run:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e72cf40c84aa

Still uses PRBool for the moment, will fix as part of finalizing.
Still has blockers. (working on it)
I have the patch queue prefixed by a patch which emits the implementation of created GLContexts, to assure we're not falling back when we shouldn't be.
Comment 59 Jeff Gilbert [:jgilbert] 2011-10-05 07:08:04 PDT
Also note that with the fix for bug 681791, we are expecting some oranges for mochi-1, as drawingbuffer-* tests should unexpectedly pass.
Comment 60 Jeff Gilbert [:jgilbert] 2011-10-11 14:17:16 PDT
Created attachment 566341 [details] [diff] [review]
Prepatch 0.2: Fixes GLContext::ResizeOffscreen leaving a mess on failure
Comment 61 Jeff Gilbert [:jgilbert] 2011-10-11 14:18:09 PDT
Created attachment 566343 [details] [diff] [review]
Prepatch 0.3: Adds method to disable GL implementations
Comment 62 Jeff Gilbert [:jgilbert] 2011-10-11 14:18:53 PDT
Created attachment 566344 [details] [diff] [review]
Patch 1: Fixes WebGL to default to attempting AA
Comment 63 Jeff Gilbert [:jgilbert] 2011-10-11 14:19:44 PDT
Created attachment 566345 [details] [diff] [review]
Patch 2: Adds support for split Draw/Read buffers to GLContext
Comment 64 Jeff Gilbert [:jgilbert] 2011-10-11 14:20:25 PDT
Created attachment 566346 [details] [diff] [review]
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples'
Comment 65 Jeff Gilbert [:jgilbert] 2011-10-11 14:21:23 PDT
Created attachment 566347 [details] [diff] [review]
Patch 4: Adds glRenderBufferStorageMultisample, extension detection for framebuffer_multisample
Comment 66 Jeff Gilbert [:jgilbert] 2011-10-11 14:22:17 PDT
Created attachment 566349 [details] [diff] [review]
Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths
Comment 67 Jeff Gilbert [:jgilbert] 2011-10-11 14:23:03 PDT
Created attachment 566350 [details] [diff] [review]
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts
Comment 68 Jeff Gilbert [:jgilbert] 2011-10-11 14:24:16 PDT
Created attachment 566351 [details] [diff] [review]
Patch 5.1.1: Fixes layer compositing with AA on Mac
Comment 69 Jeff Gilbert [:jgilbert] 2011-10-11 14:25:50 PDT
Created attachment 566352 [details] [diff] [review]
Patch 5.2: Fixes for support for multisampled Linux/GLX GLContexts
Comment 70 Jeff Gilbert [:jgilbert] 2011-10-11 14:26:48 PDT
Created attachment 566353 [details] [diff] [review]
Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed
Comment 71 Jeff Gilbert [:jgilbert] 2011-10-13 08:13:28 PDT
Created attachment 566823 [details] [diff] [review]
Patch 2: Adds support for split Draw/Read buffers to GLContext

Updates backporting fixes to patch 2 from patches 5 and 6, some function renaming.
Comment 72 Jeff Gilbert [:jgilbert] 2011-10-13 08:14:34 PDT
Created attachment 566824 [details] [diff] [review]
Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths
Comment 73 Jeff Gilbert [:jgilbert] 2011-10-13 08:15:47 PDT
Created attachment 566826 [details] [diff] [review]
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts
Comment 74 Jeff Gilbert [:jgilbert] 2011-10-13 08:16:50 PDT
Created attachment 566828 [details] [diff] [review]
Patch 5.1.1: Fixes layer compositing with AA on Mac
Comment 75 Jeff Gilbert [:jgilbert] 2011-10-13 08:17:55 PDT
Created attachment 566830 [details] [diff] [review]
Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed
Comment 76 Jeff Gilbert [:jgilbert] 2011-10-13 08:32:53 PDT
Created attachment 566837 [details] [diff] [review]
Patch 2: Adds support for split Draw/Read buffers to GLContext

One more backpatch I didn't see.
Comment 77 Jeff Gilbert [:jgilbert] 2011-10-13 08:33:54 PDT
Created attachment 566838 [details] [diff] [review]
Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths
Comment 78 Benoit Jacob [:bjacob] (mostly away) 2011-10-13 13:00:03 PDT
Comment on attachment 564828 [details] [diff] [review]
Prepatch 0.1: Printf of the resulting implementation of GLContext

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

printfs like that are acceptable #ifdef DEBUG only (in that case I agree they're useful)
Comment 79 Benoit Jacob [:bjacob] (mostly away) 2011-10-13 13:21:32 PDT
Comment on attachment 566341 [details] [diff] [review]
Prepatch 0.2: Fixes GLContext::ResizeOffscreen leaving a mess on failure

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

almost r+ but not quite due to indentation problems. also, a bit more explanation would have been welcome. Took me a while to understand the new approach --- create a new separate framebuffer, check for completeness, replace old framebuffer.

::: gfx/thebes/GLContext.cpp
@@ +1030,3 @@
>  
> +	fGenFramebuffers(1, &newOffscreenFBO);
> +	fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, newOffscreenFBO);

indentation problem?

@@ +1038,5 @@
>  
> +	if (depth && stencil && useDepthStencil) {
> +		fGenRenderbuffers(1, &newOffscreenDepthRB);
> +	} else {
> +		if (depth) {

Whoa 8 character indentation ? :-)

@@ +1126,5 @@
>      }
>  
> +    // Now assemble the FBO
> +	fFramebufferTexture2D(LOCAL_GL_FRAMEBUFFER,
> +						  LOCAL_GL_COLOR_ATTACHMENT0,

indentation problem.
Comment 80 Benoit Jacob [:bjacob] (mostly away) 2011-10-13 13:33:29 PDT
Comment on attachment 566346 [details] [diff] [review]
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples'

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

r+ but please address at least by all.js comment below.

::: gfx/thebes/GLContext.h
@@ +484,5 @@
>          memset(this, 0, sizeof(*this));
>      }
>  
> +    ContextFormat(const ContextFormat& b) {
> +        memcpy(this, &b, sizeof(*this));

This is fine but i've seen a few times people forgetting the * in sizeof(*this), actually including in this very file, that I prefer sizeof(ContextFormat) if you don't mind.

::: modules/libpref/src/init/all.js
@@ +3286,5 @@
>  pref("webgl.force_osmesa", false);
>  pref("webgl.osmesalib", "");
>  pref("webgl.verbose", false);
>  pref("webgl.prefer-native-gl", false);
> +pref("webgl.max-samples", 2);

so does 2 mean 2 samples per pixel or 2x2 samples per pixel?

if the latter, then isn't this a misleading name? not sure what standard terminology is here.
Comment 81 Benoit Jacob [:bjacob] (mostly away) 2011-10-13 17:16:53 PDT
Comment on attachment 566837 [details] [diff] [review]
Patch 2: Adds support for split Draw/Read buffers to GLContext

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

As discussed with Jeff, a new version is coming.
Comment 82 Benoit Jacob [:bjacob] (mostly away) 2011-10-13 17:25:40 PDT
Comment on attachment 566347 [details] [diff] [review]
Patch 4: Adds glRenderBufferStorageMultisample, extension detection for framebuffer_multisample

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

r+ with an important comment below, please address.

::: gfx/thebes/GLContext.cpp
@@ +330,5 @@
>      };
>  
>      SymLoadStruct auxSymbols[] = {
>              { (PRFuncPtr*) &mSymbols.fBlitFramebuffer, { "BlitFramebuffer", "BlitFramebufferEXT", "BlitFramebufferANGLE", NULL } },
> +            { (PRFuncPtr*) &mSymbols.fRenderbufferStorageMultisample, { "RenderbufferStorageMultisample", "RenderbufferStorageMultisampleEXT", "RenderbufferStorageMultisampleANGLE", NULL } },

This is a question I should already have asked about part 2: these symbols are only present if corresponding extensions are present. I think we want to:

  * if the corresponding extensions are not present, then don't even try to load these. In any case, don't printf anything about these symbols being absent

  * if the corresponding extensions are present, and these symbols are absent, we should probably mark the extensions as absent to make sure we don't use them and print a big warning. That, or a hard assertion that's still present in release builds (NS_RUNTIMEABORT)
Comment 83 Benoit Jacob [:bjacob] (mostly away) 2011-10-13 19:06:30 PDT
Comment on attachment 566838 [details] [diff] [review]
Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths

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

Looks good but r- because several points need more explanation and some printfs are too verbose.

::: content/canvas/src/WebGLContext.cpp
@@ +493,1 @@
>      LogMessage("aaHint: %d stencil: %d depth: %d alpha: %d premult: %d preserve: %d\n",

Please don't do that: LogMessage generates a JS warning that shows up e.g. in the Web console.

I'd say this is not even worth a console message in debug mode (even that, shouldn't become arbitrarily verbose).

@@ +602,5 @@
>      }
>  
>      if (mOptions.antialias) {
>          PRUint32 maxSamples = Preferences::GetUint("webgl.max-samples", 2);
> +        printf_stderr("webgl.max-samples: %d\n", maxSamples);

This is really too verbose to print, even #ifdef DEBUG. Preferences:: can be trusted, so if you want to check this you can simply go to about:config or about:support

::: gfx/thebes/GLContext.cpp
@@ +1021,5 @@
>      int depth = mCreationFormat.depth;
>      int stencil = mCreationFormat.stencil;
> +    int samples = mCreationFormat.samples;
> +
> +    const bool useDrawMSFBO = (samples > 0) &&

>0 or >1 ?

@@ +1029,5 @@
> +        return PR_TRUE;
> +
> +    if (!useDrawMSFBO)
> +        samples = 0;
> +    printf_stderr("samples: %d\n", samples);

too verbose.

@@ +1127,4 @@
>  #else
> +            cf.red = 5;
> +            cf.green = 6;
> +            cf.blue = 5;

I realize that this preexists your patch bug isn't this a bug? Above we requested 888 channels (UNSIGNED_BYTE) if !mIsGLES2. So we probably want to do that here?

@@ +1139,5 @@
> +        GLenum colorFormat;
> +        if (!mIsGLES2 || IsExtensionSupported(OES_rgb8_rgba8))
> +            colorFormat = alpha ? LOCAL_GL_RGBA8 : LOCAL_GL_RGB8;
> +        else
> +            colorFormat = alpha ? LOCAL_GL_RGBA4 : LOCAL_GL_RGB565;

If RGBA4, shouldn't we have cf.red and friends set accordingly?

@@ +1339,5 @@
>      mOffscreenActualSize = aSize;
>      mActualFormat = cf;
>  
>  #ifdef DEBUG
> +    printf("%s %dx%d offscreen FBO: r: %d g: %d b: %d a: %d depth: %d stencil: %d samples: %d\n",

too verbose (I suppose this printf was introduced by an earlier patch in your series)

::: gfx/thebes/GLContextProviderEGL.cpp
@@ +1071,5 @@
>              return nsnull;
>          }
>  
> +        if (!ResizeOffscreenFBO(aNewSize, false))
> +            return PR_FALSE;

Erm, we're in the if(mIsPBuffer) path here. What's this ResizeOffscreenFBO call doing here? Above we just created a PBuffer.

@@ +2005,5 @@
>  
>      nsTArray<EGLint> attribs(32);
>      int attribAttempt = 0;
>  
> +    int tryDepthSize = (aFormat.depth > 0) ? 24 : 0;

OK, looking at preexisting code this doesn't seem like it could lead to a regression.

What I don't understand is that this is EGL, which is what we  use on Android, so 24 might easily fail, right?

@@ +2257,5 @@
> +    if (!glContext)
> +        return nsnull;
> +
> +    if (!glContext->ResizeOffscreenFBO(glContext->OffscreenActualSize(), false))
> +        return nsnull;

Again i'm a bit lost: we just created a PBuffer so what's ResizeOffscreenFBO doing here?
Comment 84 Benoit Jacob [:bjacob] (mostly away) 2011-10-14 07:13:16 PDT
Comment on attachment 566826 [details] [diff] [review]
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts

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

r- just because I need an explanation , see below. Also a minor indentation problem.

::: gfx/thebes/GLContextProviderCGL.mm
@@ +279,5 @@
>          }
> +        
> +        if (!ResizeOffscreenFBO(aNewSize, false)) {
> +        	return PR_FALSE;
> +       	}

Again I need an explanation here. Just above we created a PBuffer, didnt we. So why call ResizeOffscreenFBO here?

@@ +583,4 @@
>  
> +            printf("GL Offscreen: CGL+PBuffer\n");
> +
> +            return glContext.forget();

Indentation problem
Comment 85 Benoit Jacob [:bjacob] (mostly away) 2011-10-14 07:14:00 PDT
Comment on attachment 566826 [details] [diff] [review]
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts

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

r- just because I need an explanation , see below. Also a minor indentation problem.

::: gfx/thebes/GLContextProviderCGL.mm
@@ +279,5 @@
>          }
> +        
> +        if (!ResizeOffscreenFBO(aNewSize, false)) {
> +        	return PR_FALSE;
> +       	}

Again I need an explanation here. Just above we created a PBuffer, didnt we. So why call ResizeOffscreenFBO here?

@@ +583,4 @@
>  
> +            printf("GL Offscreen: CGL+PBuffer\n");
> +
> +            return glContext.forget();

Indentation problem
Comment 86 Benoit Jacob [:bjacob] (mostly away) 2011-10-14 07:19:55 PDT
Comment on attachment 566830 [details] [diff] [review]
Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed

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

::: content/canvas/src/WebGLContext.cpp
@@ +602,5 @@
>      }
>  
>      if (mOptions.antialias) {
>          PRUint32 maxSamples = Preferences::GetUint("webgl.max-samples", 2);
>          printf_stderr("webgl.max-samples: %d\n", maxSamples);

Again, this is too verbose.

@@ +603,5 @@
>  
>      if (mOptions.antialias) {
>          PRUint32 maxSamples = Preferences::GetUint("webgl.max-samples", 2);
>          printf_stderr("webgl.max-samples: %d\n", maxSamples);
> +        format.samples = maxSamples*maxSamples;

so ContextFormat doesn't want the square root?

::: content/canvas/test/webgl/conformance/more-than-65536-points.html
@@ +34,5 @@
>  }
>  </script>
>  <script>
>  var wtu = WebGLTestUtils;
> +var gl = initWebGL("example", "vs", "fs", ["vPosition", "vColor"], [0, 0, 0, 1], 1, { antialias: false });

Did you send the patch to khronos.org already?

Or do we need to maintain this as a local patch?
Comment 87 Jeff Gilbert [:jgilbert] 2011-10-14 11:25:31 PDT
I've updated the patch for bug 681791, which is the prepatch 0.2 here.
Comment 88 Jeff Gilbert [:jgilbert] 2011-10-14 11:49:05 PDT
(In reply to Benoit Jacob [:bjacob] from comment #80)
> ::: gfx/thebes/GLContext.h
> @@ +484,5 @@
> >          memset(this, 0, sizeof(*this));
> >      }
> >  
> > +    ContextFormat(const ContextFormat& b) {
> > +        memcpy(this, &b, sizeof(*this));
> 
> This is fine but i've seen a few times people forgetting the * in
> sizeof(*this), actually including in this very file, that I prefer
> sizeof(ContextFormat) if you don't mind.

I used sizeof(*this) because the functions directly around that point in the file use sizeof(*this). I would either prefer using sizeof(*this) for consistency or also changing the other nearby uses to sizeof(ContextFormat).
Comment 89 Jeff Muizelaar [:jrmuizel] 2011-10-14 12:12:05 PDT
Comment on attachment 566346 [details] [diff] [review]
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples'

>+    ContextFormat(const ContextFormat& b) {
>+        memcpy(this, &b, sizeof(*this));
>+    }
>+

Is the a reason to have this code at all? i.e. Why is the default copy constructor insufficient?
Comment 90 Jeff Gilbert [:jgilbert] 2011-10-14 13:16:04 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #89)
> Comment on attachment 566346 [details] [diff] [review] [diff] [details] [review]
> Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples'
> 
> >+    ContextFormat(const ContextFormat& b) {
> >+        memcpy(this, &b, sizeof(*this));
> >+    }
> >+
> 
> Is the a reason to have this code at all? i.e. Why is the default copy
> constructor insufficient?

You are correct. I spent too much time away from c++ and forgot about the implicit copy constructor.
Comment 91 Jeff Gilbert [:jgilbert] 2011-10-14 14:36:37 PDT
(In reply to Benoit Jacob [:bjacob] from comment #85)
> Comment on attachment 566826 [details] [diff] [review] [diff] [details] [review]
> Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts
> 
> Review of attachment 566826 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> r- just because I need an explanation , see below. Also a minor indentation
> problem.
> 
> ::: gfx/thebes/GLContextProviderCGL.mm
> @@ +279,5 @@
> >          }
> > +        
> > +        if (!ResizeOffscreenFBO(aNewSize, false)) {
> > +        	return PR_FALSE;
> > +       	}
> 
> Again I need an explanation here. Just above we created a PBuffer, didnt we.
> So why call ResizeOffscreenFBO here?
> 
> @@ +583,4 @@
> >  
> > +            printf("GL Offscreen: CGL+PBuffer\n");
> > +
> > +            return glContext.forget();
> 
> Indentation problem

In patch 5 I added a parameter to make it ResizeOffscreenFBO(format, bool aUseReadFBO). Even though we use PBuffers for our packing surface on some platforms, we still need to use a multisampled draw FBO to get antialiasing. Passing aUseReadFBO=false tells ResizeOffscreenFBO to not create a single-sampled read FBO, instead using the backing offscreen buffer (0). Passing true would make it create an offscreen FBO like normal, whether or not it's creating an MS draw buffer.

Also note that if it's not multisampled, and aUseReadFBO==false, ResizeOffscreenFBO succeeds out early, skipping most of the function, since we need neither of the two FBOs. In this case, it is effectively a no-op.
Comment 92 Jeff Gilbert [:jgilbert] 2011-10-17 20:33:54 PDT
Also note that for some of the non-windows EGL implementations, I erred on the side of caution and used aUseReadFBO=true, which will work for any implementation that has at least a valid context to call against.
Comment 93 Jeff Gilbert [:jgilbert] 2011-10-18 12:11:18 PDT
Created attachment 567831 [details] [diff] [review]
Patch 2: Adds support for split Draw/Read buffers to GLContext

Fixes including:
GetDraw/ReadFBO->GetBoundDraw/ReadFBO
SwapDraw/ReadFBO->SwapBoundDraw/ReadFBO
Backed out unnecessary changes given the 0=offscreen FBO convention.
Converted all remnants from the PRBool era.
No regressions in tests.
Comment 94 Jeff Gilbert [:jgilbert] 2011-10-18 12:16:21 PDT
Created attachment 567836 [details] [diff] [review]
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples'

Removed explicit copy constructor.
Changed pref to 'webgl.msaa-level', using standard 2/4/8x terminology.
Comment 95 Jeff Gilbert [:jgilbert] 2011-10-18 12:18:44 PDT
Created attachment 567838 [details] [diff] [review]
Patch 3: Adds support for samples in ContextFormat, pref 'webgl.msaa-level'

Updated desc and obsoleted old patches.
Comment 96 Jeff Gilbert [:jgilbert] 2011-10-18 12:21:23 PDT
Created attachment 567839 [details] [diff] [review]
Patch 4: Adds glRenderBufferStorageMultisample, extension detection for framebuffer_multisample

Updated to reflect new approach to loading extension-dependent symbols from Patch 2.
Comment 97 Jeff Gilbert [:jgilbert] 2011-10-18 13:54:04 PDT
(In reply to Benoit Jacob [:bjacob] from comment #83)
> ::: content/canvas/src/WebGLContext.cpp
> @@ +493,1 @@
> >      LogMessage("aaHint: %d stencil: %d depth: %d alpha: %d premult: %d preserve: %d\n",
> 
> Please don't do that: LogMessage generates a JS warning that shows up e.g.
> in the Web console.
> 
> I'd say this is not even worth a console message in debug mode (even that,
> shouldn't become arbitrarily verbose)

> @@ +602,5 @@
> >      }
> >  
> >      if (mOptions.antialias) {
> >          PRUint32 maxSamples = Preferences::GetUint("webgl.max-samples", 2);
> > +        printf_stderr("webgl.max-samples: %d\n", maxSamples);
> 
> This is really too verbose to print, even #ifdef DEBUG. Preferences:: can be
> trusted, so if you want to check this you can simply go to about:config or
> about:support

> @@ +1029,5 @@
> > +        return PR_TRUE;
> > +
> > +    if (!useDrawMSFBO)
> > +        samples = 0;
> > +    printf_stderr("samples: %d\n", samples);
> 
> too verbose.

> @@ +1339,5 @@
> >      mOffscreenActualSize = aSize;
> >      mActualFormat = cf;
> >  
> >  #ifdef DEBUG
> > +    printf("%s %dx%d offscreen FBO: r: %d g: %d b: %d a: %d depth: %d stencil: %d samples: %d\n",
> 
> too verbose (I suppose this printf was introduced by an earlier patch in
> your series)

Accidentally left these in from debugging. They have now been removed.

> ::: gfx/thebes/GLContext.cpp
> @@ +1021,5 @@
> >      int depth = mCreationFormat.depth;
> >      int stencil = mCreationFormat.stencil;
> > +    int samples = mCreationFormat.samples;
> > +
> > +    const bool useDrawMSFBO = (samples > 0) &&
> 
> >0 or >1 ?

'>0' is basically how the OGL spec has it, with 1 being still-'multisampled', but only sampled once. It's single-sampled, but uses the same structures as multisampling.

> @@ +1127,4 @@
> >  #else
> > +            cf.red = 5;
> > +            cf.green = 6;
> > +            cf.blue = 5;
> 
> I realize that this preexists your patch bug isn't this a bug? Above we
> requested 888 channels (UNSIGNED_BYTE) if !mIsGLES2. So we probably want to
> do that here?
> 
> @@ +1139,5 @@
> > +        GLenum colorFormat;
> > +        if (!mIsGLES2 || IsExtensionSupported(OES_rgb8_rgba8))
> > +            colorFormat = alpha ? LOCAL_GL_RGBA8 : LOCAL_GL_RGB8;
> > +        else
> > +            colorFormat = alpha ? LOCAL_GL_RGBA4 : LOCAL_GL_RGB565;
> 
> If RGBA4, shouldn't we have cf.red and friends set accordingly?
> 

I agree, but this should really be a separate patch. Really I think that the selection of the formats should be an entirely separate function, which would simplify ResizeOffscreenFBO nicely. I can submit a bug and throw a patch together after this.

> 
> ::: gfx/thebes/GLContextProviderEGL.cpp
> @@ +1071,5 @@
> >              return nsnull;
> >          }
> >  
> > +        if (!ResizeOffscreenFBO(aNewSize, false))
> > +            return PR_FALSE;
> 
> Erm, we're in the if(mIsPBuffer) path here. What's this ResizeOffscreenFBO
> call doing here? Above we just created a PBuffer.

> 
> @@ +2257,5 @@
> > +    if (!glContext)
> > +        return nsnull;
> > +
> > +    if (!glContext->ResizeOffscreenFBO(glContext->OffscreenActualSize(), false))
> > +        return nsnull;
> 
> Again i'm a bit lost: we just created a PBuffer so what's ResizeOffscreenFBO
> doing here?

I explained these in another comment, but the short story is that with AA, all implementations have FBOs, but not all impls have read FBOs. (Many use PBuffers as the final offscreen buffer) All impls which have some sort of legitimate backing buffer call ResizeOffscreenFBO with aUseReadFBO=false, whereas for impls without a real back buffer, we request a read FBO with aUseReadFBO=true.

> @@ +2005,5 @@
> >  
> >      nsTArray<EGLint> attribs(32);
> >      int attribAttempt = 0;
> >  
> > +    int tryDepthSize = (aFormat.depth > 0) ? 24 : 0;
> 
> OK, looking at preexisting code this doesn't seem like it could lead to a
> regression.
> 
> What I don't understand is that this is EGL, which is what we  use on
> Android, so 24 might easily fail, right?

This actually is odd code to begin with, as it actually tries requesting better-than-specified buffers (that is, it supersedes what's in aFormat), trying to get 8-bit color channels and 24-bit depth buffer, then trying just the 24-bit depth buffer, then just asking for what's requested by aFormat.
The issue was that we were requesting a depth buffer even when we didn't want one, which this fixes.
Comment 98 Jeff Gilbert [:jgilbert] 2011-10-18 13:58:53 PDT
Created attachment 567869 [details] [diff] [review]
Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths

Bunch of fixes as per review.
Comment 99 Jeff Gilbert [:jgilbert] 2011-10-18 14:01:27 PDT
Created attachment 567870 [details] [diff] [review]
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts

Fixed indentation problems.
Comment 100 Jeff Gilbert [:jgilbert] 2011-10-18 14:03:17 PDT
Created attachment 567871 [details] [diff] [review]
Patch 5.1.1: Fixes layer compositing with AA on Mac

Un-bit-rotted r+ from bjacob.
Comment 101 Jeff Gilbert [:jgilbert] 2011-10-18 14:04:25 PDT
Created attachment 567872 [details] [diff] [review]
Patch 5.2: Fixes for support for multisampled Linux/GLX GLContexts

Un-bit-rotted r+ from bjacob.
Comment 102 Jeff Gilbert [:jgilbert] 2011-10-18 14:05:34 PDT
Created attachment 567873 [details] [diff] [review]
Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed

Un-bit-rotted r+ from bjacob.
Comment 103 Jeff Gilbert [:jgilbert] 2011-10-18 14:25:59 PDT
Partial try (up to Patch 2 inclusive):
https://tbpl.mozilla.org/?tree=Try&rev=c3255f263036

Full try:
https://tbpl.mozilla.org/?tree=Try&rev=14df2eb343d5

It looks like we are unexpected-passing renderbuffers-initialization in mochi 1 on linux.
Comment 104 Jeff Gilbert [:jgilbert] 2011-10-18 15:17:31 PDT
Looks like I messed up part of Patch 5.1.

New full try:
https://tbpl.mozilla.org/?tree=Try&rev=eac0015ee128
Comment 105 Jeff Gilbert [:jgilbert] 2011-10-18 15:19:06 PDT
Created attachment 567903 [details] [diff] [review]
Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts

Update to fix compile error on Mac.

(Unfortunately this also means the patch order is messed up again)
Comment 107 Jeff Gilbert [:jgilbert] 2011-10-19 17:05:35 PDT
Example for easy-to-see AA/no-AA:
http://www.ibiblio.org/e-notes/webgl/deflate/ant.html
Comment 108 ojab 2011-10-19 19:41:13 PDT
After this patch I have 
>Can't get WebGL
On http://www.ibiblio.org/e-notes/webgl/deflate/ant.html
and
>ResizeOffscreenFBO failed with AA, retrying without...
 in console. webgl.force-enable=true doesn't change anything.

Adapter DescriptionX.Org -- Gallium 0.4 on AMD RS780
Driver Version2.1 Mesa 7.12-devel (git-0b3842e)
WebGL Rendererfalse
Comment 110 Benoit Jacob [:bjacob] (mostly away) 2011-10-20 08:03:04 PDT
Jeff: please have a look at comment 108. This driver probably doesn't support MSAA and this report suggest that we don't correctly fall back to non-AA.
Comment 111 ojab 2011-10-20 08:35:37 PDT
There is GL_EXT_framebuffer_multisample, but not GL_OES_rgb8_rgba8, see glxinfo output in bug 696093.
Comment 112 Benoit Jacob [:bjacob] (mostly away) 2011-10-20 14:04:31 PDT
ojab: we don't require GL_OES_rgb8_rgba8 on desktop OpenGL, we only require it on OpenGL ES ( = mobile devices and ANGLE)
Comment 113 Emanuel Hoogeveen [:ehoogeveen] 2011-10-21 06:36:16 PDT
In today's nightly, webgl.msaa-level=2 and webgl.msaa-level=4 works great (on the ant model), but webgl.msaa-level=8 fails with the message noted in comment 108. Is this just unsupported overkill for the setting?
Comment 114 Benoit Jacob [:bjacob] (mostly away) 2011-10-21 06:37:38 PDT
Yes. msaa-level=8 means you're requesting 64 (8x8) samples per pixel. Only some high-end GPUs support that.
Comment 115 Emanuel Hoogeveen [:ehoogeveen] 2011-10-21 06:39:58 PDT
I was a bit hasty in pressing Save Changes. I should note that I'm on Windows 7 x64 w/ SP1, using a GeForce GTX 580 with the 285.38 drivers. I also have the latest DirectX runtimes, which might be relevant for ANGLE, but I get the same result even if I set webgl.prefer-native-gl = true.
Comment 116 Benoit Jacob [:bjacob] (mostly away) 2011-10-21 06:42:22 PDT
Indeed, a GTX 580 should support that. Jeff was able to get level 8 on his machine. Can you get 8x8 MSAA in any OpenGL or Direct3D app?
Comment 117 Emanuel Hoogeveen [:ehoogeveen] 2011-10-21 06:52:35 PDT
Well, I tried setting (in the "Manage 3D settings" tab of the Nvidia Control Panel) the "Antialiasing - Mode" to "Override any application setting" and "Antialiasing - Setting" to 8x, and it certainly made the spinning logo in the Nvidia Control Panel look better than at 2x. I also tried playing Deus Ex - Human Revolution with it and that seemed to work fine, but I'm not sure how objective that is. The override doesn't seem to work inside Firefox (on the ant model, setting webgl.msaa-level=0 for the sake of comparison).
Comment 118 Benoit Jacob [:bjacob] (mostly away) 2011-10-21 07:08:03 PDT
I don't think that even the NVIDIA driver could override webgl.msaa-level=0. Try webgl.msaa-level=1 instead.
Comment 119 :Ms2ger (⌚ UTC+1/+2) 2011-10-21 07:13:36 PDT
Comment on attachment 567872 [details] [diff] [review]
Patch 5.2: Fixes for support for multisampled Linux/GLX GLContexts

>--- a/gfx/thebes/GLContextProviderGLX.cpp
>+++ b/gfx/thebes/GLContextProviderGLX.cpp
> GLContextProviderGLX::CreateOffscreen(const gfxIntSize& aSize,
>                                       const ContextFormat& aFormat)
> {
>     ContextFormat actualFormat(aFormat);
>-    actualFormat.samples = 0;
>+    // actualFormat.samples = 0;

We don't leave commented-out code behind. Please remove.
Comment 120 :Ms2ger (⌚ UTC+1/+2) 2011-10-21 07:14:17 PDT
Comment on attachment 567873 [details] [diff] [review]
Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed

>--- a/gfx/thebes/GLContext.h
>+++ b/gfx/thebes/GLContext.h
>+    bool ResizeOffscreenFBO(const gfxIntSize& aSize, const bool aUseReadFBO) {
>+        printf("ResizeOffscreenFBO failed with AA, retrying without...\n");

And no printfs either.
Comment 121 Benoit Jacob [:bjacob] (mostly away) 2011-10-21 07:16:55 PDT
(In reply to Ms2ger from comment #119)
> We don't leave commented-out code behind. Please remove.

This is a good rule, but I didn't know about it. Where does it come from?

(In reply to Ms2ger from comment #120)
> And no printfs either.

Fixed already in bug 696093 which replaced it by a printf_stderr and only did it in MOZ_GL_DEBUG mode.
Comment 122 Emanuel Hoogeveen [:ehoogeveen] 2011-10-21 07:17:37 PDT
(In reply to Benoit Jacob [:bjacob] from comment #118)
> I don't think that even the NVIDIA driver could override webgl.msaa-level=0.
> Try webgl.msaa-level=1 instead.
Hmm, it doesn't seem to work regardless.
Comment 123 Benoit Jacob [:bjacob] (mostly away) 2011-10-21 07:20:36 PDT
(In reply to Emanuel Hoogeveen from comment #122)
> (In reply to Benoit Jacob [:bjacob] from comment #118)
> > I don't think that even the NVIDIA driver could override webgl.msaa-level=0.
> > Try webgl.msaa-level=1 instead.
> Hmm, it doesn't seem to work regardless.

Maybe it only works for direct rendering to a window/pbuffer and not for rendering to a FBO. Try creating this hidden preference in about:config: wgl.prefer-fbo , boolean, set to 'true'.
And set webgl.prefer-native-gl.
Comment 124 Benoit Jacob [:bjacob] (mostly away) 2011-10-21 07:21:21 PDT
(In reply to Benoit Jacob [:bjacob] from comment #123)
> (In reply to Emanuel Hoogeveen from comment #122)
> > (In reply to Benoit Jacob [:bjacob] from comment #118)
> > > I don't think that even the NVIDIA driver could override webgl.msaa-level=0.
> > > Try webgl.msaa-level=1 instead.
> > Hmm, it doesn't seem to work regardless.
> 
> Maybe it only works for direct rendering to a window/pbuffer and not for
> rendering to a FBO. Try creating this hidden preference in about:config:
> wgl.prefer-fbo , boolean, set to 'true'.
> And set webgl.prefer-native-gl.

Oops, ignore this. I meant 'false'... and it's already the default value so my theory is wrong.
Comment 125 Theliel 2011-10-21 08:01:52 PDT
Same here, webgl.msaa-level > 5 = "Can't get WebGL" with Win 7 x64 SP1 and nVidia 460 GTX (latest drivers), regardless nvidia control panel or any nvidiaInspector AA tweak
Comment 126 Jeff Gilbert [:jgilbert] 2011-10-21 11:54:00 PDT
I would guess that the nvidia overrides only work when you're drawing to the screen, which we don't actually do here. This is only done in the final compositing step, iirc. Forcing a higher AA level for the application will probably just result in better subpixels precision for the edges of the final composited quads, which doesn't really help anything.

I suppose it's useful to note that we have the same limitation for WebGL antialiasing: If a page uses WebGL to render to a framebuffer, and then just copy that to the WebGL 'screen', you also will not see any antialiasing. As it stands, the only way to get antialiasing in WebGL is to draw to the WebGL 'screen'. (WebGL doesn't (yet?) expose the requisite functions for multisampled offscreen buffers)
Comment 127 Benoit Jacob [:bjacob] (mostly away) 2011-10-21 12:01:27 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #126)
> As
> it stands, the only way to get antialiasing in WebGL is to draw to the WebGL
> 'screen'. (WebGL doesn't (yet?) expose the requisite functions for
> multisampled offscreen buffers)

Indeed, there is no framebuffer_multisample extension for WebGL yet.
Comment 128 Jeff Gilbert [:jgilbert] 2011-10-21 16:47:02 PDT
Created attachment 568827 [details] [diff] [review]
Post-Patch Cleanup

Supplemental patch to clean up errant printfs and comments.
Comment 129 Jeff Gilbert [:jgilbert] 2011-10-24 11:05:03 PDT
Created attachment 569112 [details] [diff] [review]
Post-Patch Cleanup

Update to fix additional compiler warning and undetected compiler error for MOZ_X11 && MOZ_EGL_XRENDER_COMPOSITE.
Carrying over r+ from bjacob via offline review.
Comment 130 Benoit Jacob [:bjacob] (mostly away) 2011-10-26 13:02:21 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/49ae6e1468dd
Comment 131 Jeff Gilbert [:jgilbert] 2011-10-26 14:43:29 PDT
*** Bug 697490 has been marked as a duplicate of this bug. ***
Comment 132 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-27 01:46:48 PDT
https://hg.mozilla.org/mozilla-central/rev/49ae6e1468dd

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