Bug 166968 - [Cairo] Limit the number of active contexts in GraphicsContext3DCairo
Summary: [Cairo] Limit the number of active contexts in GraphicsContext3DCairo
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
Depends on: 174860
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-12 05:22 PST by Zan Dobersek
Modified: 2017-11-28 23:08 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.52 KB, patch)
2017-01-12 06:06 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (7.62 KB, patch)
2017-11-27 10:43 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (7.64 KB, patch)
2017-11-27 10:44 PST, Zan Dobersek
achristensen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-01-12 05:22:24 PST
[Cairo] Limit the number of active contexts in GraphicsContext3DCairo
Comment 1 Zan Dobersek 2017-01-12 06:06:46 PST
Created attachment 298674 [details]
Patch
Comment 2 Alex Christensen 2017-01-12 09:56:49 PST
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.
Comment 3 Zan Dobersek 2017-01-12 10:21:59 PST
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?
Comment 4 Alex Christensen 2017-01-12 13:34:57 PST
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?
Comment 5 Zan Dobersek 2017-01-13 07:15:26 PST
(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).
Comment 6 Zan Dobersek 2017-11-27 10:43:10 PST
Created attachment 327646 [details]
Patch
Comment 7 Zan Dobersek 2017-11-27 10:44:07 PST
Created attachment 327647 [details]
Patch
Comment 8 EWS Watchlist 2017-11-27 10:46:59 PST
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.
Comment 9 Zan Dobersek 2017-11-28 23:07:38 PST
Committed r225260: <https://trac.webkit.org/changeset/225260>
Comment 10 Radar WebKit Bug Importer 2017-11-28 23:08:30 PST
<rdar://problem/35742143>