Bug 58626 - Add restoreContext() to WEBKIT_lose_context
Summary: Add restoreContext() to WEBKIT_lose_context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-14 18:58 PDT by Kenneth Russell
Modified: 2011-07-20 16:13 PDT (History)
6 users (show)

See Also:


Attachments
Patch (15.68 KB, patch)
2011-07-20 14:01 PDT, Kenneth Russell
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2011-04-14 18:58:36 PDT
Based on feedback on the public_webgl list, a restoreContext() method was added to the WebGL WEBKIT_lose_context extension object. To track the extension spec, this needs to be implemented in WebKit.
Comment 1 Kenneth Russell 2011-07-20 14:01:27 PDT
Created attachment 101505 [details]
Patch
Comment 2 Kenneth Russell 2011-07-20 14:02:30 PDT
I'd greatly appreciate a quick review as I have another dependent patch to this code to bring it up to spec compliance.
Comment 3 James Robinson 2011-07-20 14:07:43 PDT
Comment on attachment 101505 [details]
Patch

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

Left some comments.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4768
> +    // There is no direct way to clear errors from a GL implementation and
> +    // looping until getError() becomes NO_ERROR might cause an infinite loop if
> +    // the driver or context implementation had a bug. So, loop a reasonably
> +    // large number of times to clear any existing errors.
> +    for (int i = 0; i < 100; ++i) {
> +        if (m_context->getError() == GraphicsContext3D::NO_ERROR)
> +            break;
> +    }

I'm not sure why we have to do this - what are the semantics of getError() after a lost context event is fired?  We replace m_context on restoration, so why is it important what errors exist on the old m_context?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4818
> +    RefPtr<GraphicsContext3D> context(GraphicsContext3D::create(m_attributes, canvas()->document()->view()->root()->hostWindow()));

Do you need any null checks here?  What if the <canvas> is inside an iframe that's display:none (I think document()->view() will be NULL then)?
Comment 4 Kenneth Russell 2011-07-20 14:16:13 PDT
Comment on attachment 101505 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4768
>> +    }
> 
> I'm not sure why we have to do this - what are the semantics of getError() after a lost context event is fired?  We replace m_context on restoration, so why is it important what errors exist on the old m_context?

See http://www.khronos.org/registry/webgl/specs/latest/#5.14.3 . This is an attempt to clear the OpenGL error state upon context loss and signal the CONTEXT_LOST_WEBGL OpenGL error to the application. (Note also that this is just a refactoring of existing code.)

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4818
>> +    RefPtr<GraphicsContext3D> context(GraphicsContext3D::create(m_attributes, canvas()->document()->view()->root()->hostWindow()));
> 
> Do you need any null checks here?  What if the <canvas> is inside an iframe that's display:none (I think document()->view() will be NULL then)?

I'm not sure. This is just a refactoring; that call chain has been there for a while. If we run into problems with it let's file another bug.
Comment 5 James Robinson 2011-07-20 15:33:46 PDT
Comment on attachment 101505 [details]
Patch

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

OK, R=me.  Left one suggestion for your consideration.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:347
> +    WebGLRenderingContextLostCallback(WebGLRenderingContext* cb) : m_context(cb) { }

nit: should be explicit

>>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4768
>>> +    }
>> 
>> I'm not sure why we have to do this - what are the semantics of getError() after a lost context event is fired?  We replace m_context on restoration, so why is it important what errors exist on the old m_context?
> 
> See http://www.khronos.org/registry/webgl/specs/latest/#5.14.3 . This is an attempt to clear the OpenGL error state upon context loss and signal the CONTEXT_LOST_WEBGL OpenGL error to the application. (Note also that this is just a refactoring of existing code.)

I see.  Would it be easier to do this faking out in WebGLRenderingContext::getError() and not touch the underlying context at all after it is lost?
Comment 6 Kenneth Russell 2011-07-20 15:54:35 PDT
Comment on attachment 101505 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:347
>> +    WebGLRenderingContextLostCallback(WebGLRenderingContext* cb) : m_context(cb) { }
> 
> nit: should be explicit

Will fix upon landing (and also the WebGLRenderingContextRestoreTimer constructor in the header).

>>>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4768
>>>> +    }
>>> 
>>> I'm not sure why we have to do this - what are the semantics of getError() after a lost context event is fired?  We replace m_context on restoration, so why is it important what errors exist on the old m_context?
>> 
>> See http://www.khronos.org/registry/webgl/specs/latest/#5.14.3 . This is an attempt to clear the OpenGL error state upon context loss and signal the CONTEXT_LOST_WEBGL OpenGL error to the application. (Note also that this is just a refactoring of existing code.)
> 
> I see.  Would it be easier to do this faking out in WebGLRenderingContext::getError() and not touch the underlying context at all after it is lost?

Possibly. Earlier there were efforts to hande synthetic errors only within the GraphicsContext3D but that probably doesn't make sense when the context's been lost. I'll keep this in mind for future changes.
Comment 7 Kenneth Russell 2011-07-20 16:13:41 PDT
Committed r91414: <http://trac.webkit.org/changeset/91414>