Bug 166968 – [Cairo] Limit the number of active contexts in GraphicsContext3DCairo

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[Cairo] Limit the number of active contexts in GraphicsContext3DCairo

[Zan Dobersek]

Created attachment 298674 [details]
Patch

[Alex Christensen]

Comment on attachment 298674 [details]
Patch

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

We should add a test for this.  Make 17 canvases and make sure the first one was recycled.
My review comments also apply to things that should be changed in GraphicsContext3DMac

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:63
> +    static NeverDestroyed<Vector<GraphicsContext3D*>> s_activeContexts;

The way you are using this would make a Deque more efficient than a Vector.

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:91
> +    // Calling recycleContext() above should have lead to the graphics context being
> +    // destroyed and thus removed from the active contexts list.
> +    if (contexts.size() >= MaxActiveContexts)
> +        return nullptr;

If this is true, this should just be an assertion instead of a code path that can never be hit.

[Zan Dobersek]

Comment on attachment 298674 [details]
Patch

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

>> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:91
>> +        return nullptr;
> 
> If this is true, this should just be an assertion instead of a code path that can never be hit.

Rethinking this, the Linux ports are using GraphicsContext3D liberally in GStreamer and TextureMapper code. So the recycling wouldn't work in those cases. This could be avoided if in those cases the GraphicsContext3D objects are created with some specific attribute set.

Otherwise, the GraphicsContext3D objects are only created in the WebGLRenderingContextBase implementation. In the constructor of that class, the passed-in GraphicsContext3D object is associated with the WebGLRenderingContextBase via the setWebGLContext() call. But a GraphicsContext3D object is also created in WebGLRenderingContextBase::maybeRestoreContext(), but there's no association via setWebGLContext() there -- I assume that's a bug?

[Alex Christensen]

Somewhat unrelated, but why is GraphicsContext3DPrivate a PlatformLayer when using Texture Mapper?  This is causing conceptual problems in my Mac EGL/GLESv2 WebGL implementation where a PlatformLayer is a CALayer.  Could this be reorganized?

[Zan Dobersek]

(In reply to comment #4)
> Somewhat unrelated, but why is GraphicsContext3DPrivate a PlatformLayer when
> using Texture Mapper?  This is causing conceptual problems in my Mac
> EGL/GLESv2 WebGL implementation where a PlatformLayer is a CALayer.  Could
> this be reorganized?

It can probably be moved into an equivalent of m_webGLLayer that's used on PLATFORM(COCOA) and PLATFORM(WIN).

[Zan Dobersek]

Created attachment 327646 [details]
Patch

[Zan Dobersek]

Created attachment 327647 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:274:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 3 files


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

[Zan Dobersek]

Committed r225260: <https://trac.webkit.org/changeset/225260>

[Radar WebKit Bug Importer]

<rdar://problem/35742143>