Bug 184265 – [OpenVR][WebVR] Retrieve FrameData in WebVR's rAF

• Component: New Bugs
• Status: RESOLVED FIXED

[Sergio Villar Senin]

[OpenVR][WebVR] Retrieve FrameData in WebVR's rAF

[Sergio Villar Senin]

Created attachment 337078 [details]
Patch

[Sergio Villar Senin]

Created attachment 337261 [details]
Patch

Mac/iOS build fix

[Sergio Villar Senin]

Created attachment 337349 [details]
Patch

Build fix for WPE

[Sergio Villar Senin]

Created attachment 337350 [details]
Patch

Rebased patch

[Sergio Villar Senin]

Created attachment 337351 [details]
Patch

Another build fix. Seems that the way unified sources are built affect a lot the compilation results

[Sergio Villar Senin]

Created attachment 337352 [details]
Patch

And another build fix :(

[Sergio Villar Senin]

Created attachment 337353 [details]
Patch

Mac build fix

[Sergio Villar Senin]

Created attachment 337355 [details]
Patch

Hopefully the last Mac build fix

[Zan Dobersek]

Comment on attachment 337355 [details]
Patch

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

> Source/WebCore/Modules/webvr/VRDisplay.cpp:113
> +    auto* document = downcast<Document>(scriptExecutionContext());

This can be moved into the (!m_scriptedAnimationController) block.

> Source/WebCore/Modules/webvr/VRDisplay.cpp:118
> +        // FIXME: Get the display id of the HMD as it should use the HMD native refresh rate.
> +        PlatformDisplayID displayID = document->page() ? document->page()->chrome().displayID() : 0;
> +        m_scriptedAnimationController = ScriptedAnimationController::create(*document, displayID);

Is this necessary at this point, for animation frame functionality? I would just default to the 0 ID until it's possible to acquire the display ID from the HMD device.

> Source/WebCore/Modules/webvr/VRFrameData.cpp:91
> +    return projectionMatrix;

You could use the 16-argument TransformationMatrix constructor here, and pass all the values in the return statement.

> Source/WebCore/Modules/webvr/VRFrameData.cpp:124
> +    return matrix;

Same here. You could also alias normalizedQuaternion variable to something shorter, e.g. `nq`, and then compute all the values on the fly.

> Source/WebCore/Modules/webvr/VRFrameData.cpp:148
> +    FloatPoint3D leftEyeOffset = leftEye.rawOffset();
> +    m_leftViewMatrix = rotationMatrix;
> +    applyHeadToEyeTransform(m_leftViewMatrix, leftEyeOffset);

Inline leftEye.rawOffset() into the applyHeadToEyeTransform() call.

> Source/WebCore/Modules/webvr/VRFrameData.cpp:152
> +    FloatPoint3D rightEyeOffset = rightEye.rawOffset();
> +    m_rightViewMatrix = rotationMatrix;
> +    applyHeadToEyeTransform(m_rightViewMatrix, rightEyeOffset);

Ditto.

> Source/WebCore/Modules/webvr/VRFrameData.h:28
> +

Unnecessary whitespace.

> Source/WebCore/Modules/webvr/VRPose.cpp:48
> +    auto position = Float32Array::create(3);
> +    float* positionData = position->data();
> +    positionData[0] = m_trackingInfo.position->x();
> +    positionData[1] = m_trackingInfo.position->y();
> +    positionData[2] = m_trackingInfo.position->z();
> +    return position.releaseNonNull();

To follow other Float32Array::create() calls, first construct positionData[3] and then call `Float32Array::create(positionData, 3).releaseNonNull();`.

> Source/WebCore/Modules/webvr/VRPose.cpp:72
> +    auto orientation = Float32Array::create(4);
> +    float* orientationData = orientation->data();
> +    orientationData[0] = m_trackingInfo.orientation->x;
> +    orientationData[1] = m_trackingInfo.orientation->y;
> +    orientationData[2] = m_trackingInfo.orientation->z;
> +    orientationData[3] = m_trackingInfo.orientation->w;
> +    return orientation.releaseNonNull();

Ditto.

> Source/WebCore/Modules/webvr/VRPose.h:28
> +

Unnecessary whitespace.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:157
> +    double timestamp;

Default-initialize this to 0.

[Sergio Villar Senin]

(In reply to Zan Dobersek from comment #9)
> Comment on attachment 337355 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337355&action=review
> 
> > Source/WebCore/Modules/webvr/VRDisplay.cpp:113
> > +    auto* document = downcast<Document>(scriptExecutionContext());
> 
> This can be moved into the (!m_scriptedAnimationController) block.
> 
> > Source/WebCore/Modules/webvr/VRDisplay.cpp:118
> > +        // FIXME: Get the display id of the HMD as it should use the HMD native refresh rate.
> > +        PlatformDisplayID displayID = document->page() ? document->page()->chrome().displayID() : 0;
> > +        m_scriptedAnimationController = ScriptedAnimationController::create(*document, displayID);
> 
> Is this necessary at this point, for animation frame functionality? I would
> just default to the 0 ID until it's possible to acquire the display ID from
> the HMD device.

I think it does not hurt and we're going to need it really soon.

> > Source/WebCore/Modules/webvr/VRFrameData.cpp:91
> > +    return projectionMatrix;
> 
> You could use the 16-argument TransformationMatrix constructor here, and
> pass all the values in the return statement.

Yeah, but the end result is really confusing IMHO. So unless you feel strongly about it I prefer to leave it as is.
 
> > Source/WebCore/Modules/webvr/VRFrameData.cpp:124
> > +    return matrix;
> 
> Same here. You could also alias normalizedQuaternion variable to something
> shorter, e.g. `nq`, and then compute all the values on the fly.

I don't think it's a good idea to compute all the values on the fly because we precisely store some of them in variables in order to save some multiplications. That said, in this case I think we could use the 16-argument constructor here as variable names are short in this case.

> > Source/WebCore/Modules/webvr/VRFrameData.cpp:148
> > +    FloatPoint3D leftEyeOffset = leftEye.rawOffset();
> > +    m_leftViewMatrix = rotationMatrix;
> > +    applyHeadToEyeTransform(m_leftViewMatrix, leftEyeOffset);
> 
> Inline leftEye.rawOffset() into the applyHeadToEyeTransform() call.

ACK

> > Source/WebCore/Modules/webvr/VRFrameData.cpp:152
> > +    FloatPoint3D rightEyeOffset = rightEye.rawOffset();
> > +    m_rightViewMatrix = rotationMatrix;
> > +    applyHeadToEyeTransform(m_rightViewMatrix, rightEyeOffset);
> 
> Ditto.
> 
> > Source/WebCore/Modules/webvr/VRFrameData.h:28
> > +
> 
> Unnecessary whitespace.
> 
> > Source/WebCore/Modules/webvr/VRPose.cpp:48
> > +    auto position = Float32Array::create(3);
> > +    float* positionData = position->data();
> > +    positionData[0] = m_trackingInfo.position->x();
> > +    positionData[1] = m_trackingInfo.position->y();
> > +    positionData[2] = m_trackingInfo.position->z();
> > +    return position.releaseNonNull();
> 
> To follow other Float32Array::create() calls, first construct
> positionData[3] and then call `Float32Array::create(positionData,
> 3).releaseNonNull();`.

OK, will do

> > Source/WebCore/Modules/webvr/VRPose.cpp:72
> > +    auto orientation = Float32Array::create(4);
> > +    float* orientationData = orientation->data();
> > +    orientationData[0] = m_trackingInfo.orientation->x;
> > +    orientationData[1] = m_trackingInfo.orientation->y;
> > +    orientationData[2] = m_trackingInfo.orientation->z;
> > +    orientationData[3] = m_trackingInfo.orientation->w;
> > +    return orientation.releaseNonNull();
> 
> Ditto.
> 
> > Source/WebCore/Modules/webvr/VRPose.h:28
> > +
> 
> Unnecessary whitespace.
> 
> > Source/WebCore/platform/vr/VRPlatformDisplay.h:157
> > +    double timestamp;
> 
> Default-initialize this to 0.

[Zan Dobersek]

Comment on attachment 337355 [details]
Patch

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

>>> Source/WebCore/Modules/webvr/VRFrameData.cpp:91
>>> +    return projectionMatrix;
>> 
>> You could use the 16-argument TransformationMatrix constructor here, and pass all the values in the return statement.
> 
> Yeah, but the end result is really confusing IMHO. So unless you feel strongly about it I prefer to leave it as is.

I don't feel strongly about it.

>>> Source/WebCore/Modules/webvr/VRFrameData.cpp:124
>>> +    return matrix;
>> 
>> Same here. You could also alias normalizedQuaternion variable to something shorter, e.g. `nq`, and then compute all the values on the fly.
> 
> I don't think it's a good idea to compute all the values on the fly because we precisely store some of them in variables in order to save some multiplications. That said, in this case I think we could use the 16-argument constructor here as variable names are short in this case.

OK, makes sense.

[Sergio Villar Senin]

Created attachment 337490 [details]
Patch

[Zan Dobersek]

Comment on attachment 337490 [details]
Patch

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

> Source/WebCore/Modules/webvr/VRFrameData.h:59
> +    double m_timestamp;

This too should be default-initialized to 0.

> Source/WebCore/Modules/webvr/VRPose.cpp:43
> +    float positionData[3] = { m_trackingInfo.position->x(), m_trackingInfo.position->y(), m_trackingInfo.position->z() };

Nit: IMO it's cleaner to retrieve the reference of the m_trackingInfo.position object and then access x/y/z values through that:
```
    auto& position = *m_trackingInfo.position;
    float positionData[3] = { position.x(), position.y(), position.z() };
```

> Source/WebCore/Modules/webvr/VRPose.cpp:62
> +    float orientationData[4] = { m_trackingInfo.orientation->x, m_trackingInfo.orientation->y, m_trackingInfo.orientation->z, m_trackingInfo.orientation->w };

Nit: similarly:
```
    auto& orientation = *m_trackingInfo.orientation;
    float orientationData[4] = { orientation.x, orientation.y, orientation.z, orientation.w };
```

[Sergio Villar Senin]

Committed r230428: <https://trac.webkit.org/changeset/230428>

[Radar WebKit Bug Importer]

<rdar://problem/39279853>