Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834843002:
Add event listening function to openvr device (Closed)
|
Created:
3 years, 8 months ago by shaobo.yan Modified:
3 years, 7 months ago CC:
chromium-reviews, feature-vr-reviews_chromium.org, hokein.wu_gmail.com Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd event listening function to openvr device
This patch add event listening function to openvr device by integrate pollNextEvent function into
OpenVRDevice::OpenVRRenderLoop class and regist a callback function from OpenVRDevice to response to events.
Currently, it only deal with events which generated by openvr device setting changes.
BUG=714302
Review-Url: https://codereview.chromium.org/2834843002
Cr-Commit-Position: refs/heads/master@{#470771}
Committed: https://chromium.googlesource.com/chromium/src/+/f548f57a4ec26eabaec6e466ee668aa29834b6c2
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address nhu@ some comments and temporary fix initial vr_system in each OnChange call issue #
Total comments: 1
Patch Set 3 : Address Ningxin@ comments and rebase #Patch Set 4 : Modify some indents #
Total comments: 3
Patch Set 5 : fix ditto #Patch Set 6 : using RunPollingEventCallback to safe thread #Patch Set 7 : Initialize main_thread_task_runner_ #
Total comments: 4
Patch Set 8 : Address mthiesse@ comments #
Total comments: 9
Patch Set 9 : Address billorr1@ comments #Messages
Total messages: 47 (18 generated)
shaobo.yan@intel.com changed reviewers: + bajones@chromium.org, billorr@chromium.org
billorr@ and bajones@ : PTAL.
Description was changed from ========== Add event listening function to openvr device This patch add event listening function to openvr device by integrate pollNextEvent function into OpenVRDevice::OpenVRRenderLoop class and regist a callback function from OpenVRDevice to response to events. Currently, it only deal with events which generated by openvr device setting changes. BUG= ========== to ========== Add event listening function to openvr device This patch add event listening function to openvr device by integrate pollNextEvent function into OpenVRDevice::OpenVRRenderLoop class and regist a callback function from OpenVRDevice to response to events. Currently, it only deal with events which generated by openvr device setting changes. BUG= ==========
Description was changed from ========== Add event listening function to openvr device This patch add event listening function to openvr device by integrate pollNextEvent function into OpenVRDevice::OpenVRRenderLoop class and regist a callback function from OpenVRDevice to response to events. Currently, it only deal with events which generated by openvr device setting changes. BUG= ========== to ========== Add event listening function to openvr device This patch add event listening function to openvr device by integrate pollNextEvent function into OpenVRDevice::OpenVRRenderLoop class and regist a callback function from OpenVRDevice to response to events. Currently, it only deal with events which generated by openvr device setting changes. BUG=714302 ==========
The CQ bit was checked by shaobo.yan@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
shaobo.yan@intel.com changed reviewers: + ningxin.hu@intel.com
nhu@ : Hi, Ningxin, could you also helped on reviewing this ? :)
Just provide some inputs for your reference. Please wait for billorr@ and bajones@ for formal review. Thanks! https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... File device/vr/openvr/openvr_device.cc (right): https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... device/vr/openvr/openvr_device.cc:288: is_changed = true; If there is the only one place to trigger Changed event, can we call VRDevice::OnChanged right here? https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... File device/vr/openvr/openvr_device.h (right): https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... device/vr/openvr/openvr_device.h:21: enum VREvent { CONNECTED = 0, DISCONNECTED = 1, CHANGED = 2 }; No need to define general vr events in OpenVRDevice implementation, define them or re-use exiting ones in VRDevice. https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... device/vr/openvr/openvr_device.h:44: void OnPollingEvent(OpenVRDevice::VREvent event); VRDevice already defines OnXXX functions for event handling (See https://cs.chromium.org/chromium/src/device/vr/vr_device.h?type=cs&l=53). Can we use them instead of introducing this new "OnPollingEvent"? https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... device/vr/openvr/openvr_device.h:53: const base::Callback<void(OpenVRDevice::VREvent)>& onPollingVREvent); Can we use a VRDevice reference to OpenVRDevice instance? Then OpenVRRenderLoop is able to call OnXXX event handlers of VRDevice to notify. https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... device/vr/openvr/openvr_device.h:62: void PollEvent(); naming: PollEvent -> PollEvents?
Ningxin@: I've explain some of the design, pls review them :) https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... File device/vr/openvr/openvr_device.cc (right): https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... device/vr/openvr/openvr_device.cc:194: OnChanged(); I just find that this will cause vr_system created several times based on current logic. I'll update it but I questioned that vr_system is reasonable to be held in either OpenVRRenderLoop or OpenVRDevice(maybe a utility class is better?) https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... device/vr/openvr/openvr_device.cc:288: is_changed = true; On 2017/04/24 01:47:06, nhu wrote: > If there is the only one place to trigger Changed event, can we call > VRDevice::OnChanged right here? I introduce this bool variable here for the situation that OnChanged may be called several times in one rAF call. Since each call of OnChanged will cause vrdisplayInfo updated (fetch new data, transfer data and update on blink side), I think combine them and call OnChanged only once per rAF is a performance optimization. Pls correct me if any mistake :) https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... File device/vr/openvr/openvr_device.h (right): https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... device/vr/openvr/openvr_device.h:44: void OnPollingEvent(OpenVRDevice::VREvent event); On 2017/04/24 01:47:06, nhu wrote: > VRDevice already defines OnXXX functions for event handling (See > https://cs.chromium.org/chromium/src/device/vr/vr_device.h?type=cs&l=53). > > Can we use them instead of introducing this new "OnPollingEvent"? Due to the current class relationship, I think OpenVRRenderLoop cannot access OpenVRDevice non-static function directly(Am I right?). So if I re-use these functions I need to either transfer an OpenVRDeivce instance(which I think will break the access control) or register a set of callbacks. Pls correct me if I'm wrong :) https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... device/vr/openvr/openvr_device.h:53: const base::Callback<void(OpenVRDevice::VREvent)>& onPollingVREvent); On 2017/04/24 01:47:06, nhu wrote: > Can we use a VRDevice reference to OpenVRDevice instance? Then OpenVRRenderLoop > is able to call OnXXX event handlers of VRDevice to notify. Yes, I think this is the base question need to discsussion. All of the patch design is based on avoiding OpenVRRenderLoop instance and OpenVRDevice holding each other. If they hold each other the life-cycle management is a little bit complex(maybe :) ). And If OpenVRRenderLoop need VRDevice to do more jobs I think it indeed could hold VRDevice reference. but if only need event handling I prefer a callback :) https://codereview.chromium.org/2834843002/diff/1/device/vr/openvr/openvr_dev... device/vr/openvr/openvr_device.h:62: void PollEvent(); On 2017/04/24 01:47:06, nhu wrote: > naming: PollEvent -> PollEvents? That's a better name !
Fix multi initial vr_system issue by adding GetVRSystem in OpenVRRenderLoop. However, I think a standalone utility class for IVRSystem management is needed in future. Ningxin@: Pls take another look at it :) thx! billorr@ and bajones@ : if any problem in design, pls correct me. thx !
https://codereview.chromium.org/2834843002/diff/20001/device/vr/openvr/openvr... File device/vr/openvr/openvr_device.cc (right): https://codereview.chromium.org/2834843002/diff/20001/device/vr/openvr/openvr... device/vr/openvr/openvr_device.cc:89: auto vr_system = In https://codereview.chromium.org/2825203004/, we consolidated the VR_Init to OpenVRDeviceProvider to avoid multiple initializations. Will you follow that approach? billorr@, any thoughts?
Modify patch based on nhu@ suggestion. Ningxin@ : Pls take another look at patch set 4, thx ! billorr@ : Would you like to offer some suggestions? :) thx !
On 2017/04/26 04:18:31, shaobo.yan wrote: > Modify patch based on nhu@ suggestion. > Ningxin@ : Pls take another look at patch set 4, thx ! Thanks for the update! The VR_init change looks good. One tip, according to https://www.chromium.org/developers/contributing-code, it would be easy for review by uploading the rebase as a dedicated patch set. The polling event callback design is still open to me, as mentioned in https://codereview.chromium.org/2834843002/#msg11. Looking forward to billorr@ and bajones@ review. Thanks.
https://codereview.chromium.org/2834843002/diff/60001/device/vr/openvr/openvr... File device/vr/openvr/openvr_device.cc (right): https://codereview.chromium.org/2834843002/diff/60001/device/vr/openvr/openvr... device/vr/openvr/openvr_device.cc:147: // careated and callback need to be registed. careated => created, I assume https://codereview.chromium.org/2834843002/diff/60001/device/vr/openvr/openvr... device/vr/openvr/openvr_device.cc:296: on_polling_events_.Run(); This does not appear to be thread safe to me, given that the OpenVRRenderLoop is in a different thread from the OpenVRDevice. Given that the OpenVRRenderLoop already has access to vr_system_, though, it seems like the cleanest thing to do would be to put OnPollingEvents on the OpenVRRenderLoop object as well and, if changes are detected, call OnChanged via a PostMessage.
mthiesse@chromium.org changed reviewers: + mthiesse@chromium.org
https://codereview.chromium.org/2834843002/diff/60001/device/vr/openvr/openvr... File device/vr/openvr/openvr_device.cc (right): https://codereview.chromium.org/2834843002/diff/60001/device/vr/openvr/openvr... device/vr/openvr/openvr_device.cc:296: on_polling_events_.Run(); On 2017/04/26 18:06:05, bajones wrote: > This does not appear to be thread safe to me, given that the OpenVRRenderLoop is > in a different thread from the OpenVRDevice. > > Given that the OpenVRRenderLoop already has access to vr_system_, though, it > seems like the cleanest thing to do would be to put OnPollingEvents on the > OpenVRRenderLoop object as well and, if changes are detected, call OnChanged via > a PostMessage. Alternatively you can post to an anonymous-namespace function that just runs the callback on the thread you post to. See RunVRDisplayInfoCallback in vr_shell_gl.cc for an example.
On 2017/04/26 18:06:05, bajones wrote: > https://codereview.chromium.org/2834843002/diff/60001/device/vr/openvr/openvr... > File device/vr/openvr/openvr_device.cc (right): > > https://codereview.chromium.org/2834843002/diff/60001/device/vr/openvr/openvr... > device/vr/openvr/openvr_device.cc:147: // careated and callback need to be > registed. > careated => created, I assume > > https://codereview.chromium.org/2834843002/diff/60001/device/vr/openvr/openvr... > device/vr/openvr/openvr_device.cc:296: on_polling_events_.Run(); > This does not appear to be thread safe to me, given that the OpenVRRenderLoop is > in a different thread from the OpenVRDevice. > > Given that the OpenVRRenderLoop already has access to vr_system_, though, it > seems like the cleanest thing to do would be to put OnPollingEvents on the > OpenVRRenderLoop object as well and, if changes are detected, call OnChanged via > a PostMessage. bajones@ : Thx for reviewing! Patch Set 5 fix typo :P I'd like to explain Patch Set 6 and my worries at little. Since OnPollingEvents uses while loop to poll events(and poll events until no events), I worry that if call this in GetVSync synchronizally, it may slow down VSync. So I'd like to use RunPollingEventsCallback and postTask to solve thread safety issue. Pls take another look at Patch Set 6, thx a lot !
On 2017/04/26 18:14:16, mthiesse wrote: > https://codereview.chromium.org/2834843002/diff/60001/device/vr/openvr/openvr... > File device/vr/openvr/openvr_device.cc (right): > > https://codereview.chromium.org/2834843002/diff/60001/device/vr/openvr/openvr... > device/vr/openvr/openvr_device.cc:296: on_polling_events_.Run(); > On 2017/04/26 18:06:05, bajones wrote: > > This does not appear to be thread safe to me, given that the OpenVRRenderLoop > is > > in a different thread from the OpenVRDevice. > > > > Given that the OpenVRRenderLoop already has access to vr_system_, though, it > > seems like the cleanest thing to do would be to put OnPollingEvents on the > > OpenVRRenderLoop object as well and, if changes are detected, call OnChanged > via > > a PostMessage. > > Alternatively you can post to an anonymous-namespace function that just runs the > callback on the thread you post to. See RunVRDisplayInfoCallback in > vr_shell_gl.cc for an example. mthiesse@: Thx for your reviewing ! Patch Set 5 fix typo :) Patch Set 6 using RunPollingEventsCallback and postTasks to resolve thread safety( refer RunVRDisplayInfoCallback in vr_shell_gl.cc and thx for your info!) Pls take another look at Patch Set 6! thx a lot!
On 2017/04/26 07:48:30, nhu wrote: > On 2017/04/26 04:18:31, shaobo.yan wrote: > > Modify patch based on nhu@ suggestion. > > Ningxin@ : Pls take another look at patch set 4, thx ! > > Thanks for the update! The VR_init change looks good. > > One tip, according to https://www.chromium.org/developers/contributing-code, it > would be easy for review by uploading the rebase as a dedicated patch set. > > The polling event callback design is still open to me, as mentioned in > https://codereview.chromium.org/2834843002/#msg11. > > Looking forward to billorr@ and bajones@ review. Thanks. Thx for the tips ! I'll follow this. And based on bajones@ and mthiesse@ comments, I updated my patch. Pls take another look at Patch Set 6! thx a lot!
Patch Set 6 initialize main_thread_task_runner_ in OpenVRRenderLoop to enable postTask. The patch set could compile smooth and run simple test by modify Ipd and trigger OnChange() on blink side. mthiesse@ and bajones@ : Pls take another look at this patch set :) thx very much.
On 2017/05/02 02:00:34, shaobo.yan wrote: > Patch Set 6 initialize main_thread_task_runner_ in OpenVRRenderLoop to enable > postTask. > The patch set could compile smooth and run simple test by modify Ipd and trigger > OnChange() on blink side. > > mthiesse@ and bajones@ : Pls take another look at this patch set :) thx very > much. Sorry, it is Patch Set 7 :P
https://codereview.chromium.org/2834843002/diff/120001/device/vr/openvr/openv... File device/vr/openvr/openvr_device.cc (right): https://codereview.chromium.org/2834843002/diff/120001/device/vr/openvr/openv... device/vr/openvr/openvr_device.cc:75: void RunPollingEventsCallback(const base::Callback<void()>& callback) { Remove this. (See comment below) https://codereview.chromium.org/2834843002/diff/120001/device/vr/openvr/openv... device/vr/openvr/openvr_device.cc:304: FROM_HERE, base::Bind(&RunPollingEventsCallback, on_polling_events_)); Sorry, I gave you bad advice earlier. This should just be main_thread_task_runner_->PostTask(FROM_HERE, on_polling_events_); https://codereview.chromium.org/2834843002/diff/120001/device/vr/openvr/openv... File device/vr/openvr/openvr_device.h (right): https://codereview.chromium.org/2834843002/diff/120001/device/vr/openvr/openv... device/vr/openvr/openvr_device.h:50: void RegistPollingEventCallback( nit: RegisterPollingEventCallback https://codereview.chromium.org/2834843002/diff/120001/device/vr/openvr/openv... device/vr/openvr/openvr_device.h:53: void UnregistPollingEventCallback(); nit: UnregisterPollingEventCallback
On 2017/05/02 14:46:10, mthiesse wrote: > https://codereview.chromium.org/2834843002/diff/120001/device/vr/openvr/openv... > File device/vr/openvr/openvr_device.cc (right): > > https://codereview.chromium.org/2834843002/diff/120001/device/vr/openvr/openv... > device/vr/openvr/openvr_device.cc:75: void RunPollingEventsCallback(const > base::Callback<void()>& callback) { > Remove this. (See comment below) > > https://codereview.chromium.org/2834843002/diff/120001/device/vr/openvr/openv... > device/vr/openvr/openvr_device.cc:304: FROM_HERE, > base::Bind(&RunPollingEventsCallback, on_polling_events_)); > Sorry, I gave you bad advice earlier. This should just be > main_thread_task_runner_->PostTask(FROM_HERE, on_polling_events_); > > https://codereview.chromium.org/2834843002/diff/120001/device/vr/openvr/openv... > File device/vr/openvr/openvr_device.h (right): > > https://codereview.chromium.org/2834843002/diff/120001/device/vr/openvr/openv... > device/vr/openvr/openvr_device.h:50: void RegistPollingEventCallback( > nit: RegisterPollingEventCallback > > https://codereview.chromium.org/2834843002/diff/120001/device/vr/openvr/openv... > device/vr/openvr/openvr_device.h:53: void UnregistPollingEventCallback(); > nit: UnregisterPollingEventCallback Thx for reviewing ! I think this is a good chance for me to learn chromium thread mechanism and terms related to it. Patch Set 8 has corrected function name and using postTask as the suggested way :) mthiesse@ : Pls take another look at it! thx! BR, Yan,Shaobo
lgtm
The CQ bit was checked by shaobo.yan@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thx for mthiesse@ reviewing! bajones@ : Could you pls take another look at this? thx!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping bajones@ softly. Could you pls take another look at Patch Set 8 ? thx :)
bajones@: Softly ping :)
Ping bajones@ softly
softly ping bajones@ :)
billorr@google.com changed reviewers: + billorr@google.com
LGTM. A few nitpicking comments, mostly about comment wording. bajones won't be able to respond for a few more days, but mthiesse is also an owner of these files so you don't need to wait. https://codereview.chromium.org/2834843002/diff/140001/device/vr/openvr/openv... File device/vr/openvr/openvr_device.cc (right): https://codereview.chromium.org/2834843002/diff/140001/device/vr/openvr/openv... device/vr/openvr/openvr_device.cc:80: : vr_system_(vr), weak_ptr_factory_(this), is_polling_events_(false) {} What is is_polling_events_ used for? I never see it set to anything other than the default value. https://codereview.chromium.org/2834843002/diff/140001/device/vr/openvr/openv... device/vr/openvr/openvr_device.cc:146: // If it is first initialization, OpenVRRenderLoop instance need to be nit: If it is the first initialization, OpenVRRenderLoop instance needs to be created and the polling event callback needs to be registered. https://codereview.chromium.org/2834843002/diff/140001/device/vr/openvr/openv... device/vr/openvr/openvr_device.cc:247: // Only deal with event which will cause displayInfo changed now. Only deal with events that will cause displayInfo changes for now. https://codereview.chromium.org/2834843002/diff/140001/device/vr/openvr/openv... device/vr/openvr/openvr_device.cc:279: // Register a callback function to deal with system event. nit: system events. https://codereview.chromium.org/2834843002/diff/140001/device/vr/openvr/openv... device/vr/openvr/openvr_device.cc:297: // VSync could be used as a signal to poll event. Vsync could be used as a signal to poll events.
Thx billorr1@ for reviewing. Patch Set 9 has uploaded to address these comments. https://codereview.chromium.org/2834843002/diff/140001/device/vr/openvr/openv... File device/vr/openvr/openvr_device.cc (right): https://codereview.chromium.org/2834843002/diff/140001/device/vr/openvr/openv... device/vr/openvr/openvr_device.cc:80: : vr_system_(vr), weak_ptr_factory_(this), is_polling_events_(false) {} On 2017/05/09 15:50:03, billorr1 wrote: > What is is_polling_events_ used for? I never see it set to anything other than > the default value. Thx for this ! The usage of is_polling_events_ is for preventing multiple run of polling events through vr_system at same time. I've added it in latest patch set. https://codereview.chromium.org/2834843002/diff/140001/device/vr/openvr/openv... device/vr/openvr/openvr_device.cc:247: // Only deal with event which will cause displayInfo changed now. On 2017/05/09 15:50:02, billorr1 wrote: > Only deal with events that will cause displayInfo changes for now. Done. https://codereview.chromium.org/2834843002/diff/140001/device/vr/openvr/openv... device/vr/openvr/openvr_device.cc:279: // Register a callback function to deal with system event. On 2017/05/09 15:50:03, billorr1 wrote: > nit: system events. Done. https://codereview.chromium.org/2834843002/diff/140001/device/vr/openvr/openv... device/vr/openvr/openvr_device.cc:297: // VSync could be used as a signal to poll event. On 2017/05/09 15:50:03, billorr1 wrote: > Vsync could be used as a signal to poll events. Done.
Hi, billorr1@ Since patch set 9 has some new modification which is not reviewed, Would you pls take another look at it? thx!
LGTM
The CQ bit was checked by shaobo.yan@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2834843002/#ps160001 (title: "Address billorr1@ comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494465194321120,
"parent_rev": "4e5beb03dc2aed6e11e4ea8acf8ce8e77e10c08e", "commit_rev":
"f548f57a4ec26eabaec6e466ee668aa29834b6c2"}
Message was sent while issue was closed.
Description was changed from ========== Add event listening function to openvr device This patch add event listening function to openvr device by integrate pollNextEvent function into OpenVRDevice::OpenVRRenderLoop class and regist a callback function from OpenVRDevice to response to events. Currently, it only deal with events which generated by openvr device setting changes. BUG=714302 ========== to ========== Add event listening function to openvr device This patch add event listening function to openvr device by integrate pollNextEvent function into OpenVRDevice::OpenVRRenderLoop class and regist a callback function from OpenVRDevice to response to events. Currently, it only deal with events which generated by openvr device setting changes. BUG=714302 Review-Url: https://codereview.chromium.org/2834843002 Cr-Commit-Position: refs/heads/master@{#470771} Committed: https://chromium.googlesource.com/chromium/src/+/f548f57a4ec26eabaec6e466ee66... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/f548f57a4ec26eabaec6e466ee66... |
