Create the SharedGraphicsContext3D through its own method.

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

Create the SharedGraphicsContext3D through its own method.

[Dana Jansens]

Created attachment 187408 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Dana Jansens]

This won't pass tests until the TestWebKitPlatformSupport implements the method to provide a shared context. It'll crash in any test that uses the shared context until then. I'll intend to land that piece first.

This adds an API to GraphicsContext3D to get the shared context in an explicit way, so that we can get that context from the embedder along a different code path.

On Chromium, we could get rid of the SharedContext3D class other than a call to the Platform API, but that wouldn't work for other platforms, so the class is left alone except to call through GraphicsContext3D::createSharedContext instead of GraphicsContext3D::create.

[Dana Jansens]

Created attachment 187413 [details]
Patch

Fix typo

[Dana Jansens]

Created attachment 187414 [details]
Patch

Fix QT build

[James Robinson]

Comment on attachment 187414 [details]
Patch

I don't understand the compile guards - do you need those?

[James Robinson]

Comment on attachment 187414 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext3D.h:491
> +    static PassRefPtr<GraphicsContext3D> createSharedContext();

I think it would be better to provide chromium-specific implementations of SharedGraphicsContext3D functions and not modify the GraphicsContext3D interface itself

[Dana Jansens]

Created attachment 187416 [details]
Patch

[Dana Jansens]

(In reply to comment #7)
> I think it would be better to provide chromium-specific implementations of SharedGraphicsContext3D functions and not modify the GraphicsContext3D interface itself

Done.

[James Robinson]

Comment on attachment 187416 [details]
Patch

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

R=me but needs an if

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:75
> +        m_context = GraphicsContext3DPrivate::createGraphicsContextFromWebContext(webContext.release(), false);

i don't think this call is safe to do if webContext is null

[Dana Jansens]

Created attachment 187419 [details]
Patch

With an if

[Build Bot]

Comment on attachment 187419 [details]
Patch

Attachment 187419 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16475530

New failing tests:
compositing/checkerboard.html
accessibility/anchor-linked-anonymous-block-crash.html
http/tests/cache/cancel-multiple-post-xhrs.html
animations/3d/state-at-end-event-transform.html
animations/animation-add-events-in-handler.html
animations/3d/replace-filling-transform.html
http/tests/cache/history-only-cached-subresource-loads.html
compositing/bounds-in-flipped-writing-mode.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
accessibility/accessibility-object-detached.html
accessibility/anonymous-render-block-in-continuation-causes-crash.html
animations/animation-controller-drt-api.html
compositing/absolute-position-changed-with-composited-parent-layer.html
compositing/absolute-position-changed-in-composited-layer.html
http/tests/cache/iframe-304-crash.html
animations/3d/transform-perspective.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
animations/3d/transform-origin-vs-functions.html
animations/animation-css-rule-types.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[WebKit Review Bot]

Comment on attachment 187419 [details]
Patch

Attachment 187419 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16478911

New failing tests:
css3/filters/effect-blur-hw.html
platform/chromium/virtual/gpu/fast/canvas/canvas-bg.html
platform/chromium/virtual/gpu/fast/canvas/canvas-as-image.html
compositing/culling/filter-occlusion-alpha.html
css3/filters/effect-invert-hw.html
platform/chromium/virtual/gpu/fast/canvas/canvas-as-image-incremental-repaint.html
inspector/audits/audits-panel-functional.html
css3/filters/effect-reference-ordering-hw.html
css3/filters/filter-repaint-composited-fallback-crash.html
fast/loader/text-document-wrapping.html
compositing/layer-creation/spanOverlapsCanvas.html
compositing/culling/filter-occlusion-alpha-large.html
inspector-protocol/heap-profiler/heap-snapshot-with-event-listener.html
css3/filters/effect-sepia-hw.html
css3/filters/effect-opacity-hw.html
css3/filters/filter-change-repaint-composited.html
css3/filters/effect-hue-rotate-hw.html
css3/filters/effect-drop-shadow-hw.html
css3/filters/filtered-compositing-descendant.html
compositing/culling/filter-occlusion-blur.html
platform/chromium/virtual/gpu/fast/canvas/canvas-bg-zoom.html
css3/filters/filter-repaint-composited-fallback.html
css3/filters/effect-grayscale-hw.html
css3/filters/effect-reference-hw.html
css3/filters/filter-change-repaint.html
compositing/culling/filter-occlusion-blur-large.html
fast/loader/javascript-url-in-object.html
css3/filters/effect-contrast-hw.html
css3/filters/effect-saturate-hw.html
css3/filters/composited-reflected.html

[Dana Jansens]

Created attachment 188014 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188014 [details]
Patch

Attachment 188014 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16517579

[Peter Beverloo (cr-android ews)]

Comment on attachment 188014 [details]
Patch

Attachment 188014 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16454688

[WebKit Review Bot]

Comment on attachment 188014 [details]
Patch

Attachment 188014 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16521606

[Dana Jansens]

Created attachment 188017 [details]
Patch

[Dana Jansens]

New patch to not try take ownership of the WGC3D or GrContext provided by the embedder.

[James Robinson]

Comment on attachment 188017 [details]
Patch

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

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:75
> +        GrContext* grContext = WebKit::Platform::current()->sharedOffscreenGrContext();

to stage this, perhaps this should fall back on the current path if the embedder fails to provide a context?  or will you just make sure to land an implementation of this function in chromium before landing this?

[Dana Jansens]

(In reply to comment #20)
> (From update of attachment 188017 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188017&action=review
> 
> > Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:75
> > +        GrContext* grContext = WebKit::Platform::current()->sharedOffscreenGrContext();
> 
> to stage this, perhaps this should fall back on the current path if the embedder fails to provide a context?  or will you just make sure to land an implementation of this function in chromium before landing this?

I'm intending to land the chromium implementation of the path first. We'll end up in a temporary state where we can have a context3d for the compositor owned by chromium, and for canvas owned by webkit. It's still correct, just potentially wasteful, until this lands and then the canvas one will be owned by chromium as well.

[Build Bot]

Comment on attachment 188017 [details]
Patch

Attachment 188017 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16543026

New failing tests:
platform/mac/fast/forms/attributed-strings.html

[Dana Jansens]

Created attachment 188124 [details]
Patch

Moved to getOrCreate so this is used for the main thread context, and don't hold onto the GC3D if the WGC3D from the embedder changed

[Dana Jansens]

Created attachment 188125 [details]
Patch

Do pushGroupMarkerEXT("SharedGraphicsContext") on the context from the embedder

[James Robinson]

Comment on attachment 188125 [details]
Patch

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

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:51
> +        if (m_context && GraphicsContext3DPrivate::extractWebGraphicsContext3D(m_context.get()) == webContext)
> +            return m_context;

where's the check for "is this context lost" ? is that on the implementation side?

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:58
> +        if (m_context)
> +            m_context->getExtensions()->pushGroupMarkerEXT("SharedGraphicsContext");

you have to be careful only to add this marker on the first creation for a new context, not every instantiation or it'll be a big memory leak

[Dana Jansens]

Comment on attachment 188125 [details]
Patch

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

>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:51
>> +            return m_context;
> 
> where's the check for "is this context lost" ? is that on the implementation side?

The other side does check for loss. What I'm not sure about yet is how this side is supposed to hear about the loss before the WGC3D is deleted, so that it doesn't try to use a deleted WGC3D*.

Currently, since WebKit owns the WGC3D, it doesn't have to worry about this. How do you think I should have the embedder notify WebKit that the WGC3D it's using no longer exists? Should I instead have the shared GC3D query the playform for the WGC3D* on every operation instead of holding onto the pointer internally?

>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:58
>> +            m_context->getExtensions()->pushGroupMarkerEXT("SharedGraphicsContext");
> 
> you have to be careful only to add this marker on the first creation for a new context, not every instantiation or it'll be a big memory leak

Oh okay, thanks.

[Dana Jansens]

(In reply to comment #26)
> (From update of attachment 188125 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188125&action=review
> 
> >> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:51
> >> +            return m_context;
> > 
> > where's the check for "is this context lost" ? is that on the implementation side?
> 
> The other side does check for loss. What I'm not sure about yet is how this side is supposed to hear about the loss before the WGC3D is deleted, so that it doesn't try to use a deleted WGC3D*.
> 
> Currently, since WebKit owns the WGC3D, it doesn't have to worry about this. How do you think I should have the embedder notify WebKit that the WGC3D it's using no longer exists? Should I instead have the shared GC3D query the playform for the WGC3D* on every operation instead of holding onto the pointer internally?

FWIW I'm solving this in the compositor by refcounting the thing that the embedder gives, and I null out the stuff inside that struct. I don't think I can do that for the Platform API tho, can I?

[WebKit Review Bot]

Comment on attachment 188125 [details]
Patch

Attachment 188125 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16617198

New failing tests:
css3/filters/effect-blur-hw.html
fast/forms/datalist/update-range-with-datalist.html
compositing/culling/filter-occlusion-alpha.html
css3/filters/effect-invert-hw.html
css3/filters/effect-reference-ordering-hw.html
css3/filters/filter-repaint-composited-fallback-crash.html
fast/loader/text-document-wrapping.html
fast/layers/no-clipping-overflow-hidden-added-after-transition.html
compositing/layer-creation/spanOverlapsCanvas.html
compositing/culling/filter-occlusion-alpha-large.html
css3/filters/effect-brightness-hw.html
css3/filters/effect-sepia-hw.html
css3/filters/effect-opacity-hw.html
css3/filters/effect-combined-hw.html
css3/filters/filter-change-repaint-composited.html
css3/filters/effect-hue-rotate-hw.html
css3/filters/effect-drop-shadow-hw.html
css3/filters/filtered-compositing-descendant.html
compositing/culling/filter-occlusion-blur.html
css3/filters/effect-brightness-clamping-hw.html
css3/filters/filter-repaint-composited-fallback.html
css3/filters/effect-grayscale-hw.html
fast/layers/no-clipping-overflow-hidden-added-after-transform.html
css3/filters/filter-change-repaint.html
compositing/culling/filter-occlusion-blur-large.html
fast/loader/javascript-url-in-object.html
css3/filters/effect-contrast-hw.html
css3/filters/effect-saturate-hw.html
css3/filters/composited-reflected.html
fast/layers/no-clipping-overflow-hidden-hardware-acceleration.html

[James Robinson]

Still need this?

[Dana Jansens]

Comment on attachment 188125 [details]
Patch

Yes, but it needs a refresh.

[Dana Jansens]

Created attachment 190089 [details]
Patch

New patch with a fallback to the legacy code path, and added a parameter to the API to tell when a new context was created (to avoid pointer comparisons for this).

[Dana Jansens]

Created attachment 190131 [details]
Patch

wasCreated for the WGC3D should cause a new m_context to be made

[James Robinson]

(In reply to comment #31)
> Created an attachment (id=190089) [details]
> Patch
> 
> New patch with a fallback to the legacy code path, and added a parameter to the API to tell when a new context was created (to avoid pointer comparisons for this).

What's the problem with pointer comparisons this is intended to resolve?

[Dana Jansens]

(In reply to comment #33)
> (In reply to comment #31)
> > Created an attachment (id=190089) [details] [details]
> > Patch
> > 
> > New patch with a fallback to the legacy code path, and added a parameter to the API to tell when a new context was created (to avoid pointer comparisons for this).
> 
> What's the problem with pointer comparisons this is intended to resolve?

I'd say its a bad practice. You could delete and allocate and get the same pointer address. Or someone else could delete and create then you do the same and get back the old address. It just seems flaky and not robust.

[James Robinson]

(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #31)
> > > Created an attachment (id=190089) [details] [details] [details]
> > > Patch
> > > 
> > > New patch with a fallback to the legacy code path, and added a parameter to the API to tell when a new context was created (to avoid pointer comparisons for this).
> > 
> > What's the problem with pointer comparisons this is intended to resolve?
> 
> I'd say its a bad practice. You could delete and allocate and get the same pointer address. Or someone else could delete and create then you do the same and get back the old address. It just seems flaky and not robust.

Well, it matters who has ownership.  Who's responsible for deleting the WGC3D when it becomes lost?

[Dana Jansens]

(In reply to comment #35)
> (In reply to comment #34)
> > (In reply to comment #33)
> > > (In reply to comment #31)
> > > > Created an attachment (id=190089) [details] [details] [details] [details]
> > > > Patch
> > > > 
> > > > New patch with a fallback to the legacy code path, and added a parameter to the API to tell when a new context was created (to avoid pointer comparisons for this).
> > > 
> > > What's the problem with pointer comparisons this is intended to resolve?
> > 
> > I'd say its a bad practice. You could delete and allocate and get the same pointer address. Or someone else could delete and create then you do the same and get back the old address. It just seems flaky and not robust.
> 
> Well, it matters who has ownership.  Who's responsible for deleting the WGC3D when it becomes lost?

Ownership works like:

WebKitPlatformSupport on the chromium side owns a refptr to a ContextProvider.
ContextProvider owns the WGC3D.

When SharedContext3D asks for a WGC3D, the WebKitPlatformSupport may replace its ContextProvider with a new one if the old one held a lost context. Then the WGC3D is deleted immediately, or after the compositor also drops its ContextProvider if it has a reference. WebKitPlatformSupport returns the new WGC3D out of its ContextProvider.


WebKit side the assumption is that calling this function may cause the last thing it returned to be deleted. Thus only one person should ever be calling it.

[Dana Jansens]

(In reply to comment #36)
> (In reply to comment #35)
> > (In reply to comment #34)
> > > (In reply to comment #33)
> > > > (In reply to comment #31)
> > > > > Created an attachment (id=190089) [details] [details] [details] [details] [details]
> > > > > Patch
> > > > > 
> > > > > New patch with a fallback to the legacy code path, and added a parameter to the API to tell when a new context was created (to avoid pointer comparisons for this).
> > > > 
> > > > What's the problem with pointer comparisons this is intended to resolve?
> > > 
> > > I'd say its a bad practice. You could delete and allocate and get the same pointer address. Or someone else could delete and create then you do the same and get back the old address. It just seems flaky and not robust.
> > 
> > Well, it matters who has ownership.  Who's responsible for deleting the WGC3D when it becomes lost?
> 
> Ownership works like:
> 
> WebKitPlatformSupport on the chromium side owns a refptr to a ContextProvider.
> ContextProvider owns the WGC3D.
> 
> When SharedContext3D asks for a WGC3D, the WebKitPlatformSupport may replace its ContextProvider with a new one if the old one held a lost context. Then the WGC3D is deleted immediately, or after the compositor also drops its ContextProvider if it has a reference. WebKitPlatformSupport returns the new WGC3D out of its ContextProvider.
> 
> 
> WebKit side the assumption is that calling this function may cause the last thing it returned to be deleted. Thus only one person should ever be calling it.

https://codereview.chromium.org/12217099 is a reasonably simple example of this ownership I think. It implements the behaviour for DRT.

[James Robinson]

(In reply to comment #36)
> (In reply to comment #35)
> > (In reply to comment #34)
> > > (In reply to comment #33)
> > > > (In reply to comment #31)
> > > > > Created an attachment (id=190089) [details] [details] [details] [details] [details]
> > > > > Patch
> > > > > 
> > > > > New patch with a fallback to the legacy code path, and added a parameter to the API to tell when a new context was created (to avoid pointer comparisons for this).
> > > > 
> > > > What's the problem with pointer comparisons this is intended to resolve?
> > > 
> > > I'd say its a bad practice. You could delete and allocate and get the same pointer address. Or someone else could delete and create then you do the same and get back the old address. It just seems flaky and not robust.
> > 
> > Well, it matters who has ownership.  Who's responsible for deleting the WGC3D when it becomes lost?
> 
> Ownership works like:
> 
> WebKitPlatformSupport on the chromium side owns a refptr to a ContextProvider.
> ContextProvider owns the WGC3D.
> 
> When SharedContext3D asks for a WGC3D, the WebKitPlatformSupport may replace its ContextProvider with a new one if the old one held a lost context. Then the WGC3D is deleted immediately, or after the compositor also drops its ContextProvider if it has a reference. WebKitPlatformSupport returns the new WGC3D out of its ContextProvider.
> 
> 
> WebKit side the assumption is that calling this function may cause the last thing it returned to be deleted. Thus only one person should ever be calling it.

I see.  So it sounds like a precondition for calling this function is that the WGC3D is a valid not-deallocated object.  If that's the case then its address cannot collide with a newly allocated one, assuming the implementation decides to make a new one.  So I'm not sure I see the issue with pointer comparisons in this instance.

[Dana Jansens]

(In reply to comment #38)
> (In reply to comment #36)
> > (In reply to comment #35)
> > > (In reply to comment #34)
> > > > (In reply to comment #33)
> > > > > (In reply to comment #31)
> > > > > > Created an attachment (id=190089) [details] [details] [details] [details] [details] [details]
> > > > > > Patch
> > > > > > 
> > > > > > New patch with a fallback to the legacy code path, and added a parameter to the API to tell when a new context was created (to avoid pointer comparisons for this).
> > > > > 
> > > > > What's the problem with pointer comparisons this is intended to resolve?
> > > > 
> > > > I'd say its a bad practice. You could delete and allocate and get the same pointer address. Or someone else could delete and create then you do the same and get back the old address. It just seems flaky and not robust.
> > > 
> > > Well, it matters who has ownership.  Who's responsible for deleting the WGC3D when it becomes lost?
> > 
> > Ownership works like:
> > 
> > WebKitPlatformSupport on the chromium side owns a refptr to a ContextProvider.
> > ContextProvider owns the WGC3D.
> > 
> > When SharedContext3D asks for a WGC3D, the WebKitPlatformSupport may replace its ContextProvider with a new one if the old one held a lost context. Then the WGC3D is deleted immediately, or after the compositor also drops its ContextProvider if it has a reference. WebKitPlatformSupport returns the new WGC3D out of its ContextProvider.
> > 
> > 
> > WebKit side the assumption is that calling this function may cause the last thing it returned to be deleted. Thus only one person should ever be calling it.
> 
> I see.  So it sounds like a precondition for calling this function is that the WGC3D is a valid not-deallocated object.  If that's the case then its address cannot collide with a newly allocated one, assuming the implementation decides to make a new one.  So I'm not sure I see the issue with pointer comparisons in this instance.

That is true. I'm worried about multiple sites calling this function, but then I guess all bet are off and undefined behaviour is somewhat unavoidable. I'll drop the bool.

[Dana Jansens]

Created attachment 190289 [details]
Patch

Dropped the wasCreated flag

[WebKit Review Bot]

Comment on attachment 190289 [details]
Patch

Clearing flags on attachment: 190289

Committed r144072: <http://trac.webkit.org/changeset/144072>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.