Bug 46850 – Add GetGraphicsResetStatusARB entry point from ARB_robustness extension to GraphicsContext3D

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Alexey Marinichev]

ARB_robustness extension provides a mechanism for OpenGL applications to to learn about graphics resets that affect the context.  This will allow proper repopulating of textures in Chrome with active compositor.

[Alexey Marinichev]

Created attachment 69279 [details]
Patch

[Chris Marrin]

Comment on attachment 69279 [details]
Patch

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

> WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1456
> +

I don't understand the point of this patch. It doesn't hook anything up. It simply adds placeholders. You should do at least one implementation to show that it actually works, along with a test case with a bad shader that will crash if ARB_robustness is not there.

I understand if this is just a stepping stone to that goal. But this bug is for "hooking up" the extension, which this patch doesn't do.

[Kenneth Russell]

(In reply to comment #2)
> (From update of attachment 69279 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69279&action=review
> 
> > WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1456
> > +
> 
> I don't understand the point of this patch. It doesn't hook anything up. It simply adds placeholders. You should do at least one implementation to show that it actually works, along with a test case with a bad shader that will crash if ARB_robustness is not there.
> 
> I understand if this is just a stepping stone to that goal. But this bug is for "hooking up" the extension, which this patch doesn't do.

Several steps are needed toward the goal of handling "context lost" events in WebGL and other functionality such as CSS 3D support. Adding the needed entry points to GraphicsContext3D allows some of this work to occur in parallel.

I have updated the synopsis of this bug to reflect the intent.

Alexey, by way of review of your initial patch, I think you should leave the ARB suffix on the new enums and entry point. There is also an ongoing discussion about exactly how extensions should be added (see https://bugs.webkit.org/show_bug.cgi?id=45939 ), but to keep things consistent with the current style, I think you should add a query function indicating whether the extension is supported.

[Alexey Marinichev]

I did not see any enum values or entry points with ARB in them anywhere else related to GraphicsContext3D.  Where can I find an example?

[Chris Marrin]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 69279 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=69279&action=review
> > 
> > > WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1456
> > > +
> > 
> > I don't understand the point of this patch. It doesn't hook anything up. It simply adds placeholders. You should do at least one implementation to show that it actually works, along with a test case with a bad shader that will crash if ARB_robustness is not there.
> > 
> > I understand if this is just a stepping stone to that goal. But this bug is for "hooking up" the extension, which this patch doesn't do.
> 
> Several steps are needed toward the goal of handling "context lost" events in WebGL and other functionality such as CSS 3D support. Adding the needed entry points to GraphicsContext3D allows some of this work to occur in parallel.
> 
> I have updated the synopsis of this bug to reflect the intent.

Ok, that's better. Maybe you should create an umbrella bug so we can keep track of all the work?

I will amend my review

[Chris Marrin]

Comment on attachment 69279 [details]
Patch

r+

[Alexey Marinichev]

Created attachment 69291 [details]
added ARB suffix

[Kenneth Russell]

Could you please fix the synopsis in the ChangeLogs to match the new synopsis of the bug?

[Alexey Marinichev]

Created attachment 69293 [details]
fixed changelogs

[Kenneth Russell]

This looks fine to me. Since Chris reviewed the original patch I'll defer to him to review this one.

[Chris Marrin]

Comment on attachment 69293 [details]
fixed changelogs

r=me. But we need to factor this call into what we ever do in https://bugs.webkit.org/show_bug.cgi?id=46894.

[WebKit Commit Bot]

Comment on attachment 69293 [details]
fixed changelogs

Rejecting patch 69293 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 69293]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=69293&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=46850&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 69293 from bug 46850.
Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Chris Marrin', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4226031

[Kenneth Russell]

What happened with this commit queue failure? There doesn't seem to be any error detail. I'll try resubmitting the patch.

[WebKit Commit Bot]

Comment on attachment 69293 [details]
fixed changelogs

Rejecting patch 69293 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 69293]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=69293&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=46850&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 69293 from bug 46850.
Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Chris Marrin', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4149034

[Kenneth Russell]

Ah, the issue is that part of the patch is against WebKit/chromium/src/GraphicsContext3D.cpp, which Alexey just renamed and which no longer exists.

[Alexey Marinichev]

Created attachment 69391 [details]
renamed file fix

[WebKit Commit Bot]

Comment on attachment 69391 [details]
renamed file fix

Clearing flags on attachment: 69391

Committed r68850: <http://trac.webkit.org/changeset/68850>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Yeah, the cq logs are less useful atm.  We're working on a fix.