immersive-web / depth-sensing

Specification: https://immersive-web.github.io/depth-sensing/ Explainer: https://github.com/immersive-web/depth-sensing/blob/main/explainer.md
Other
52 stars 15 forks source link

Add support for rui16 and allow empty preference #49

Closed cabanier closed 1 month ago

cabanier commented 1 month ago

@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 commented 1 month ago

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.

alcooper91 commented 1 month ago

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

bialpio commented 1 month ago

@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 commented 1 month ago

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?

cabanier commented 1 month ago

@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 commented 1 month ago

@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.

bialpio commented 1 month ago

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

LGTM w/ nitpicks.

Maksims commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago
  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 commented 5 days ago
  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 commented 5 days ago

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 commented 5 days ago

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.