Bug 35626 - Implement WebGLContextLostEvent and WebGLContextRestoredEvent
Summary: Implement WebGLContextLostEvent and WebGLContextRestoredEvent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-02 19:19 PST by Kenneth Russell
Modified: 2010-11-03 17:07 PDT (History)
10 users (show)

See Also:


Attachments
Patch (83.88 KB, patch)
2010-11-01 10:29 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (88.57 KB, patch)
2010-11-01 15:15 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (88.47 KB, patch)
2010-11-02 13:52 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (89.63 KB, patch)
2010-11-02 15:03 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (89.91 KB, patch)
2010-11-03 11:27 PDT, Adrienne Walker
kbr: review+
kbr: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2010-03-02 19:19:31 PST
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.
Comment 1 Kenneth Russell 2010-06-08 11:25:36 PDT
Per recent spec updates, WebGLContextRestoredEvent must also be implemented, as must isContextLost().
Comment 2 Kenneth Russell 2010-06-08 11:26:14 PDT
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.
Comment 3 Kenneth Russell 2010-06-08 11:29:54 PDT
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.
Comment 4 Adrienne Walker 2010-09-27 15:39:25 PDT
Assigning to myself.
Comment 5 Adrienne Walker 2010-11-01 10:29:42 PDT
Created attachment 72519 [details]
Patch
Comment 6 Zhenyao Mo 2010-11-01 13:28:48 PDT
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.
Comment 7 Adrienne Walker 2010-11-01 14:10:10 PDT
(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.
Comment 8 Adrienne Walker 2010-11-01 15:15:37 PDT
Created attachment 72572 [details]
Patch
Comment 9 Adrienne Walker 2010-11-01 15:17:44 PDT
(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.
Comment 10 Kenneth Russell 2010-11-02 12:36:27 PDT
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.
Comment 11 Adrienne Walker 2010-11-02 12:41:28 PDT
(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.
Comment 12 Adrienne Walker 2010-11-02 13:52:14 PDT
Created attachment 72729 [details]
Patch
Comment 13 Adrienne Walker 2010-11-02 14:33:28 PDT
Comment on attachment 72729 [details]
Patch

Removing r? because I realized I need to touch the js bindings as well as the v8 bindings.
Comment 14 Adrienne Walker 2010-11-02 15:03:11 PDT
Created attachment 72745 [details]
Patch
Comment 15 Kenneth Russell 2010-11-02 16:21:15 PDT
Comment on attachment 72745 [details]
Patch

Looks good.
Comment 16 WebKit Commit Bot 2010-11-02 20:50:29 PDT
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
Comment 17 Kenneth Russell 2010-11-03 10:26:32 PDT
Adrienne, looks like you'll have to regenerate the patch to fix merge conflicts.
Comment 18 Adrienne Walker 2010-11-03 11:27:24 PDT
Created attachment 72845 [details]
Patch
Comment 19 Kenneth Russell 2010-11-03 11:38:12 PDT
Comment on attachment 72845 [details]
Patch

Let's try again.
Comment 20 Adrienne Walker 2010-11-03 11:41:12 PDT
(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.
Comment 21 Zhenyao Mo 2010-11-03 12:19:37 PDT
OK, I won't commit anything to WebGLRenderingContext until this lands.
Comment 22 Kenneth Russell 2010-11-03 15:06:09 PDT
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.
Comment 23 Kenneth Russell 2010-11-03 15:07:31 PDT
Committed r71274: <http://trac.webkit.org/changeset/71274>
Comment 24 Adrienne Walker 2010-11-03 15:12:08 PDT
(In reply to comment #23)
> Committed r71274: <http://trac.webkit.org/changeset/71274>

Thank you.
Comment 25 WebKit Review Bot 2010-11-03 16:57:06 PDT
http://trac.webkit.org/changeset/71274 might have broken SnowLeopard Intel Release (Tests)
Comment 26 Adrienne Walker 2010-11-03 17:07:00 PDT
(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