Why does synced_frame_data take a &self rather than a &mut self?

[asajeffrey]

When servo uses a VRDisplay it has &mut access to it, so we could make synced_frame_data and friends take &mut self arguments rather than &self. This would remove some unnecessary uses of interior mutability.

[asajeffrey]

There is a clash between synced_frame_data taking a &self argument (and therefore needing interior mutability) and the requirement that VRDisplay: Sync. The obvious thing to do is use a Cell or RefCell but then:

error[E0277]: `std::cell::RefCell<std::boxed::Box<std::option::Option<rust_webvr_api::VRFrameData>>>` cannot be shared between threads safely
  --> /Users/ajeffrey/github/asajeffrey/rust-webvr/rust-webvr/src/api/magicleap/display.rs:55:6
   |
55 | impl VRDisplay for MagicLeapVRDisplay {
   |      ^^^^^^^^^ `std::cell::RefCell<std::boxed::Box<std::option::Option<rust_webvr_api::VRFrameData>>>` cannot be shared between threads safely
   |
   = help: within `api::magicleap::display::MagicLeapVRDisplay`, the trait `std::marker::Sync` is not implemented for `std::cell::RefCell<std::boxed::Box<std::option::Option<rust_webvr_api::VRFrameData>>>`
   = note: required because it appears within the type `api::magicleap::display::MagicLeapVRDisplay`

As far as I can tell, the only option is to use a mutex, so this API makes it difficult to provide a lock-free implementation.

[asajeffrey]

Sigh, or do what the Oculus implementation does:

unsafe impl Send for OculusVRDisplay {}
unsafe impl Sync for OculusVRDisplay {}

[MortimerGoro]

I assumed that synced_frame_data doesn't need to modify anything and was always going to do readonly/inmutable operations internally so I used &self.

The idea was that sync_poses is the one saving the state and synced_frame_data just returns the cached information with some readonly live calculations taking into account parameters that could change after sync_poses (e.g. near_z and far_z)

[asajeffrey]

Hmm, the problem with that is that sync_poses doesn't get near and far. In order to get the projection matrices from the device, the VRDisplay implementation has to send near and far to the device and wait for the projection matrices to come back. Is the assumption that this communication can be done with just read-only access?

In fact, this raises a related question, which is why sync_poses and synced_frame_data are separate functions. They're called right next to each other in Servo: https://github.com/servo/servo/blob/7ed6b9d3ce81ff562344cd9f683be224080de9c7/components/webvr/webvr_thread.rs#L392-L393

[MortimerGoro]

They are separate methods mostly to:

• provide a getter if you want to get the data again without syncing poses again
• near and far could change between the interval between sync_poses is dispatched and JavaScript calls getFrameData()

It's true that in practice we have not ended up using those scenarios so I'm ok with unifying the methods and simplifying the API.

VRDisplay implementation has to send nearandfar to the device and wait for the projection matrices to come back. Is the assumption that this communication can be done with just read-only access?

Yes, it's a read-only operation on all the SDKs implemented at the time

[asajeffrey]

@MortimerGoro Is the reason why I'm hitting this for the ML1 (and it's not come up for the other implementations) the ML1's requirements about which methods run on the main thread?

How bad would it be if the FrameData was based on the near and far values from the previous rAF? If a user writes code like:

    display.near = 1;
    display.far = 10;
    display.getFrameData(frame_data);

Is the frame_data values expected to reflect the new values for near and far, or is it OK if they don't get reflected until the next time round the rAF loop?

Asking because if frame_data has to reflect the current values, then extra synchronization is required.

[MortimerGoro]

@asajeffrey Yes, I think we are good if we avoid the requirement of using the latest near and far values from the current RAF. For 2 reasons:

• Normally those values are only set once by the WebXR applications and there are not more changes during the whole sessions.
• The new spec is only going to allow to set those values on a XRSession initialization, and not dynamically at any time (https://immersive-web.github.io/webxr/#dictdef-xrrenderstateinit)

[asajeffrey]

OK, in which case should the API for the setters and getters for near and far be split off from the API for getting the frame data?

[MortimerGoro]

OK, in which case should the API for the setters and getters for near and far be split off from the API for getting the frame data?

We could do that. Normally near and far are set once before starting a WebVR session