[WebVR][OpenVR] Implement requestPresent()/exitPresent() and getLayers()
Created attachment 337784 [details] Patch
Created attachment 337787 [details] Patch rebased patch
Comment on attachment 337787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337787&action=review > Source/WebCore/Modules/webvr/VRDisplay.cpp:143 > + Ref<DeferredPromise> promise = WTFMove(deferredPromise); Is this necessary? It usually isn't. > Source/WebCore/Modules/webvr/VRDisplay.cpp:161 > + RELEASE_ASSERT(layers.size() == 1); > + auto layer = layers[0]; This should handle more than one layer at some point, right? But it's currently limited to the maxLayers() value in VRDisplayCapabilities? > Source/WebCore/Modules/webvr/VRDisplay.cpp:168 > + if (!layer.source->getContext("webgl")->isWebGL()) { This should handle HTMLCanvas::getContext() returning nullptr. > Source/WebCore/Modules/webvr/VRDisplay.h:123 > + bool m_isPresenting; > + std::optional<VRLayerInit> m_presentingLayer; Can m_isPresenting be deduced from m_presentingLayer being non-nullopt wherever it's used.
Comment on attachment 337787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337787&action=review >> Source/WebCore/Modules/webvr/VRDisplay.h:123 >> + std::optional<VRLayerInit> m_presentingLayer; > > Can m_isPresenting be deduced from m_presentingLayer being non-nullopt wherever it's used. i.e, an m_isPresenting be eliminated?
Comment on attachment 337787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337787&action=review >> Source/WebCore/Modules/webvr/VRDisplay.cpp:143 >> + Ref<DeferredPromise> promise = WTFMove(deferredPromise); > > Is this necessary? It usually isn't. No, not really >> Source/WebCore/Modules/webvr/VRDisplay.cpp:161 >> + auto layer = layers[0]; > > This should handle more than one layer at some point, right? But it's currently limited to the maxLayers() value in VRDisplayCapabilities? Not in WebVR 1.1. As you can see in the spec (https://immersive-web.github.io/webvr/spec/1.1/#vrdisplaycapabilities-attributes) "maxLayers Indicates the maximum length of the array that requestPresent() will accept. MUST be 1 if canPresent is true, 0 otherwise." In the next specification, WebXR, maxLayers() does not exist for example. >> Source/WebCore/Modules/webvr/VRDisplay.cpp:168 >> + if (!layer.source->getContext("webgl")->isWebGL()) { > > This should handle HTMLCanvas::getContext() returning nullptr. Right. >>> Source/WebCore/Modules/webvr/VRDisplay.h:123 >>> + std::optional<VRLayerInit> m_presentingLayer; >> >> Can m_isPresenting be deduced from m_presentingLayer being non-nullopt wherever it's used. > > i.e, an m_isPresenting be eliminated? So far yes. I was just not sure and decided to play safe, but I can remove it now and add it later if needed.
Comment on attachment 337787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337787&action=review >>>> Source/WebCore/Modules/webvr/VRDisplay.h:123 >>>> + std::optional<VRLayerInit> m_presentingLayer; >>> >>> Can m_isPresenting be deduced from m_presentingLayer being non-nullopt wherever it's used. >> >> i.e, an m_isPresenting be eliminated? > > So far yes. I was just not sure and decided to play safe, but I can remove it now and add it later if needed. Let's keep this extra boolean for now then, and remove it and simplify things around it when it's determined to not be required.
Created attachment 337881 [details] Patch
Comment on attachment 337881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337881&action=review > Source/WebCore/Modules/webvr/VRDisplay.cpp:138 > +void VRDisplay::rejectRequestAndStopPresenting(Ref<DeferredPromise>& promise, ExceptionCode exceptionCode, const String& message) > +{ > + promise->reject(Exception { exceptionCode, message }); > + if (m_presentingLayer) > + stopPresenting(); > +} Given this method is specific to requestPresent(), it might fit better as a local lambda. But it's not super-important. > Source/WebCore/Modules/webvr/VRDisplay.cpp:184 > +void VRDisplay::stopPresenting() > { > + m_presentingLayer = std::nullopt; > } Given this is now a one-liner, m_presentingLayer could just be nulled out eveywhere instead of having to call this method.
Comment on attachment 337881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337881&action=review > Source/WebCore/Modules/webvr/VRDisplay.cpp:141 > +void VRDisplay::requestPresent(const Vector<VRLayerInit>& layers, Ref<DeferredPromise>&& promise) > +{ OTOH, this method is guaranteed to either null out the m_presentingLayer value or replace it with the passed-in object. If you null out m_presentingLayer at the beginning, you can replace rejectRequestAndStopPresenting() with direct `promise->reject()` calls and stop calling `stopPresenting()`.
Comment on attachment 337881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337881&action=review >> Source/WebCore/Modules/webvr/VRDisplay.cpp:138 >> +} > > Given this method is specific to requestPresent(), it might fit better as a local lambda. But it's not super-important. Makes sense indeed. >> Source/WebCore/Modules/webvr/VRDisplay.cpp:141 >> +{ > > OTOH, this method is guaranteed to either null out the m_presentingLayer value or replace it with the passed-in object. If you null out m_presentingLayer at the beginning, you can replace rejectRequestAndStopPresenting() with direct `promise->reject()` calls and stop calling `stopPresenting()`. With the current code that's true, but when we add the required code to effectively render the layer content in the HMD, stopPresenting() would likely have to do more things. Now that I think about it, the specs mandate us to reject the promise in all the cases listed bellow but they don't specify that the presentation should be stopped, I'll change that. >> Source/WebCore/Modules/webvr/VRDisplay.cpp:184 >> } > > Given this is now a one-liner, m_presentingLayer could just be nulled out eveywhere instead of having to call this method. I know but I we'd have to do more things whenever we integrate the requestPresent with the underlying SDK, i.e., when we effectively start using the passed in layer to render stereo content in the HMD
(In reply to Sergio Villar Senin from comment #10) > Now that I think about it, the specs mandate us to reject the promise in all > the cases listed bellow but they don't specify that the presentation should > be stopped, I'll change that. Nah, the code is correct. Literally from specs "If a call to requestPresent() is rejected while the VRDisplay is already presenting, the VRDisplay MUST end presentation."
Created attachment 337885 [details] Patch Patch for landing
Committed r230630: <https://trac.webkit.org/changeset/230630>
<rdar://problem/39412653>