Add WebXR mesh API foundation [chromium/src : main]
David Li (Gerrit)
David Li added 3 comments![]( "Open in Gerrit")
#include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"This header is included again on line 7.
DLOG(ERROR) << __func__ << "mesh_id=" << idDid you mean to keep this?
std::vector<device::MeshId> meshes_to_remove;Have you considered using `erase_if()` instead of allocating a temporary vector?
Related details
- Adam Ren (xWF)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Li (Gerrit)
David Li added 1 comment![]( "Open in Gerrit")
mesh_ids_to_meshes_.erase(mesh_id);Fyi it says "no modifications are allowed during iteration" here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/hash_map.h;drc=fbc9ca1278998238b92a143217b6ac48162f0ccc;l=191
Related details
- Adam Ren (xWF)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Li (Gerrit)
David Li added 1 comment![]( "Open in Gerrit")
mesh_ids_to_meshes_.erase(mesh_id);Fyi it says "no modifications are allowed during iteration" here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/hash_map.h;drc=fbc9ca1278998238b92a143217b6ac48162f0ccc;l=191
oh nvm.
Related details
- Adam Ren (xWF)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: dch...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): dch...@chromium.org
Reviewer source(s):
dch...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Alex Russell
- Alexander Cooper
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Ren (xWF) (Gerrit)
Adam Ren (xWF) added 3 comments![]( "Open in Gerrit")
#include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"This header is included again on line 7.
Done
DLOG(ERROR) << __func__ << "mesh_id=" << idDid you mean to keep this?
Thanks, I have removed it.
std::vector<device::MeshId> meshes_to_remove;Have you considered using `erase_if()` instead of allocating a temporary vector?
- Alex Russell
- Alexander Cooper
- Daniel Cheng
- David Li
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Li (Gerrit)
David Li added 2 comments![]( "Open in Gerrit")
String SemanticLabelToString(This is nearly identical to the one in third_party/blink/renderer/modules/xr/xr_plane.cc.
Maybe it makes sense to consolidate it in third_party/blink/renderer/modules/xr/xr_utils.h?
String SemanticLabelToString(Fyi: Gemini says it might be better to use AtomicString here.
I don't work on chrome so no idea if that's right.
Related details
- Adam Ren (xWF)
- Alex Russell
- Alexander Cooper
- Brandon Jones
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brandon Jones (Gerrit)
Brandon Jones added 7 comments![]( "Open in Gerrit")
Overall looking good! A couple of minor comments, but I feel like this is close.
Pose? mojo_from_mesh;Not a blocker, but I have concerns that the pose might update much more frequently than the vertices/indices of the mesh. (We see that other tracked objects, such as anchors, often update once per frame.) I worry that with this structure we may end up pushing several Kb of vertex/index data over the mojo channel each frame to keep up with pose changes.
We can probably land this as-is, but it would be good to verify frequency against real hardware (ideally from multiple different runtimes.) If we do see signs that the pose changes more often than the mesh, it would be good to try and restructure this so that the pose can be updates independently.
#include "third_party/blink/renderer/platform/heap/collection_support/heap_vector.h"Why was this added here?
#include "base/numerics/safe_conversions.h"Same question, why is this include necessary now?
String SemanticLabelToString(This is nearly identical to the one in third_party/blink/renderer/modules/xr/xr_plane.cc.
Maybe it makes sense to consolidate it in third_party/blink/renderer/modules/xr/xr_utils.h?
Yes, that would be a good idea.
String SemanticLabelToString(Fyi: Gemini says it might be better to use AtomicString here.
I don't work on chrome so no idea if that's right.
I'm not sure AtomicString would be appropriate here? And as you mentioned, it's almost identical to the existing one. No point in breaking from the existing code IMO.
default:I'd actually prefer to leave the default off. That was developers who add new semantic labels to the mojo file will get a compiler warning indicating that they need to handle it here.
Related details
- Adam Ren (xWF)
- Alex Russell
- Alexander Cooper
- Daniel Cheng
- David Li
Adam Ren (xWF) (Gerrit)
Adam Ren (xWF) added 5 comments![]( "Open in Gerrit")
Not a blocker, but I have concerns that the pose might update much more frequently than the vertices/indices of the mesh. (We see that other tracked objects, such as anchors, often update once per frame.) I worry that with this structure we may end up pushing several Kb of vertex/index data over the mojo channel each frame to keep up with pose changes.
We can probably land this as-is, but it would be good to verify frequency against real hardware (ideally from multiple different runtimes.) If we do see signs that the pose changes more often than the mesh, it would be good to try and restructure this so that the pose can be updates independently.
Thanks for the detailed feedback, Brandon.
Anchors (XRAnchorData) are just a pose, but planes and meshes both bundle pose and geometry together, because the vertices are expressed in the object's local coordinate space defined by the pose. Separating them would require the consumer to carefully match which geometry version goes with which pose. And it can add another layer of complexity.
We do have incremental updates as a guard. Vertices and indices are only sent over Mojo when the runtime reports a change via lastUpdatedTime. Please check the CR 7579917: Implement Android OpenXR mesh manager with incremental streaming.
else if (cache_it->second.last_updated_time != state.lastUpdatedTime) {
needs_update = true;
}So if a submesh's pose and geometry are both stable, nothing is sent. Given this behavior, I think we can keep the Mojo structure as-is for now. I'll note your suggestion as a potential optimization in the task backlog for future investigation.
#include "third_party/blink/renderer/platform/heap/collection_support/heap_vector.h"Why was this added here?
Thanks. It has been removed
Same question, why is this include necessary now?
Done
String SemanticLabelToString(This is nearly identical to the one in third_party/blink/renderer/modules/xr/xr_plane.cc.
Maybe it makes sense to consolidate it in third_party/blink/renderer/modules/xr/xr_utils.h?
Yes, that would be a good idea.
Thanks, it has been moved to xr_utils.h
I'd actually prefer to leave the default off. That was developers who add new semantic labels to the mojo file will get a compiler warning indicating that they need to handle it here.
- Alex Russell
- Alexander Cooper
- Brandon Jones
- Daniel Cheng
- David Li
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brandon Jones (Gerrit)
Brandon Jones added 2 comments![]( "Open in Gerrit")
Pose? mojo_from_mesh;Adam Ren (xWF)Not a blocker, but I have concerns that the pose might update much more frequently than the vertices/indices of the mesh. (We see that other tracked objects, such as anchors, often update once per frame.) I worry that with this structure we may end up pushing several Kb of vertex/index data over the mojo channel each frame to keep up with pose changes.
We can probably land this as-is, but it would be good to verify frequency against real hardware (ideally from multiple different runtimes.) If we do see signs that the pose changes more often than the mesh, it would be good to try and restructure this so that the pose can be updates independently.
Thanks for the detailed feedback, Brandon.
Anchors (XRAnchorData) are just a pose, but planes and meshes both bundle pose and geometry together, because the vertices are expressed in the object's local coordinate space defined by the pose. Separating them would require the consumer to carefully match which geometry version goes with which pose. And it can add another layer of complexity.
We do have incremental updates as a guard. Vertices and indices are only sent over Mojo when the runtime reports a change via lastUpdatedTime. Please check the CR 7579917: Implement Android OpenXR mesh manager with incremental streaming.
else if (cache_it->second.last_updated_time != state.lastUpdatedTime) {
needs_update = true;
}So if a submesh's pose and geometry are both stable, nothing is sent. Given this behavior, I think we can keep the Mojo structure as-is for now. I'll note your suggestion as a potential optimization in the task backlog for future investigation.
I'm not convinced that there is a tight pairing of the vertices and pose that you suggest. While it's true that the vertices should be expressed in the local coordinate space of the submesh pose, that's also the case for almost any 3D object that the page will be displaying. Placement in space is typically done by transforming the vertices in a shader with a matrix, either defined by the page or created from an XRPose, and those transforms are usually independent of changes to the vertex data itself.
In any case, the OpenXR docs are not very clear on what is updated when, and I'm happy to follow up myself to clarify the issue. Until then this can land as is.
// LINT.ThenChange(//tools/metrics/histograms/metadata/blink/enums.xml:FeatureObserver)Please fix this ERROR reported by If This Then That: Changes in the preceding block may need to be reflected in these files: /tools/m...
Changes in the preceding block may need to be reflected in these files: /tools/metrics/histograms/metadata/blink/enums.xml If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
Related details
- Adam Ren (xWF)
- Alex Russell
- Alexander Cooper
- Daniel Cheng
- David Li
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Cooper (Gerrit)
Alexander Cooper added 3 comments![]( "Open in Gerrit")
// Mesh objects are not considered stationary since their pose may vary
// dramatically from frame to frame.I think this is changed based on one of the comments I saw go by?
origin_trial_feature_name: "WebXRMeshDetection",I don't think this is needed
base_feature: "none",If we leave this off it'll autogenerate a feature for us that we can just check and things are a bit simplified. See WebXrVisibilityMask for more. Otherwise, a future change will need to tie whatever feature is made to this feature if we want to control the state of if this is queried in the //device/ code.
Related details
- Adam Ren (xWF)
- Alex Russell
- Daniel Cheng
- David Li
Adam Ren (xWF) (Gerrit)
Adam Ren (xWF) added 3 comments![]( "Open in Gerrit")
// Mesh objects are not considered stationary since their pose may vary
// dramatically from frame to frame.I think this is changed based on one of the comments I saw go by?
This wasn’t added in response to a previous comment. — I don’t see one in the resolved comments list of CL/7508029. This design follows the same pattern as XRPlane: both planes and meshes can have poses that change significantly between frames (plane boundaries and mesh geometry can update), so I think we should have both return false from IsStationary().
I don't think this is needed
Done
base_feature: "none",If we leave this off it'll autogenerate a feature for us that we can just check and things are a bit simplified. See WebXrVisibilityMask for more. Otherwise, a future change will need to tie whatever feature is made to this feature if we want to control the state of if this is queried in the //device/ code.
Sure. base_feature:"none" has been removed. And I added a flag: flag_descriptions::kWebXrMeshDetectionName for mesh detection.
Related details
- Alex Russell
- Alexander Cooper
- Daniel Cheng
- David Li
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Ren (xWF) (Gerrit)
Adam Ren (xWF) added 1 comment![]( "Open in Gerrit")
base_feature: "none",Adam Ren (xWF)If we leave this off it'll autogenerate a feature for us that we can just check and things are a bit simplified. See WebXrVisibilityMask for more. Otherwise, a future change will need to tie whatever feature is made to this feature if we want to control the state of if this is queried in the //device/ code.
Sure. base_feature:"none" has been removed. And I added a flag: flag_descriptions::kWebXrMeshDetectionName for mesh detection.
Done
Adam Ren (xWF) (Gerrit)
Adam Ren (xWF) added 1 comment![]( "Open in Gerrit")
// LINT.ThenChange(//tools/metrics/histograms/metadata/blink/enums.xml:FeatureObserver)Please fix this ERROR reported by If This Then That: Changes in the preceding block may need to be reflected in these files: /tools/m...
Changes in the preceding block may need to be reflected in these files: /tools/metrics/histograms/metadata/blink/enums.xml If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
Will modify this after all CLs have LGTM and rebasing to the mainline.
Related details
- Alex Russell
- Alexander Cooper
- Brandon Jones
- Daniel Cheng
- David Li
Alexander Cooper (Gerrit)
Alexander Cooper added 3 comments![]( "Open in Gerrit")
// LINT.ThenChange(//tools/metrics/histograms/metadata/blink/enums.xml:FeatureObserver)Adam Ren (xWF)Please fix this ERROR reported by If This Then That: Changes in the preceding block may need to be reflected in these files: /tools/m...
Changes in the preceding block may need to be reflected in these files: /tools/metrics/histograms/metadata/blink/enums.xml If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
Will modify this after all CLs have LGTM and rebasing to the mainline.
This is the first CL; so you could fix and merge it now and then merge the others as appropriate.
// Mesh objects are not considered stationary since their pose may vary
// dramatically from frame to frame.Adam Ren (xWF)I think this is changed based on one of the comments I saw go by?
This wasn’t added in response to a previous comment. — I don’t see one in the resolved comments list of CL/7508029. This design follows the same pattern as XRPlane: both planes and meshes can have poses that change significantly between frames (plane boundaries and mesh geometry can update), so I think we should have both return false from IsStationary().
I could've sworn there was feedback from Salar somewhere that these are mostly stable, but maybe that's a runtime implementaion detail as well
origin_trial_feature_name: "WebXRMeshDetection",Adam Ren (xWF)I don't think this is needed
Done
- Adam Ren (xWF)
- Alex Russell