Update WebVR interface to match the 1.1 spec

Update WebVR interface to match the 1.1 spec

This update doesn't change the VR API backends at all, just transforms
the data that they're currently providing to match the WebVR 1.1 spec.
A future CL will plumb the changes all the way back to /device/vr.

BUG=389343
Committed: https://crrev.com/aa686778a64f59ffab3fcda0531d7d2d1ab0ecc2
Cr-Commit-Position: refs/heads/master@{#418153}

[bajones, 2016-09-09 22:51:32]

bajones@chromium.org changed reviewers:
+ mkwst@chromium.org

[bajones, 2016-09-09 22:51:33]

mkwst@: Could you review this modules change? I primarily need a review of the
.gni change, but I'd appreciate your feedback on any of it.

[bajones, 2016-09-09 22:57:11]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 22:57:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 23:08:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 23:08:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bajones, 2016-09-09 23:55:15]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 23:55:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 00:12:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-10 00:12:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bajones, 2016-09-10 05:14:47]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-10 05:14:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 09:40:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-10 09:40:01]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-09-12 00:27:49]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-09-12 00:27:50]

https://codereview.chromium.org/2331553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2331553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:89:
Platform::current()->currentThread()->addTaskObserver(this);

Why do you need to add a task observer?

(This adds a lot of overhead to the Blink scheduler.)

[bajones, 2016-09-12 00:52:55]

https://codereview.chromium.org/2331553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2331553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:89:
Platform::current()->currentThread()->addTaskObserver(this);
On 2016/09/12 00:27:50, haraken wrote:
> 
> Why do you need to add a task observer?
> 
> (This adds a lot of overhead to the Blink scheduler.)

The WebVR spec requires this function to return the same values for the duration
of a "frame" so that multiple disjoint libraries/files/functions that query it
will get synchronized values. A previous version of the spec indicated that this
was till javascript returned control to the browser. (Hence the use of a task
observer here and in getPose below).

The latest iteration of the spec
(https://w3c.github.io/webvr/#dom-vrdisplay-getframedata) actually indicates
that it should return the same value until the next call to submitFrame. In
reality that should amount to roughly the same thing, since it would be
impractical for an app to call submitFrame outside the same callback that it
calls getFrameData.

I'm realizing that there's some issues with that bit of the spec now that I'm
thinking about it further. I'll bring them up with the other spec authors.
(submitFrame may not always be called.) If this is really that bad for the
scheduler, though, I'll can work out something that doesn't depend on it.

[haraken, 2016-09-12 01:08:48]

https://codereview.chromium.org/2331553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2331553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:89:
Platform::current()->currentThread()->addTaskObserver(this);
On 2016/09/12 00:52:55, bajones wrote:
> On 2016/09/12 00:27:50, haraken wrote:
> > 
> > Why do you need to add a task observer?
> > 
> > (This adds a lot of overhead to the Blink scheduler.)
> 
> The WebVR spec requires this function to return the same values for the
duration
> of a "frame" so that multiple disjoint libraries/files/functions that query it
> will get synchronized values. A previous version of the spec indicated that
this
> was till javascript returned control to the browser. (Hence the use of a task
> observer here and in getPose below).
> 
> The latest iteration of the spec
> (https://w3c.github.io/webvr/#dom-vrdisplay-getframedata) actually indicates
> that it should return the same value until the next call to submitFrame. In
> reality that should amount to roughly the same thing, since it would be
> impractical for an app to call submitFrame outside the same callback that it
> calls getFrameData.

If we really need that behavior, I'd prefer introducing a notification callback
to submitFrame so that VRDisplay can understand the timing.

WebThread::TaskObserver is a more generic thing to control all tasks in the
renderer process.

> I'm realizing that there's some issues with that bit of the spec now that I'm
> thinking about it further. I'll bring them up with the other spec authors.
> (submitFrame may not always be called.) If this is really that bad for the
> scheduler, though, I'll can work out something that doesn't depend on it.

Thanks!

[bajones, 2016-09-12 01:57:44]

Okay, updated to a version that doesn't depend on the TaskObserver. Has the
potential to do some extra work if the frame data is queried multiple times in
an animation loop that's not actively presenting to a VR device, but that's
definitely an edge case and I think an acceptable compromise while we tighten up
the spec on this point.

Spec bug filed here: https://github.com/w3c/webvr/issues/81

Thanks for pointing out that this wasn't a great pattern to use. I would have
had no idea otherwise!

[haraken, 2016-09-12 05:03:18]

On 2016/09/12 01:57:44, bajones wrote:
> Okay, updated to a version that doesn't depend on the TaskObserver. Has the
> potential to do some extra work if the frame data is queried multiple times in
> an animation loop that's not actively presenting to a VR device, but that's
> definitely an edge case and I think an acceptable compromise while we tighten
up
> the spec on this point.
> 
> Spec bug filed here: https://github.com/w3c/webvr/issues/81
> 
> Thanks for pointing out that this wasn't a great pattern to use. I would have
> had no idea otherwise!

Thanks for the update! Looks good on my side (but I'm clearly not eligible to
review the VR part).

[Mike West, 2016-09-12 07:16:15]

The `.gni` change and general structure LGTM, but I can barely add numbers
together. I'd love it if you found someone else to review the matrix math
portions of the CL.

[bajones, 2016-09-12 15:33:56]

bajones@chromium.org changed reviewers:
+ klausw@chromium.org, mthiesse@chromium.org

[bajones, 2016-09-12 15:33:57]

Thanks mkwst@ and haraken@!

mthiesse@ or klausw@: Could you take a look at the VR logic here?

[mthiesse, 2016-09-12 15:58:41]

I'm new to the blink/idl/V8/spec world, but I left some
hopefully-not-too-ignorant comments anyways.

https://codereview.chromium.org/2331553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2331553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:87: if (m_canUpdateFramePose)
{
Extract this block into a separate function? It's duplicated below.

https://codereview.chromium.org/2331553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:102: VRPose*
VRDisplay::getPose()
Not sure where to leave this comment for
https://w3c.github.io/webvr/#dom-vrdisplay-getpose

spec nit: Should the spec say that calls to getPose() must return the pose
computed for getFrameData, and not update on the first call to getPose()?

https://codereview.chromium.org/2331553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:113: VRPose* pose =
VRPose::create();
Do you need to create a new VRPose if you haven't updated it? I'm unfamiliar
with the blink/V8 world.

(Also to check my understanding, V8 takes ownership of the returned object,
right?)

[bajones, 2016-09-12 18:29:34]

On 2016/09/12 15:58:41, mthiesse wrote:
> third_party/WebKit/Source/modules/vr/VRDisplay.cpp:87: if
(m_canUpdateFramePose)
> {
> Extract this block into a separate function? It's duplicated below.

Done.

> spec nit: Should the spec say that calls to getPose() must return the pose
> computed for getFrameData, and not update on the first call to getPose()?

Good point. I'll work on updating the spec to be more explicit on this point.

> third_party/WebKit/Source/modules/vr/VRDisplay.cpp:113: VRPose* pose =
> VRPose::create();
> Do you need to create a new VRPose if you haven't updated it? I'm unfamiliar
> with the blink/V8 world.

Yes, this turned out to be a bug in a previous version. The problem was that the
site could then update the values in the VRPose and they would unintentionally
feed back into the rendering pipeline, causing weird artifacts. Thus the safe
thing to do is a new object each time. (This function is deprecated by the way,
and it's replacement is getFrameData, which takes an explicit object to be
populated each time to avoid EXACTLY this issue.)
 
> (Also to check my understanding, V8 takes ownership of the returned object,
> right?)

Correct.

[mthiesse, 2016-09-12 18:47:13]

> > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:113: VRPose* pose =
> > VRPose::create();
> > Do you need to create a new VRPose if you haven't updated it? I'm unfamiliar
> > with the blink/V8 world.
> 
> Yes, this turned out to be a bug in a previous version. The problem was that
the
> site could then update the values in the VRPose and they would unintentionally
> feed back into the rendering pipeline, causing weird artifacts. Thus the safe
> thing to do is a new object each time. (This function is deprecated by the
way,
> and it's replacement is getFrameData, which takes an explicit object to be
> populated each time to avoid EXACTLY this issue.)

So I understand the reason to create a new object at each frame - but is there
also a reason to create a new object at each call, even when it hasn't been
updated?
Maybe you've already explained that reason and I'm just not following, or maybe
this is all irrelevant anyways because this is deprecated.

[bajones, 2016-09-12 18:52:34]

I'd say it's mostly irrelevant because of the deprecation, but let me look at
another potential scenario:

Page is using two libraries: One renders a VR scene, the other overlays data
(like debug stats) on top of it. Neither library needs to be aware of the
content of the other, and this still works. Library 1 may query the object and
then change the values in the VRPose in the course of doing it's own
calculations. If we re-use the same Pose object, library 2 would pick up the
changes library 1 made without knowing it, which probably has a lot of
unintended side effects. Returning a new object each time preserves data
integrity, and the new function forces each library to provide their own object
which accomplishes the same goal AND cuts down on the amount of garbage created.

[mthiesse, 2016-09-12 19:43:11]

matrix math lgtm with minor comment.

I should note that this is very domain specific math, so I mostly verified by
comparing it to code I already knew to be working.
matrixfromRotationTranslation, matrixTranslate, and matrixInvert are equivalent
to their VR Shell implementations. projectionFromFieldOfView is equivalent to
the implementation in https://github.com/toji/gl-matrix/

https://codereview.chromium.org/2331553002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRFrameData.cpp (right):

https://codereview.chromium.org/2331553002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRFrameData.cpp:25: float xScale = 2.0f /
(leftTan + rightTan);
I expect that for any realistic values, these tan values will be non-zero. Does
it make sense to check for 0/NaN here?

[bajones, 2016-09-12 20:26:07]

On 2016/09/12 19:43:11, mthiesse wrote:
> third_party/WebKit/Source/modules/vr/VRFrameData.cpp:25: float xScale = 2.0f /
> (leftTan + rightTan);
> I expect that for any realistic values, these tan values will be non-zero.
Does
> it make sense to check for 0/NaN here?

Given this is a web-visible API we probably don't want to allow malicious site
to produce a divide by zero in native code and take down the tab.

[bajones, 2016-09-12 20:26:12]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-09-12 20:26:12]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2331553002/#ps100001
(title: "Addressed further feedback")

[commit-bot: I haz the power, 2016-09-12 20:26:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bajones, 2016-09-12 20:59:22]

On 2016/09/12 20:26:07, bajones wrote:
> On 2016/09/12 19:43:11, mthiesse wrote:
> > third_party/WebKit/Source/modules/vr/VRFrameData.cpp:25: float xScale = 2.0f
/
> > (leftTan + rightTan);
> > I expect that for any realistic values, these tan values will be non-zero.
> Does
> > it make sense to check for 0/NaN here?
> 
> Given this is a web-visible API we probably don't want to allow malicious site
> to produce a divide by zero in native code and take down the tab.

Derp. I should probably note that this case is actually checked for in
VRDisplay.cpp before we call into the functions in question.

[commit-bot: I haz the power, 2016-09-13 00:41:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 00:41:50]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bajones, 2016-09-13 00:45:58]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2016-09-13 00:46:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 03:06:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-13 03:07:25]

Description was changed from

==========
Update WebVR interface to match the 1.1 spec

This update doesn't change the VR API backends at all, just transforms
the data that they're currently providing to match the WebVR 1.1 spec.
A future CL will plumb the changes all the way back to /device/vr.

BUG=389343
==========

to

==========
Update WebVR interface to match the 1.1 spec

This update doesn't change the VR API backends at all, just transforms
the data that they're currently providing to match the WebVR 1.1 spec.
A future CL will plumb the changes all the way back to /device/vr.

BUG=389343

Committed: https://crrev.com/aa686778a64f59ffab3fcda0531d7d2d1ab0ecc2
Cr-Commit-Position: refs/heads/master@{#418153}
==========

[commit-bot: I haz the power, 2016-09-13 03:07:26]

Patchset 6 (id:??) landed as
https://crrev.com/aa686778a64f59ffab3fcda0531d7d2d1ab0ecc2
Cr-Commit-Position: refs/heads/master@{#418153}