Bug 111780

Summary: Check to ensure MultisampleRenderbuffer creation succeeds
Product: WebKit Reporter: Brandon Jones <bajones>
Component: New BugsAssignee: Brandon Jones <bajones>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dino, gman, kbr, rniwa, webkit-ews, webkit.review.bot, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 112217    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Brandon Jones 2013-03-07 15:21:09 PST
Check to ensure MultisampleRenderbuffer creation succeeds
Comment 1 Brandon Jones 2013-03-07 15:21:44 PST
Created attachment 192092 [details]
Patch
Comment 2 Brandon Jones 2013-03-07 15:24:43 PST
This will allow us to re-enable WebGL antialiasing on AMD Macs while we wait for a driver fix.
Comment 3 Early Warning System Bot 2013-03-07 15:26:48 PST
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
Comment 4 Early Warning System Bot 2013-03-07 15:27:42 PST
Comment on attachment 192092 [details]
Patch

Attachment 192092 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17024054
Comment 5 Brandon Jones 2013-03-07 15:33:27 PST
Created attachment 192095 [details]
Patch
Comment 6 Early Warning System Bot 2013-03-07 15:39:16 PST
Comment on attachment 192095 [details]
Patch

Attachment 192095 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17075184
Comment 7 Early Warning System Bot 2013-03-07 15:41:41 PST
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
Comment 8 Kenneth Russell 2013-03-07 15:47:14 PST
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.
Comment 9 Brandon Jones 2013-03-08 15:01:13 PST
Created attachment 192291 [details]
Patch
Comment 10 Dean Jackson 2013-03-08 15:05:14 PST
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.
Comment 11 Brandon Jones 2013-03-08 15:08:12 PST
Created attachment 192292 [details]
Patch
Comment 12 WebKit Review Bot 2013-03-08 15:49:23 PST
Comment on attachment 192292 [details]
Patch

Clearing flags on attachment: 192292

Committed r145280: <http://trac.webkit.org/changeset/145280>
Comment 13 WebKit Review Bot 2013-03-08 15:49:26 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 WebKit Review Bot 2013-03-12 18:10:03 PDT
Re-opened since this is blocked by bug 112217
Comment 15 Build Bot 2013-03-13 00:46:01 PDT
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
Comment 16 Brandon Jones 2013-03-14 14:45:03 PDT
Created attachment 193187 [details]
Patch
Comment 17 Brandon Jones 2013-03-14 14:48:01 PDT
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.
Comment 18 Kenneth Russell 2013-03-14 15:37:55 PDT
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?
Comment 19 Brandon Jones 2013-03-14 15:48:28 PDT
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?
Comment 20 Kenneth Russell 2013-03-14 16:01:11 PDT
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.
Comment 21 Brandon Jones 2013-03-14 16:05:27 PDT
Created attachment 193196 [details]
Patch
Comment 22 WebKit Review Bot 2013-03-14 16:08:59 PDT
Comment on attachment 193196 [details]
Patch

Rejecting attachment 193196 [details] from commit-queue.

bajones@chromium.org 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.
Comment 23 WebKit Review Bot 2013-03-14 16:35:37 PDT
Comment on attachment 193196 [details]
Patch

Clearing flags on attachment: 193196

Committed r145856: <http://trac.webkit.org/changeset/145856>
Comment 24 WebKit Review Bot 2013-03-14 16:35:42 PDT
All reviewed patches have been landed.  Closing bug.