[Webkit-unassigned] [Bug 220979] Complete XRSession::requestAnimationFrame implementation

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Feb 3 10:57:00 PST 2021 

https://bugs.webkit.org/show_bug.cgi?id=220979

--- Comment #13 from Imanol Fernandez <ifernandez at igalia.com> ---
Comment on attachment 418924
  --> https://bugs.webkit.org/attachment.cgi?id=418924
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        - Apply pending render state changes
> 
> It would be nice to have some more info in the ChangeLog. Maybe even per-file notes, for areas where there was a lot of change.

I improved the changelog in the new patch

>>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:46
>>> +    , m_session(session)
>> 
>> WebXRFrame is refcounted so could potentially be kept alive longer than its session.
>> Might be better to keep a ref or use a weakptr.
> 
> Since you can access the session from the XRFrame, I think this need to be a ref. The session itself might get disconnected, but the object still needs to exist.

One of the problems was that session holds a Ref<XRFrame>, and XRFrame another Ref<XRSession> that could cause reference cycles.
The WebXR spec states that session from the XRFrame is always valid so I have added a Ref<XRSession> there, because it needs to keep the session alive. 
In XRSession I have changed Ref<XRFrame> to RefPtr<XRFrame>. This RefPtr is cleaned when the session is ended. I added m_ended bailouts and asserts to make sure that we never access a empty m_animationFrame.

>>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:55
>>> +    return *m_session;
>> 
>> How can we guarantee this?
> 
> +1
> 
> If the session can never be null, then we shouldn't be storing a pointer.

Not needed anymore after switching to Ref<XRSession>

>> Source/WebCore/Modules/webxr/WebXRFrame.h:60
>> +    WebXRFrame(WebXRSession*);
> 
> explicit

done

>> Source/WebCore/Modules/webxr/WebXRFrame.h:62
>> +    bool m_active;
> 
> add { false }

done

>> Source/WebCore/Modules/webxr/WebXRFrame.h:66
>> +    WebXRSession* m_session;
> 
> Why is this now a pointer rather than a Ref? When can a Frame be created without a Session?

Fixes as mentioned in prev comments

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:59
>> +    , m_animationFrame(WebXRFrame::create(this))
> 
> Seems like we could use *this there.

switched to Ref<>

>>> Source/WebCore/Modules/webxr/WebXRSession.cpp:208
>>> +    scriptExecutionContext()->postTask([this, promise = WTFMove(promise), type](auto& context) mutable {
>> 
>> There is no guarantee this will be valid within the callback.
> 
> weakThis = makeWeakPtr(this)
> then check the value in the callback

fixed

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:221
>> +        queueTaskKeepingObjectAlive(*this, TaskSource::WebXR, [this, type, promise = WTFMove(promise), protectedDocument = makeRef(downcast<Document>(context))]() mutable {
> 
> Why keeping a protected document here?
> I guess context is equal to this->scriptExecutionContext().
> Can we just use that?

done

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:248
>> +    if (m_callbacks.size() == 1 && !m_animationFrame->isActive())
> 
> Why does the number of callbacks need to equal 1? Shouldn't this just be a non-zero test?

I added some comments to make it clearer. Script can add many RAFs but whe only need to request/schedule the frame once. As the callback is being added before to the vector the size for the check is 1.

We could move m_callbacks.append() and use isEmpty() test but even if the frame runs on a next tick, it felt a bit weird to request a callback before all the data was ready (just thinking on the risk of a port calling the lambda on a sync way)

>>> Source/WebCore/Modules/webxr/WebXRSession.cpp:463
>>> +    m_device->requestFrame([this, protectedThis = makeRef(*this)](PlatformXR::Device::FrameData frameData)
>> 
>> s/PlatformXR::Device::FrameDat/auto/
> 
> Oh cool. I didn't know you could do that.

That's cool, nice C++14 feature :)

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:464
>> +    {
> 
> Should be at the end of the next line.

fixed

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:479
>> +        DOMHighResTimeStamp now = (MonotonicTime::now() - m_timeOrigin).milliseconds();
> 
> auto?

done

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:483
>> +        m_animationFrame->setTime(static_cast<DOMHighResTimeStamp>(frameData.predictedDisplayTime));
> 
> Is there a way to not have to do the static_cast.

The problem is that most SDKs provide the data using a long timestamp but JS doesn't support 64 bit integers. I think is better to make sure we have the most accurate data in the ports and only potentially lose precision in JS.

>>> Source/WebCore/Modules/webxr/WebXRSession.cpp:512
>>> +                callback->handleEvent(now, m_animationFrame.get());
>> 
>> Is there a chance the event will cancel everything (in which case we should stop everything), or will allow changing m_runningCallbacks synchronously?
> 
> The script here can do anything, but it looks like runningCallbacks is only used here, so it might be safe. But that makes me ask why it is exposed as a member variable rather than just a local variable in this block.

runningCallbacks is also accessed from cancelRequestAnimationFrame in order to cancel ongoing rafs. Only the `cancel` flag is modified so multiple vector item modifications are not possible.

>> Source/WebCore/Modules/webxr/WebXRSystem.cpp:475
>> +class InlineRequestAnimationFrameCallback: public RequestAnimationFrameCallback {
> 
> Should it be final?

done

>> Source/WebCore/Modules/webxr/WebXRSystem.cpp:482
>> +    InlineRequestAnimationFrameCallback(ScriptExecutionContext& scriptExecutionContext, Function<void()>&& callback)
> 
> Please move this to private.
> Otherwise people can create it outside of create.

done

>> Source/WebCore/Modules/webxr/WebXRSystem.cpp:487
>> +    CallbackResult<void> handleEvent(double highResTimeMs) override
> 
> final?
> Remove highResTimeMs

done

>> Source/WebCore/Modules/webxr/WebXRSystem.cpp:507
>> +    auto document = downcast<Document>(m_scriptExecutionContext);
> 
> What is the guarantee m_scriptExecutionContext will be a Document?
> Should constructor take a Document?
> What is the guarantee for m_scriptExecutionContext to be valid here?

It uses the same m_scriptExecutionContext from the WebXRSystem, which holds the DummyInlineDevice. The scriptExecutionContext is already downcasted to document in other places in (e.g. WebXRSystem::isSessionSupported)

I have added a make the access to it safer.

>> Source/WebCore/Modules/webxr/WebXRSystem.h:109
>> +        DummyInlineDevice(ScriptExecutionContext&);
> 
> explicit

done

>> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:205
>> +    HTMLCanvasElement* c = WTF::switchOn(m_context, [](const RefPtr<WebGLRenderingContextBase>& baseContext) {
> 
> return WTF::switchOn(m_context, [](const RefPtr<WebGLRenderingContextBase>& baseContext) -> HTMLCanvasElement* {
> maybe?

done

>> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:212
>> +            [](RefPtr<OffscreenCanvas>) {
> 
> const RefPtr<OffscreenCanvas>&?

done

>> Source/WebCore/platform/xr/PlatformXR.h:87
>> +                    float x, y, z, w;
> 
> Separate line for each of these.

done

>> Source/WebCore/platform/xr/PlatformXR.h:96
>> +    using RequestFrameCallback = WTF::Function<void(FrameData)>;
> 
> No need for WTF::
> Probably also want to pass a FrameData&&

done

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:435
>> +    callOnMainThread([this, weakThis = makeWeakPtr(this)]() {
> 
> Why is this necessary? Is this just causing the pending callbacks to be flushed? Wouldn't it be better to delete them directly?

OpenXR needs to wait for the XR_SESSION_STATE_STOPPING state to properly end the session. We were reusing the frame loop which already checked this event.

I agree that is confusing. I have done some refactor to improve the event handling and added a separate function that handle the shutdown process (which also uses the sessionDidEnd() notification now)

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:438
>> +        requestFrame([](FrameData) { });
> 
> s/FrameData/auto

not needed anymore

>> Source/WebCore/testing/WebFakeXRDevice.cpp:46
>> +    m_supportsOrientationTracking = true;
> 
> Why not:
>     , m_supportsOrientationTracking(true)
> Or even better in the header declaration directly.

in this case is a protected member from base class

>> Source/WebCore/testing/WebFakeXRDevice.cpp:76
>> +    runningCallbacks.swap(m_callbacks);
> 
> Could be written as:
> auto callbacks = WTFMove(m_callbacks)?

done

>> Source/WebCore/testing/WebFakeXRDevice.cpp:85
>> +        m_frameTimer.startOneShot(15_ms);
> 
> Might be good to use a constant

done

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210203/902ecc6c/attachment-0001.htm>

More information about the webkit-unassigned mailing list