Bug 153000 – [iOS] WebGL Antialiasing doesn't work

• Component: WebGL
• Status: RESOLVED FIXED

[Dean Jackson]

We never set up multisampling correctly on iOS. Fix this.

<rdar://problem/9165531>

[Dean Jackson]

Created attachment 268737 [details]
Patch

[Build Bot]

Comment on attachment 268737 [details]
Patch

Attachment 268737 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/680718

New failing tests:
fast/canvas/webgl/antialiasing-enabled.html

[Build Bot]

Created attachment 268739 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268737 [details]
Patch

Attachment 268737 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/680765

New failing tests:
fast/canvas/webgl/antialiasing-enabled.html

[Build Bot]

Created attachment 268741 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Dean Jackson]

Letting EWS chew on it, since I'm not sure about the bots, software rendering, EFL and GTK.

[Dean Jackson]

I don't understand why it wouldn't get a context on Yosemite.

[Dean Jackson]

Oh, WK1

[Dean Jackson]

Created attachment 268814 [details]
Patch

[Alex Christensen]

Comment on attachment 268814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268814&action=review

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:404
> +        resolveMultisamplingIfNecessary();

Is this needed on non-iOS WebGL implementations?

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:143
> +        ::glRenderbufferStorageMultisampleEXT(GL_RENDERBUFFER_EXT, sampleCount, GL_RGBA8_OES, width, height);

Why not m_internalColorFormat?

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:245
> +    ::glBindFramebuffer(GL_FRAMEBUFFER, boundFrameBuffer);

Is this needed for non-iOS implementations of WebGL?

> LayoutTests/ChangeLog:8
> +        New test to check if WebGL antialiasing happened on a rendered canvas.

This test is great, but we already have tests that fail if antialiasing doesn't happen.

[Dean Jackson]

Comment on attachment 268814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268814&action=review

>> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:404
>> +        resolveMultisamplingIfNecessary();
> 
> Is this needed on non-iOS WebGL implementations?

It's called from prepareTexture on non-iOS. endPaint() is an iOS-only bit of code.

At some point we should clean all this up. I think we don't really need the GL CALayer types. Instead we should create IOSurfaces and render directly to them, just like we do for other stuff. At the same time we should combine the rendering code so that iOS and other ports are more similar.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:143
>> +        ::glRenderbufferStorageMultisampleEXT(GL_RENDERBUFFER_EXT, sampleCount, GL_RGBA8_OES, width, height);
> 
> Why not m_internalColorFormat?

This is specific to iOS multisampling. m_internalColorFormat is probably defined to be GL_RGBA8 which doesn't work :(

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:245
>> +    ::glBindFramebuffer(GL_FRAMEBUFFER, boundFrameBuffer);
> 
> Is this needed for non-iOS implementations of WebGL?

Not really. I could move it into the PLATFORM(IOS) bit, but it shouldn't harm anything.

[Alex Christensen]

Comment on attachment 268814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268814&action=review

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:143
>> +        ::glRenderbufferStorageMultisampleEXT(GL_RENDERBUFFER_EXT, sampleCount, GL_RGBA8_OES, width, height);
> 
> Why not m_internalColorFormat?

m_internalColorFormat seems to only be used for non-iOS elsewhere.  This is just continuing that.

[Dean Jackson]

Comment on attachment 268814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268814&action=review

>> LayoutTests/ChangeLog:8
>> +        New test to check if WebGL antialiasing happened on a rendered canvas.
> 
> This test is great, but we already have tests that fail if antialiasing doesn't happen.

Let's add this one too!

[Alex Christensen]

Comment on attachment 268814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268814&action=review

r=me
hooray!  Now all the WebGL will look so nice!

>>> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:404
>>> +        resolveMultisamplingIfNecessary();
>> 
>> Is this needed on non-iOS WebGL implementations?
> 
> It's called from prepareTexture on non-iOS. endPaint() is an iOS-only bit of code.
> 
> At some point we should clean all this up. I think we don't really need the GL CALayer types. Instead we should create IOSurfaces and render directly to them, just like we do for other stuff. At the same time we should combine the rendering code so that iOS and other ports are more similar.

Sure enough.

>>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:245
>>> +    ::glBindFramebuffer(GL_FRAMEBUFFER, boundFrameBuffer);
>> 
>> Is this needed for non-iOS implementations of WebGL?
> 
> Not really. I could move it into the PLATFORM(IOS) bit, but it shouldn't harm anything.

It adds unnecessary operations to the other implementations.

[Dean Jackson]

Committed r194933: <http://trac.webkit.org/changeset/194933>