Bug 184530 – [WebVR][OpenVR] Implement requestPresent()/exitPresent() and getLayers()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[WebVR][OpenVR] Implement requestPresent()/exitPresent() and getLayers()

[Sergio Villar Senin]

Created attachment 337784 [details]
Patch

[Sergio Villar Senin]

Created attachment 337787 [details]
Patch

rebased patch

[Zan Dobersek]

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.

[Zan Dobersek]

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?

[Sergio Villar Senin]

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.

[Zan Dobersek]

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.

[Sergio Villar Senin]

Created attachment 337881 [details]
Patch

[Zan Dobersek]

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.

[Zan Dobersek]

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()`.

[Sergio Villar Senin]

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

[Sergio Villar Senin]

(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."

[Sergio Villar Senin]

Created attachment 337885 [details]
Patch

Patch for landing

[Sergio Villar Senin]

Committed r230630: <https://trac.webkit.org/changeset/230630>

[Radar WebKit Bug Importer]

<rdar://problem/39412653>