Bug 111780 – Check to ensure MultisampleRenderbuffer creation succeeds

• Component: New Bugs
• Status: RESOLVED FIXED

[Brandon Jones]

Check to ensure MultisampleRenderbuffer creation succeeds

[Brandon Jones]

Created attachment 192092 [details]
Patch

[Brandon Jones]

This will allow us to re-enable WebGL antialiasing on AMD Macs while we wait for a driver fix.

[Early Warning System Bot]

Comment on attachment 192092 [details]
Patch

Attachment 192092 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17070270

[Early Warning System Bot]

Comment on attachment 192092 [details]
Patch

Attachment 192092 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17024054

[Brandon Jones]

Created attachment 192095 [details]
Patch

[Early Warning System Bot]

Comment on attachment 192095 [details]
Patch

Attachment 192095 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17075184

[Early Warning System Bot]

Comment on attachment 192095 [details]
Patch

Attachment 192095 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17038111

[Kenneth Russell]

Comment on attachment 192095 [details]
Patch

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

Looks pretty good overall but a few changes are needed. Also, please ensure you've tested this with multisampling re-enabled on all GPU types on OS X.

> Source/WebCore/ChangeLog:8
> +        No new tests.

Why not? Please describe the issue and workaround, and why an automated test can't be written if necessary. Please also indicate what testing was done for this patch.

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:193
> +// TODO(bajones): This can be removed once renderbufferStorageMultisample starts reporting GL_OUT_OF_MEMORY properly

Nit: please use periods after sentences, here and throughout the patch.

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> +    uint32 pixel = 0;

Please use an unsigned char[4] instead of a uint32 to avoid byte ordering problems.

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:310
> +#if PLATFORM(MAC)

This won't take effect for the Chromium port. For that port you need to use PLATFORM(CHROMIUM) and OS(DARWIN). Just using OS(DARWIN) may be the best way to ensure this runs on all WebKit ports on the Mac. It would be better if you could figure out a way to turn this into a run-time check that runs only on the appropriate GPUs.

[Brandon Jones]

Created attachment 192291 [details]
Patch

[Dean Jackson]

Comment on attachment 192291 [details]
Patch

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

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:193
> +// TODO(bajones): This can be removed once renderbufferStorageMultisample starts reporting GL_OUT_OF_MEMORY properly.

We don't normally use names in comments, and WebKit tends to use "FIXME:" rather than TODO.

[Brandon Jones]

Created attachment 192292 [details]
Patch

[WebKit Review Bot]

Comment on attachment 192292 [details]
Patch

Clearing flags on attachment: 192292

Committed r145280: <http://trac.webkit.org/changeset/145280>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 112217

[Build Bot]

Comment on attachment 192292 [details]
Patch

Attachment 192292 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17136144

New failing tests:
xmlviewer/extensions-api.html

[Brandon Jones]

Created attachment 193187 [details]
Patch

[Brandon Jones]

Issue breaking the tests was that glColorMask or GL_SCISSOR state could interfere with the validation check by preventing the correct color from being written. Updated patch ensures valid state before testing.

[Kenneth Russell]

Comment on attachment 193187 [details]
Patch

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

Good catch. Couple of questions.

> Source/WebCore/ChangeLog:-6566
> -

Don't remove the old ChangeLog entry.

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:202
> +    m_context->colorMask(true, true, true, true);

Don't you also need to restore the user's color mask afterward?

[Brandon Jones]

Comment on attachment 193187 [details]
Patch

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

>> Source/WebCore/ChangeLog:-6566
>> -
> 
> Don't remove the old ChangeLog entry.

Okay, thanks. Wasn't sure what the protocol was in this case.

>> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:202
>> +    m_context->colorMask(true, true, true, true);
> 
> Don't you also need to restore the user's color mask afterward?

If so then we're already broken. reset() does the same thing (Line 342) without ever restoring it. Perhaps this is a bug?

[Kenneth Russell]

Comment on attachment 193187 [details]
Patch

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

Looks fine modulo the ChangeLog entry.

>>> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:202
>>> +    m_context->colorMask(true, true, true, true);
>> 
>> Don't you also need to restore the user's color mask afterward?
> 
> If so then we're already broken. reset() does the same thing (Line 342) without ever restoring it. Perhaps this is a bug?

Looking again, WebGLRenderingContext::restoreStateAfterClear resets the color mask, so this is OK.

[Brandon Jones]

Created attachment 193196 [details]
Patch

[WebKit Review Bot]

Comment on attachment 193196 [details]
Patch

Rejecting attachment 193196 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 193196 [details]
Patch

Clearing flags on attachment: 193196

Committed r145856: <http://trac.webkit.org/changeset/145856>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.