Implement XRVisibilityMaskChange event [chromium/src : main]
Alexander Cooper (Gerrit)
Alexander Cooper added 2 comments![]( "Open in Gerrit")
// This is an opportunity for the session to dispatch any initial set of
// events. Called by |XrSystem| after the session query has resolved.
void DispatchInitialEvents();OpenXR doesn't actually need this because we don't query the mask until we're sending up views with frame data, we don't send it up with the initial set of views. I could remove this, and just leave the logic that on every platform we essentially send it up on the first frame, since `UpdateViews` always has to be called; but the spec *does* allow for us to send them early like this would do if we sent them up initially. Thoughts?
Related details
- Brandon Jones
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 voted and added 5 comments![]( "Open in Gerrit")
Votes added by Brandon Jones
| Code-Review | +1 |
5 comments
LGTM with one must-fix (but simple) issue.
visibility_mask->indices = {0, 2, 4};This is a minor thing that doesn't affect the functionality being tested, but the indicies here are out of bounds. You've provided 3 vertices (X/Y pairs) but index into the 5th vertex (index 4). The indices are given in vertex counts, not array offsets.
```suggestion
visibility_mask->indices = {0, 1, 2};
```
for (uint32_t i = 0; i < device::kNumVisibilityMaskVerticesForTest; i++) {Where does device::kNumVisibilityMaskVerticesForTest and device::kNumVisibilityMaskIndicesForTest come from?
(EDIT: NM, found it. Didn't come up in a search on the page for some reason.)
cached_mask.mask->indices.push_back(2 * index);Nope! Leave those indices alone! As mentioned above, they should refer to the vertex, not the flattened array index.
// This is an opportunity for the session to dispatch any initial set of
// events. Called by |XrSystem| after the session query has resolved.
void DispatchInitialEvents();OpenXR doesn't actually need this because we don't query the mask until we're sending up views with frame data, we don't send it up with the initial set of views. I could remove this, and just leave the logic that on every platform we essentially send it up on the first frame, since `UpdateViews` always has to be called; but the spec *does* allow for us to send them early like this would do if we sent them up initially. Thoughts?
I think this is OK even if we know it won't trigger currently, as it's clearly the right thing to do if there was data and will get rid of a future gotcha if we do start exposing view data earlier.
Related details
- Alexander Cooper
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. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
static XRVisibilityMaskChangeEvent* Create(This class mixes static `Create()` factory methods with public constructors, which is discouraged. To comply with the style guide, please make the constructors private and use a `PassKey` pattern from the `Create` methods to construct the object. (Blink Style Guide: Don't mix Create () factory methods and public constructors in one class.)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- Alexander Cooper
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 voted and added 4 comments![]( "Open in Gerrit")
Votes added by Alexander Cooper
| Commit-Queue | +2 |
4 comments
This is a minor thing that doesn't affect the functionality being tested, but the indicies here are out of bounds. You've provided 3 vertices (X/Y pairs) but index into the 5th vertex (index 4). The indices are given in vertex counts, not array offsets.
```suggestion
visibility_mask->indices = {0, 1, 2};
```
Done
Nope! Leave those indices alone! As mentioned above, they should refer to the vertex, not the flattened array index.
Done
static XRVisibilityMaskChangeEvent* Create(This class mixes static `Create()` factory methods with public constructors, which is discouraged. To comply with the style guide, please make the constructors private and use a `PassKey` pattern from the `Create` methods to construct the object. (Blink Style Guide: Don't mix Create () factory methods and public constructors in one class.)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
**Won't fix:** Seems to be standard pattern for DOM events.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Cooper (Gerrit)
Alexander Cooper voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexander Cooper
| Commit-Queue | +2 |
1 comment
Daniel PTAL event_type_names.json5/runtime_enabled_features.json5 and for device/vr/public/mojom files.
Related details
- Alexander Cooper
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 4 comments![]( "Open in Gerrit")
visibility_masks_[type].contains(view_index)) {It's a bit unfortunate to have multiple map lookups in a row like this–and similarly below.
if (!visibility_masks_.contains(type) ||
!visibility_masks_[type].contains(view_index)) {
UpdateOrCreateVisibilityMask(type, view_index);
}
const auto& mask = visibility_masks_[type][view_index].mask;e.g. this function and the one below both do 4 lookups into `visibility_masks_` per call, which seems a bit excessive.
At the very least, it seems like this could just call `UpdateOrCreateVisibilityMask()` if needed unconditionally, and that function should just return a reference to `visibility_masks_[type][view_index]` and we'd only need a single lookup here and below.
`OnVisibilityMaskChanged()` could do something similar–we could extract the bits that actually update the cached mask out to a helper function that takes a reference to the cached mask value.
// build the visibility mask.Does that mean this list is always expected to have an even number of elements? Or are x and y somehow packed into a single float? Based on what I'm reading, there's actually a secret invariant here that this is always a size that is a multiple of two.
Can we use gfx.mojom.PointF or the like here and just have Blink flatten?
array<uint32> indices;This isn't great either, but I don't have a better recommendation (because it can contain invalid indices). I'm assuming it's not possible/reasonable to simply split it into two lists, so we can haven't invalid indices at all?
Related details
- Alexander Cooper
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 4 comments![]( "Open in Gerrit")
visibility_masks_[type].contains(view_index)) {It's a bit unfortunate to have multiple map lookups in a row like this–and similarly below.
From my understanding the absl::flat_hash_map lookup should be a cheap constant? This event is definitely not happening during a hot-path, and I'd definitely prefer to avoid creating/storing if we get events for masks we don't care about.
if (!visibility_masks_.contains(type) ||
!visibility_masks_[type].contains(view_index)) {
UpdateOrCreateVisibilityMask(type, view_index);
}
const auto& mask = visibility_masks_[type][view_index].mask;e.g. this function and the one below both do 4 lookups into `visibility_masks_` per call, which seems a bit excessive.
At the very least, it seems like this could just call `UpdateOrCreateVisibilityMask()` if needed unconditionally, and that function should just return a reference to `visibility_masks_[type][view_index]` and we'd only need a single lookup here and below.
`OnVisibilityMaskChanged()` could do something similar–we could extract the bits that actually update the cached mask out to a helper function that takes a reference to the cached mask value.
FWIW, my understanding was that the lookups should be cheap/constant (e.g. essentially free). It feels like it'd be making the code more complicated for practically no runtime gain? Unless I'm misunderstanding something about the flat_hash_map performance.
The Update I *had* considered if you'd be okay with it is taking a `mojom::ViewDataPtr&` and assigning the mask and Id, to prevent the need to have both of these functions, which at least cuts down on it some?
The general flow is that this call is really a `CreateIfNeeded` call, and the `OnVisibilityMask` call is really an `UpdateIfExists` call. Simply calling the operator doesn't give me information about if the item was created, and so I think I'd *have* to call the `.contains` in either method if I wanted to avoid calling the `xrGetVisibilityMaskKHR` function every frame or for each mask I got an event for, which seems like the better trade-off?
I guess I could add a `newly_created` bool that defaults to `true` in the cached mask and set it to false, but then the `Changed` event would still be creating empty objects.
// build the visibility mask.Does that mean this list is always expected to have an even number of elements? Or are x and y somehow packed into a single float? Based on what I'm reading, there's actually a secret invariant here that this is always a size that is a multiple of two.
Can we use gfx.mojom.PointF or the like here and just have Blink flatten?
Given that this is flowing from trusted->untrusted, my preference was to just unpack it before serializing it across, as otherwise that's two conversions that need to be done on the data.
Technically yes, there would be an invariant that this has to have an even number of elements as it exists at present.
array<uint32> indices;This isn't great either, but I don't have a better recommendation (because it can contain invalid indices). I'm assuming it's not possible/reasonable to simply split it into two lists, so we can haven't invalid indices at all?
I'm not sure I follow your suggestion of splitting it into two lists? If you're saying e.g. do the processing to make a single list of points that we send across and then unpack it I think that's definitely not feasible. Again, this is something that I'd think is mitigated by coming from a trusted process and being sent to an untrusted one.
Related details
- 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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 4 comments![]( "Open in Gerrit")
visibility_masks_[type].contains(view_index)) {Alexander CooperIt's a bit unfortunate to have multiple map lookups in a row like this–and similarly below.
From my understanding the absl::flat_hash_map lookup should be a cheap constant? This event is definitely not happening during a hot-path, and I'd definitely prefer to avoid creating/storing if we get events for masks we don't care about.
It's still possible to do all these operations with a single lookup into `visibility_masks_`, e.g. here:
```
auto type_it = visibility_masks_.find(type);
if (it == visibility_masks_.end()) {
return;
}
auto mask_it = type_it->find(view_index);
if (mask_it == type_it->end()) {
return;
}
UpdateVisibilityMask(/* any additional args needed here */, &*mask_it);
```
You can certainly shorten some of it if you want using `base::FindOrNull()`, though I think the expanded form is readable enough as-is.
if (!visibility_masks_.contains(type) ||
!visibility_masks_[type].contains(view_index)) {
UpdateOrCreateVisibilityMask(type, view_index);
}
const auto& mask = visibility_masks_[type][view_index].mask;Alexander Coopere.g. this function and the one below both do 4 lookups into `visibility_masks_` per call, which seems a bit excessive.
At the very least, it seems like this could just call `UpdateOrCreateVisibilityMask()` if needed unconditionally, and that function should just return a reference to `visibility_masks_[type][view_index]` and we'd only need a single lookup here and below.
`OnVisibilityMaskChanged()` could do something similar–we could extract the bits that actually update the cached mask out to a helper function that takes a reference to the cached mask value.
FWIW, my understanding was that the lookups should be cheap/constant (e.g. essentially free). It feels like it'd be making the code more complicated for practically no runtime gain? Unless I'm misunderstanding something about the flat_hash_map performance.
The Update I *had* considered if you'd be okay with it is taking a `mojom::ViewDataPtr&` and assigning the mask and Id, to prevent the need to have both of these functions, which at least cuts down on it some?
The general flow is that this call is really a `CreateIfNeeded` call, and the `OnVisibilityMask` call is really an `UpdateIfExists` call. Simply calling the operator doesn't give me information about if the item was created, and so I think I'd *have* to call the `.contains` in either method if I wanted to avoid calling the `xrGetVisibilityMaskKHR` function every frame or for each mask I got an event for, which seems like the better trade-off?
I guess I could add a `newly_created` bool that defaults to `true` in the cached mask and set it to false, but then the `Changed` event would still be creating empty objects.
It's relatively low-overhead but it's certainly not "essentially free". google3 widely uses Abseil's swisstables and yet there's still an internal totw (go/totw/132) to avoid redundant map lookups.
// build the visibility mask.Alexander CooperDoes that mean this list is always expected to have an even number of elements? Or are x and y somehow packed into a single float? Based on what I'm reading, there's actually a secret invariant here that this is always a size that is a multiple of two.
Can we use gfx.mojom.PointF or the like here and just have Blink flatten?
Given that this is flowing from trusted->untrusted, my preference was to just unpack it before serializing it across, as otherwise that's two conversions that need to be done on the data.
Technically yes, there would be an invariant that this has to have an even number of elements as it exists at present.
If this isn't a hot path, then using better types seems fine to me?
I don't think we would consider this (passing an array of 2d points as a flattened array) as a reasonable way to pass arguments across API boundaries in Chrome, even if that's how the underlying JS API is specified.
array<uint32> indices;Alexander CooperThis isn't great either, but I don't have a better recommendation (because it can contain invalid indices). I'm assuming it's not possible/reasonable to simply split it into two lists, so we can haven't invalid indices at all?
I'm not sure I follow your suggestion of splitting it into two lists? If you're saying e.g. do the processing to make a single list of points that we send across and then unpack it I think that's definitely not feasible. Again, this is something that I'd think is mitigated by coming from a trusted process and being sent to an untrusted one.
It comes from a trusted source today. But for things crossing trust boundaries, we should try to default to expressing things in ways that it's impossible to violate invariants.
TBH I don't really understand the data format here and why we need to include points that aren't in vertices. I suppose that's probably obvious to someone with more background, but right now there really isn't enough context for less-informed readers (such as myself) to make that judgement.
Related details
- Alexander Cooper
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Brandon Jones
| Code-Review | +1 |
1 comment
array<uint32> indices;Alexander CooperThis isn't great either, but I don't have a better recommendation (because it can contain invalid indices). I'm assuming it's not possible/reasonable to simply split it into two lists, so we can haven't invalid indices at all?
Daniel ChengI'm not sure I follow your suggestion of splitting it into two lists? If you're saying e.g. do the processing to make a single list of points that we send across and then unpack it I think that's definitely not feasible. Again, this is something that I'd think is mitigated by coming from a trusted process and being sent to an untrusted one.
It comes from a trusted source today. But for things crossing trust boundaries, we should try to default to expressing things in ways that it's impossible to violate invariants.
TBH I don't really understand the data format here and why we need to include points that aren't in vertices. I suppose that's probably obvious to someone with more background, but right now there really isn't enough context for less-informed readers (such as myself) to make that judgement.
For some additional context: Both the vertices and indices are formatted in a way that in intended for them to be fed directly to the GPU via WebGL or WebGPU. The split between the vertices/indices here is because that is the most efficient form for a GPU to consume and render this data. Removing the indices would require significant data duplication for the vertices and be slower to render.
While technically the indices *could* refer to a vertex that is out of bounds, that would be indicative of a implementation bug, and the sources that may use that data for a vertex lookup (either one of the GPU APIs or JavaScript) all have robustness guarantees for handling out of bounds array accesses.
Alexander Cooper (Gerrit)
Alexander Cooper added 4 comments![]( "Open in Gerrit")
Alexander CooperIt's a bit unfortunate to have multiple map lookups in a row like this–and similarly below.
Daniel ChengFrom my understanding the absl::flat_hash_map lookup should be a cheap constant? This event is definitely not happening during a hot-path, and I'd definitely prefer to avoid creating/storing if we get events for masks we don't care about.
It's still possible to do all these operations with a single lookup into `visibility_masks_`, e.g. here:
```
auto type_it = visibility_masks_.find(type);
if (it == visibility_masks_.end()) {
return;
}
auto mask_it = type_it->find(view_index);
if (mask_it == type_it->end()) {
return;
}
UpdateVisibilityMask(/* any additional args needed here */, &*mask_it);
```You can certainly shorten some of it if you want using `base::FindOrNull()`, though I think the expanded form is readable enough as-is.
I ended up adding an `initialized` bool to simplify some code; but overall I think you'll be satisfied with the resulting logic.
if (!visibility_masks_.contains(type) ||
!visibility_masks_[type].contains(view_index)) {
UpdateOrCreateVisibilityMask(type, view_index);
}
const auto& mask = visibility_masks_[type][view_index].mask;Alexander Coopere.g. this function and the one below both do 4 lookups into `visibility_masks_` per call, which seems a bit excessive.
At the very least, it seems like this could just call `UpdateOrCreateVisibilityMask()` if needed unconditionally, and that function should just return a reference to `visibility_masks_[type][view_index]` and we'd only need a single lookup here and below.
`OnVisibilityMaskChanged()` could do something similar–we could extract the bits that actually update the cached mask out to a helper function that takes a reference to the cached mask value.
Daniel ChengFWIW, my understanding was that the lookups should be cheap/constant (e.g. essentially free). It feels like it'd be making the code more complicated for practically no runtime gain? Unless I'm misunderstanding something about the flat_hash_map performance.
The Update I *had* considered if you'd be okay with it is taking a `mojom::ViewDataPtr&` and assigning the mask and Id, to prevent the need to have both of these functions, which at least cuts down on it some?
The general flow is that this call is really a `CreateIfNeeded` call, and the `OnVisibilityMask` call is really an `UpdateIfExists` call. Simply calling the operator doesn't give me information about if the item was created, and so I think I'd *have* to call the `.contains` in either method if I wanted to avoid calling the `xrGetVisibilityMaskKHR` function every frame or for each mask I got an event for, which seems like the better trade-off?
I guess I could add a `newly_created` bool that defaults to `true` in the cached mask and set it to false, but then the `Changed` event would still be creating empty objects.
It's relatively low-overhead but it's certainly not "essentially free". google3 widely uses Abseil's swisstables and yet there's still an internal totw (go/totw/132) to avoid redundant map lookups.
Done
Alexander CooperDoes that mean this list is always expected to have an even number of elements? Or are x and y somehow packed into a single float? Based on what I'm reading, there's actually a secret invariant here that this is always a size that is a multiple of two.
Can we use gfx.mojom.PointF or the like here and just have Blink flatten?
Daniel ChengGiven that this is flowing from trusted->untrusted, my preference was to just unpack it before serializing it across, as otherwise that's two conversions that need to be done on the data.
Technically yes, there would be an invariant that this has to have an even number of elements as it exists at present.
If this isn't a hot path, then using better types seems fine to me?
I don't think we would consider this (passing an array of 2d points as a flattened array) as a reasonable way to pass arguments across API boundaries in Chrome, even if that's how the underlying JS API is specified.
Done
array<uint32> indices;Alexander CooperThis isn't great either, but I don't have a better recommendation (because it can contain invalid indices). I'm assuming it's not possible/reasonable to simply split it into two lists, so we can haven't invalid indices at all?
Daniel ChengI'm not sure I follow your suggestion of splitting it into two lists? If you're saying e.g. do the processing to make a single list of points that we send across and then unpack it I think that's definitely not feasible. Again, this is something that I'd think is mitigated by coming from a trusted process and being sent to an untrusted one.
Brandon JonesIt comes from a trusted source today. But for things crossing trust boundaries, we should try to default to expressing things in ways that it's impossible to violate invariants.
TBH I don't really understand the data format here and why we need to include points that aren't in vertices. I suppose that's probably obvious to someone with more background, but right now there really isn't enough context for less-informed readers (such as myself) to make that judgement.
For some additional context: Both the vertices and indices are formatted in a way that in intended for them to be fed directly to the GPU via WebGL or WebGPU. The split between the vertices/indices here is because that is the most efficient form for a GPU to consume and render this data. Removing the indices would require significant data duplication for the vertices and be slower to render.
While technically the indices *could* refer to a vertex that is out of bounds, that would be indicative of a implementation bug, and the sources that may use that data for a vertex lookup (either one of the GPU APIs or JavaScript) all have robustness guarantees for handling out of bounds array accesses.
The indices are also typically representing triangles, so to give an example, we may represent a square by passing e.g.:
vertices: (0,0), (1,0), (1,1), (0,1)
indices: 0, 1, 2, 1, 2, 3
Related details
- 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. |
Alexander Cooper (Gerrit)
Alexander Cooper voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexander Cooper
| Commit-Queue | +1 |
1 comment
Issues appear to be with device setup, not actual logic.
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 4 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
4 comments
// The operator[] returns a reference of the value type, creating it if it
// needs to be created.
auto& mask = visibility_masks_[type][view_index];
if (!mask.initialized) {
UpdateVisibilityMask(type, view_index, mask);
mask.initialized = true;
}I think we can eliminate the need to have the `initialized` field; use `try_emplace()` instead.
```
auto [it, inserted] = visibility_masks_[type].try_emplace(view_index);
if (inserted) {
UpdateVisibilityMask(...);
}
```
array<uint32> indices;Alexander CooperThis isn't great either, but I don't have a better recommendation (because it can contain invalid indices). I'm assuming it's not possible/reasonable to simply split it into two lists, so we can haven't invalid indices at all?
Daniel ChengI'm not sure I follow your suggestion of splitting it into two lists? If you're saying e.g. do the processing to make a single list of points that we send across and then unpack it I think that's definitely not feasible. Again, this is something that I'd think is mitigated by coming from a trusted process and being sent to an untrusted one.
Brandon JonesIt comes from a trusted source today. But for things crossing trust boundaries, we should try to default to expressing things in ways that it's impossible to violate invariants.
TBH I don't really understand the data format here and why we need to include points that aren't in vertices. I suppose that's probably obvious to someone with more background, but right now there really isn't enough context for less-informed readers (such as myself) to make that judgement.
Alexander CooperFor some additional context: Both the vertices and indices are formatted in a way that in intended for them to be fed directly to the GPU via WebGL or WebGPU. The split between the vertices/indices here is because that is the most efficient form for a GPU to consume and render this data. Removing the indices would require significant data duplication for the vertices and be slower to render.
While technically the indices *could* refer to a vertex that is out of bounds, that would be indicative of a implementation bug, and the sources that may use that data for a vertex lookup (either one of the GPU APIs or JavaScript) all have robustness guarantees for handling out of bounds array accesses.
The indices are also typically representing triangles, so to give an example, we may represent a square by passing e.g.:
vertices: (0,0), (1,0), (1,1), (0,1)
indices: 0, 1, 2, 1, 2, 3
OK, thanks. I would give this example in the Mojom comments.
Can we also rename `indices` to `unvalidated_indices` or `potentially_invalid_indices` or the like? That way it'll hopefully a bit more obvious if we do end up reusing this to send data in the other direction (untrustworthy -> more trustworthy).
// We need to flatten the vertices before sendign to the page.Please fix this WARNING reported by Spellchecker: "sendign" is a possible misspelling of "sending".
To bypass Spellchecker, add a...
"sendign" is a possible misspelling of "sending".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Related details
- Alexander Cooper
- Brandon Jones
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")
// The operator[] returns a reference of the value type, creating it if it
// needs to be created.
auto& mask = visibility_masks_[type][view_index];
if (!mask.initialized) {
UpdateVisibilityMask(type, view_index, mask);
mask.initialized = true;
}I think we can eliminate the need to have the `initialized` field; use `try_emplace()` instead.
```
auto [it, inserted] = visibility_masks_[type].try_emplace(view_index);
if (inserted) {
UpdateVisibilityMask(...);
}
```
TBH, it feels slightly less readable, especially because the `try_emplace` name is a bit weird with our goal here, even if it does and guarantees what we want; but it is nice to not "worry" about `initialized` flag.
array<uint32> indices;Alexander CooperThis isn't great either, but I don't have a better recommendation (because it can contain invalid indices). I'm assuming it's not possible/reasonable to simply split it into two lists, so we can haven't invalid indices at all?
Daniel ChengI'm not sure I follow your suggestion of splitting it into two lists? If you're saying e.g. do the processing to make a single list of points that we send across and then unpack it I think that's definitely not feasible. Again, this is something that I'd think is mitigated by coming from a trusted process and being sent to an untrusted one.
Brandon JonesIt comes from a trusted source today. But for things crossing trust boundaries, we should try to default to expressing things in ways that it's impossible to violate invariants.
TBH I don't really understand the data format here and why we need to include points that aren't in vertices. I suppose that's probably obvious to someone with more background, but right now there really isn't enough context for less-informed readers (such as myself) to make that judgement.
Alexander CooperFor some additional context: Both the vertices and indices are formatted in a way that in intended for them to be fed directly to the GPU via WebGL or WebGPU. The split between the vertices/indices here is because that is the most efficient form for a GPU to consume and render this data. Removing the indices would require significant data duplication for the vertices and be slower to render.
While technically the indices *could* refer to a vertex that is out of bounds, that would be indicative of a implementation bug, and the sources that may use that data for a vertex lookup (either one of the GPU APIs or JavaScript) all have robustness guarantees for handling out of bounds array accesses.
Daniel ChengThe indices are also typically representing triangles, so to give an example, we may represent a square by passing e.g.:
vertices: (0,0), (1,0), (1,1), (0,1)
indices: 0, 1, 2, 1, 2, 3
OK, thanks. I would give this example in the Mojom comments.
Can we also rename `indices` to `unvalidated_indices` or `potentially_invalid_indices` or the like? That way it'll hopefully a bit more obvious if we do end up reusing this to send data in the other direction (untrustworthy -> more trustworthy).
I've included the comment; though there's nothing special here as this is a standard mesh representation.
I thought practice was that we should be validating all mojom inputs from untrusted processes anyways? We'd need a second mojom review to send this from an untrusted->trusted process; so this feels like overkill that only ends up with a more unwieldy name with our 80 character limit that reduces readability.
// We need to flatten the vertices before sendign to the page.Please fix this WARNING reported by Spellchecker: "sendign" is a possible misspelling of "sending".
To bypass Spellchecker, add a...
"sendign" is a possible misspelling of "sending".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
- 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. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
1 comment
array<uint32> indices;Alexander CooperThis isn't great either, but I don't have a better recommendation (because it can contain invalid indices). I'm assuming it's not possible/reasonable to simply split it into two lists, so we can haven't invalid indices at all?
Daniel ChengI'm not sure I follow your suggestion of splitting it into two lists? If you're saying e.g. do the processing to make a single list of points that we send across and then unpack it I think that's definitely not feasible. Again, this is something that I'd think is mitigated by coming from a trusted process and being sent to an untrusted one.
Brandon JonesIt comes from a trusted source today. But for things crossing trust boundaries, we should try to default to expressing things in ways that it's impossible to violate invariants.
TBH I don't really understand the data format here and why we need to include points that aren't in vertices. I suppose that's probably obvious to someone with more background, but right now there really isn't enough context for less-informed readers (such as myself) to make that judgement.
Alexander CooperFor some additional context: Both the vertices and indices are formatted in a way that in intended for them to be fed directly to the GPU via WebGL or WebGPU. The split between the vertices/indices here is because that is the most efficient form for a GPU to consume and render this data. Removing the indices would require significant data duplication for the vertices and be slower to render.
While technically the indices *could* refer to a vertex that is out of bounds, that would be indicative of a implementation bug, and the sources that may use that data for a vertex lookup (either one of the GPU APIs or JavaScript) all have robustness guarantees for handling out of bounds array accesses.
Daniel ChengThe indices are also typically representing triangles, so to give an example, we may represent a square by passing e.g.:
vertices: (0,0), (1,0), (1,1), (0,1)
indices: 0, 1, 2, 1, 2, 3
Alexander CooperOK, thanks. I would give this example in the Mojom comments.
Can we also rename `indices` to `unvalidated_indices` or `potentially_invalid_indices` or the like? That way it'll hopefully a bit more obvious if we do end up reusing this to send data in the other direction (untrustworthy -> more trustworthy).
I've included the comment; though there's nothing special here as this is a standard mesh representation.
I thought practice was that we should be validating all mojom inputs from untrusted processes anyways? We'd need a second mojom review to send this from an untrusted->trusted process; so this feels like overkill that only ends up with a more unwieldy name with our 80 character limit that reduces readability.
as this is a standard mesh representation.
It just helps to have basic/high-level pointers; I believe you that it's standard, but it's somewhat domain-specific knowledge.
so this feels like overkill that only ends up with a more unwieldy name with our 80 character limit that reduces readability.
The alternative is to typemap it and validate it always. But that's more heavyweight in terms of boilerplate. But if you want to do that, I certainly wouldn't say no :)
The goal here is to reduce the possibility for mistakes. You're right that it would need additional review. But the reviewer would have to notice that this is a list of indices into the `vertices` array, which TBH, is something that seems quite missable in a larger review.
Should a reviewer catch this? Ideally. But we can still do things to highlight more risky/non-idiomatic constructs like this.
Related details
- Alexander Cooper
- Brandon Jones
Alexander Cooper (Gerrit)
Alexander Cooper voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alexander Cooper
| Commit-Queue | +2 |
2 comments
array<uint32> indices;- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/55583.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Cooper (Gerrit)
Alexander Cooper voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
18 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: device/vr/openxr/openxr_visibility_mask_handler.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: device/vr/public/mojom/vr_service.mojom
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/modules/xr/xr_visibility_mask_change_event.cc
Insertions: 4, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/modules/xr/xr_session.h
Insertions: 2, Deletions: 0.
The diff is too large to show. Please review the diff.
```
Change information
Implement XRVisibilityMaskChange event
Implements the XRVisibilityMaskChange event in blink, including all
infrastructure needed to send the real data for the event as well as
adding a minor test case for the scenario. Note that because it is
related to the VisibilityMaskChange event, the index of the XRView is
also now exposed per the spec.
- M chrome/browser/vr/test/mock_xr_device_hook_base.cc
- M chrome/browser/vr/test/mock_xr_device_hook_base.h
- M chrome/browser/vr/test/webxr_vr_browser_test.cc
- M chrome/browser/vr/webxr_vr_transition_browser_test.cc
- A chrome/test/data/xr/e2e_test_files/html/test_visibility_mask_change_event.html
- M components/webxr/xr_test_hook_wrapper.cc
- M components/webxr/xr_test_hook_wrapper.h
- M device/vr/BUILD.gn
- M device/vr/DEPS
- M device/vr/openxr/openxr_api_wrapper.cc
- M device/vr/openxr/openxr_api_wrapper.h
- M device/vr/openxr/openxr_extension_helper.cc
- M device/vr/openxr/openxr_extension_helper.h
- M device/vr/openxr/openxr_platform_helper.cc
- A device/vr/openxr/openxr_visibility_mask_handler.cc
- A device/vr/openxr/openxr_visibility_mask_handler.h
- M device/vr/openxr/test/fake_openxr_impl_api.cc
- M device/vr/openxr/test/openxr_test_helper.cc
- M device/vr/openxr/test/openxr_test_helper.h
- M device/vr/public/mojom/BUILD.gn
- M device/vr/public/mojom/browser_test_interfaces.mojom
- A device/vr/public/mojom/visibility_mask_id.h
- M device/vr/public/mojom/vr_service.mojom
- M device/vr/public/mojom/vr_service_mojom_traits.h
- M device/vr/test/test_hook.h
- M third_party/blink/renderer/bindings/generated_in_modules.gni
- M third_party/blink/renderer/bindings/idl_in_modules.gni
- M third_party/blink/renderer/core/events/event_type_names.json5
- M third_party/blink/renderer/modules/xr/BUILD.gn
- M third_party/blink/renderer/modules/xr/DEPS
- M third_party/blink/renderer/modules/xr/xr_session.cc
- M third_party/blink/renderer/modules/xr/xr_session.h
- M third_party/blink/renderer/modules/xr/xr_session.idl
- M third_party/blink/renderer/modules/xr/xr_system.cc
- M third_party/blink/renderer/modules/xr/xr_utils.cc
- M third_party/blink/renderer/modules/xr/xr_utils.h
- M third_party/blink/renderer/modules/xr/xr_view.cc
- M third_party/blink/renderer/modules/xr/xr_view.h
- M third_party/blink/renderer/modules/xr/xr_view.idl
- A third_party/blink/renderer/modules/xr/xr_visibility_mask_change_event.cc
- A third_party/blink/renderer/modules/xr/xr_visibility_mask_change_event.h
- A third_party/blink/renderer/modules/xr/xr_visibility_mask_change_event.idl
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/external/wpt/resources/chromium/webxr-test.js
- M third_party/blink/web_tests/external/wpt/webxr/idlharness.https.window-expected.txt
- M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
Code-Review: +1 by Daniel Cheng, +1 by Brandon Jones
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/55583
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |