[WebGL] Abstract attaching/detaching external images #14006
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
EWS run on previous version of this PR (hash 60eb32a) |
webkit-ews-buildbot
added
the
merging-blocked
djg
force-pushed
the
webgl/gpup-attaching-external-images
branch
from
60eb32a
to
8b4522c
Compare
|
EWS run on previous version of this PR (hash 8b4522c)
|
| #if OS(WINDOWS) | ||
| // Defined in winerror.h | ||
| #ifdef NO_ERROR | ||
| #undef NO_ERROR | ||
| #endif | ||
| #endif | ||
|
|
||
|
|
||
| enum class GCGLExternalImage : uint8_t; |
There was a problem hiding this comment.
Problems:
- The name
GCGLExternalImagewould give impression it's the object, when it's an enumeration for the type - ifdefing functions, manual
- move away from using GCGLints and such when normal WebCore types exists (IntSize)
- That JSIPCBinding seems like something is fishy. We are already transfering pointers as values, in GCGLsync.
- ExternalImageAttachment should not be optional, rather the return value should be optional<..>.
Instead, :
- other platforms can easily opt in once they need
- generator generates the code
- use webcore types where applicable
- etc
#ifdef PLATFORM(COCOA)
struct ExternalImageSourceSharedTextureIOSurfaceHandle {
MachSendRight handle;
};
struct ExternalImageSourceSharedTextureHandle {
MachSendRight handle;
};
using ExternalImageSource = std::variant<ExternalImageSourceSharedTextureIOSurfaceHandle, ExternalImageSourceSharedTextureHandle >;
#else
using ExternalImageSource = int;
#endif
...
using ExternalImageAttachResult = std::tuple<GCGLImage, IntSize>;
virtual std::optional<ExternalImageAttachmentResult> attachExternalImage(GCGLenum, ExternalImageSource) = 0;
virtual destroyExternalImage(GCGLImage) = 0;
// ========== Other functions.
#if PLATFORM(COCOA)
// Short-term support for in-process WebGL.
virtual std::optional<ExternalImageAttachmentResult> attachExternalImage(GCGLenum, IOSurface&) = 0;
#endif
Alternatively call the function "create and bind external image".
Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp
Outdated
Show resolved
Hide resolved
| @@ -665,27 +665,25 @@ static bool needsEAGLOnMac() | |||
| } | |||
|
|
|||
| #if !PLATFORM(IOS_FAMILY_SIMULATOR) | |||
| GraphicsContextGLCocoa::IOSurfaceTextureAttachment GraphicsContextGLCocoa::attachIOSurfaceToSharedTexture(GCGLenum target, IOSurface* surface) | |||
| static GraphicsContextGL::ExternalImageAttachment attachMTLTextureToExternalImage(GCGLDisplay platformDisplay, GCGLenum target, MTLSharedTextureHandle* handle) | |||
There was a problem hiding this comment.
This could be still called something about "shared texture" instead of "mtltexture", as it's using the shared texture instead of normal mtl texture.
| case GCGLExternalImage::IOSurface: { | ||
| auto surface = IOSurface::createFromSendRight(WTFMove(imageSendRight)); | ||
| if (!surface) { | ||
| LOG(WebGL, "Unable to create an IOSurface from imagePort in attachExternalImage."); |
There was a problem hiding this comment.
maybe we could remove the excessive logging. The log messages are already not consistent with the code.
There was a problem hiding this comment.
I was copying the existing style; when in doubt make it look like what was there.
I agree that the logging is inconsistent with the rest of the code.
There was a problem hiding this comment.
With inconsistency, I meant that it refers to, for example, "imagePort" which doesn't exist. Feel free to choose, match the existing logging or not.
| @@ -254,7 +254,7 @@ void WebXROpaqueFramebuffer::endFrame() | |||
| auto gCGL = static_cast<GraphicsContextGLCocoa*>(&gl); | |||
| if (m_ioSurfaceTextureHandleIsShared) { | |||
| #if !PLATFORM(IOS_FAMILY_SIMULATOR) | |||
| gCGL->detachIOSurfaceFromSharedTexture(m_ioSurfaceTextureHandle); | |||
| gCGL->detachExternalImage(m_ioSurfaceTextureHandle); | |||
There was a problem hiding this comment.
I believe if you're now abstracting the call, you want to invoke it through gl and not through gCGL.
| @@ -137,7 +137,7 @@ void WebXROpaqueFramebuffer::startFrame(const PlatformXR::Device::FrameData::Lay | |||
| // Tell the GraphicsContextGL to use the IOSurface as the backing store for m_opaqueTexture. | |||
| if (data.isShared) { | |||
| #if !PLATFORM(IOS_FAMILY_SIMULATOR) | |||
| auto surfaceTextureAttachment = gCGL->attachIOSurfaceToSharedTexture(textureTarget, data.surface.get()); | |||
| auto surfaceTextureAttachment = gCGL->attachIOSurfaceToExternalImage(textureTarget, data.surface.get()); | |||
There was a problem hiding this comment.
I believe if you're now abstracting the call, you want to invoke it through gl and not through gCGL.
So you would have:
gl.attachToExternalImage(textureTarget, *data.surface);
The in-process variant of GraphicsContextGL::attachToExternalImage(..., WebCore::IOSurface&) needs to be implemented twice:
- once for current code-path in GraphicsContextGLCocoa
- once for RemoteGraphicsContextGLProxyCocoa, obtaining the mach send right from the iosurface.
There was a problem hiding this comment.
But we can't create an in process IOSurface when RemoteGraphicsContextGLProxyCocoa is in use, can we?
There was a problem hiding this comment.
But we can't create an in process IOSurface when RemoteGraphicsContextGLProxyCocoa is in use, can we?
Yeah, we can. So "WebGL GPUP Yes/No" is distinct choice to "IOKit blocking".
Note: the question is not logical, there is need in creating in-process IOSurface for RemoteGraphicsContextGLProxyCocoa.
The client gives use either send right to IOSurface or the IOSurface instance.
The cases with RemoteGraphicsContextGLProxyCocoa are:
- If we already have in process IOSurface, we can give that to RemoteGraphicsContextGLProxyCocoa via obtaining send right to it.
- If we already have Mach send right to a IOSurface, we can give that to RemoteGraphicsContextGLProxyCocoa.
djg
force-pushed
the
webgl/gpup-attaching-external-images
branch
from
8b4522c
to
6e04f81
Compare
|
EWS run on previous version of this PR (hash 6e04f81) |
webkit-ews-buildbot
added
the
merging-blocked
kkinnunen-apple
approved these changes
May 29, 2023
|
|
||
| // Tell the GraphicsContextGL to use the IOSurface as the backing store for m_opaqueTexture. | ||
| if (data.isShared) { | ||
| #if !PLATFORM(IOS_FAMILY_SIMULATOR) | ||
| auto surfaceTextureAttachment = gCGL->attachIOSurfaceToSharedTexture(textureTarget, data.surface.get()); | ||
| auto surfaceTextureAttachment = gCGL->createAndBindExternalImage(textureTarget, data.surface.get()); |
There was a problem hiding this comment.
here we might still use the abstracted interface via gl. unless osmething else prevents that.
| MTLSharedTextureHandle* handle = nil; | ||
| return WTF::switchOn(source, | ||
| [&](const ExternalImageSourceIOSurfaceHandle& ioSurface) -> std::optional<ExternalImageAttachResult> { | ||
| auto surface = IOSurface::createFromSendRight(WTFMove(ioSurface.handle)); |
There was a problem hiding this comment.
this looks somehow odd, e.g. you move internals of the const ref object into the IOSurface::createFromSendRight.
This works because of bug in IOSurface::createFromSendRight type signature.
So you should have form of:
return WTF::switchOn(WTFMove(source),
[&](ExternalImageSourceIOSurfaceHandle&& ioSurface) -> std::optional<ExternalImageAttachResult> {
....
},
[&](ExternalImageSourceMTLSharedTextureHandle&& sharedTexture) -> std::optional<ExternalImageAttachResult> {
....
}
| @@ -1681,6 +1708,38 @@ IMPLEMENT_GCGL_OWNED(Texture) | |||
|
|
|||
| #undef IMPLEMENT_GCGL_OWNED | |||
|
|
|||
| #if PLATFORM(COCOA) | |||
There was a problem hiding this comment.
If you want to style this, I think this can be declared in the Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in
#if ENABLE(GPU_PROCESS) && ENABLE(WEBGL)
....
header: <WebCore/GraphicsContextGL.h>
#if PLATFORM(COCOA)
struct WebCore::ExternalImageSourceIOSurfaceHandle {
MachSendRight handle;
};
struct WebCore::ExternalImageSourceMTLSharedTextureHandle {
MachSendRight handle;
};
#endif
6e04f81
to
862ff1b
Compare
|
EWS run on previous version of this PR (hash 862ff1b)
|
djg
force-pushed
the
webgl/gpup-attaching-external-images
branch
from
862ff1b
to
388b6a9
Compare
|
EWS run on previous version of this PR (hash 388b6a9)
|
djg
force-pushed
the
webgl/gpup-attaching-external-images
branch
from
388b6a9
to
daa073c
Compare
|
EWS run on previous version of this PR (hash daa073c)
|
webkit-ews-buildbot
added
the
merging-blocked
djg
force-pushed
the
webgl/gpup-attaching-external-images
branch
from
daa073c
to
6ee2844
Compare
|
EWS run on previous version of this PR (hash 6ee2844)
|
djg
force-pushed
the
webgl/gpup-attaching-external-images
branch
from
6ee2844
to
f554ae9
Compare
|
EWS run on current version of this PR (hash f554ae9)
|
djg
added
merge-queue
https://bugs.webkit.org/show_bug.cgi?id=256945 rdar://109495127 Reviewed by Kimmo Kinnunen. When the GPU process is enabled, the Web process no long has access to the APIs required to create images passed from the UI process. This change added APIs to allow attaching and detaching external images that can be represented by a MachSendRight (either IOSurface or MTLSharedTexture), that can be used in the Web or GPU process. Support work for WebXR in GPUP. * Source/WebCore/Modules/webxr/WebXROpaqueFramebuffer.cpp: (WebCore::WebXROpaqueFramebuffer::startFrame): (WebCore::WebXROpaqueFramebuffer::endFrame): * Source/WebCore/PAL/pal/spi/cocoa/MetalSPI.h: * Source/WebCore/platform/graphics/GraphicsContextGL.h: * Source/WebCore/platform/graphics/GraphicsTypesGL.h: * Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp: (WebCore::GraphicsContextGLANGLE::createAndBindExternalImage): (WebCore::GraphicsContextGLANGLE::destroyEGLImage): * Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.h: * Source/WebCore/platform/graphics/cocoa/ANGLEUtilitiesCocoa.mm: (WebCore::newSharedEventWithMachPort): (WebCore::newSharedEvent): * Source/WebCore/platform/graphics/cocoa/GraphicsContextGLCocoa.h: * Source/WebCore/platform/graphics/cocoa/GraphicsContextGLCocoa.mm: (WebCore::createAndBindSharedTextureToExternalImage): (WebCore::GraphicsContextGLCocoa::createAndBindExternalImage): (WebCore::GraphicsContextGLCocoa::attachIOSurfaceToSharedTexture): Deleted. (WebCore::GraphicsContextGLCocoa::detachIOSurfaceFromSharedTexture): Deleted. * Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.cpp: (WebKit::RemoteGraphicsContextGL::createAndBindExternalImage): * Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.h: * Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.messages.in: * Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGLFunctionsGenerated.h: (destroyEGLImage): * Source/WebKit/Scripts/webkit/messages.py: (types_that_cannot_be_forward_declared): (headers_for_type): * Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm: (IPC::ArgumentCoder<WebCore::GraphicsContextGL::ExternalImageSourceIOSurfaceHandle>::encode): (IPC::ArgumentCoder<WebCore::GraphicsContextGL::ExternalImageSourceIOSurfaceHandle>::decode): (IPC::ArgumentCoder<WebCore::GraphicsContextGL::ExternalImageSourceMTLSharedTextureHandle>::encode): (IPC::ArgumentCoder<WebCore::GraphicsContextGL::ExternalImageSourceMTLSharedTextureHandle>::decode): * Source/WebKit/Shared/WebCoreArgumentCoders.h: * Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxy.cpp: (WebKit::RemoteGraphicsContextGLProxy::createAndBindExternalImage): * Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxy.h: * Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxyFunctionsGenerated.cpp: (WebKit::RemoteGraphicsContextGLProxy::destroyEGLImage): * Tools/Scripts/generate-gpup-webgl: Canonical link: https://commits.webkit.org/264712@main
webkit-commit-queue
force-pushed
the
webgl/gpup-attaching-external-images
branch
from
f554ae9
to
891ad78
Compare
|
Committed 264712@main (891ad78): https://commits.webkit.org/264712@main Reviewed commits have been landed. Closing PR #14006 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
891ad78
f554ae9
🧪 style🛠 ios🛠 mac🛠 wpe🧪 bindings🛠 ios-sim🧪 wpe-wk2🧪 webkitperl🧪 ios-wk2🧪 api-mac🛠 gtk🧪 webkitpy🧪 ios-wk2-wpt🧪 mac-wk1🧪 gtk-wk2🧪 api-ios🧪 mac-wk2🧪 api-gtk🧪 mac-AS-debug-wk2🧪 mac-wk2-stress🛠 watch-sim