Bug 212099 – [WebXR] Implement XRSession::requestAnimationFrame()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[WebXR] Implement XRSession::requestAnimationFrame()

[Sergio Villar Senin]

Created attachment 399760 [details]
Patch

[Sergio Villar Senin]

Gentle ping

[youenn fablet]

Comment on attachment 399760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399760&action=review

> Source/WebCore/Modules/webxr/WebXRSession.cpp:124
> +int WebXRSession::requestAnimationFrame(Ref<XRFrameRequestCallback>&& callback)

Why do we return a signed integer, should we return an unsigned?

> Source/WebCore/Modules/webxr/WebXRSession.cpp:135
> +    scheduleAnimation();

scheduleAnimation() is only called there right now. In the future, it will be called more?
How is it supposed to be hooked, a repeat timer?

> Source/WebCore/Modules/webxr/XRFrameRequestCallback.h:44
> +    int callbackId() { return m_id; }

const

> Source/WebCore/Modules/webxr/XRFrameRequestCallback.h:45
> +    void setCallbackId(int id) { m_id = id; }

I guess we should assert that m_id is zero if we initialise it to zero.

[Sergio Villar Senin]

Comment on attachment 399760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399760&action=review

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:124
>> +int WebXRSession::requestAnimationFrame(Ref<XRFrameRequestCallback>&& callback)
> 
> Why do we return a signed integer, should we return an unsigned?

I thought the same but the IDL specifies a  "long" which is a signed integer. I might raise an issue to the WG because I don't think it makes much sense either.

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:135
>> +    scheduleAnimation();
> 
> scheduleAnimation() is only called there right now. In the future, it will be called more?
> How is it supposed to be hooked, a repeat timer?

Authors need to call requestAnimationFrame() in their callbacks to get the next frame.

Note that we aren't returning pose information yet. The retrieval of pose information from devices will be done independently of the existence of requests or not. We should generate XRFrame's as we get data from devices, and pass it to the author in the callbacks of requestAnimationFrame() if any.

>> Source/WebCore/Modules/webxr/XRFrameRequestCallback.h:44
>> +    int callbackId() { return m_id; }
> 
> const

OK.

>> Source/WebCore/Modules/webxr/XRFrameRequestCallback.h:45
>> +    void setCallbackId(int id) { m_id = id; }
> 
> I guess we should assert that m_id is zero if we initialise it to zero.

Good point although the thing is that id is a signed integer to in theory we could overflow positive integers and then reach 0 again after passing by all the negative ones.

[youenn fablet]

Comment on attachment 399760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399760&action=review

>>> Source/WebCore/Modules/webxr/WebXRSession.cpp:124
>>> +int WebXRSession::requestAnimationFrame(Ref<XRFrameRequestCallback>&& callback)
>> 
>> Why do we return a signed integer, should we return an unsigned?
> 
> I thought the same but the IDL specifies a  "long" which is a signed integer. I might raise an issue to the WG because I don't think it makes much sense either.

An issue sounds good.

>>> Source/WebCore/Modules/webxr/XRFrameRequestCallback.h:45
>>> +    void setCallbackId(int id) { m_id = id; }
>> 
>> I guess we should assert that m_id is zero if we initialise it to zero.
> 
> Good point although the thing is that id is a signed integer to in theory we could overflow positive integers and then reach 0 again after passing by all the negative ones.

Sure, but still that might capture the issue of overwriting the id, which would not be great.
We should also probably add an assert to the getter that it is not zero.
Or maybe we should have a class taking a Ref<XRFrameRequestCallback> and an ID in constructor, plus a settable boolean m_cancelled.

[Sergio Villar Senin]

Created attachment 400255 [details]
Patch

Added ASSERTs

[Sergio Villar Senin]

(In reply to youenn fablet from comment #5)
> Comment on attachment 399760 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399760&action=review
> 
> >>> Source/WebCore/Modules/webxr/WebXRSession.cpp:124
> >>> +int WebXRSession::requestAnimationFrame(Ref<XRFrameRequestCallback>&& callback)
> >> 
> >> Why do we return a signed integer, should we return an unsigned?
> > 
> > I thought the same but the IDL specifies a  "long" which is a signed integer. I might raise an issue to the WG because I don't think it makes much sense either.
> 
> An issue sounds good.

JFTR https://github.com/immersive-web/webxr/pull/1062

It looks like it will be approved, it's sensible and it also matches window's rAF (consistency yay!). I'll likely replace it before landing or after the next review.

[Sergio Villar Senin]

(In reply to Sergio Villar Senin from comment #7)
> (In reply to youenn fablet from comment #5)
> > Comment on attachment 399760 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=399760&action=review
> > 
> > >>> Source/WebCore/Modules/webxr/WebXRSession.cpp:124
> > >>> +int WebXRSession::requestAnimationFrame(Ref<XRFrameRequestCallback>&& callback)
> > >> 
> > >> Why do we return a signed integer, should we return an unsigned?
> > > 
> > > I thought the same but the IDL specifies a  "long" which is a signed integer. I might raise an issue to the WG because I don't think it makes much sense either.
> > 
> > An issue sounds good.
> 
> JFTR https://github.com/immersive-web/webxr/pull/1062
> 
> It looks like it will be approved, it's sensible and it also matches
> window's rAF (consistency yay!). I'll likely replace it before landing or
> after the next review.

Oh I didn't notice that you had r+'ed the previous patch and uploaded a new version. I guess you're fine with me landing this one too. I'll wait a bit for the WG resolution though as I just need one more approval to land the spec change (already got one).

[Sergio Villar Senin]

Created attachment 400315 [details]
Patch for landing

[Sergio Villar Senin]

Committed r262188: <https://trac.webkit.org/changeset/262188>

[Radar WebKit Bug Importer]

<rdar://problem/63667956>