Move layer support detection to the backend #1912

svillar · 2025-09-04T12:00:42Z

The check for support for composition layers must be done in the backend (OpenXR) which is the one with enough data to decide.

We were doing it the other way around, checking which devices did support them and forcing it in the Java side because we knew that would work on the native side.

It's much safer and less error prone to do it on the backend. This forces us to make some Java calls asynchronous because we don't know whether the backend has done its checks or not.

javifernandez · 2025-09-16T07:30:15Z

app/src/common/shared/com/igalia/wolvic/VRBrowserActivity.java

    }

+    private boolean supportsCompositionLayers() {
+        assert(mMaxCompositionLayers.isPresent());


Why the assert ? I mean, if it needs to exists always, why then we need an optional at all ? What's the difference, in terms of business logic, between absence and 0 ?

Because not having a value means that we haven't received the message from the native code. Whereas 0 is a value that an runtime might return. Very likely no runtime will return that because that would mean that it is unusable but from the API point of view, 0 is a "valid" value.

Yeah, what I meant is what does it mean that "we haven´t received the message from the native code" ? Is that an stable state, or is part of the Wolvic's initialization and we just want to avoid blocking the app while waiting ? I want to understand why we haven't received it yet and when it's supposed to be received.

Exactly, I wanted to avoid blocking the main java thread waiting for the native code to reply. Perhaps we can do it synchronously and it is not a big deal after that because that should be fast.

javifernandez · 2025-09-16T07:32:02Z

app/src/common/shared/com/igalia/wolvic/VRBrowserActivity.java

+    }
+
+    @Override
+    public void areCompositionLayersSupported(@NonNull CompositionLayersCallback callback) {


I'm confused about this method and supportsCompositionLayers(). The method name suggests a check and a boolean return type, bit it seems it adds a new callback to the linked list.

Yes because it's an asynchronous operation. Do you want me to add the Async suffix to make it clearer?

umm, it'd help, indeed. Perhaps use the term "check" to suggest it'd would launch an operation, more than just inspecting some local fields. And the callback could have a name that suggests that it's called when the checks are done.

Makes sense. Will do that

javifernandez · 2025-09-16T08:09:38Z

app/src/common/shared/com/igalia/wolvic/VRBrowserActivity.java

+        mMaxCompositionLayers = OptionalInt.of(aNumLayers);
+        Log.i(LOGTAG, "Max composition layers: " + aNumLayers + " so " + (supportsCompositionLayers() ? "enabling" : "disabling") + " layers support");
+        runOnUiThread(() -> {
+            boolean supportsLayers = supportsCompositionLayers();


Has the supportCompositionLayers() method need to run on the UI thread ?

Not that one but the callbacks just bellow do

but then we may declare the boolean at the beginning and reuse it in the log message, can't we ?

Ah OK, the problem was calling it twice. Yeah I can change that.

javifernandez · 2025-09-16T08:19:08Z

app/src/common/shared/com/igalia/wolvic/ui/widgets/menus/HamburgerMenuWidget.java

-        mWidgetPlacement.proxifyLayer = mProxify;
-        super.show(aShowFlags);
+    private void internalShow(boolean proxifyLayer) {
+        mWidgetPlacement.proxifyLayer = proxifyLayer;


I'm not sure how to review this change. In proxifyLayerIfNeeded we add a callback to the mWidgetsManager where the mWidgetPlacement's proxifyLayer field is modified as well. Apparently we do the same here. Are these logics related ? If they are, could you explain how ?

Not sure whether I follow your comment. The code in internalShow is the code that was in the old show method. It's just that the check for layers is asynchronous so we might have to run this asynchronously,

javifernandez · 2025-09-16T08:20:10Z

app/src/main/cpp/DeviceDelegate.h

  virtual bool PopulateTrackedKeyboardInfo(TrackedKeyboardInfo& keyboardInfo) { return false; };
  virtual void SetHandTrackingEnabled(bool value) {};
  virtual float GetSelectThreshold(int32_t controllerIndex) { return 1.f; };
+  virtual unsigned MaxCompositionLayers() const { return 1; }


Why are we returning "1" unconditionally ?

Because that's the minimum. If you have 0 composition layers then you cannot render anything

Ok, I understand.

javifernandez · 2025-09-16T08:21:42Z

app/src/openxr/cpp/DeviceDelegateOpenXR.cpp

    CHECK_XRCMD(xrGetSystemProperties(instance, system, &systemProperties))
    VRB_LOG("OpenXR system name: %s", systemProperties.systemName);

+    layersEnabled = systemProperties.graphicsProperties.maxLayerCount > 1 && OpenXRExtensions::IsExtensionSupported(XR_KHR_ANDROID_SURFACE_SWAPCHAIN_EXTENSION_NAME);


Wouldn't me more efficient to check first the OpenXRExtension bit ?

the OpenXR extension check is actually a hash lookup so the comparison is more efficient.

javifernandez

I have left some comments but the changes looks good overall.

The check for support for composition layers must be done in the backend (OpenXR) which is the one with enough data to decide. We were doing it the other way around, checking which devices did support them and forcing it in the Java side because we knew that would work on the native side. It's much safer and less error prone to do it on the backend. This forces us to make some Java calls asynchronous because we don't know whether the backend has done its checks or not.

svillar · 2025-09-16T14:42:35Z

I have left some comments but the changes looks good overall.

Thanks! Added the changes following your suggestions

svillar requested review from felipeerias and javifernandez September 4, 2025 12:00

svillar mentioned this pull request Sep 4, 2025

Fix layer enabling condition #1911

Closed

javifernandez reviewed Sep 16, 2025

View reviewed changes

javifernandez approved these changes Sep 16, 2025

View reviewed changes

svillar force-pushed the layers_enabled branch from 386a660 to 559ee2f Compare September 16, 2025 14:42

svillar merged commit 7542ccf into main Sep 16, 2025
22 checks passed

svillar deleted the layers_enabled branch September 16, 2025 15:04

Uh oh!

Move layer support detection to the backend #1912

Move layer support detection to the backend #1912

Uh oh!

Conversation

svillar commented Sep 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javifernandez left a comment

Choose a reason for hiding this comment

Uh oh!

svillar commented Sep 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants