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

[bugzilla-daemon at webkit.org]

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

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

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

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:505
>> +            auto callbacks = m_callbacks;
> 
> Can we do:
> auto callbacks = WTFMove(m_callbacks).

We can't because m_callbacks is used in cancelAnimationFrame() in order to set up the fired or cancelled flag. So we need a copy here.

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:508
>> +            frame->setActive(true);
> 
> We went from a model where we were reusing the same frame over and over to creating a new frame each time.
> This is observable from JS so we should add a test for it.
> And verify other browsers are doing the same.

Chromium is also creating a new XRFrame each time. Gecko is using a pool of XRFrames. I'll create a follow-up issue to track the GC impact of all the potentially short living objects. I know that V8 handles it well because of their three generation GC. SpiderMonkey didn't handle it as well, so it had to rely on object pooling in general. We can do some profiles in WebKit, but based on my initial tests running some WebXR samples locally it does a good job. I tested it on desktop though, I'll test it on mobile OpenXR when possible.

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:527
>> +            });
> 
> Not needed if we can clear m_callbacks above.

We can't clear m_callbacks above

>> Source/WebCore/Modules/webxr/WebXRSystem.h:118
>> +        ScriptExecutionContext* m_scriptExecutionContext { nullptr };
> 
> This is probably incorrect. Let's make it a ContextDesutrctionObserver.

done

>> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:205
>> +    return WTF::switchOn(m_context, [](const RefPtr<WebGLRenderingContextBase>& baseContext) {
> 
> What about #if ENABLE(WEBGL2)?

What's the problem with WebGL2? we are covering both WebGL1 and WebGL2 with the WebGLRenderingContextBase base class

-- 
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/20210211/2cb0ee1a/attachment-0001.htm>