Create the SharedGraphicsContext3D through its own method.
Created attachment 187408 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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.
Created attachment 187413 [details] Patch Fix typo
Created attachment 187414 [details] Patch Fix QT build
Comment on attachment 187414 [details] Patch I don't understand the compile guards - do you need those?
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
Created attachment 187416 [details] Patch
(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.
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
Created attachment 187419 [details] Patch With an if
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
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
Created attachment 188014 [details] Patch
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
Comment on attachment 188014 [details] Patch Attachment 188014 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16454688
Comment on attachment 188014 [details] Patch Attachment 188014 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16521606
Created attachment 188017 [details] Patch
New patch to not try take ownership of the WGC3D or GrContext provided by the embedder.
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?
(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.
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
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
Created attachment 188125 [details] Patch Do pushGroupMarkerEXT("SharedGraphicsContext") on the context from the embedder
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
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.
(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?
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
Still need this?
Comment on attachment 188125 [details] Patch Yes, but it needs a refresh.
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).
Created attachment 190131 [details] Patch wasCreated for the WGC3D should cause a new m_context to be made
(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?
(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.
(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?
(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.
(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.
(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.
(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.
Created attachment 190289 [details] Patch Dropped the wasCreated flag
Comment on attachment 190289 [details] Patch Clearing flags on attachment: 190289 Committed r144072: <http://trac.webkit.org/changeset/144072>
All reviewed patches have been landed. Closing bug.