Closed Bug 615976 Opened 14 years ago Closed 13 years ago

Implement WebGL antialiasing

Categories

(Core :: Graphics: CanvasWebGL, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: alvaro.segura, Assigned: jgilbert)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [tilt])

Attachments

(13 files, 45 obsolete files)

4.36 KB, patch
bjacob
: review-
Details | Diff | Splinter Review
17.35 KB, patch
bjacob
: review-
Details | Diff | Splinter Review
3.25 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
856 bytes, patch
bjacob
: review+
Details | Diff | Splinter Review
26.62 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
9.19 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
5.60 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
27.89 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
829 bytes, patch
jgilbert
: review+
Details | Diff | Splinter Review
1.32 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
4.26 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
4.72 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
7.18 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
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
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/
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.
Yes, the problem affects every WebGL content.
Benoit, Joe, Vladimir, any comment on this?
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
we'd love to have AA support in webgl for the Tilt visualization project.
Whiteboard: [tilt]
Assignee: nobody → jgilbert
OS: Windows XP → All
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?
Status: NEW → ASSIGNED
(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.
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?
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.
PBuffers now also have antialiasing.
Can you upload your works in progress? A user repo of your patch queue is fine.
Attached patch WIP for adding AA to WebGL (obsolete) — Splinter Review
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)
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.
It also works for me on linux with no changes. Not that surprising since GLX uses the GLContext FBOs, but nice.
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.
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.
Attached patch WIP fix for bug 615976 (obsolete) — Splinter Review
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.
Attachment #545810 - Attachment is obsolete: true
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.
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?
(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.
(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.
(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.
Attached patch WIP fix (obsolete) — Splinter Review
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.
Attachment #547203 - Attachment is obsolete: true
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)
Attached patch WIP fix (obsolete) — Splinter Review
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
Attachment #549543 - Attachment is obsolete: true
Attached patch WIP fix for bug 615976 (obsolete) — Splinter Review
Updated WIP:

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

AA is disabled by default, enable with 'webgl.disable-aa false'.
Attachment #552333 - Attachment is obsolete: true
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.
Depends on: 681791
Depends on: 688112
Attachment #557679 - Attachment is obsolete: true
Attachment #562940 - Flags: review?(bjacob)
Attachment #562940 - Attachment description: Fixes WebGL to default to attempting AA → Patch 1: Fixes WebGL to default to attempting AA
Attachment #562941 - Attachment description: Adds support for split Draw/Read buffers to GLContext → Patch 2: Adds support for split Draw/Read buffers to GLContext
Patch 5 alone will almost certainly cause issues for non-windows platforms. Supplement with 5.* sub-patches until they get merged.
Attachment #562944 - Flags: review?(bjacob)
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.
Attachment #562948 - Flags: review?(bjacob)
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.
Also, be advised that this iteration still has a huge amount of console printf noise.
Depends on: 686083
Note: Patch from bug 688112 is required, and 686083 is recommended.
Fixed the layering issue on Mac, so AA can be re-enabled.
Attachment #562948 - Attachment is obsolete: true
Attachment #563613 - Flags: review?(bjacob)
Attachment #562948 - Flags: review?(bjacob)
Will submit a new try build for this shortly.
Attachment #563613 - Attachment is obsolete: true
Attachment #563613 - Flags: review?(bjacob)
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.
Then please update it here, as I'm about to start reviewing.
Summary: No antialiasing in WebGL (reopening) → Implement WebGL antialiasing
Attachment #562940 - Attachment is obsolete: true
Attachment #562940 - Flags: review?(bjacob)
Attachment #562941 - Attachment is obsolete: true
Attachment #564817 - Attachment is obsolete: true
Attachment #562941 - Flags: review?(bjacob)
Attachment #562942 - Attachment is obsolete: true
Attachment #562942 - Flags: review?(bjacob)
Attachment #562944 - Attachment is obsolete: true
Attachment #562944 - Flags: review?(bjacob)
Attachment #562945 - Attachment is obsolete: true
Attachment #563618 - Attachment is obsolete: true
Attachment #562945 - Flags: review?(bjacob)
Attachment #562947 - Attachment is obsolete: true
Attachment #562947 - Flags: review?(bjacob)
Attachment #564828 - Attachment description: Prefix 0.1: Printf of the resulting implementation of GLContext → Prepatch 0.1: Printf of the resulting implementation of GLContext
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.
Also note that with the fix for bug 681791, we are expecting some oranges for mochi-1, as drawingbuffer-* tests should unexpectedly pass.
Attachment #564816 - Attachment is obsolete: true
Attachment #566344 - Flags: review?(bjacob)
Attachment #564818 - Attachment is obsolete: true
Attachment #566345 - Flags: review?(bjacob)
Attachment #564819 - Attachment is obsolete: true
Attachment #566346 - Flags: review?(bjacob)
Attachment #564821 - Attachment is obsolete: true
Attachment #566349 - Flags: review?(bjacob)
Attachment #564822 - Attachment is obsolete: true
Attachment #566350 - Flags: review?(bjacob)
Attachment #564823 - Attachment is obsolete: true
Attachment #566351 - Flags: review?(bjacob)
Attachment #564824 - Attachment is obsolete: true
Attachment #566352 - Flags: review?(bjacob)
Attachment #564825 - Attachment is obsolete: true
Attachment #566353 - Flags: review?(bjacob)
Updates backporting fixes to patch 2 from patches 5 and 6, some function renaming.
Attachment #566345 - Attachment is obsolete: true
Attachment #566823 - Flags: review?(bjacob)
Attachment #566345 - Flags: review?(bjacob)
Attachment #566349 - Attachment is obsolete: true
Attachment #566824 - Flags: review?(bjacob)
Attachment #566349 - Flags: review?(bjacob)
Attachment #566350 - Attachment is obsolete: true
Attachment #566826 - Flags: review?(bjacob)
Attachment #566350 - Flags: review?(bjacob)
Attachment #566351 - Attachment is obsolete: true
Attachment #566828 - Flags: review?(bjacob)
Attachment #566351 - Flags: review?(bjacob)
Attachment #566353 - Attachment is obsolete: true
Attachment #566830 - Flags: review?(bjacob)
Attachment #566353 - Flags: review?(bjacob)
One more backpatch I didn't see.
Attachment #566823 - Attachment is obsolete: true
Attachment #566837 - Flags: review?(bjacob)
Attachment #566823 - Flags: review?(bjacob)
Attachment #566824 - Attachment is obsolete: true
Attachment #566838 - Flags: review?(bjacob)
Attachment #566824 - Flags: review?(bjacob)
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)
Attachment #564828 - Flags: review-
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.
Attachment #566341 - Flags: review-
Attachment #566344 - Flags: review?(bjacob) → review+
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.
Attachment #566346 - Flags: review?(bjacob) → review+
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.
Attachment #566837 - Flags: review?(bjacob) → review-
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)
Attachment #566347 - Flags: review?(bjacob) → review+
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?
Attachment #566838 - Flags: review?(bjacob) → review-
Attachment #566828 - Flags: review?(bjacob) → review+
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 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
Attachment #566826 - Flags: review?(bjacob) → review-
Attachment #566352 - Flags: review?(bjacob) → review+
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?
Attachment #566830 - Flags: review?(bjacob) → review+
I've updated the patch for bug 681791, which is the prepatch 0.2 here.
(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 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?
(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.
(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.
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.
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.
Attachment #566837 - Attachment is obsolete: true
Attachment #567831 - Flags: review?(bjacob)
Removed explicit copy constructor.
Changed pref to 'webgl.msaa-level', using standard 2/4/8x terminology.
Attachment #567836 - Flags: review?(bjacob)
Updated desc and obsoleted old patches.
Attachment #566346 - Attachment is obsolete: true
Attachment #567836 - Attachment is obsolete: true
Attachment #567838 - Flags: review?(bjacob)
Attachment #567836 - Flags: review?(bjacob)
Updated to reflect new approach to loading extension-dependent symbols from Patch 2.
Attachment #566347 - Attachment is obsolete: true
Attachment #567839 - Flags: review?(bjacob)
(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.
Bunch of fixes as per review.
Attachment #566838 - Attachment is obsolete: true
Attachment #567869 - Flags: review?(bjacob)
Fixed indentation problems.
Attachment #566826 - Attachment is obsolete: true
Attachment #567870 - Flags: review?(bjacob)
Un-bit-rotted r+ from bjacob.
Attachment #566828 - Attachment is obsolete: true
Attachment #567871 - Flags: review+
Un-bit-rotted r+ from bjacob.
Attachment #566352 - Attachment is obsolete: true
Attachment #567872 - Flags: review+
Un-bit-rotted r+ from bjacob.
Attachment #566830 - Attachment is obsolete: true
Attachment #567873 - Flags: review+
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.
Looks like I messed up part of Patch 5.1.

New full try:
https://tbpl.mozilla.org/?tree=Try&rev=eac0015ee128
Update to fix compile error on Mac.

(Unfortunately this also means the patch order is messed up again)
Attachment #567870 - Attachment is obsolete: true
Attachment #567903 - Flags: review?(bjacob)
Attachment #567870 - Flags: review?(bjacob)
Attachment #567831 - Flags: review?(bjacob) → review+
Attachment #567838 - Flags: review?(bjacob) → review+
Attachment #567839 - Flags: review?(bjacob) → review+
Attachment #567869 - Flags: review?(bjacob) → review+
Attachment #567903 - Flags: review?(bjacob) → review+
Blocks: 695912
Example for easy-to-see AA/no-AA:
http://www.ibiblio.org/e-notes/webgl/deflate/ant.html
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
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.
Depends on: 696093
There is GL_EXT_framebuffer_multisample, but not GL_OES_rgb8_rgba8, see glxinfo output in bug 696093.
ojab: we don't require GL_OES_rgb8_rgba8 on desktop OpenGL, we only require it on OpenGL ES ( = mobile devices and ANGLE)
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?
Yes. msaa-level=8 means you're requesting 64 (8x8) samples per pixel. Only some high-end GPUs support that.
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.
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?
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).
I don't think that even the NVIDIA driver could override webgl.msaa-level=0. Try webgl.msaa-level=1 instead.
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 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.
(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.
(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.
(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.
(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.
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
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)
(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.
Attached patch Post-Patch Cleanup (obsolete) — Splinter Review
Supplemental patch to clean up errant printfs and comments.
Attachment #568827 - Flags: review?(bjacob)
Attachment #568827 - Flags: review?(bjacob) → review+
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.
Attachment #568827 - Attachment is obsolete: true
Attachment #569112 - Flags: review+
Depends on: 697490
You need to log in before you can comment on or make changes to this bug.