Bug 141567 – WebGL: Destroy the GLContext when a GPU restart has been detected.

• Component: WebGL
• Status: RESOLVED FIXED

[Roger Fong]

rdar://problem/18507496

[Roger Fong]

Created attachment 246534 [details]
patch

[Dean Jackson]

Comment on attachment 246534 [details]
patch

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

> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:93
> +    GLint restartStatus = 0;
> +    CGLGetParameter(glContext, kCGLCPGPURestartStatus, &restartStatus);
> +    if (restartStatus == kCGLCPGPURestartStatusCaused || restartStatus == kCGLCPGPURestartStatusBlacklisted) {
> +        CGLDestroyContext(glContext);
> +        return;
> +    }
> +    

As discussed in IRC, I think we need a little more intelligence here.

1. Check that this triggers a WebGL context lost (or make sure that this does that)
2. Ensure a page doesn't keep recreating the context in this case and submitting the same bad data. Maybe allow 3 lost contexts before crashing the process?

[Roger Fong]

Created attachment 246923 [details]
patch

[Roger Fong]

(In reply to comment #3)
> Created attachment 246923 [details]
> patch

I've now triggered a context lost event and made sure that the context does not get restored.
It also adopts the same reload policy as all other webcontent crashes. 3 tries and an error.

[Dean Jackson]

Comment on attachment 246923 [details]
patch

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

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1445
> +    
> +    WebGLRenderingContextBase* m_webglContext;

Do we ever think there might be a time when the WebGL context goes away before the GC3D?

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:202
> +    GLint abortOnBlacklist = 0;
> +    CGLSetParameter(m_contextObj, kCGLCPAbortOnGPURestartStatusBlacklisted, &abortOnBlacklist);

You'll need to #if around these since they are only available on OS X 10.10+

> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:180
> +    GLint restartStatus = 0;
> +    CGLGetParameter(_context->platformGraphicsContext3D(), kCGLCPGPURestartStatus, &restartStatus);
> +    if (restartStatus == kCGLCPGPURestartStatusCaused || restartStatus == kCGLCPGPURestartStatusBlacklisted) {
> +        CGLDestroyContext(_context->platformGraphicsContext3D());
> +        _context->forceContextLost();
> +    }

Similarly here.

One concern I have is that we're only doing this test on display. Which means you could hose the system if you executed a lot of draw calls before allowing the system to draw. We might want to have this code in the drawing entry points, drawElements and drawArrays, but only test every X iterations or something.

Anyway, if this makes an improvement on our hanging examples, let's stick with it.

[Roger Fong]

(In reply to comment #5)
> Comment on attachment 246923 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246923&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext3D.h:1445
> > +    
> > +    WebGLRenderingContextBase* m_webglContext;
> 
> Do we ever think there might be a time when the WebGL context goes away
> before the GC3D?

Hmm, if the destructor for WebGLRenderingContextBase gets called we always also destroy the associated graphicsContext3D, so this shouldn't be an issue.
I believe I added a nullcheck to the forceContextLost call just in case. Maybe an assert would be more appropriate though.

> 
> > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:202
> > +    GLint abortOnBlacklist = 0;
> > +    CGLSetParameter(m_contextObj, kCGLCPAbortOnGPURestartStatusBlacklisted, &abortOnBlacklist);
> 
> You'll need to #if around these since they are only available on OS X 10.10+
> 
> > Source/WebCore/platform/graphics/mac/WebGLLayer.mm:180
> > +    GLint restartStatus = 0;
> > +    CGLGetParameter(_context->platformGraphicsContext3D(), kCGLCPGPURestartStatus, &restartStatus);
> > +    if (restartStatus == kCGLCPGPURestartStatusCaused || restartStatus == kCGLCPGPURestartStatusBlacklisted) {
> > +        CGLDestroyContext(_context->platformGraphicsContext3D());
> > +        _context->forceContextLost();
> > +    }
> 
> Similarly here.
> 
> One concern I have is that we're only doing this test on display. Which
> means you could hose the system if you executed a lot of draw calls before
> allowing the system to draw. We might want to have this code in the drawing
> entry points, drawElements and drawArrays, but only test every X iterations
> or something.
How does 5 iterations sound?

> 
> Anyway, if this makes an improvement on our hanging examples, let's stick
> with it.

Hmm if I do stick this stuff in drawArrays I feel like I won't need to have it in the display method, since the draw methods will always come first. Does that sound right?

Although, I suppose, it could help though if there is only 1 draw call, we'll probably want to catch the error before having to attempt to draw more frames. (Thus in the display step), so maybe ew want both.

[Roger Fong]

Created attachment 246934 [details]
patch

[Roger Fong]

Created attachment 247207 [details]
patch

Checks GPU status in draw method calls as well.
Also does the appropriate gpu checking for iOS, which uses a slightly different API.

[Roger Fong]

Created attachment 247256 [details]
patch

[WebKit Commit Bot]

Attachment 247256 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:36:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:37:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:38:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Roger Fong]

Created attachment 247258 [details]
patch

[WebKit Commit Bot]

Attachment 247258 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:36:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:37:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:38:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Roger Fong]

Created attachment 247259 [details]
patch

[WebKit Commit Bot]

Attachment 247259 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:38:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:40:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Roger Fong]

Created attachment 247262 [details]
patch

[WebKit Commit Bot]

Attachment 247262 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:38:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:40:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 247262 [details]
patch

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

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1278
> +    // Call to make during draw calls to check on the GPU's status

Nit: missing .

Naming comments coming.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:68
> +const int checkGPUStatusDrawIterations = 5;

Maybe GPUStatusCheckThreshold?

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:69
> +int GraphicsContext3D::drawCounter = 0;

And checkCounter? I don't do names well, so whatever you think is best.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:370
> +void GraphicsContext3D::checkGPUStatus()

Suggest naming this checkGPUStatusIfNecessary or something.

> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:193
> +    GLint restartStatus = 0;
> +    EAGLContext* currentContext = _context->platformGraphicsContext3D();
> +    [currentContext getParameter:kEAGLCPGPURestartStatus to:&restartStatus];
> +    if (restartStatus == kEAGLCPGPURestartStatusCaused || restartStatus == kEAGLCPGPURestartStatusBlacklisted) {
> +        [EAGLContext setCurrentContext:0];
> +        [static_cast<EAGLContext*>(currentContext) release];
> +        _context->forceContextLost();
> +    }
>  #else
>      [super display];
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101000
> +    GLint restartStatus = 0;
> +    CGLGetParameter(_context->platformGraphicsContext3D(), kCGLCPGPURestartStatus, &restartStatus);
> +    if (restartStatus == kCGLCPGPURestartStatusCaused || restartStatus == kCGLCPGPURestartStatusBlacklisted) {
> +        CGLSetCurrentContext(0);
> +        CGLDestroyContext(_context->platformGraphicsContext3D());
> +        _context->forceContextLost();
> +    }
> +#endif

Do you think we need to test here? These calls should be fine. It's the actual draw calls that you tested above that would cause a restart. Unless you disagree, I say remove these ones.

> Source/WebCore/platform/spi/ios/OpenGLESSPI.h:2
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

2015

[Roger Fong]

http://trac.webkit.org/changeset/180609

[Roger Fong]

(In reply to comment #18)
> http://trac.webkit.org/changeset/180609

With Dean's fixed applied.

[Ryosuke Niwa]

Landed a Mavericks build fix attempt: https://trac.webkit.org/changeset/180614

[Roger Fong]

(In reply to comment #20)
> Landed a Mavericks build fix attempt:
> https://trac.webkit.org/changeset/180614

Thanks rniwa!