Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for rui16 and allow empty preference #49

Merged
merged 3 commits into from
May 31, 2024

Conversation

cabanier
Copy link
Member

@cabanier cabanier commented May 28, 2024

  • as discussed in today's meeting, I added support for an empty array to indicate that the author has no preference and wants the optimal preferences for the current platform
  • add another format option which matches Quest browser's implementation

@bialpio , should I define the definitions of the user agent's preferred format and usage?
Also, luminance-alpha and unsigned-int are very similar. Is there a reason you chose luminance-alpha?


Preview | Diff

@Maksims
Copy link

Maksims commented May 29, 2024

Just a small note, but in the rendering world usually the precision of data is defined in bits, not bytes. E.g. R32F - 32 bits single channel.
LA8 - two channel 8 bits per each.

index.bs Outdated Show resolved Hide resolved
@alcooper91
Copy link

Algorithm change seems good to me, with one NIT to consider; I'll defer to Piotr on the new enum naming.

@cabanier cabanier requested a review from alcooper91 May 30, 2024 14:59
Copy link

@alcooper91 alcooper91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd prefer Brandon/Piotr to weigh in on the enum naming.

@bialpio
Copy link
Contributor

bialpio commented May 30, 2024

@bialpio , should I define the definitions of the user agent's preferred format and usage?

It'd be nice to have some prose around this - mentioning that UA can have a preference for the format in the native device concepts section should be sufficient.

Also, luminance-alpha and unsigned-int are very similar. Is there a reason you chose luminance-alpha?

I seem to recall that luminance-alpha was the only 16bit format that is supported by WebGL1.

@cabanier
Copy link
Member Author

Also, luminance-alpha and unsigned-int are very similar. Is there a reason you chose luminance-alpha?

I seem to recall that luminance-alpha was the only 16bit format that is supported by WebGL1.

Do we still care about being compatible with WebGL1?

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd prefer Brandon/Piotr to weigh in on the enum naming.

Enum naming LGTM!

Do we still care about being compatible with WebGL1?

Luminance+Alpha has been part of this spec from the beginning, so I don't think we benefit from trying to remove it. That said, I'm not personally opposed to pursuing WebGL 2-only integrations in the future. WebGL 2 is effectively ubiquitous at this point, and certainly any devices that are attempting to use advanced WebXR functionality will have access to it.

@cabanier
Copy link
Member Author

@bialpio , should I define the definitions of the user agent's preferred format and usage?

It'd be nice to have some prose around this - mentioning that UA can have a preference for the format in the native device concepts section should be sufficient.

I added definitions for preferred values.

@cabanier
Copy link
Member Author

@bialpio does this look reasonable? If not, can we submit the PR and iterate on the language a bit more? I'd like to update browser as soon as possible so we're compliant.
I already update three.js and the webxr samples and am about to submit changes to our browser so we can release it next week.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@bialpio
Copy link
Contributor

bialpio commented May 31, 2024

@bialpio does this look reasonable? If not, can we submit the PR and iterate on the language a bit more?

LGTM w/ nitpicks.

@cabanier cabanier merged commit dcb4d23 into immersive-web:main May 31, 2024
2 checks passed
@cabanier cabanier deleted the depth-changes branch May 31, 2024 17:08
github-actions bot added a commit that referenced this pull request May 31, 2024
SHA: dcb4d23
Reason: push, by cabanier

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Maksims
Copy link

Maksims commented Jul 2, 2024

I believe there is an issue with this change, that is related to lack of specifics in specs regarding to "unknown" or "not provided" options in dataFormatPreference.
When unknown string is provided in dataFormatPreference e.g.: [ 'float32', 'unsigned-short', 'luminance-alpha' ] in Chrome on Windows, then it fails due to "unsigned-short" - "not a valid enum value of XRDepthDataFormat".
But if [ 'float32', 'luminance-alpha' ] is provided on platform like Quest 3, then it fails with internal error, as it likely expects for all possible XRDepthDataFormat values to be present.

This creates a nasty situation in that we don't know what values are available on the platform (XRDepthDataFormat - is not exposed in implementations as an array), and we have to use conditional navigator.userAgent to provide different options based on the platform.

This could easily be avoided and improved for future options changes, by specifying rules of how "unknown" or "missing" values in dataFormatPreference are handled:

  1. If value is unknown (not in XRDepthDataFormat), then skip to next value.
  2. The list of provided values for dataFormatPreference might not include all the possible values.
@alcooper91
Copy link

This is a naturally enforced fallout of the fact that dataFormatPreference is an enum, so we currently have no choice but to enforce this if we don't recognize a value. FWIW, unsigned-short is added to Chrome for M127, which should release later this month.

Quest is throwing because I believe it only supports gpu-optimized unsigned-short. In specifying only the other two options (float32 and luminance-alpha), you're telling Quest that those are the only two that you can support, it sees that it can't provide you anything that you want and rejects the request.

A workaround added as of this spec change (though Chrome will likely still give an error, at least if it's a required feature until M127), is that you can specify simply an empty dataFormatPreference ([]) if you're willing to work with any format that you receive.

The reason that XRDepthDataFormat is not exposed as an array is because the exposed value is the format for the current session, which may be different from session to session.

@Maksims
Copy link

Maksims commented Jul 2, 2024

A workaround added as of this spec change (though Chrome will likely still give an error, at least if it's a required feature until M127), is that you can specify simply an empty dataFormatPreference ([]) if you're willing to work with any format that you receive.

Chrome currently does not support an empty array of usagePreference and dataFormatPreference, so it is not an option to use it atm. And taking in account existing platforms and browsers (people don't upgrade all at once), we will have to resort to conditional rules.

Quest is throwing because I believe it only supports gpu-optimized unsigned-short. In specifying only the other two options (float32 and luminance-alpha), you're telling Quest that those are the only two that you can support, it sees that it can't provide you anything that you want and rejects the request.

That is not what I get (Browser version 34.0.0.121.29.615295309, OS version: 67.0.0.504.356.616599631). XRSession reports that
depthDataFormat is "float32", and not "unsigned-short". Regardless of dataFormatPreference array provided during init. While it fails to start if it is not an empty array and without "unsigned-short". Also we did not implement "unsigned-short" path, and it is still working as float32 correctly.

@cabanier
Copy link
Member Author

cabanier commented Jul 2, 2024

Quest is throwing because I believe it only supports gpu-optimized unsigned-short. In specifying only the other two options (float32 and luminance-alpha), you're telling Quest that those are the only two that you can support, it sees that it can't provide you anything that you want and rejects the request.

That is not what I get (Browser version 34.0.0.121.29.615295309, OS version: 67.0.0.504.356.616599631). XRSession reports that depthDataFormat is "float32", and not "unsigned-short". Regardless of dataFormatPreference array provided during init.

That's a bug in Quest browser. It will be fixed in 34.2

@alcooper91
Copy link

alcooper91 commented Jul 2, 2024

Chrome currently does not support an empty array of usagePreference and dataFormatPreference, so it is not an option to use it atm.

If it's under optionalFeatures does it work? Or not at all?

Edit: Oops, missed his reply.

@Maksims
Copy link

Maksims commented Jul 2, 2024

If it's under optionalFeatures does it work? Or not at all?

Tested more cases, and apparently there are many implementation inconsistencies/issues:

  1. In Chrome, if optionalFeatures contains depth-sensing, usagePreference is empty array and dataFormatPreference is empty array, session will start, but will throw an exception when accessing XRSession.depthUsage with error: Depth sensing feature is not supported by the session.. But the problem is that session.enabledFeatures contains depth-sensing. So it should not contain depth-sensing, and provide some warning details of why it failed to enable that feature.
  2. In Oculus, if optionalFeatures contains depth-sensing, dataFormatPreference does not contain 'unsigned-short', then it will fail to start a session with error mentioned before. Same story here: it should start and provide a warning details of why a specific configuration is not supported.
@cabanier
Copy link
Member Author

cabanier commented Jul 2, 2024

  1. In Oculus, if optionalFeatures contains depth-sensing, dataFormatPreference does not contain 'unsigned-short', then it will fail to start a session with error mentioned before. Same story here: it should start and provide a warning details of why a specific configuration is not supported.

I just tried this by passing an empty dataFormatPreference in https://github.com/immersive-web/webxr-samples/blob/main/layers-samples/proj-multiview-occlusion.html#L161 and the session started with no errors.

@Maksims
Copy link

Maksims commented Jul 2, 2024

  1. In Oculus, if optionalFeatures contains depth-sensing, dataFormatPreference does not contain 'unsigned-short', then it will fail to start a session with error mentioned before. Same story here: it should start and provide a warning details of why a specific configuration is not supported.

I just tried this by passing an empty dataFormatPreference in https://github.com/immersive-web/webxr-samples/blob/main/layers-samples/proj-multiview-occlusion.html#L161 and the session started with no errors.

Yes, an empty dataFormatPreference works well in Oculus, but a list with other options and without unsigned-short (e.g. [ 'float32', 'luminance-alpha' ]) will fail to start a session even if requested as optionalFeatures.

@alcooper91
Copy link

In Chrome, if optionalFeatures contains depth-sensing, usagePreference is empty array and dataFormatPreference is empty array, session will start, but will throw an exception when accessing XRSession.depthUsage with error: Depth sensing feature is not supported by the session.. But the problem is that session.enabledFeatures contains depth-sensing. So it should not contain depth-sensing, and provide some warning details of why it failed to enable that feature.

Okay, that's a bug that the feature is still listed as enabled there. Can you test on Beta with depth under optionalFeatures and specify only float32 to dataFormatPreference (empty array for usagePreference is fine)? That should grant the session but reject the feature, if Depth is still listed under enabledFeatures let me know and I'll look into that bug.

Yes, an empty dataFormatPreference works well in Oculus, but a list with other options and without unsigned-short (e.g. [ 'float32', 'luminance-alpha' ]) will fail to start a session even if requested as optionalFeatures.

IIUC, this is a known bug fixed in Oculus 34.2

@alcooper91
Copy link

Okay, that's a bug that the feature is still listed as enabled there. Can you test on Beta with depth under optionalFeatures and specify only float32 to dataFormatPreference (empty array for usagePreference is fine)? That should grant the session but reject the feature, if Depth is still listed under enabledFeatures let me know and I'll look into that bug.

Test locally and seems to be fixed. I think the fix went into M127.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants