Bug 58626 – Add restoreContext() to WEBKIT_lose_context

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

[Kenneth Russell]

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.

[Kenneth Russell]

Created attachment 101505 [details]
Patch

[Kenneth Russell]

I'd greatly appreciate a quick review as I have another dependent patch to this code to bring it up to spec compliance.

[James Robinson]

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)?

[Kenneth Russell]

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.

[James Robinson]

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?

[Kenneth Russell]

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.

[Kenneth Russell]

Committed r91414: <http://trac.webkit.org/changeset/91414>