615976 - Implement WebGL antialiasing

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Alvaro]

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]]

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]

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]]

Yes, the problem affects every WebGL content.

Comment 4

[Csaba Kozák [:WonderCsabo]]

Benoit, Joe, Vladimir, any comment on this?

Comment 5

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

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)]

we'd love to have AA support in webgl for the Tilt visualization project.

Comment 7

[Kelsey Gilbert [:jgilbert]]

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)]

(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

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

PBuffers now also have antialiasing.

Comment 12

[Joe Drew (not getting mail)]

Can you upload your works in progress? A user repo of your patch queue is fine.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: 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

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

It also works for me on linux with no changes. Not that surprising since GLX uses the GLContext FBOs, but nice.

Comment 16

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

Attachment: 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)]

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

[Kelsey Gilbert [:jgilbert]]

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)]

(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)]

(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)]

(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

[Kelsey Gilbert [:jgilbert]]

Attachment: 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

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

Attachment: 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

[Kelsey Gilbert [:jgilbert]]

Attachment: 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)]

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

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 1: Fixes WebGL to default to attempting AA

Comment 30

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 2: Adds support for split Draw/Read buffers to GLContext

Comment 31

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples'

Comment 32

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 4: Adds glRenderBufferStorageMultisample, extension detection for framebuffer_multisample

Comment 33

[Kelsey Gilbert [:jgilbert]]

Attachment: 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

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts

Comment 35

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.2: Fixes for support for multisampled Linux/GLX GLContexts

Comment 36

[Kelsey Gilbert [:jgilbert]]

Attachment: 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

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

Also, be advised that this iteration still has a huge amount of console printf noise.

Comment 39

[Kelsey Gilbert [:jgilbert]]

Note: Patch from bug 688112 is required, and 686083 is recommended.

Comment 40

[Kelsey Gilbert [:jgilbert]]

Fixed the layering issue on Mac, so AA can be re-enabled.

Comment 41

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.3: Fixes layer compositing issue resulting in garbage WebGL canvases on Mac

Comment 42

[Kelsey Gilbert [:jgilbert]]

Attachment: Fixes layer compositing with AA on Mac

Will submit a new try build for this shortly.

Comment 43

[Kelsey Gilbert [:jgilbert]]

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)]

Then please update it here, as I'm about to start reviewing.

Comment 45

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 1: Fixes WebGL to default to attempting AA

Comment 46

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples'

Comment 47

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 2: Adds support for split Draw/Read buffers to GLContext

Comment 48

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples'

Comment 49

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 4: Adds glRenderBufferStorageMultisample, extension detection for framebuffer_multisample

Comment 50

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths

Comment 51

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts

Comment 52

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.1.1: Fixes layer compositing with AA on Mac

Comment 53

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.2: Fixes for support for multisampled Linux/GLX GLContexts

Comment 54

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed

Comment 55

[Kelsey Gilbert [:jgilbert]]

Attachment: Prepatch 0.1: Printf of the resulting implementation of GLContext

Comment 56

[Kelsey Gilbert [:jgilbert]]

Attachment: Prepatch 0.2: Fixes GLContext::ResizeOffscreen leaving a mess on failure

Comment 57

[Kelsey Gilbert [:jgilbert]]

Attachment: Prepatch 0.3: Adds method to disable GL implementations

Comment 58

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

Attachment: Prepatch 0.2: Fixes GLContext::ResizeOffscreen leaving a mess on failure

Comment 61

[Kelsey Gilbert [:jgilbert]]

Attachment: Prepatch 0.3: Adds method to disable GL implementations

Comment 62

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 1: Fixes WebGL to default to attempting AA

Comment 63

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 2: Adds support for split Draw/Read buffers to GLContext

Comment 64

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 3: Adds support for samples in ContextFormat, pref 'webgl.max-samples'

Comment 65

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 4: Adds glRenderBufferStorageMultisample, extension detection for framebuffer_multisample

Comment 66

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths

Comment 67

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts

Comment 68

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.1.1: Fixes layer compositing with AA on Mac

Comment 69

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.2: Fixes for support for multisampled Linux/GLX GLContexts

Comment 70

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed

Comment 71

[Kelsey Gilbert [:jgilbert]]

Attachment: 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

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths

Comment 73

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts

Comment 74

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.1.1: Fixes layer compositing with AA on Mac

Comment 75

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed

Comment 76

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 2: Adds support for split Draw/Read buffers to GLContext

One more backpatch I didn't see.

Comment 77

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths

Comment 78

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

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)]

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)]

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)]

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)]

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)]

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)]

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)]

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)]

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

[Kelsey Gilbert [:jgilbert]]

I've updated the patch for bug 681791, which is the prepatch 0.2 here.

Comment 88

[Kelsey Gilbert [:jgilbert]]

(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]]

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

[Kelsey Gilbert [:jgilbert]]

(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

[Kelsey Gilbert [:jgilbert]]

(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

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

Attachment: 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

[Kelsey Gilbert [:jgilbert]]

Attachment: 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

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 3: Adds support for samples in ContextFormat, pref 'webgl.msaa-level'

Updated desc and obsoleted old patches.

Comment 96

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 4: Adds glRenderBufferStorageMultisample, extension detection for framebuffer_multisample

Updated to reflect new approach to loading extension-dependent symbols from Patch 2.

Comment 97

[Kelsey Gilbert [:jgilbert]]

(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

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5: Adds support for multisampled GLContexts, esp. EGL, WGL paths

Bunch of fixes as per review.

Comment 99

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.1: Fixes for support for multisampled Mac/CGL GLContexts

Fixed indentation problems.

Comment 100

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.1.1: Fixes layer compositing with AA on Mac

Un-bit-rotted r+ from bjacob.

Comment 101

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 5.2: Fixes for support for multisampled Linux/GLX GLContexts

Un-bit-rotted r+ from bjacob.

Comment 102

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 6: Fix ResizeOffscreenFBO to attempt resize without AA if resize with AA failed

Un-bit-rotted r+ from bjacob.

Comment 103

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

Looks like I messed up part of Patch 5.1.

New full try:
https://tbpl.mozilla.org/?tree=Try&rev=eac0015ee128

Comment 105

[Kelsey Gilbert [:jgilbert]]

Attachment: 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 106

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/17c58be07034
http://hg.mozilla.org/integration/mozilla-inbound/rev/941b7fe14983
http://hg.mozilla.org/integration/mozilla-inbound/rev/695f7fad2f53
http://hg.mozilla.org/integration/mozilla-inbound/rev/4d12cfb7ef31
http://hg.mozilla.org/integration/mozilla-inbound/rev/b9efa34601af
http://hg.mozilla.org/integration/mozilla-inbound/rev/4cada6574ad4
http://hg.mozilla.org/integration/mozilla-inbound/rev/16849a9b4745
http://hg.mozilla.org/integration/mozilla-inbound/rev/69dfebdcfdd3
http://hg.mozilla.org/integration/mozilla-inbound/rev/bf09dbe3fcd7

Comment 107

[Kelsey Gilbert [:jgilbert]]

Example for easy-to-see AA/no-AA:
http://www.ibiblio.org/e-notes/webgl/deflate/ant.html

Comment 108

[ojab]

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 109

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/17c58be07034
https://hg.mozilla.org/mozilla-central/rev/941b7fe14983
https://hg.mozilla.org/mozilla-central/rev/695f7fad2f53
https://hg.mozilla.org/mozilla-central/rev/4d12cfb7ef31
https://hg.mozilla.org/mozilla-central/rev/b9efa34601af
https://hg.mozilla.org/mozilla-central/rev/4cada6574ad4
https://hg.mozilla.org/mozilla-central/rev/16849a9b4745
https://hg.mozilla.org/mozilla-central/rev/69dfebdcfdd3
https://hg.mozilla.org/mozilla-central/rev/bf09dbe3fcd7

Comment 110

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

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]

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)]

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]]

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)]

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]]

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)]

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]]

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)]

I don't think that even the NVIDIA driver could override webgl.msaa-level=0. Try webgl.msaa-level=1 instead.

Comment 119

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 (he/him; ⌚ UTC+1/+2)]

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)]

(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]]

(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)]

(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)]

(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]

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

[Kelsey Gilbert [:jgilbert]]

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)]

(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

[Kelsey Gilbert [:jgilbert]]

Attachment: Post-Patch Cleanup

Supplemental patch to clean up errant printfs and comments.

Comment 129

[Kelsey Gilbert [:jgilbert]]

Attachment: 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)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/49ae6e1468dd

Comment 132

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/49ae6e1468dd

Comment 133

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/getContext#Parameters