Bug 184265 - [OpenVR][WebVR] Retrieve FrameData in WebVR's rAF
Summary: [OpenVR][WebVR] Retrieve FrameData in WebVR's rAF
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-03 07:22 PDT by Sergio Villar Senin
Modified: 2018-04-09 07:41 PDT (History)
5 users (show)

See Also:


Attachments
Patch (24.72 KB, patch)
2018-04-03 07:44 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (24.61 KB, patch)
2018-04-05 02:26 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (25.24 KB, patch)
2018-04-06 02:08 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (25.28 KB, patch)
2018-04-06 02:11 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (25.34 KB, patch)
2018-04-06 03:14 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (25.40 KB, patch)
2018-04-06 03:20 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (25.41 KB, patch)
2018-04-06 03:48 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (25.26 KB, patch)
2018-04-06 03:59 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (24.79 KB, patch)
2018-04-09 04:03 PDT, Sergio Villar Senin
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2018-04-03 07:22:22 PDT
[OpenVR][WebVR] Retrieve FrameData in WebVR's rAF
Comment 1 Sergio Villar Senin 2018-04-03 07:44:08 PDT
Created attachment 337078 [details]
Patch
Comment 2 Sergio Villar Senin 2018-04-05 02:26:43 PDT
Created attachment 337261 [details]
Patch

Mac/iOS build fix
Comment 3 Sergio Villar Senin 2018-04-06 02:08:21 PDT
Created attachment 337349 [details]
Patch

Build fix for WPE
Comment 4 Sergio Villar Senin 2018-04-06 02:11:39 PDT
Created attachment 337350 [details]
Patch

Rebased patch
Comment 5 Sergio Villar Senin 2018-04-06 03:14:38 PDT
Created attachment 337351 [details]
Patch

Another build fix. Seems that the way unified sources are built affect a lot the compilation results
Comment 6 Sergio Villar Senin 2018-04-06 03:20:36 PDT
Created attachment 337352 [details]
Patch

And another build fix :(
Comment 7 Sergio Villar Senin 2018-04-06 03:48:00 PDT
Created attachment 337353 [details]
Patch

Mac build fix
Comment 8 Sergio Villar Senin 2018-04-06 03:59:52 PDT
Created attachment 337355 [details]
Patch

Hopefully the last Mac build fix
Comment 9 Zan Dobersek 2018-04-09 00:15:28 PDT
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.
Comment 10 Sergio Villar Senin 2018-04-09 02:37:55 PDT
(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.
Comment 11 Zan Dobersek 2018-04-09 02:46:55 PDT
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.
Comment 12 Sergio Villar Senin 2018-04-09 04:03:48 PDT
Created attachment 337490 [details]
Patch
Comment 13 Zan Dobersek 2018-04-09 04:43:32 PDT
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 };
```
Comment 14 Sergio Villar Senin 2018-04-09 07:40:58 PDT
Committed r230428: <https://trac.webkit.org/changeset/230428>
Comment 15 Radar WebKit Bug Importer 2018-04-09 07:41:19 PDT
<rdar://problem/39279853>