[webvr] Support HTC Vive Haptic Feedback

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: kip, Assigned: daoshengmu)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [gfx-noted][webvr], URL)

Attachments

(6 attachments)

(Reporter)

Description

3 years ago
The HTC Vive controllers include haptic feedback.  Discussion in the WebVR Spec GitHub is ongoing on how this may be exposed through the Gamepad API.

The webidl should be written in a hardware agnostic manner so that other VR and non-VR controllers can use the same API.
(Reporter)

Updated

3 years ago
See Also: → gamepad-rumble
Got a specific issue this discussion is happening on? Didn't see anything obvious in the spec repo.
(Reporter)

Comment 2

3 years ago
Updated URL for wrong bug, reverting...

Updated

3 years ago
Whiteboard: [gfx-noted]
(Assignee)

Comment 4

3 years ago
According to OpenVR spec, https://github.com/ValveSoftware/openvr/blob/5bc41e4b55d11dfc8fb4b958a6600aa7f8cee051/headers/openvr.h#L1320, it mentions the vibrate trigger rate is ~ 5ms, but I only can receive the response at ~ 3.9 ms from OpenVR sample.

Moreover, we have to consider that if the duration time is longer than 5ms, we need to separate the rest time to a loop for updating.
Whiteboard: [gfx-noted] → [gfx-noted][webvr]
(Assignee)

Comment 8

3 years ago
These patches are very close to review. Just need to confirm the inconsistent of the vibrate duration time. Probably, it would be good in release build.
I'm not ok with this API. Having functions to send single pulses through the whole IPC chain is silly, which is why I pushed back on using WebVibrate for this in bug 680289 (though that also lacked intensity). We should at least allow sending arrays of duration/timing pairs (which roughly approximate effect sending) that can be handled in the gamepad thread, versus expecting everyone to send single changes every time they want to update vibration values.
Ok. Found https://github.com/w3c/gamepad/issues/39 so I at least see work is happening toward those ends. Will take grievances there.
(Assignee)

Comment 11

2 years ago
(In reply to Kyle Machulis [:qdot] from comment #10)
> Ok. Found https://github.com/w3c/gamepad/issues/39 so I at least see work is
> happening toward those ends. Will take grievances there.

qdot. Do you agree that we implement this current pulse API at the first stage and waiting for the following discussion to enhance it?
(Assignee)

Comment 12

2 years ago
(In reply to Kyle Machulis [:qdot] from comment #10)
> Ok. Found https://github.com/w3c/gamepad/issues/39 so I at least see work is
> happening toward those ends. Will take grievances there.

qdot. Do you agree that we implement this current pulse API at the first stage and waiting for the following discussion to enhance it?
Flags: needinfo?(kyle)
Well, if it's already in chrome, I guess we're at the web-compat point anyways.
Flags: needinfo?(kyle)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8810790 [details]
Bug 1299937 - Part 2: Support gamepad haptic in GamepadManager;

https://reviewboard.mozilla.org/r/93126/#review101190
Attachment #8810790 - Flags: review?(cleu) → review+
(Reporter)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8810790 [details]
Bug 1299937 - Part 2: Support gamepad haptic in GamepadManager;

https://reviewboard.mozilla.org/r/93126/#review101494

Looks great, thanks!
Attachment #8810790 - Flags: review?(kgilbert) → review+
(Reporter)

Comment 21

2 years ago
mozreview-review
Comment on attachment 8810791 [details]
Bug 1299937 - Part 3: Implement haptic pulse for OpenVR controller;

https://reviewboard.mozilla.org/r/93128/#review101496

Great, thanks!
Attachment #8810791 - Flags: review?(kgilbert) → review+
Sorry for the review delay on my end, this came in right before I left for holiday vacation. Will try to take care of it in the next couple of days.
(Assignee)

Comment 23

2 years ago
(In reply to Kyle Machulis [:qdot] from comment #22)
> Sorry for the review delay on my end, this came in right before I left for
> holiday vacation. Will try to take care of it in the next couple of days.

qDot, thanks a lot.

Comment 24

2 years ago
mozreview-review
Comment on attachment 8810791 [details]
Bug 1299937 - Part 3: Implement haptic pulse for OpenVR controller;

https://reviewboard.mozilla.org/r/93128/#review102716

See comments, both on my review and Part 3 (which I wasn't asked to review but is a r- from me, due to lack of thread guarding/handling). 

Also, where are the tests? I don't see tests in any commit for this bug.

::: gfx/vr/gfxVROpenVR.cpp:739
(Diff revision 3)
> +  // on this controller and axis combination for 5ms.
> +  const double kVibrateRate = 5.0;
> +  if (duration >= kVibrateRate) {
> +    MOZ_ASSERT(mVibrateThread);
> +
> +    RefPtr<Runnable> runnable =

What thread does this function run on? It looks like you're calling VibrateHaptic from the main thread but then it can then assign itself out to mVibrateThread? Please put thread info guards on your functions.
Attachment #8810791 - Flags: review-

Comment 25

2 years ago
mozreview-review
Comment on attachment 8810789 [details]
Bug 1299937 - Part 1: Implement GamepadHapticActuator in Gamepad API;

https://reviewboard.mozilla.org/r/93124/#review102712

::: dom/gamepad/GamepadHapticActuator.cpp:41
(Diff revision 2)
> +}
> +
> +nsISupports*
> +GamepadHapticActuator::GetParentObject() const
> +{
> +return mParent;

nit: indent

::: dom/gamepad/GamepadHapticActuator.cpp:50
(Diff revision 2)
> +GamepadHapticActuator::Pulse(double aValue, double aDuration, ErrorResult& aRv)
> +{
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetParentObject());
> +  MOZ_ASSERT(global);
> +
> +  if (!global) {

You already asserted there's a global. If the global doesn't exist, that should fail tests. So why is this block here?

::: dom/gamepad/GamepadHapticActuator.cpp:55
(Diff revision 2)
> +  if (!global) {
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return nullptr;
> +  }
> +
> +  RefPtr<Promise> promise = Promise::Create(global, aRv);

Why are you creating the promise before you even know if you have a gamepad manager? And it doesn't look like you actually do anything with the promise in case of success?

::: dom/gamepad/GamepadHapticActuator.cpp:64
(Diff revision 2)
> +
> +  RefPtr<GamepadManager> gamepadManager(GamepadManager::GetService());
> +  MOZ_ASSERT(gamepadManager);
> +
> +  // Clamp intensity aValue to be 0~1.
> +  double value = (aValue < 0) ? 0 : ((aValue > 1) ? 1 : aValue);

Here and below, you can use Clamp from mfbt/MathAlgorithms.h

::: dom/gamepad/GamepadHapticActuator.cpp:68
(Diff revision 2)
> +  // Clamp intensity aValue to be 0~1.
> +  double value = (aValue < 0) ? 0 : ((aValue > 1) ? 1 : aValue);
> +  // aDuration should be always positive.
> +  double duration = (aDuration < 0) ? 0 : aDuration;
> +
> +  if (gamepadManager) {

Same as the global above, you already asserted there's a gamepad manager. If the manager doesn't exist, that should be caught in tests. So why are you checking again?

::: dom/gamepad/GamepadHapticActuator.cpp:79
(Diff revision 2)
> +      default:
> +        // We need to implement other types of haptic
> +        MOZ_ASSERT(false);
> +        aRv.Throw(NS_ERROR_FAILURE); // This will reject the promise.
> +        return promise.forget();
> +        break;

Unneeded break. You just returned if you got here.

::: dom/webidl/GamepadHapticActuator.webidl:6
(Diff revision 2)
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +

nit: Put URL of spec in comments
Attachment #8810789 - Flags: review?(kyle) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

2 years ago
mozreview-review-reply
Comment on attachment 8810789 [details]
Bug 1299937 - Part 1: Implement GamepadHapticActuator in Gamepad API;

https://reviewboard.mozilla.org/r/93124/#review102712

> Here and below, you can use Clamp from mfbt/MathAlgorithms.h

Clamp from mfbt/MathAlgorithms.h only supports Integral type, so I make a Clamp() macro function to help me to do this stuff.
(Assignee)

Comment 35

2 years ago
mozreview-review-reply
Comment on attachment 8810791 [details]
Bug 1299937 - Part 3: Implement haptic pulse for OpenVR controller;

https://reviewboard.mozilla.org/r/93128/#review102716

Currently, we have no tests for all WebVR API. We are working on Bug 1229479 and hope we will have our solution in 2017 Q1. It is a difficult topic because we have to make a virtual device to mimic the behavior from the real VR devices.

> What thread does this function run on? It looks like you're calling VibrateHaptic from the main thread but then it can then assign itself out to mVibrateThread? Please put thread info guards on your functions.

I add some comments at my function. They are at on the compositor thread at the first run and then they will be at the mVibrateThread.
(In reply to Daosheng Mu[:daoshengmu] from comment #35)
> Comment on attachment 8810791 [details]
> Bug 1299937 - Part 3: Implement haptic pulse for OpenVR controller;
> 
> https://reviewboard.mozilla.org/r/93128/#review102716
> 
> Currently, we have no tests for all WebVR API. We are working on Bug 1229479
> and hope we will have our solution in 2017 Q1. It is a difficult topic
> because we have to make a virtual device to mimic the behavior from the real
> VR devices.

This is not just WebVR. This is an extension for the gamepad api, which has a whole gamepad api test service specifically for testing things like this. If there were tests here, you might've noticed things like never resolving your own promise, without me having to spend review time on it.

I'm not reviewing any more changes/patches until there are tests for this. It's a waste of my time.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 42

2 years ago
(In reply to Kyle Machulis [:qdot] from comment #36)
> (In reply to Daosheng Mu[:daoshengmu] from comment #35)
> > Comment on attachment 8810791 [details]
> > Bug 1299937 - Part 3: Implement haptic pulse for OpenVR controller;
> > 
> > https://reviewboard.mozilla.org/r/93128/#review102716
> > 
> > Currently, we have no tests for all WebVR API. We are working on Bug 1229479
> > and hope we will have our solution in 2017 Q1. It is a difficult topic
> > because we have to make a virtual device to mimic the behavior from the real
> > VR devices.
> 
> This is not just WebVR. This is an extension for the gamepad api, which has
> a whole gamepad api test service specifically for testing things like this.
> If there were tests here, you might've noticed things like never resolving
> your own promise, without me having to spend review time on it.
> 
> I'm not reviewing any more changes/patches until there are tests for this.
> It's a waste of my time.

Sorry qdot, I should aware the importance of tests beyond sending the review request. I have added the test for gamepad extension api. Please help me review again if you are available. Thanks.

Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ccbc2f28b3351119d1f0187d7b951e76a1577d9&selectedJob=67549838
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8825322 - Flags: review?(kyle)
(Assignee)

Comment 44

2 years ago
Cancel the review request because there is an unknown crash issue on Mac OSX debug.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 50

2 years ago
(In reply to Daosheng Mu[:daoshengmu] from comment #44)
> Cancel the review request because there is an unknown crash issue on Mac OSX
> debug.

Well, I find the root cause is I didn't release the gamepad refPtr of GamepadHapticActuator while it is called cycle collection.

Try looks good now, https://treeherder.mozilla.org/#/jobs?repo=try&revision=7810ae52f61f992cf1b70e3ab64dff49aeb31a3d

Comment 51

2 years ago
mozreview-review
Comment on attachment 8810789 [details]
Bug 1299937 - Part 1: Implement GamepadHapticActuator in Gamepad API;

https://reviewboard.mozilla.org/r/93124/#review103034

::: dom/webidl/Gamepad.webidl:88
(Diff revision 4)
> +
> +  /**
> +   * The current haptic actuator of the device, an array of
> +   * GamepadHapticActuator.
> +   */
> +  [Pure, Cached, Frozen, Pref="dom.gamepad.extensions.enabled"]

Why is this Pure, instead of Constant? And if it's cached, where are you invalidating the cache?

::: dom/gamepad/GamepadHapticActuator.h:48
(Diff revision 6)
> +private:
> +  virtual ~GamepadHapticActuator() {}
> +
> +protected:
> +  nsCOMPtr<nsISupports> mParent;
> +  RefPtr<Gamepad> mGamepad;

Why not just store the hash key of the gamepad, instead a refptr to the gamepad? It should never change, right? Seems like that might be less complicated.

::: dom/gamepad/GamepadHapticActuator.cpp:67
(Diff revision 6)
> +    return nullptr;
> +  }
> +  switch (mType) {
> +    case GamepadHapticActuatorType::Vibration:
> +    {
> +      RefPtr<Promise> promise = gamepadManager->VibrateHaptic(

Nit: Line-break on the = here to fix indentation.

::: dom/gamepad/GamepadHapticActuator.cpp:69
(Diff revision 6)
> +  switch (mType) {
> +    case GamepadHapticActuatorType::Vibration:
> +    {
> +      RefPtr<Promise> promise = gamepadManager->VibrateHaptic(
> +                                  mGamepad->HashKey(), mIndex,
> +                                  value, duration, global, aRv);

Promise creation can fail, check that here before returning.

::: dom/webidl/Gamepad.webidl:88
(Diff revision 6)
> +
> +  /**
> +   * The current haptic actuator of the device, an array of
> +   * GamepadHapticActuator.
> +   */
> +  [Pure, Cached, Frozen, Pref="dom.gamepad.extensions.enabled"]

Why is this Pure instead of just Constant? How does it depend on DOMState?

::: dom/webidl/GamepadHapticActuator.webidl:20
(Diff revision 6)
> +  HeaderFile="mozilla/dom/GamepadHapticActuator.h"]
> +interface GamepadHapticActuator
> +{
> +  readonly attribute GamepadHapticActuatorType type;
> +
> +  [Throws]

There isn't an algorithm in the spec denoting that this throws. Why is it annotated to throw here?
Attachment #8810789 - Flags: review?(kyle) → review-

Comment 52

2 years ago
mozreview-review
Comment on attachment 8810791 [details]
Bug 1299937 - Part 3: Implement haptic pulse for OpenVR controller;

https://reviewboard.mozilla.org/r/93128/#review110042

::: gfx/vr/gfxVROpenVR.cpp:96
(Diff revision 7)
>  };
>  
>  const uint32_t gNumOpenVRAxis = sizeof(gOpenVRAxes) /
>                                  sizeof(uint32_t);
>  
> +const uint32_t gNumOpenVRHaptcs = 1;

nit: prefix with k, not g, for constants. Could probably also make this static const.

::: gfx/vr/gfxVROpenVR.cpp:567
(Diff revision 7)
>      vr_ShutdownInternal();
>      return false;
>    }
>  
> +  if (!mVibrateThread) {
> +    nsresult rv = NS_NewThread(getter_AddRefs(mVibrateThread));

Assuming we even need the haptics thread, why not spin up the haptics thread lazily, on the first haptics call?

::: gfx/vr/gfxVROpenVR.cpp:721
(Diff revision 7)
> +                                               double aDuration,
> +                                               uint64_t aVibrateIndex)
> +{
> +  // UpdateVibrateHaptic() only can be called by the compositor thread from
> +  // VibrateHaptic() for the first execution, and then it will be run by mVibrateThread
> +  // for the rest duration time.

There's not enough information here to figure out what's going on. Does TriggerHapticPulse block? If so, why does it ever run on the compositor thread? If not, why are you creating another thread and not just delay dispatching a runnable to the current thread?

::: gfx/vr/gfxVROpenVR.cpp:723
(Diff revision 7)
> +{
> +  // UpdateVibrateHaptic() only can be called by the compositor thread from
> +  // VibrateHaptic() for the first execution, and then it will be run by mVibrateThread
> +  // for the rest duration time.
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  RefPtr<impl::VRControllerOpenVR> controller = mOpenVRController[aControllerIdx];

Can this ever fail? What happens if a controller is disconnected between the time the DOM method is called and the IPC transaction happens? I see this happening other places in this file with no checks also.
Attachment #8810791 - Flags: review?(kyle) → review-

Comment 53

2 years ago
mozreview-review
Comment on attachment 8824019 [details]
Bug 1299937 - Part 4: Handling vibrate haptic promise in VRManager;

https://reviewboard.mozilla.org/r/102524/#review110054

It's kind of hard to review this patch based on the comments in the earlier ones. I have a feeling it may change due to those.

::: dom/gamepad/GamepadManager.cpp:676
(Diff revision 4)
> -void
> +already_AddRefed<Promise>
>  GamepadManager::VibrateHaptic(uint32_t aControllerIdx, uint32_t aHapticIndex,
> -                              double aIntensity, double aDuration)
> +                              double aIntensity, double aDuration,
> +                              nsIGlobalObject* aGlobal, ErrorResult& aRv)
>  {
> +  RefPtr<Promise> promise = Promise::Create(aGlobal, aRv);

Promise creation can fail. Check that and fail early.

::: gfx/vr/ipc/VRManagerChild.cpp:600
(Diff revision 4)
> +  RefPtr<dom::Promise> p;
> +  if (!mGamepadPromiseList.Get(aPromiseID, getter_AddRefs(p))) {
> +    MOZ_CRASH("We should always have a promise.");
> +  }
> +
> +  p->MaybeResolve(true);

Are you sure this can never error?
Attachment #8824019 - Flags: review?(kyle) → review-

Comment 54

2 years ago
mozreview-review
Comment on attachment 8825322 [details]
Bug 1299937 - Part 5: Add gamepad extension API tests;

https://reviewboard.mozilla.org/r/103496/#review104266

I think this looks ok. Gonna have cleu look at it also since they've dealt with intermittents in gamepad tests more than I have, just want to make sure there aren't any potential oranges I'm missing.

::: dom/gamepad/GamepadManager.cpp:685
(Diff revision 3)
>      mVRChannelChild->AddPromise(mPromiseID, promise);
>      mVRChannelChild->SendVibrateHaptic(index, aHapticIndex,
>                                         aIntensity, aDuration,
>                                         mPromiseID);
>    } else {
> -    // TODO: Bug 680289, implement for standard gamepads
> +    for (uint32_t i = 0; i < mChannelChildren.Length(); ++i) {

nit: Range iteration might be cleaner here? Not a big deal.

::: dom/gamepad/cocoa/CocoaGamepad.cpp:290
(Diff revision 3)
>    uint32_t index = service->AddGamepad(buffer,
>                                         mozilla::dom::GamepadMappingType::_empty,
> +                                       mozilla::dom::GamepadHand::_empty,
>                                         (int)mGamepads[slot].numButtons(),
> -                                       (int)mGamepads[slot].numAxes());
> +                                       (int)mGamepads[slot].numAxes(),
> +                                       0);

Nit: Add a TODO here (and for the other platform specific libs) that this needs to be updated when we have gamepad haptics for cocoa. Reference bug 680289.
Attachment #8825322 - Flags: review?(kyle) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 60

2 years ago
mozreview-review-reply
Comment on attachment 8810789 [details]
Bug 1299937 - Part 1: Implement GamepadHapticActuator in Gamepad API;

https://reviewboard.mozilla.org/r/93124/#review103034

> Why is this Pure, instead of Constant? And if it's cached, where are you invalidating the cache?

Yep. I think using Constant is better. In the case of Constant, I needn't call ClearCacheXXXValue(), right? do I still need to use other way to invalidate the cache?
(Assignee)

Comment 61

2 years ago
mozreview-review-reply
Comment on attachment 8824019 [details]
Bug 1299937 - Part 4: Handling vibrate haptic promise in VRManager;

https://reviewboard.mozilla.org/r/102524/#review110054

> Are you sure this can never error?

Not sure if using this way to check error is right. Can you give me some suggestion?
(Assignee)

Comment 62

2 years ago
mozreview-review-reply
Comment on attachment 8810791 [details]
Bug 1299937 - Part 3: Implement haptic pulse for OpenVR controller;

https://reviewboard.mozilla.org/r/93128/#review110042

> There's not enough information here to figure out what's going on. Does TriggerHapticPulse block? If so, why does it ever run on the compositor thread? If not, why are you creating another thread and not just delay dispatching a runnable to the current thread?

In this update, I let TriggerHapticPulse() can only be called by the mVibrate thread to make sure it would not block the compositor thread.

Comment 64

2 years ago
mozreview-review
Comment on attachment 8825322 [details]
Bug 1299937 - Part 5: Add gamepad extension API tests;

https://reviewboard.mozilla.org/r/103496/#review110944

LGTM
Attachment #8825322 - Flags: review?(cleu) → review+
(Assignee)

Comment 65

2 years ago
In order to avoid blocking the implement of Oculus Touch support, I move some code from part 3 patch to Attachment #8833891 [details].
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 76

2 years ago
Rebasing because Bug 1305889 is landed.

Comment 77

2 years ago
mozreview-review
Comment on attachment 8810789 [details]
Bug 1299937 - Part 1: Implement GamepadHapticActuator in Gamepad API;

https://reviewboard.mozilla.org/r/93124/#review112556

::: dom/gamepad/GamepadHapticActuator.cpp:25
(Diff revision 9)
> +NS_INTERFACE_MAP_END
> +
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(GamepadHapticActuator, mParent)
> +
> +GamepadHapticActuator::GamepadHapticActuator(nsISupports* aParent, uint32_t aGamepadId,
> +                                            uint32_t aIndex)

nit: indent

::: dom/webidl/GamepadHapticActuator.webidl:19
(Diff revision 9)
> +[Pref="dom.gamepad.extensions.enabled",
> +  HeaderFile="mozilla/dom/GamepadHapticActuator.h"]
> +interface GamepadHapticActuator
> +{
> +  readonly attribute GamepadHapticActuatorType type;
> +  Promise<boolean> pulse(double value, double duration);

Ok, so when I said "why is it annotated to throw here", the answer is "because if you can't create a promise, you need to throw". I should've specified that. It's not a problem with your code, it's a problem with the spec (or lack thereof). This should be annotated [Throws, NewObject], and should throw if you can't create the promise. Just returning nullptr will be confusing to the user.

You should file a spec bug on this, as the gamepad haptics spec is woefully incompletely about the algorithms your code should follow.
Attachment #8810789 - Flags: review?(kyle) → review-

Comment 78

2 years ago
mozreview-review
Comment on attachment 8810790 [details]
Bug 1299937 - Part 2: Support gamepad haptic in GamepadManager;

https://reviewboard.mozilla.org/r/93126/#review112564

r-'ing this patch because there's a BIG open question here that neither you nor the spec is answering: What happens if I submit a 30s long pulse vibration, then 1s later leave focus of the triggering window, by moving to another tab? Will the controller just keep vibrating? This is a case that should be handled by the gamepad manager for the content process, which is why I'm r-'ing this.
Attachment #8810790 - Flags: review-

Comment 79

2 years ago
mozreview-review
Comment on attachment 8810791 [details]
Bug 1299937 - Part 3: Implement haptic pulse for OpenVR controller;

https://reviewboard.mozilla.org/r/93128/#review112584

r- 'cause I've got more questions about this that are bigger than nits.

::: gfx/vr/gfxVROpenVR.cpp:680
(Diff revision 10)
> +                                               double aDuration,
> +                                               uint64_t aVibrateIndex)
> +{
> +  // UpdateVibrateHaptic() only can be called by mVibrateThread
> +  MOZ_ASSERT(mVibrateThread == NS_GetCurrentThread());
> +  RefPtr<impl::VRControllerOpenVR> controller = mOpenVRController[aControllerIdx];

The cross-thread access of the controller array, even though it's a read, is making me a little jumpy, and I don't understand this check? You're making sure that the controller and haptic index are a valid pair? Why not do this before firing the task to the thread?

::: gfx/vr/gfxVROpenVR.cpp:726
(Diff revision 10)
> +  RefPtr<impl::VRControllerOpenVR> controller = mOpenVRController[aControllerIdx];
> +  MOZ_ASSERT(controller);
> +
> +  // Spinning up the haptics thread at the first haptics call.
> +  if (!mVibrateThread) {
> +    nsresult rv = NS_NewThread(getter_AddRefs(mVibrateThread));

Do we know /anything/ about if/how TriggerHapticPulse blocks? I'm curious if we couldn't make this a single thread managed by mOpenVRManager or something. Not that a thread-per-controller is too heavyweight, but less threads is always better.
Attachment #8810791 - Flags: review?(kyle) → review-

Comment 80

2 years ago
mozreview-review
Comment on attachment 8810791 [details]
Bug 1299937 - Part 3: Implement haptic pulse for OpenVR controller;

https://reviewboard.mozilla.org/r/93128/#review112594

::: gfx/vr/gfxVROpenVR.cpp:497
(Diff revision 10)
>        mOpenVRHMD = nullptr;
>      }
>      RemoveControllers();
>      mVRSystem = nullptr;
>      mOpenVRInstalled = false;
> +    mVibrateThread = nullptr;

You should probably Shutdown your thread so it joins before just losing the pointer.

Comment 81

2 years ago
mozreview-review
Comment on attachment 8824019 [details]
Bug 1299937 - Part 4: Handling vibrate haptic promise in VRManager;

https://reviewboard.mozilla.org/r/102524/#review112596

::: gfx/vr/ipc/VRManagerChild.cpp:632
(Diff revision 7)
> +  RefPtr<dom::Promise> p;
> +  if (!mGamepadPromiseList.Get(aPromiseID, getter_AddRefs(p))) {
> +    MOZ_CRASH("We should always have a promise.");
> +  }
> +
> +  if (p->State() == Promise::PromiseState::Resolved) {

This logic is confusing me. What are you checking here? That the promise has already been resolved? Is there a chance that we'd resolve things multiple times?

::: gfx/vr/ipc/VRManagerParent.cpp:339
(Diff revision 7)
> +                                   const uint32_t& aPromiseID)
>  {
>    VRManager* vm = VRManager::Get();
>    vm->VibrateHaptic(aControllerIdx, aHapticIndex, aIntensity,
>                      aDuration);
> +  if (SendReplyGamepadVibrateHaptic(aPromiseID)) {

According to the spec, this is not how this works. The gamepad haptic states

"The returned Promise will resolve true once the pulse has completed."

You are replying after the call to VibrateHaptic, which simply fires off the task to the other thread, not after the pulse is finished.
Attachment #8824019 - Flags: review?(kyle) → review-

Comment 82

2 years ago
mozreview-review
Comment on attachment 8825322 [details]
Bug 1299937 - Part 5: Add gamepad extension API tests;

https://reviewboard.mozilla.org/r/103496/#review112598

Changing this to r- as, assuming the spec update happens, I'd like to see a test that makes sure we reset forces to 0 on loss of focus. If the spec authors say this isn't required, re-r? me and we'll figure something out.
Attachment #8825322 - Flags: review+ → review-
(Assignee)

Comment 83

2 years ago
(In reply to Kyle Machulis [:qdot] from comment #78)
> Comment on attachment 8810790 [details]
> Bug 1299937 - Part 2: Support gamepad haptic in GamepadManager;
> 
> https://reviewboard.mozilla.org/r/93126/#review112564
> 
> r-'ing this patch because there's a BIG open question here that neither you
> nor the spec is answering: What happens if I submit a 30s long pulse
> vibration, then 1s later leave focus of the triggering window, by moving to
> another tab? Will the controller just keep vibrating? This is a case that
> should be handled by the gamepad manager for the content process, which is
> why I'm r-'ing this.

I have filed an issue about this loss focus problem at github/w3c, https://github.com/w3c/gamepad/issues/58, but I didn't get the response yet for a week. Could we implement this behavior by following our own design to avoid blocking to this bug review? Simultaneously, look forward the final decision from the author.

Currently, I prefer we should stop the vibrating when the tab is losing focus. How do you think?
Flags: needinfo?(kyle)
(In reply to Daosheng Mu[:daoshengmu] from comment #83)
> (In reply to Kyle Machulis [:qdot] from comment #78)
> > Comment on attachment 8810790 [details]
> > Bug 1299937 - Part 2: Support gamepad haptic in GamepadManager;
> > 
> > https://reviewboard.mozilla.org/r/93126/#review112564
> > 
> > r-'ing this patch because there's a BIG open question here that neither you
> > nor the spec is answering: What happens if I submit a 30s long pulse
> > vibration, then 1s later leave focus of the triggering window, by moving to
> > another tab? Will the controller just keep vibrating? This is a case that
> > should be handled by the gamepad manager for the content process, which is
> > why I'm r-'ing this.
> 
> I have filed an issue about this loss focus problem at github/w3c,
> https://github.com/w3c/gamepad/issues/58, but I didn't get the response yet
> for a week. Could we implement this behavior by following our own design to
> avoid blocking to this bug review? Simultaneously, look forward the final
> decision from the author.

Yeah, that's fine.

> Currently, I prefer we should stop the vibrating when the tab is losing
> focus. How do you think?

You might want to check and see what chrome does, assuming they already have this implemented. Otherwise, yeah, I'd want vibration to stop on any loss of focus. That's why I pointed it out, it's one of my major pet peeves when you defocus something, set a controller down, then a bit later it starts vibrating loudly on the desk. :)
Flags: needinfo?(kyle)
(Assignee)

Comment 85

2 years ago
(In reply to Kyle Machulis [:qdot] from comment #84)
> (In reply to Daosheng Mu[:daoshengmu] from comment #83)
> > (In reply to Kyle Machulis [:qdot] from comment #78)
> > > Comment on attachment 8810790 [details]
> > > Bug 1299937 - Part 2: Support gamepad haptic in GamepadManager;
> > > 
> > > https://reviewboard.mozilla.org/r/93126/#review112564
> > > 
> > > r-'ing this patch because there's a BIG open question here that neither you
> > > nor the spec is answering: What happens if I submit a 30s long pulse
> > > vibration, then 1s later leave focus of the triggering window, by moving to
> > > another tab? Will the controller just keep vibrating? This is a case that
> > > should be handled by the gamepad manager for the content process, which is
> > > why I'm r-'ing this.
> > 
> > I have filed an issue about this loss focus problem at github/w3c,
> > https://github.com/w3c/gamepad/issues/58, but I didn't get the response yet
> > for a week. Could we implement this behavior by following our own design to
> > avoid blocking to this bug review? Simultaneously, look forward the final
> > decision from the author.
> 
> Yeah, that's fine.
> 
> > Currently, I prefer we should stop the vibrating when the tab is losing
> > focus. How do you think?
> 
> You might want to check and see what chrome does, assuming they already have
> this implemented. Otherwise, yeah, I'd want vibration to stop on any loss of
> focus. That's why I pointed it out, it's one of my major pet peeves when you
> defocus something, set a controller down, then a bit later it starts
> vibrating loudly on the desk. :)

I have checked Chromium doesn't implement stop vibrating when loss of focus. I think implementing stop vibrating is a good way to go. I am going to do this.
Great, thanks so much for putting the work into this. :)
(Reporter)

Updated

2 years ago
Component: Graphics → WebVR
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 97

2 years ago
mozreview-review-reply
Comment on attachment 8810791 [details]
Bug 1299937 - Part 3: Implement haptic pulse for OpenVR controller;

https://reviewboard.mozilla.org/r/93128/#review112584

> Do we know /anything/ about if/how TriggerHapticPulse blocks? I'm curious if we couldn't make this a single thread managed by mOpenVRManager or something. Not that a thread-per-controller is too heavyweight, but less threads is always better.

I find if we only use one thread to manage all vibrate events that will block another controller that is emitting vibrate events as well. Therefore, I decide to give every controller a thread to serve this task.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 101

2 years ago
mozreview-review
Comment on attachment 8844375 [details]
Bug 1299937 - Part 6: Handle Stop vibrating when the window defoucses;

https://reviewboard.mozilla.org/r/117862/#review119550

::: gfx/vr/gfxVROpenVR.cpp:518
(Diff revision 3)
> +  mIsVibrating = false;
> +  RefPtr<Runnable> runnable =
> +    NewRunnableMethod<uint32_t>
> +      (vm, &VRManager::NotifyVibrateHapticCompleted, aPromiseID);
> +  mCurrentThread->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL);
> +}

qDot, please give me some feedback on the approach I am trying to send back the promise to the current thread whether it is correct.

I notice the current thead could not receive the callback at runtime, but it will receive the callback after shutting down FF. And it always happen crash at VRManagerChild::RecvReplyGamepadVibrateHaptic because the promises has been deleted.
(Assignee)

Updated

2 years ago
Attachment #8844375 - Flags: review?(kyle) → feedback?(kyle)
(Assignee)

Comment 102

2 years ago
(In reply to Kyle Machulis [:qdot] from comment #77)
> Comment on attachment 8810789 [details]
> Bug 1299937 - Part 1: Implement GamepadHapticActuator in Gamepad API;
> 
> https://reviewboard.mozilla.org/r/93124/#review112556
> 
> ::: dom/gamepad/GamepadHapticActuator.cpp:25
> (Diff revision 9)
> > +NS_INTERFACE_MAP_END
> > +
> > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(GamepadHapticActuator, mParent)
> > +
> > +GamepadHapticActuator::GamepadHapticActuator(nsISupports* aParent, uint32_t aGamepadId,
> > +                                            uint32_t aIndex)
> 
> nit: indent
> 
> ::: dom/webidl/GamepadHapticActuator.webidl:19
> (Diff revision 9)
> > +[Pref="dom.gamepad.extensions.enabled",
> > +  HeaderFile="mozilla/dom/GamepadHapticActuator.h"]
> > +interface GamepadHapticActuator
> > +{
> > +  readonly attribute GamepadHapticActuatorType type;
> > +  Promise<boolean> pulse(double value, double duration);
> 
> Ok, so when I said "why is it annotated to throw here", the answer is
> "because if you can't create a promise, you need to throw". I should've
> specified that. It's not a problem with your code, it's a problem with the
> spec (or lack thereof). This should be annotated [Throws, NewObject], and
> should throw if you can't create the promise. Just returning nullptr will be
> confusing to the user.
> 
> You should file a spec bug on this, as the gamepad haptics spec is woefully
> incompletely about the algorithms your code should follow.

I have filed a issue here,
https://github.com/w3c/gamepad/issues/59

Comment 103

2 years ago
mozreview-review
Comment on attachment 8810789 [details]
Bug 1299937 - Part 1: Implement GamepadHapticActuator in Gamepad API;

https://reviewboard.mozilla.org/r/93124/#review124534
Attachment #8810789 - Flags: review?(kyle) → review+

Comment 104

2 years ago
mozreview-review
Comment on attachment 8810790 [details]
Bug 1299937 - Part 2: Support gamepad haptic in GamepadManager;

https://reviewboard.mozilla.org/r/93126/#review124538

::: dom/gamepad/GamepadManager.cpp:679
(Diff revision 10)
> +  if (aControllerIdx >= VR_GAMEPAD_IDX_OFFSET) {
> +    uint32_t index = aControllerIdx - VR_GAMEPAD_IDX_OFFSET;
> +    mVRChannelChild->SendVibrateHaptic(index, aHapticIndex,
> +                                       aIntensity, aDuration);
> +  } else {
> +    // TODO: Bug 680289, implement for standard gamepads

nit: Dunno how we'd ever hit this, but might be worth it to MOZ_ASSERT(false) here just in case a test gets here somehow.
Attachment #8810790 - Flags: review?(kyle) → review+

Comment 105

2 years ago
mozreview-review
Comment on attachment 8810791 [details]
Bug 1299937 - Part 3: Implement haptic pulse for OpenVR controller;

https://reviewboard.mozilla.org/r/93128/#review124540

::: gfx/vr/gfxVROpenVR.h:136
(Diff revision 12)
>    virtual void HandleAxisMove(uint32_t aControllerIdx, uint32_t aAxis,
>                                float aValue) override;
>    virtual void HandlePoseTracking(uint32_t aControllerIdx,
>                                    const dom::GamepadPoseState& aPose,
>                                    VRControllerHost* aController) override;
> +  // void UpdateVibrateHaptic(uint32_t aControllerIdx,

nit: Should this be removed?
Attachment #8810791 - Flags: review?(kyle) → review+

Comment 106

2 years ago
mozreview-review
Comment on attachment 8824019 [details]
Bug 1299937 - Part 4: Handling vibrate haptic promise in VRManager;

https://reviewboard.mozilla.org/r/102524/#review124556

r+ with comments addressed.

::: dom/gamepad/GamepadManager.cpp:678
(Diff revision 10)
> -                              double aIntensity, double aDuration)
> +                              double aIntensity, double aDuration,
> +                              nsIGlobalObject* aGlobal, ErrorResult& aRv)
>  {
> +  RefPtr<Promise> promise = Promise::Create(aGlobal, aRv);
> +  if (!promise) {
> +    return nullptr;

You should probably throw here too, since you've got an errorresult.

::: dom/gamepad/GamepadManager.cpp:688
(Diff revision 10)
> +    mVRChannelChild->AddPromise(mPromiseID, promise);
>      mVRChannelChild->SendVibrateHaptic(index, aHapticIndex,
> -                                       aIntensity, aDuration);
> +                                       aIntensity, aDuration,
> +                                       mPromiseID);
>    } else {
>      // TODO: Bug 680289, implement for standard gamepads

You should probably throw here, otherwise things will just stall.
Attachment #8824019 - Flags: review?(kyle) → review+

Comment 107

2 years ago
mozreview-review
Comment on attachment 8825322 [details]
Bug 1299937 - Part 5: Add gamepad extension API tests;

https://reviewboard.mozilla.org/r/103496/#review124560
Attachment #8825322 - Flags: review?(kyle) → review+
(Assignee)

Updated

2 years ago
Attachment #8844375 - Flags: feedback?(kyle)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 114

2 years ago
mozreview-review-reply
Comment on attachment 8810790 [details]
Bug 1299937 - Part 2: Support gamepad haptic in GamepadManager;

https://reviewboard.mozilla.org/r/93126/#review124538

> nit: Dunno how we'd ever hit this, but might be worth it to MOZ_ASSERT(false) here just in case a test gets here somehow.

I have some implementation at Part 5 for gamepad tests already.
(Assignee)

Comment 115

2 years ago
mozreview-review-reply
Comment on attachment 8810791 [details]
Bug 1299937 - Part 3: Implement haptic pulse for OpenVR controller;

https://reviewboard.mozilla.org/r/93128/#review124540

> nit: Should this be removed?

yep. Thanks.
(Assignee)

Comment 116

2 years ago
mozreview-review-reply
Comment on attachment 8824019 [details]
Bug 1299937 - Part 4: Handling vibrate haptic promise in VRManager;

https://reviewboard.mozilla.org/r/102524/#review124556

> You should probably throw here, otherwise things will just stall.

I have some implementations at Part 5 for gamepad tests already.
(Assignee)

Comment 117

2 years ago
mozreview-review-reply
Comment on attachment 8844375 [details]
Bug 1299937 - Part 6: Handle Stop vibrating when the window defoucses;

https://reviewboard.mozilla.org/r/117862/#review119550

> qDot, please give me some feedback on the approach I am trying to send back the promise to the current thread whether it is correct.
> 
> I notice the current thead could not receive the callback at runtime, but it will receive the callback after shutting down FF. And it always happen crash at VRManagerChild::RecvReplyGamepadVibrateHaptic because the promises has been deleted.

This is because VR is running at Compositor process, and it is not a nsIThread so I can't use dispatch functions. I have to use PostTask() for this PlatformThread.

Comment 119

2 years ago
mozreview-review
Comment on attachment 8844375 [details]
Bug 1299937 - Part 6: Handle Stop vibrating when the window defoucses;

https://reviewboard.mozilla.org/r/117862/#review125674

::: dom/base/nsGlobalWindow.cpp:13525
(Diff revision 4)
> +nsGlobalWindow::StopGamepadHaptics()
> +{
> +  MOZ_ASSERT(IsInnerWindow());
> +  if (mHasSeenGamepadInput) {
> +    RefPtr<GamepadManager> gamepadManager(GamepadManager::GetService());
> +    for (auto iter = mGamepads.Iter(); !iter.Done(); iter.Next()) {

nit: Why not do this iteration in the Gamepad Manager? Since we're always stopping all haptics for all gamepads, seems like we could move the logic there.
Attachment #8844375 - Flags: review?(kyle) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 121

2 years ago
mozreview-review-reply
Comment on attachment 8844375 [details]
Bug 1299937 - Part 6: Handle Stop vibrating when the window defoucses;

https://reviewboard.mozilla.org/r/117862/#review125674

> nit: Why not do this iteration in the Gamepad Manager? Since we're always stopping all haptics for all gamepads, seems like we could move the logic there.

Good suggestion! Thanks.

Comment 122

2 years ago
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/56e2fa280d2a
Part 1: Implement GamepadHapticActuator in Gamepad API; r=qdot
https://hg.mozilla.org/integration/autoland/rev/691a5be9ff0d
Part 2: Support gamepad haptic in GamepadManager; r=kip,Lenzak,qdot
https://hg.mozilla.org/integration/autoland/rev/ed20b557e9a5
Part 3: Implement haptic pulse for OpenVR controller; r=kip,qdot
https://hg.mozilla.org/integration/autoland/rev/3381a8485d05
Part 4: Handling vibrate haptic promise in VRManager; r=qdot
https://hg.mozilla.org/integration/autoland/rev/3c1984bdf404
Part 5: Add gamepad extension API tests; r=Lenzak,qdot
https://hg.mozilla.org/integration/autoland/rev/5d8e162ef5f7
Part 6: Handle Stop vibrating when the window defoucses; r=qdot
Depends on: 1350682
You need to log in before you can comment on or make changes to this bug.