Bug 35626 – Implement WebGLContextLostEvent and WebGLContextRestoredEvent

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

Per the spec, delivery of WebGLContextLostEvent must be implemented and tested.

On desktop OpenGL it is not currently possible for the context to be "lost". Discussions are beginning in the OpenGL ARB to add an extension which would allow the OpenGL implementation to lose the context in an asynchronous manner, as can occur on platforms which use EGL. This would allow for more graceful recovery if the GPU is reset for various reasons.

In the interim, we can test the delivery of this event, and application recovery, via failure injection.

[Kenneth Russell]

Per recent spec updates, WebGLContextRestoredEvent must also be implemented, as must isContextLost().

[Kenneth Russell]

CONTEXT_LOST_WEBGL must also be added to the enum section, and it must be properly returned once from getError() when the context is lost.

[Kenneth Russell]

As part of this work, all WebGLObjects allocated before the context was lost must be invalidated. A generation number or something similar must be added to each object and compared against that of the context when validating incoming arguments.

[Adrienne Walker]

Assigning to myself.

[Adrienne Walker]

Created attachment 72519 [details]
Patch

[Zhenyao Mo]

You need to update the xcodeproj files.

Also, maybe we should have a bool makeContextCurrent() function which checks isContextLost()?

Se we don't need to modify each gl functions one more time when we fix See https://bugs.webkit.org/show_bug.cgi?id=45851.

[Adrienne Walker]

(In reply to comment #6)
> You need to update the xcodeproj files.

Will do.
 
> Also, maybe we should have a bool makeContextCurrent() function which checks isContextLost()?

Ah, interesting.  I didn't realize that refactoring was in the works.

This patch explicitly doesn't hook up the loseContext/restoreContext functions to anything live.  There's been a bit of discussion about how best to deliver that event (callbacks, polling, checking makeContextCurent elsewhere) and I wanted to commit this change first as it's mostly independent and WebGL's rendering context has a lot of spec-specific behavior under a lost context that needed to be implemented.

[Adrienne Walker]

Created attachment 72572 [details]
Patch

[Adrienne Walker]

(In reply to comment #8)
> Created an attachment (id=72572) [details]
> Patch

The change from the original is that the WebCore.xcodeproj was updated and build-webkit was verified to succeed.

[Kenneth Russell]

Comment on attachment 72572 [details]
Patch

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

Thanks for working through how to hook this up. It generally looks good but there are a few minor issues. Also, could you consider hooking up the webglcontextcreationerror event in WebGLRenderingContext::create in the case where the GraphicsContext3D creation fails? Also, please file a follow-on bug to actually call the loseContext / restoreContext entry points.

> LayoutTests/fast/canvas/webgl/resources/webgl-test-utils.js:287
> +  fillTexture(gl, tex, width, height, color);

Split this change into a separate patch.

> WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:242
> -        return v8::Undefined();
> +        return v8::Null();
>      }
>      if (!succeed)
> -        return v8::Undefined();
> +        return v8::Null();

These changes look unrelated and if so should be split into a separate patch.

> WebCore/html/canvas/WebGLEvent.cpp:16
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

This is actually not the correct license header. See e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp for the correct one.

> WebCore/html/canvas/WebGLEvent.cpp:36
> +WebGLEvent::WebGLEvent()

According to the spec this should be named WebGLContextEvent. Please rename it.

> WebCore/html/canvas/WebGLEvent.h:16
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

Wrong license header.

> WebCore/html/canvas/WebGLEvent.h:38
> +class WebGLEvent : public Event {

WebGLContextEvent.

> WebCore/html/canvas/WebGLEvent.h:54
> +protected:

For the time being, at least, these can be private.

> WebCore/html/canvas/WebGLEvent.idl:17
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.

Wrong license header.

> WebCore/html/canvas/WebGLEvent.idl:23
> +    ] WebGLEvent : Event {

WebGLContextEvent.

> WebCore/html/canvas/WebGLRenderingContext.idl:555
> +        [StrictTypeChecking, ConvertNullStringTo=Null] DOMString getProgramInfoLog(in WebGLProgram program) raises(DOMException);

These ConvertNullStringTo=Null changes look unrelated and if so should be split off into a separate patch.

[Adrienne Walker]

(In reply to comment #10)

> Also, please file a follow-on bug to actually call the loseContext /
> restoreContext entry points.

This is filed on the Chromium end at http://crbug.com/61507.  I can also file a matching WebKit bug, but I don't think it's necessary.
 
> > LayoutTests/fast/canvas/webgl/resources/webgl-test-utils.js:287
> > +  fillTexture(gl, tex, width, height, color);
> 
> Split this change into a separate patch.

Ah, I fixed this as a part of doing my manual testing.  I'll submit it separately. 
 
> > WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:242
> > -        return v8::Undefined();
> > +        return v8::Null();
> >      }
> >      if (!succeed)
> > -        return v8::Undefined();
> > +        return v8::Null();
> 
> These changes look unrelated and if so should be split into a separate patch.

This implements the part of the WebGL spec that says to return null for some functions in the case of a lost context.

> > WebCore/html/canvas/WebGLRenderingContext.idl:555
> > +        [StrictTypeChecking, ConvertNullStringTo=Null] DOMString getProgramInfoLog(in WebGLProgram program) raises(DOMException);
> 
> These ConvertNullStringTo=Null changes look unrelated and if so should be split off into a separate patch.

This was changed for the same reason as the above.

[Adrienne Walker]

Created attachment 72729 [details]
Patch

[Adrienne Walker]

Comment on attachment 72729 [details]
Patch

Removing r? because I realized I need to touch the js bindings as well as the v8 bindings.

[Adrienne Walker]

Created attachment 72745 [details]
Patch

[Kenneth Russell]

Comment on attachment 72745 [details]
Patch

Looks good.

[WebKit Commit Bot]

Comment on attachment 72745 [details]
Patch

Rejecting patch 72745 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', 72745]" exit_code: 2
Last 500 characters of output:
3 succeeded at 3747 (offset 14 lines).
Hunk #134 succeeded at 3758 (offset 14 lines).
1 out of 134 hunks FAILED -- saving rejects to file WebCore/html/canvas/WebGLRenderingContext.cpp.rej
patching file WebCore/html/canvas/WebGLRenderingContext.h
patching file WebCore/html/canvas/WebGLRenderingContext.idl
patching file WebCore/platform/graphics/GraphicsContext3D.h

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kenneth Russell', u'--force']" exit_code: 1

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

[Kenneth Russell]

Adrienne, looks like you'll have to regenerate the patch to fix merge conflicts.

[Adrienne Walker]

Created attachment 72845 [details]
Patch

[Kenneth Russell]

Comment on attachment 72845 [details]
Patch

Let's try again.

[Adrienne Walker]

(In reply to comment #19)
> (From update of attachment 72845 [details])
> Let's try again.

Thanks.  :)

I think this just collided with the recent change to blend function parameter validation.

[Zhenyao Mo]

OK, I won't commit anything to WebGLRenderingContext until this lands.

[Kenneth Russell]

Comment on attachment 72845 [details]
Patch

The commit queue's taking too long to handle this patch and it's already got conflicts again (partly due to a patch I just landed). I'm going to land it by hand.

[Kenneth Russell]

Committed r71274: <http://trac.webkit.org/changeset/71274>

[Adrienne Walker]

(In reply to comment #23)
> Committed r71274: <http://trac.webkit.org/changeset/71274>

Thank you.

[WebKit Review Bot]

http://trac.webkit.org/changeset/71274 might have broken SnowLeopard Intel Release (Tests)

[Adrienne Walker]

(In reply to comment #25)
> http://trac.webkit.org/changeset/71274 might have broken SnowLeopard Intel Release (Tests)

This has been filed here: https://bugs.webkit.org/show_bug.cgi?id=48962