Bug 153000 - [iOS] WebGL Antialiasing doesn't work
Summary: [iOS] WebGL Antialiasing doesn't work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-11 18:10 PST by Dean Jackson
Modified: 2016-01-12 16:13 PST (History)
7 users (show)

See Also:


Attachments
Patch (12.94 KB, patch)
2016-01-11 18:13 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (894.38 KB, application/zip)
2016-01-11 18:42 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (871.35 KB, application/zip)
2016-01-11 19:06 PST, Build Bot
no flags Details
Patch (13.82 KB, patch)
2016-01-12 15:00 PST, Dean Jackson
achristensen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2016-01-11 18:10:57 PST
We never set up multisampling correctly on iOS. Fix this.

<rdar://problem/9165531>
Comment 1 Dean Jackson 2016-01-11 18:13:04 PST
Created attachment 268737 [details]
Patch
Comment 2 Build Bot 2016-01-11 18:42:20 PST
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
Comment 3 Build Bot 2016-01-11 18:42:23 PST
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
Comment 4 Build Bot 2016-01-11 19:06:04 PST
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
Comment 5 Build Bot 2016-01-11 19:06:07 PST
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
Comment 6 Dean Jackson 2016-01-11 20:08:50 PST
Letting EWS chew on it, since I'm not sure about the bots, software rendering, EFL and GTK.
Comment 7 Dean Jackson 2016-01-12 14:05:30 PST
I don't understand why it wouldn't get a context on Yosemite.
Comment 8 Dean Jackson 2016-01-12 14:07:33 PST
Oh, WK1
Comment 9 Dean Jackson 2016-01-12 15:00:25 PST
Created attachment 268814 [details]
Patch
Comment 10 Alex Christensen 2016-01-12 15:33:05 PST
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.
Comment 11 Dean Jackson 2016-01-12 15:43:13 PST
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.
Comment 12 Alex Christensen 2016-01-12 15:43:17 PST
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.
Comment 13 Dean Jackson 2016-01-12 15:47:51 PST
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!
Comment 14 Alex Christensen 2016-01-12 15:54:26 PST
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.
Comment 15 Dean Jackson 2016-01-12 16:13:11 PST
Committed r194933: <http://trac.webkit.org/changeset/194933>