WebXR: add enforcement for opaque textures [chromium/src : main]
Piotr Bialecki (Gerrit)
Attention is currently required from: Brandon Jones.
Piotr Bialecki would like Brandon Jones to review this change.
WebXR: add enforcement for opaque textures
This should implement the limitations that are introduced by the concept
of opaque textures. For details, see:
https://www.w3.org/TR/webxrlayers-1/#xropaquetextures
Bug: 1104340
Change-Id: Ibae7457646aadea5cf26479ffb051eaedfad1294
---
M third_party/blink/renderer/modules/webgl/webgl2_rendering_context_base.cc
M third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.cc
M third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.h
M third_party/blink/renderer/modules/webgl/webgl_texture.h
M third_party/blink/renderer/modules/webgl/webgl_unowned_texture.h
5 files changed, 170 insertions(+), 14 deletions(-)
To view, visit change 3712481. To unsubscribe, or for help writing mail filters, visit settings.
Piotr Bialecki (Gerrit)
Attention is currently required from: Brandon Jones.
1 comment:
Patchset:
bajones@, PTAL when you have a moment. I intend the changes to only affect opaque textures - tests will likely have to happen in raw camera access API WPTs, unless you have some advice on how this could be tested now.
To view, visit change 3712481. To unsubscribe, or for help writing mail filters, visit settings.
Brandon Jones (Gerrit)
Attention is currently required from: Piotr Bialecki.
3 comments:
Patchset:
Logic looks good across the board, but I feel like the CL could be quite a bit more compact if we shifted where the validation was happening.
File third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.cc:
Patch Set #5, Line 2397:
} else if (tex->IsOpaqueTexture()) {Could this check be added to `ValidateTexture2DBinding` instead, with an arg that could be passed into that method that indicates if opaque textures are allowed or not? (Defaulted to false, possibly, since it seems like texture operations that allow opaque textures are the minority.)
Patch Set #5, Line 5417:
if (auto* tex = ValidateTexImageBinding(params); !tex) {`ValidateTexImageBinding` calls into `ValidateTexture2DBinding`, and every single usage of `ValidateTexImageBinding` also blocks opaque textures, so if the opaque check is added to `ValidateTexture2DBinding` then you'd get this behavior for free.
To view, visit change 3712481. To unsubscribe, or for help writing mail filters, visit settings.
Piotr Bialecki (Gerrit)
Attention is currently required from: Brandon Jones.
Patch set 8:Commit-Queue +1
2 comments:
File third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.cc:
Patch Set #5, Line 2397:
} else if (tex->IsOpaqueTexture()) {
Could this check be added to `ValidateTexture2DBinding` instead, with an arg that could be passed in […]
Added, but note that texStorage* calls we have did not have any validation done so we can't rely on any of the helpers (the validations would cause different errors being raised, which the conformance tests don't like).
Patch Set #5, Line 5417:
if (auto* tex = ValidateTexImageBinding(params); !tex) {
`ValidateTexImageBinding` calls into `ValidateTexture2DBinding`, and every single usage of `Validate […]
Done
To view, visit change 3712481. To unsubscribe, or for help writing mail filters, visit settings.
Brandon Jones (Gerrit)
Attention is currently required from: Piotr Bialecki.
Patch set 8:Code-Review +1
1 comment:
Patchset:
Thanks for making the update! This LGTM now, it's certainly a lot less noisy.
To view, visit change 3712481. To unsubscribe, or for help writing mail filters, visit settings.
Piotr Bialecki (Gerrit)
Attention is currently required from: Piotr Bialecki.
Patch set 8:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
WebXR: add enforcement for opaque textures
This should implement the limitations that are introduced by the concept
of opaque textures. For details, see:
https://www.w3.org/TR/webxrlayers-1/#xropaquetextures
Bug: 1104340
Change-Id: Ibae7457646aadea5cf26479ffb051eaedfad1294
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3712481
Reviewed-by: Brandon Jones <baj...@chromium.org>
Commit-Queue: Piotr Bialecki <bia...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1018755}
---
M third_party/blink/renderer/modules/webgl/webgl2_rendering_context_base.cc
M third_party/blink/renderer/modules/webgl/webgl2_rendering_context_base.h
M third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.cc
M third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.h
M third_party/blink/renderer/modules/webgl/webgl_texture.h
M third_party/blink/renderer/modules/webgl/webgl_unowned_texture.h
6 files changed, 109 insertions(+), 20 deletions(-)