Bug 212099 - [WebXR] Implement XRSession::requestAnimationFrame()
Summary: [WebXR] Implement XRSession::requestAnimationFrame()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on:
Blocks: 208988
  Show dependency treegraph
 
Reported: 2020-05-19 12:04 PDT by Sergio Villar Senin
Modified: 2020-05-27 02:37 PDT (History)
10 users (show)

See Also:


Attachments
Patch (15.62 KB, patch)
2020-05-19 12:33 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (15.68 KB, patch)
2020-05-26 09:45 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing (17.11 KB, patch)
2020-05-27 01:23 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2020-05-19 12:04:47 PDT
[WebXR] Implement XRSession::requestAnimationFrame()
Comment 1 Sergio Villar Senin 2020-05-19 12:33:52 PDT
Created attachment 399760 [details]
Patch
Comment 2 Sergio Villar Senin 2020-05-22 08:52:44 PDT
Gentle ping
Comment 3 youenn fablet 2020-05-25 01:47:37 PDT
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.
Comment 4 Sergio Villar Senin 2020-05-25 12:35:53 PDT
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.
Comment 5 youenn fablet 2020-05-26 03:29:44 PDT
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.
Comment 6 Sergio Villar Senin 2020-05-26 09:45:22 PDT
Created attachment 400255 [details]
Patch

Added ASSERTs
Comment 7 Sergio Villar Senin 2020-05-26 09:53:01 PDT
(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.
Comment 8 Sergio Villar Senin 2020-05-26 11:16:20 PDT
(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).
Comment 9 Sergio Villar Senin 2020-05-27 01:23:17 PDT
Created attachment 400315 [details]
Patch for landing
Comment 10 Sergio Villar Senin 2020-05-27 02:36:56 PDT
Committed r262188: <https://trac.webkit.org/changeset/262188>
Comment 11 Radar WebKit Bug Importer 2020-05-27 02:37:16 PDT
<rdar://problem/63667956>