WebXR Layers API: Add layer IDs to frame submit sequence [chromium/src : main]
Evgenii Alekseev (xWF) (Gerrit)
Evgenii Alekseev (xWF) added 2 comments![]( "Open in Gerrit")
bool has_backend_{false};Evgenii Alekseev (xWF)nit: Per the Blink Style Guide, boolean member variables should be prefixed with 'is' or 'did'. Please consider renaming `has_backend_` to something like `is_backend_created_` for consistency with `is_modified_` in this file. (Blink Style Guide: Naming - Precede boolean values with words like “is” and “did”)
_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)_
moved to another CL
void OnAddLayer(XRLayer* layer);Evgenii Alekseev (xWF)The parameter name 'layer' is obvious from its type 'XRLayer*' and can be omitted from the function declaration in the header file. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
_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)_
methods are moved to another CL.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evgenii Alekseev (xWF) (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Alexander Cooper
- 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 added 30 comments![]( "Open in Gerrit")
Highlighted a few places where I think some subtle behavior changes might be taking place that could be causing the test issues, including one place that I think is a non-spec compliant change.
Unfortunately, I think there's a lot of subtleties with some of the `OnFrame` or `updateRenderState` logic.
// Temporarily store the transport delegate of the last submitted layer as a
// source of the sync token. Will be empty after OnFrameEnd.Not really how it needs to work. The TransportDelegate is technically layer agnostic. I don't think we can (due to code-layering issues), hold onto just the sync token, and really we need the delegate to pass for the actual end-frame logic, so instead I think we should make `OnFrameEnd` require a XRFrameTransportDelegate.
HashMap<uint32_t, scoped_refptr<StaticBitmapImage>> current_frame_images_;LayerId ?
// SubmitWebGLLyayer or SubmitWebGPULayer to submit layers. Call OnFrameEndNot accurate anymore.
void SubmitLayer(uint32_t layer_id, XrLayerClient*, bool was_changed);LayerId.
But also... maybe queryable from the XrLayerClient?
void XRFrameProvider::SubmitLayer(uint32_t layer_id,Use LayerId type.
CHECK_NE(client->session()->GraphicsApi(), XRGraphicsBinding::Api::kWebGPU)
<< "WebGPU layers only support shared buffer submission modes";Should still have this after the `DrawingIntoSharedBuffer` if statement would return.
if (frame_id_ < 0) {
// There is no valid frame_id_, and the browser side is not currently
// expecting a frame to be submitted. That can happen for the first
// immersive frame if the animation loop submits without a preceding
// immersive GetFrameData response, in that case frame_id_ is -1 (see
// https://crbug.com/855722).
return;
}TBH, this check should probably be left in `SubmitLayer` as well.
// Reset an active frame id, since anything we'd want to do (resizing/etc) can
// no-longer happen to this frame.
frame_id_ = -1;While this is technically the correct change; This *may* be the cause of some of the test errors, since it is a change in behavior.
if (!layers_enabled && layers_->size()) {NIT: Prefer an explicit check either empty or > 0.
return (base_layer_ || (layers_ && layers_->size() > 0));NIT: Outer parens not needed.
HeapVector<Member<XRRenderStateInit>> pending_render_state_;Alternatively, there may be some tests that relied on some beahvior around this that could be causing test failures.
pending_render_state_.push_back(init);I think this breaks spec compliant behavior. Essentially, we don't update the render state until after the frame is finished processing, and essentially per the spec, if there's an unset field, then that field shouldn't be updated.
E.g. It's valid behavior to call:
updateRenderState({depthNear: ...});
updateRenderState({depthFar:...});
And have the current state remain unchanged except that *both* depthNear and depthFar get updated. I believe your current logic throws out the ability to do this.
This *might* be the cause of issues with the tests as well, since they could e.g. set a base layer then call `updateRenderState` again, and the base layer gets cleared.
FURTHER, "Valid" doesn't mean what you think it means I think, since technically we still *can* end with no base layer.
if (render_state_->IsValid()) {
// Update render state on animation frame submit.
pending_render_state_ = init;
} else {
// Update render state on first frame request.
requested_render_state_ = init;
}I'm not seeing the need to split things between `pending_render_state_` and `requested_render_state_`? The render state doesn't get applied until after the frame exits, and if e.g. `updateRenderState` is called multiple times, we'd really only be applying the last one anyways.
render_state_->IsValid() || requested_render_state_;FWIW, we could have a state where we have a `render_state_` that is invalid, and `requested_render_state_` does not have any layers set yet, as the validation only checks that *if* they are set, they are valid. But not every `updateRenderState` call is required to try to set the layers. So we still need the previous check.
AND In fact, the check in `updateRenderState` *does* allow us to end up with a `null` base layer or empty layers (the check is only that either there isn't a layer or IF THERE IS that the session matches).
ApplyRenderState();So it looks like this is *after* we get a new frame, rather than right before we request a new frame, and that's why we had the `prev_base_layer_` as a member...? This is the other behavior I'd be most suspicious of causing the test failures.
if (!render_state_->baseLayer() && !render_state_->layers().size()) {Here and elsewhere: Prefer checks to empty or comparison to 0.
xr_->frameProvider()->OnFrameEnd();This looks like another potentially subtle change. This never had OnFrameStart/OnFrameEnd called on it in the old flow.
// Return base layer only if 'layers' feature enabled.
XRLayer* frame_base_layer = render_state_->GetFirstLayer(layers_enabled_);That's not really what this method implies to me; so perhaps we should either change our logic based on `layers_enabled_` and let render_state_ have methods that are more explicit about what we want? (GetFirstLayer() or GetBaseLayer()) ?
MaybeRequestFrame();I don't *think* anything has changed in this function that would cause `MaybeRequestFrame()` to change it's mind? Essentially we don't call it here also because the `requestAnimationFrame` call from the page should also cause it to get triggered.
frame_base_layer->OnFrameStart();This parallel with frameProvider() *also* having an OnFrameStarty/OnFrameEnd is a bit confusing. Not sure if one or the other could have a better name here.
if (frame_base_layer) {
frame_base_layer->OnFrameEnd();
} else {
for (XRLayer* layer : render_state_->layers()) {
layer->OnFrameEnd();
}
}As far as these are concerned, they don't really care if it's a base layer or a layer. I wonder if maybe we just have `layers()` handle that internally if it's returning all the layers or a vector of just the base layer? I wonder if that simplifies any other logic as well?
// Request a new render state on animation frame callback copletion.
requested_render_state_ = pending_render_state_;
pending_render_state_ = nullptr;See my other comments about not understanding this and thinking that this *set* isn't actually happening appropriately?
XRLayer* base_layer = render_state_->baseLayer();Why this update?
blink::HashMap<uint32_t, scoped_refptr<StaticBitmapImage>> previous_images_;LayerId
blink::HashMap<uint32_t, scoped_refptr<StaticBitmapImage>> image_refs,LayerId
vr_presentation_provider->SubmitFrameMissing(
vr_frame_id,
(delegate ? delegate->GenerateSyncToken() : gpu::SyncToken()));If we make the change to require this in OnFrameEnd, we can keep the simplified logic here.
blink::HashMap<uint32_t, scoped_refptr<StaticBitmapImage>> image_refs,LayerId
ids.reserve(image_refs.size());
std::transform(image_refs.begin(), image_refs.end(),
std::back_inserter(ids),
[](const auto& pair) { return device::LayerId(pair.key); });We should be using device::LayerId throughout blink, the intention is to remove `uint32_t`s, so this becomes less necessary. (May still need it to extract all the keys, not sure if the hashmap has that functionality otherwise).
virtual void WaitOnFence(gfx::GpuFence* fence) {}This should still be pure virtual?
Related details
- Brandon Jones
- Evgenii Alekseev (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. |
Alexander Cooper (Gerrit)
Alexander Cooper added 2 comments![]( "Open in Gerrit")
frame_base_layer->OnFrameStart();This parallel with frameProvider() *also* having an OnFrameStarty/OnFrameEnd is a bit confusing. Not sure if one or the other could have a better name here.
(But also not a huge deal if nothing is immediately apparent).
if (frame_base_layer) {
frame_base_layer->OnFrameEnd();
} else {
for (XRLayer* layer : render_state_->layers()) {
layer->OnFrameEnd();
}
}As far as these are concerned, they don't really care if it's a base layer or a layer. I wonder if maybe we just have `layers()` handle that internally if it's returning all the layers or a vector of just the base layer? I wonder if that simplifies any other logic as well?
Also, if it's too tricky with types, could consider just having render_state_ expose the `OnFrameStart`/`OnFrameEnd` methods.
Alexander Cooper (Gerrit)
Alexander Cooper added 1 comment![]( "Open in Gerrit")
BTW, if the WTF maps/sets give you issues you'll likely just need to add a specialization to: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/xr/xr_id_hash_traits.h
Evgenii Alekseev (xWF) (Gerrit)
Evgenii Alekseev (xWF) added 29 comments![]( "Open in Gerrit")
// Temporarily store the transport delegate of the last submitted layer as a
// source of the sync token. Will be empty after OnFrameEnd.Not really how it needs to work. The TransportDelegate is technically layer agnostic. I don't think we can (due to code-layering issues), hold onto just the sync token, and really we need the delegate to pass for the actual end-frame logic, so instead I think we should make `OnFrameEnd` require a XRFrameTransportDelegate.
removed, provided through the OnFrameEnd.
HashMap<uint32_t, scoped_refptr<StaticBitmapImage>> current_frame_images_;Evgenii Alekseev (xWF)LayerId ?
Done
// SubmitWebGLLyayer or SubmitWebGPULayer to submit layers. Call OnFrameEndEvgenii Alekseev (xWF)Not accurate anymore.
Thanks, renamed methods and rephrased comment correspondingly.
void SubmitLayer(uint32_t layer_id, XrLayerClient*, bool was_changed);LayerId.
But also... maybe queryable from the XrLayerClient?
I though about it, but in that case we have to add a static cast to the XRwebGL Layer AND/OR XrCompoistionLayer.
void XRFrameProvider::SubmitLayer(uint32_t layer_id,Evgenii Alekseev (xWF)Use LayerId type.
Done
CHECK_NE(client->session()->GraphicsApi(), XRGraphicsBinding::Api::kWebGPU)
<< "WebGPU layers only support shared buffer submission modes";Should still have this after the `DrawingIntoSharedBuffer` if statement would return.
Added, thanks.
if (frame_id_ < 0) {
// There is no valid frame_id_, and the browser side is not currently
// expecting a frame to be submitted. That can happen for the first
// immersive frame if the animation loop submits without a preceding
// immersive GetFrameData response, in that case frame_id_ is -1 (see
// https://crbug.com/855722).
return;
}TBH, this check should probably be left in `SubmitLayer` as well.
Good point, added
// Reset an active frame id, since anything we'd want to do (resizing/etc) can
// no-longer happen to this frame.
frame_id_ = -1;While this is technically the correct change; This *may* be the cause of some of the test errors, since it is a change in behavior.
I could not see test failures anymore. I suppose that it should be ok.
NIT: Prefer an explicit check either empty or > 0.
Done
return (base_layer_ || (layers_ && layers_->size() > 0));NIT: Outer parens not needed.
Done
HeapVector<Member<XRRenderStateInit>> pending_render_state_;Alternatively, there may be some tests that relied on some beahvior around this that could be causing test failures.
removed the pending render_state.
pending_render_state_.push_back(init);I think this breaks spec compliant behavior. Essentially, we don't update the render state until after the frame is finished processing, and essentially per the spec, if there's an unset field, then that field shouldn't be updated.
E.g. It's valid behavior to call:
updateRenderState({depthNear: ...});
updateRenderState({depthFar:...});And have the current state remain unchanged except that *both* depthNear and depthFar get updated. I believe your current logic throws out the ability to do this.
This *might* be the cause of issues with the tests as well, since they could e.g. set a base layer then call `updateRenderState` again, and the base layer gets cleared.
FURTHER, "Valid" doesn't mean what you think it means I think, since technically we still *can* end with no base layer.
Thanks, reverted.
if (render_state_->IsValid()) {
// Update render state on animation frame submit.
pending_render_state_ = init;
} else {
// Update render state on first frame request.
requested_render_state_ = init;
}I'm not seeing the need to split things between `pending_render_state_` and `requested_render_state_`? The render state doesn't get applied until after the frame exits, and if e.g. `updateRenderState` is called multiple times, we'd really only be applying the last one anyways.
reverted. Thanks.
render_state_->IsValid() || requested_render_state_;FWIW, we could have a state where we have a `render_state_` that is invalid, and `requested_render_state_` does not have any layers set yet, as the validation only checks that *if* they are set, they are valid. But not every `updateRenderState` call is required to try to set the layers. So we still need the previous check.
AND In fact, the check in `updateRenderState` *does* allow us to end up with a `null` base layer or empty layers (the check is only that either there isn't a layer or IF THERE IS that the session matches).
Thanks for detailed description. Now it is more clear how to handle it.
So it looks like this is *after* we get a new frame, rather than right before we request a new frame, and that's why we had the `prev_base_layer_` as a member...? This is the other behavior I'd be most suspicious of causing the test failures.
Test are good now, replaced prev_base_layer with prev_transport_delegate.
if (!render_state_->baseLayer() && !render_state_->layers().size()) {Here and elsewhere: Prefer checks to empty or comparison to 0.
Done
This looks like another potentially subtle change. This never had OnFrameStart/OnFrameEnd called on it in the old flow.
Thanks. removed.
// Return base layer only if 'layers' feature enabled.
XRLayer* frame_base_layer = render_state_->GetFirstLayer(layers_enabled_);That's not really what this method implies to me; so perhaps we should either change our logic based on `layers_enabled_` and let render_state_ have methods that are more explicit about what we want? (GetFirstLayer() or GetBaseLayer()) ?
Yeh, it was not good idea to propagate layers_enabled to the method.
Base on spec, it is possible to add XRWebGLLayer to the base layer OR add it to the layers list.
So, we do not need to map shared buffer to first layer ID. Added corresponding comment.
I don't *think* anything has changed in this function that would cause `MaybeRequestFrame()` to change it's mind? Essentially we don't call it here also because the `requestAnimationFrame` call from the page should also cause it to get triggered.
Removed. Thanks for pointing this out.
Alexander CooperThis parallel with frameProvider() *also* having an OnFrameStarty/OnFrameEnd is a bit confusing. Not sure if one or the other could have a better name here.
(But also not a huge deal if nothing is immediately apparent).
Moved layers calls to the RenderState, thinking about renaming of the frame provider's methods.
if (frame_base_layer) {
frame_base_layer->OnFrameEnd();
} else {
for (XRLayer* layer : render_state_->layers()) {
layer->OnFrameEnd();
}
}Alexander CooperAs far as these are concerned, they don't really care if it's a base layer or a layer. I wonder if maybe we just have `layers()` handle that internally if it's returning all the layers or a vector of just the base layer? I wonder if that simplifies any other logic as well?
Also, if it's too tricky with types, could consider just having render_state_ expose the `OnFrameStart`/`OnFrameEnd` methods.
Moved to the render state, good point, thanks.
// Request a new render state on animation frame callback copletion.
requested_render_state_ = pending_render_state_;
pending_render_state_ = nullptr;See my other comments about not understanding this and thinking that this *set* isn't actually happening appropriately?
reverted.
XRLayer* base_layer = render_state_->baseLayer();Evgenii Alekseev (xWF)Why this update?
Replaced with render_state_ ->onResize.
According to spce there is no limitations to add multiple XRWebGLLayer instances to the layers list, we need to call onResize for all of them.
blink::HashMap<uint32_t, scoped_refptr<StaticBitmapImage>> previous_images_;Evgenii Alekseev (xWF)LayerId
Done
blink::HashMap<uint32_t, scoped_refptr<StaticBitmapImage>> image_refs,Evgenii Alekseev (xWF)LayerId
Done
vr_presentation_provider->SubmitFrameMissing(
vr_frame_id,
(delegate ? delegate->GenerateSyncToken() : gpu::SyncToken()));If we make the change to require this in OnFrameEnd, we can keep the simplified logic here.
Replaced on CHECK for delegate,
blink::HashMap<uint32_t, scoped_refptr<StaticBitmapImage>> image_refs,Evgenii Alekseev (xWF)LayerId
Done
ids.reserve(image_refs.size());
std::transform(image_refs.begin(), image_refs.end(),
std::back_inserter(ids),
[](const auto& pair) { return device::LayerId(pair.key); });We should be using device::LayerId throughout blink, the intention is to remove `uint32_t`s, so this becomes less necessary. (May still need it to extract all the keys, not sure if the hashmap has that functionality otherwise).
Moved the layer IDs vector to the upper level.
This should still be pure virtual?
- 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 voted and added 15 comments![]( "Open in Gerrit")
Votes added by Alexander Cooper
| Code-Review | +1 |
15 comments
Mostly looks good, a few NITs and a few questions mainly to make sure I understand what's going on (though these may be opportunities for code comments as well).
bool layers_enabled_ = false;I believe layers can only be enabled on the immersive session; rather than caching this I'd prefer we either:
1) Add a method to XrSession like: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/xr/xr_session.h;drc=32ef847d4806639a036e08779c3150e760a5030c;l=360
or
2) Query https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/xr/xr_session.h;drc=32ef847d4806639a036e08779c3150e760a5030c;l=404
Could generate a local method to wrap the call for readability.
// `ClearCachedLayersData()` to begin, then `SubmitLayer()` for each layer,Ultra NIT: Sort `SubmitLayer` down here between the two other methods so that all three "lifecycle" methods are together/in-order in the header.
void SubmitLayer(uint32_t layer_id, XrLayerClient*, bool was_changed);Evgenii Alekseev (xWF)LayerId.
But also... maybe queryable from the XrLayerClient?
I though about it, but in that case we have to add a static cast to the XRwebGL Layer AND/OR XrCompoistionLayer.
How so? I think if we want to just use `kInvalidId` for BaseLayer, it's fine and doesn't need a check? (I *would* advocate that it'd be nice if we could start generating IDs for base layers, and just treat them like projection layers, but I think that's beyond the scope of this).
// TODO(crbug.com/450856064): Add layer_manager initialization.
layers_enabled_ =
session->IsFeatureEnabled(device::mojom::XRSessionFeature::LAYERS);See other comment about wrapping this in a method rather than a member.
// Reset an active frame id, since anything we'd want to do (resizing/etc) can
// no-longer happen to this frame.
frame_id_ = -1;Evgenii Alekseev (xWF)While this is technically the correct change; This *may* be the cause of some of the test errors, since it is a change in behavior.
I could not see test failures anymore. I suppose that it should be ok.
Acknowledged
if (!was_any_layer_changed || !transport_delegate) {This is CHECK'd so should no longer be checked in the if statement.
// The backend expects an empty layer ID list if the 'layers' feature is not
// enabled.
if (!layers_enabled_) {
layer_ids.clear();
}It's kind of a CHECK failure here if this is bigger than size 1 with layers_enabled_ == false right? e.g. there should not be a way to get this to be larger than just the base layer if we don't have the layers feature enabled.
XRLayer* last_modified_layer =
base_layer_ ? base_layer_
: (layers_ && !layers_->empty() ? layers_->back() : nullptr);Why does this need to be the last_modified_layer ? If our main goal is to get the transport delegate, then can we just query `GetFirstLayer()`, since it should be the same for every layer in the array? (e.g. either it's the base layer where there's only one, or it's a composition layer where it comes from the binding?)
if (last_modified_layer->LayerType() == XRLayerType::kWebGLLayer) {
transport_delegate = static_cast<XRWebGLLayer*>(last_modified_layer)
->GetTransportDelegate();
} else {
transport_delegate = static_cast<XRCompositionLayer*>(last_modified_layer)
->binding()
->GetTransportDelegate();
}I don't think I'm opposed to putting this on XrLayer if that simplifies things.
bool XRRenderState::GetAndClearUpdateLayersStatus() {
bool needs_update = needs_layers_update_;
needs_layers_update_ = false;
return needs_update;
}I think this should be split into two methods. `NeedLayersUpdate()` and `OnLayersUpdated()`.
if (!will_have_valid_render_state) {
prev_transport_delegate_ = render_state_->GetTransportDelegate();
}Why here instead of relying on ApplyPendingRenderState?
prev_transport_delegate_ = nullptr;
ApplyPendingRenderState();I think I see what you've been saying. Per the spec, we should do this after we finish processing the callbacks not here probably, per our impl we *probably* need to do this before we submit, and the backend can handle knowing that it should use the "old" value for the pending submit and the "new" value for the next frame it returns. I guess there'd probably be a further backend change needed there on top of moving this towards e.g. the end of OnFrame instead of here (with a few early returns updated/removed). I'd guess that may also remove some `prev_transport_delegate_` logic, but may need to add some logic to e.g. drop the frame request if the render state isn't good.
I think what you've got now is an okay workaround, but can you file a bug?
if (should_update_layers_backend_) {
should_update_layers_backend_ = false;
if (layers_enabled_) {So, IIUC, the issue stems from the fact that we need to have the layers be "viable" before we service the frame to the page, and we aren't currently *actually* updating the render state until we *start* processing the frame? So we essentially intentionally drop a frame and re-request it so that we can get a frame where the layers are valid before we service it to the page?
If I've got it right, can you add a comment here that this is a workaround for the bug I asked you to file in the other comment?
// TODO(crbug.com/450856064): Update backend and request frame again.I can't remember at the moment, but does the backend not complain about receiving layers it didn't expect? e.g. Shouldn't this CL be making the SetEnabledCompositionLayers call?
Related details
- Brandon Jones
- Evgenii Alekseev (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. |
Evgenii Alekseev (xWF) (Gerrit)
Evgenii Alekseev (xWF) added 14 comments![]( "Open in Gerrit")
bool layers_enabled_ = false;I believe layers can only be enabled on the immersive session; rather than caching this I'd prefer we either:
1) Add a method to XrSession like: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/xr/xr_session.h;drc=32ef847d4806639a036e08779c3150e760a5030c;l=360
or
2) Query https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/xr/xr_session.h;drc=32ef847d4806639a036e08779c3150e760a5030c;l=404Could generate a local method to wrap the call for readability.
The problem is that there is no session reference in the current class, we are getting session with argument of the methods.
I have cherry-pick'ed layer_manager_.is_bound() from the next CL.
// `ClearCachedLayersData()` to begin, then `SubmitLayer()` for each layer,Ultra NIT: Sort `SubmitLayer` down here between the two other methods so that all three "lifecycle" methods are together/in-order in the header.
Fixed, thank you for catching it.
void SubmitLayer(uint32_t layer_id, XrLayerClient*, bool was_changed);Evgenii Alekseev (xWF)LayerId.
But also... maybe queryable from the XrLayerClient?
Alexander CooperI though about it, but in that case we have to add a static cast to the XRwebGL Layer AND/OR XrCompoistionLayer.
How so? I think if we want to just use `kInvalidId` for BaseLayer, it's fine and doesn't need a check? (I *would* advocate that it'd be nice if we could start generating IDs for base layers, and just treat them like projection layers, but I think that's beyond the scope of this).
I'm sorry, I could not understand your idea. Could you please give more details?
As I can see we have the following hierarchy for the XrLayerClient:
class XRWebGLLayer final : public XRLayer, public XrLayerClient
class XRCompositionLayer : public XRLayer {
public:
XRLayerDrawingContext* drawing_context();
}
class XRLayerDrawingContext : public GarbageCollected<XRLayerDrawingContext>,
public XrLayerClient;
which means that we have to cast XRLayerClient to the XRWebGLLayer or XRCompositionLayer to make it possible to call the layer_id().
On the other hand, we have layer_id() at the context of the SubmitLayer() function call.
1. BaseLayer has a layer id on the blink side, and we do replace kInvalidId with a real layer id for the shared buffer at the XRSession::OnFrame.
So, we should never get kInvalidId as argument. I can add CHECK if it makes sense.
2. According to spec layers list could contain any XrLayer type, which means that XrWebGLLayer could be in the list too. So, that's why XrwebGLLayer's backend goes through the LayerManager API when 'layers' enabled, and it has a shared buffer (openxr composition layer) with the corresponding layer id at the backed.
// TODO(crbug.com/450856064): Add layer_manager initialization.
layers_enabled_ =
session->IsFeatureEnabled(device::mojom::XRSessionFeature::LAYERS);See other comment about wrapping this in a method rather than a member.
replaced on layer_manager initialization + XRSession::layers_enabled()
if (!was_any_layer_changed || !transport_delegate) {This is CHECK'd so should no longer be checked in the if statement.
Done
// The backend expects an empty layer ID list if the 'layers' feature is not
// enabled.
if (!layers_enabled_) {
layer_ids.clear();
}It's kind of a CHECK failure here if this is bigger than size 1 with layers_enabled_ == false right? e.g. there should not be a way to get this to be larger than just the base layer if we don't have the layers feature enabled.
1.I've separated layer_ids and image_refs into two distinct vectors.
2. We only require layer_ids for the shared buffer use case. And there is no CHECK for ids size, only for the image_refs size.
3. backend will throw an exception if the layer_ids list is non-empty when the session lacks the 'layers' feature.
XRLayer* last_modified_layer =
base_layer_ ? base_layer_
: (layers_ && !layers_->empty() ? layers_->back() : nullptr);Why does this need to be the last_modified_layer ? If our main goal is to get the transport delegate, then can we just query `GetFirstLayer()`, since it should be the same for every layer in the array? (e.g. either it's the base layer where there's only one, or it's a composition layer where it comes from the binding?)
Good point, I'll try.
if (last_modified_layer->LayerType() == XRLayerType::kWebGLLayer) {
transport_delegate = static_cast<XRWebGLLayer*>(last_modified_layer)
->GetTransportDelegate();
} else {
transport_delegate = static_cast<XRCompositionLayer*>(last_modified_layer)
->binding()
->GetTransportDelegate();
}I don't think I'm opposed to putting this on XrLayer if that simplifies things.
GetTransportDelegate() defined at the XRLayerClient,
And XrWebGLLayer implements both XRLayer and XRLayerClient.
So, I've implemented is via XrLayer::LayerClient()->GetTransportDelegate();
Please, let me know if it ok?
bool XRRenderState::GetAndClearUpdateLayersStatus() {
bool needs_update = needs_layers_update_;
needs_layers_update_ = false;
return needs_update;
}I think this should be split into two methods. `NeedLayersUpdate()` and `OnLayersUpdated()`.
Good point. updated.
if (!will_have_valid_render_state) {
prev_transport_delegate_ = render_state_->GetTransportDelegate();
}Why here instead of relying on ApplyPendingRenderState?
removed. Most likely it is not needed at all.
prev_transport_delegate_ = nullptr;
ApplyPendingRenderState();I think I see what you've been saying. Per the spec, we should do this after we finish processing the callbacks not here probably, per our impl we *probably* need to do this before we submit, and the backend can handle knowing that it should use the "old" value for the pending submit and the "new" value for the next frame it returns. I guess there'd probably be a further backend change needed there on top of moving this towards e.g. the end of OnFrame instead of here (with a few early returns updated/removed). I'd guess that may also remove some `prev_transport_delegate_` logic, but may need to add some logic to e.g. drop the frame request if the render state isn't good.
I think what you've got now is an okay workaround, but can you file a bug?
The prev_base_layer and prev_transport_delegate uses to handle the render state transition from when it has at least one layer to the state when there is no any layer in the render state.
It is mainly related to the XRFrameTransport::FrameSubmitMissing which is required the sync token as a one of the parameters of the mojom call.
So, it is more or less clear how to avoid extra frame message for the layers, but for the base layer we probably have to extend mojom API too.
Filed a bug, updated comment with comment:
https://crbugs.com/452595360
if (should_update_layers_backend_) {
should_update_layers_backend_ = false;
if (layers_enabled_) {So, IIUC, the issue stems from the fact that we need to have the layers be "viable" before we service the frame to the page, and we aren't currently *actually* updating the render state until we *start* processing the frame? So we essentially intentionally drop a frame and re-request it so that we can get a frame where the layers are valid before we service it to the page?
If I've got it right, can you add a comment here that this is a workaround for the bug I asked you to file in the other comment?
Yes, you are right, there is no pending layers sequence update on the backend.
I've filed a bug. Updated comment.
https://crbugs.com/452604976
// TODO(crbug.com/450856064): Update backend and request frame again.I can't remember at the moment, but does the backend not complain about receiving layers it didn't expect? e.g. Shouldn't this CL be making the SetEnabledCompositionLayers call?
At this point, it should go through the "submit missing frame" API, which does not include any layer.
I'll add SetEnabledCompositionLayers in the next CL.
// TODO(crbug.com/450856064): Update backend and request frame again.I can't remember at the moment, but does the backend not complain about receiving layers it didn't expect? e.g. Shouldn't this CL be making the SetEnabledCompositionLayers call?
At this point, it should go through the "submit missing frame" API, which does not include layers sequence.
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 voted and added 11 comments![]( "Open in Gerrit")
Votes added by Alexander Cooper
| Code-Review | +1 |
11 comments
bool layers_enabled_ = false;Evgenii Alekseev (xWF)I believe layers can only be enabled on the immersive session; rather than caching this I'd prefer we either:
1) Add a method to XrSession like: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/xr/xr_session.h;drc=32ef847d4806639a036e08779c3150e760a5030c;l=360
or
2) Query https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/xr/xr_session.h;drc=32ef847d4806639a036e08779c3150e760a5030c;l=404Could generate a local method to wrap the call for readability.
The problem is that there is no session reference in the current class, we are getting session with argument of the methods.
I have cherry-pick'ed layer_manager_.is_bound() from the next CL.
There is. Looks like line 148 after changes there is `Member<XRSession> immersive_session_`.
(There's also `non_immersive_data_providers_` which has the inline XRSessions, but I don't think we need to worrya bout those).
Having this store the layer_manager_ and using it's presence instead of a bool is fine too.
void SubmitLayer(uint32_t layer_id, XrLayerClient*, bool was_changed);Evgenii Alekseev (xWF)LayerId.
But also... maybe queryable from the XrLayerClient?
Alexander CooperI though about it, but in that case we have to add a static cast to the XRwebGL Layer AND/OR XrCompoistionLayer.
Evgenii Alekseev (xWF)How so? I think if we want to just use `kInvalidId` for BaseLayer, it's fine and doesn't need a check? (I *would* advocate that it'd be nice if we could start generating IDs for base layers, and just treat them like projection layers, but I think that's beyond the scope of this).
I'm sorry, I could not understand your idea. Could you please give more details?
As I can see we have the following hierarchy for the XrLayerClient:
class XRWebGLLayer final : public XRLayer, public XrLayerClient
class XRCompositionLayer : public XRLayer {
public:
XRLayerDrawingContext* drawing_context();
}
class XRLayerDrawingContext : public GarbageCollected<XRLayerDrawingContext>,
public XrLayerClient;
which means that we have to cast XRLayerClient to the XRWebGLLayer or XRCompositionLayer to make it possible to call the layer_id().On the other hand, we have layer_id() at the context of the SubmitLayer() function call.
1. BaseLayer has a layer id on the blink side, and we do replace kInvalidId with a real layer id for the shared buffer at the XRSession::OnFrame.
So, we should never get kInvalidId as argument. I can add CHECK if it makes sense.
2. According to spec layers list could contain any XrLayer type, which means that XrWebGLLayer could be in the list too. So, that's why XrwebGLLayer's backend goes through the LayerManager API when 'layers' enabled, and it has a shared buffer (openxr composition layer) with the corresponding layer id at the backed.
I was asking if it was possible to expose a method on `XRLayerClient` to get the `layer_id`; but if it's easier to not, and leave it as you have it, that's fine too.
// TODO(crbug.com/450856064): Add layer_manager initialization.
layers_enabled_ =
session->IsFeatureEnabled(device::mojom::XRSessionFeature::LAYERS);Evgenii Alekseev (xWF)See other comment about wrapping this in a method rather than a member.
replaced on layer_manager initialization + XRSession::layers_enabled()
Acknowledged
// The backend expects an empty layer ID list if the 'layers' feature is not
// enabled.
if (!layers_enabled_) {
layer_ids.clear();
}Evgenii Alekseev (xWF)It's kind of a CHECK failure here if this is bigger than size 1 with layers_enabled_ == false right? e.g. there should not be a way to get this to be larger than just the base layer if we don't have the layers feature enabled.
1.I've separated layer_ids and image_refs into two distinct vectors.
2. We only require layer_ids for the shared buffer use case. And there is no CHECK for ids size, only for the image_refs size.
3. backend will throw an exception if the layer_ids list is non-empty when the session lacks the 'layers' feature.
Right, but I'm saying if the front end is calling `SubmitLayer` more than once in a non-layers enabled session (e.g. layer_ids is bigger than size 1), that's an error isn't it?
XRLayer* last_modified_layer =
base_layer_ ? base_layer_
: (layers_ && !layers_->empty() ? layers_->back() : nullptr);Evgenii Alekseev (xWF)Why does this need to be the last_modified_layer ? If our main goal is to get the transport delegate, then can we just query `GetFirstLayer()`, since it should be the same for every layer in the array? (e.g. either it's the base layer where there's only one, or it's a composition layer where it comes from the binding?)
Good point, I'll try.
Done
if (last_modified_layer->LayerType() == XRLayerType::kWebGLLayer) {
transport_delegate = static_cast<XRWebGLLayer*>(last_modified_layer)
->GetTransportDelegate();
} else {
transport_delegate = static_cast<XRCompositionLayer*>(last_modified_layer)
->binding()
->GetTransportDelegate();
}Evgenii Alekseev (xWF)I don't think I'm opposed to putting this on XrLayer if that simplifies things.
GetTransportDelegate() defined at the XRLayerClient,
And XrWebGLLayer implements both XRLayer and XRLayerClient.
So, I've implemented is via XrLayer::LayerClient()->GetTransportDelegate();
Please, let me know if it ok?
And XrWebGLLayer implements both XRLayer and XRLayerClient.
It is technically fine for a class to implement two interfaces with the same method declaration; though it does feel a bit weird, so this is fine.
bool XRRenderState::GetAndClearUpdateLayersStatus() {
bool needs_update = needs_layers_update_;
needs_layers_update_ = false;
return needs_update;
}Evgenii Alekseev (xWF)I think this should be split into two methods. `NeedLayersUpdate()` and `OnLayersUpdated()`.
Good point. updated.
Done
if (!will_have_valid_render_state) {
prev_transport_delegate_ = render_state_->GetTransportDelegate();
}Evgenii Alekseev (xWF)Why here instead of relying on ApplyPendingRenderState?
removed. Most likely it is not needed at all.
Done
prev_transport_delegate_ = nullptr;
ApplyPendingRenderState();Evgenii Alekseev (xWF)I think I see what you've been saying. Per the spec, we should do this after we finish processing the callbacks not here probably, per our impl we *probably* need to do this before we submit, and the backend can handle knowing that it should use the "old" value for the pending submit and the "new" value for the next frame it returns. I guess there'd probably be a further backend change needed there on top of moving this towards e.g. the end of OnFrame instead of here (with a few early returns updated/removed). I'd guess that may also remove some `prev_transport_delegate_` logic, but may need to add some logic to e.g. drop the frame request if the render state isn't good.
I think what you've got now is an okay workaround, but can you file a bug?
The prev_base_layer and prev_transport_delegate uses to handle the render state transition from when it has at least one layer to the state when there is no any layer in the render state.
It is mainly related to the XRFrameTransport::FrameSubmitMissing which is required the sync token as a one of the parameters of the mojom call.
So, it is more or less clear how to avoid extra frame message for the layers, but for the base layer we probably have to extend mojom API too.
Filed a bug, updated comment with comment:
https://crbugs.com/452595360
It is mainly related to the XRFrameTransport::FrameSubmitMissing which is required the sync token as a one of the parameters of the mojom call.
Except that I think we can just pass a default constructed sync token if there's e.g. nothing to actually sync on.
I think that essentially dropping the frame if we don't have a valid layer setup is fine. But this is all discussion for the feature.
if (should_update_layers_backend_) {
should_update_layers_backend_ = false;
if (layers_enabled_) {Evgenii Alekseev (xWF)So, IIUC, the issue stems from the fact that we need to have the layers be "viable" before we service the frame to the page, and we aren't currently *actually* updating the render state until we *start* processing the frame? So we essentially intentionally drop a frame and re-request it so that we can get a frame where the layers are valid before we service it to the page?
If I've got it right, can you add a comment here that this is a workaround for the bug I asked you to file in the other comment?
Yes, you are right, there is no pending layers sequence update on the backend.
I've filed a bug. Updated comment.
https://crbugs.com/452604976
Looks like comment didn't get updated?
// TODO(crbug.com/450856064): Update backend and request frame again.Evgenii Alekseev (xWF)I can't remember at the moment, but does the backend not complain about receiving layers it didn't expect? e.g. Shouldn't this CL be making the SetEnabledCompositionLayers call?
At this point, it should go through the "submit missing frame" API, which does not include layers sequence.
- Brandon Jones
- Evgenii Alekseev (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. |
Evgenii Alekseev (xWF) (Gerrit)
Evgenii Alekseev (xWF) added 4 comments![]( "Open in Gerrit")
bool layers_enabled_ = false;Evgenii Alekseev (xWF)I believe layers can only be enabled on the immersive session; rather than caching this I'd prefer we either:
1) Add a method to XrSession like: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/xr/xr_session.h;drc=32ef847d4806639a036e08779c3150e760a5030c;l=360
or
2) Query https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/xr/xr_session.h;drc=32ef847d4806639a036e08779c3150e760a5030c;l=404Could generate a local method to wrap the call for readability.
Alexander CooperThe problem is that there is no session reference in the current class, we are getting session with argument of the methods.
I have cherry-pick'ed layer_manager_.is_bound() from the next CL.
There is. Looks like line 148 after changes there is `Member<XRSession> immersive_session_`.
(There's also `non_immersive_data_providers_` which has the inline XRSessions, but I don't think we need to worrya bout those).
Having this store the layer_manager_ and using it's presence instead of a bool is fine too.
Thank you. I've totally missed it.
void SubmitLayer(uint32_t layer_id, XrLayerClient*, bool was_changed);Evgenii Alekseev (xWF)LayerId.
But also... maybe queryable from the XrLayerClient?
Alexander CooperI though about it, but in that case we have to add a static cast to the XRwebGL Layer AND/OR XrCompoistionLayer.
Evgenii Alekseev (xWF)How so? I think if we want to just use `kInvalidId` for BaseLayer, it's fine and doesn't need a check? (I *would* advocate that it'd be nice if we could start generating IDs for base layers, and just treat them like projection layers, but I think that's beyond the scope of this).
Alexander CooperI'm sorry, I could not understand your idea. Could you please give more details?
As I can see we have the following hierarchy for the XrLayerClient:
class XRWebGLLayer final : public XRLayer, public XrLayerClient
class XRCompositionLayer : public XRLayer {
public:
XRLayerDrawingContext* drawing_context();
}
class XRLayerDrawingContext : public GarbageCollected<XRLayerDrawingContext>,
public XrLayerClient;
which means that we have to cast XRLayerClient to the XRWebGLLayer or XRCompositionLayer to make it possible to call the layer_id().On the other hand, we have layer_id() at the context of the SubmitLayer() function call.
1. BaseLayer has a layer id on the blink side, and we do replace kInvalidId with a real layer id for the shared buffer at the XRSession::OnFrame.
So, we should never get kInvalidId as argument. I can add CHECK if it makes sense.
2. According to spec layers list could contain any XrLayer type, which means that XrWebGLLayer could be in the list too. So, that's why XrwebGLLayer's backend goes through the LayerManager API when 'layers' enabled, and it has a shared buffer (openxr composition layer) with the corresponding layer id at the backed.
I was asking if it was possible to expose a method on `XRLayerClient` to get the `layer_id`; but if it's easier to not, and leave it as you have it, that's fine too.
The same case as duplicate of the GetTransportDelegate() in the XRLayer,
I mean that in that case both XRLayerClient and XRLayer should provide layer_id(),
and XRWebGLLayer should override them.
// The backend expects an empty layer ID list if the 'layers' feature is not
// enabled.
if (!layers_enabled_) {
layer_ids.clear();
}Evgenii Alekseev (xWF)It's kind of a CHECK failure here if this is bigger than size 1 with layers_enabled_ == false right? e.g. there should not be a way to get this to be larger than just the base layer if we don't have the layers feature enabled.
Alexander Cooper1.I've separated layer_ids and image_refs into two distinct vectors.
2. We only require layer_ids for the shared buffer use case. And there is no CHECK for ids size, only for the image_refs size.
3. backend will throw an exception if the layer_ids list is non-empty when the session lacks the 'layers' feature.
Right, but I'm saying if the front end is calling `SubmitLayer` more than once in a non-layers enabled session (e.g. layer_ids is bigger than size 1), that's an error isn't it?
Good point, added CHECK and corresponding comment.
if (should_update_layers_backend_) {
should_update_layers_backend_ = false;
if (layers_enabled_) {Evgenii Alekseev (xWF)So, IIUC, the issue stems from the fact that we need to have the layers be "viable" before we service the frame to the page, and we aren't currently *actually* updating the render state until we *start* processing the frame? So we essentially intentionally drop a frame and re-request it so that we can get a frame where the layers are valid before we service it to the page?
If I've got it right, can you add a comment here that this is a workaround for the bug I asked you to file in the other comment?
Alexander CooperYes, you are right, there is no pending layers sequence update on the backend.
I've filed a bug. Updated comment.
https://crbugs.com/452604976
Looks like comment didn't get updated?
Great catch. Thanks. I've updated comment, it was pointed at the cube layer support bug number.
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 voted and added 5 comments![]( "Open in Gerrit")
Votes added by Alexander Cooper
| Code-Review | +1 |
5 comments
Still LGTM, just one more tweak to the comment.
bool layers_enabled_ = false;Evgenii Alekseev (xWF)I believe layers can only be enabled on the immersive session; rather than caching this I'd prefer we either:
1) Add a method to XrSession like: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/xr/xr_session.h;drc=32ef847d4806639a036e08779c3150e760a5030c;l=360
or
2) Query https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/xr/xr_session.h;drc=32ef847d4806639a036e08779c3150e760a5030c;l=404Could generate a local method to wrap the call for readability.
Alexander CooperThe problem is that there is no session reference in the current class, we are getting session with argument of the methods.
I have cherry-pick'ed layer_manager_.is_bound() from the next CL.
Evgenii Alekseev (xWF)There is. Looks like line 148 after changes there is `Member<XRSession> immersive_session_`.
(There's also `non_immersive_data_providers_` which has the inline XRSessions, but I don't think we need to worrya bout those).
Having this store the layer_manager_ and using it's presence instead of a bool is fine too.
Thank you. I've totally missed it.
Acknowledged
void SubmitLayer(uint32_t layer_id, XrLayerClient*, bool was_changed);Acknowledged
// The backend expects an empty layer ID list if the 'layers' feature is not
// enabled.
if (!layers_enabled_) {
layer_ids.clear();
}Evgenii Alekseev (xWF)It's kind of a CHECK failure here if this is bigger than size 1 with layers_enabled_ == false right? e.g. there should not be a way to get this to be larger than just the base layer if we don't have the layers feature enabled.
Alexander Cooper1.I've separated layer_ids and image_refs into two distinct vectors.
2. We only require layer_ids for the shared buffer use case. And there is no CHECK for ids size, only for the image_refs size.
3. backend will throw an exception if the layer_ids list is non-empty when the session lacks the 'layers' feature.
Evgenii Alekseev (xWF)Right, but I'm saying if the front end is calling `SubmitLayer` more than once in a non-layers enabled session (e.g. layer_ids is bigger than size 1), that's an error isn't it?
Good point, added CHECK and corresponding comment.
Done
if (should_update_layers_backend_) {
should_update_layers_backend_ = false;
if (layers_enabled_) {Evgenii Alekseev (xWF)So, IIUC, the issue stems from the fact that we need to have the layers be "viable" before we service the frame to the page, and we aren't currently *actually* updating the render state until we *start* processing the frame? So we essentially intentionally drop a frame and re-request it so that we can get a frame where the layers are valid before we service it to the page?
If I've got it right, can you add a comment here that this is a workaround for the bug I asked you to file in the other comment?
Alexander CooperYes, you are right, there is no pending layers sequence update on the backend.
I've filed a bug. Updated comment.
https://crbugs.com/452604976
Evgenii Alekseev (xWF)Looks like comment didn't get updated?
Great catch. Thanks. I've updated comment, it was pointed at the cube layer support bug number.
Oh, sorry I meant I'd like a comment along the lines of like "This means that the page has updated the layers since it last received a new frame, but we haven't updated the backend yet, so the page won't be able to use those layers just yet as they expect. For now, drop this frame and request a new one with the updated layers, which we'll then serve to the page." (And then leave the TODO on the line below the comment).
Related details
- Brandon Jones
- Evgenii Alekseev (xWF)
- Joe Mason
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. |
Joe Mason (Gerrit)
Joe Mason voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joe Mason
| Code-Review | +1 |
1 comment
mojom LGTM
Related details
- Brandon Jones
- Evgenii Alekseev (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. |
Evgenii Alekseev (xWF) (Gerrit)
Evgenii Alekseev (xWF) added 1 comment![]( "Open in Gerrit")
if (should_update_layers_backend_) {
should_update_layers_backend_ = false;
if (layers_enabled_) {Evgenii Alekseev (xWF)So, IIUC, the issue stems from the fact that we need to have the layers be "viable" before we service the frame to the page, and we aren't currently *actually* updating the render state until we *start* processing the frame? So we essentially intentionally drop a frame and re-request it so that we can get a frame where the layers are valid before we service it to the page?
If I've got it right, can you add a comment here that this is a workaround for the bug I asked you to file in the other comment?
Alexander CooperYes, you are right, there is no pending layers sequence update on the backend.
I've filed a bug. Updated comment.
https://crbugs.com/452604976
Evgenii Alekseev (xWF)Looks like comment didn't get updated?
Alexander CooperGreat catch. Thanks. I've updated comment, it was pointed at the cube layer support bug number.
Oh, sorry I meant I'd like a comment along the lines of like "This means that the page has updated the layers since it last received a new frame, but we haven't updated the backend yet, so the page won't be able to use those layers just yet as they expect. For now, drop this frame and request a new one with the updated layers, which we'll then serve to the page." (And then leave the TODO on the line below the comment).
Do you mind if I add comment at the next CL? As far as next CL should modify an absolutely the same code lines.
Related details
- Alexander Cooper
- Brandon Jones
Evgenii Alekseev (xWF) (Gerrit)
Evgenii Alekseev (xWF) added 1 comment![]( "Open in Gerrit")
if (should_update_layers_backend_) {
should_update_layers_backend_ = false;
if (layers_enabled_) {Evgenii Alekseev (xWF)So, IIUC, the issue stems from the fact that we need to have the layers be "viable" before we service the frame to the page, and we aren't currently *actually* updating the render state until we *start* processing the frame? So we essentially intentionally drop a frame and re-request it so that we can get a frame where the layers are valid before we service it to the page?
If I've got it right, can you add a comment here that this is a workaround for the bug I asked you to file in the other comment?
Alexander CooperYes, you are right, there is no pending layers sequence update on the backend.
I've filed a bug. Updated comment.
https://crbugs.com/452604976
Evgenii Alekseev (xWF)Looks like comment didn't get updated?
Alexander CooperGreat catch. Thanks. I've updated comment, it was pointed at the cube layer support bug number.
Evgenii Alekseev (xWF)Oh, sorry I meant I'd like a comment along the lines of like "This means that the page has updated the layers since it last received a new frame, but we haven't updated the backend yet, so the page won't be able to use those layers just yet as they expect. For now, drop this frame and request a new one with the updated layers, which we'll then serve to the page." (And then leave the TODO on the line below the comment).
Do you mind if I add comment at the next CL? As far as next CL should modify an absolutely the same code lines.
- Alexander Cooper
- Brandon Jones
- Joe Mason
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
| Code-Review | +1 |
1 comment
if (should_update_layers_backend_) {
should_update_layers_backend_ = false;
if (layers_enabled_) {Evgenii Alekseev (xWF)So, IIUC, the issue stems from the fact that we need to have the layers be "viable" before we service the frame to the page, and we aren't currently *actually* updating the render state until we *start* processing the frame? So we essentially intentionally drop a frame and re-request it so that we can get a frame where the layers are valid before we service it to the page?
If I've got it right, can you add a comment here that this is a workaround for the bug I asked you to file in the other comment?
Alexander CooperYes, you are right, there is no pending layers sequence update on the backend.
I've filed a bug. Updated comment.
https://crbugs.com/452604976
Evgenii Alekseev (xWF)Looks like comment didn't get updated?
Alexander CooperGreat catch. Thanks. I've updated comment, it was pointed at the cube layer support bug number.
Evgenii Alekseev (xWF)Oh, sorry I meant I'd like a comment along the lines of like "This means that the page has updated the layers since it last received a new frame, but we haven't updated the backend yet, so the page won't be able to use those layers just yet as they expect. For now, drop this frame and request a new one with the updated layers, which we'll then serve to the page." (And then leave the TODO on the line below the comment).
Do you mind if I add comment at the next CL? As far as next CL should modify an absolutely the same code lines.
Looks like you uploaded it now anyways? UI looks like you still have code owners approval, so we won't need Joe to stamp again, just one other person apart from me.
Related details
- Brandon Jones
- Evgenii Alekseev (xWF)
- Joe Mason
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 voted Code-Review+1![]( "Open in Gerrit")
- Evgenii Alekseev (xWF)
- Joe Mason
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/55552.
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
Evgenii Alekseev (xWF) (Gerrit)
Evgenii Alekseev (xWF) voted Commit-Queue+2![]( "Open in Gerrit")
- Joe Mason
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
WebXR Layers API: Add layer IDs to frame submit sequence
This change introduces layer ids to the XR frame submission process via
mojom api.
The key changes are:
- vr_service.mojom: The SubmitFrameDrawnIntoTexture method now includes a layer_ids argument.This allows the VR service to receive a list of layers which modified shared buffer image for the current frame.
- XrFrameProvider: The frame submission sequence is now consists of multiple steps: ClearCachedLayersData -> SubmitLayer (for each layer) -> SubmitFrame.This new sequence allow us to collect layer ids, when 'layers' feature enabled and submit frame with a sing gfx::SyncToken.
- XRSession: The behavior of updateRenderState has been extended to support a layers feature. On ApplyPendingRenderState, if a modification to the layers composition sequence is detected, the current frame is dropped. The session then should update the layers sequence via the mojom layer manager API and requests a new frame.
BYPASS_LARGE_CHANGE_WARNING
- M device/vr/android/arcore/arcore_gl.cc
- M device/vr/android/arcore/arcore_gl.h
- M device/vr/android/cardboard/cardboard_render_loop.cc
- M device/vr/android/cardboard/cardboard_render_loop.h
- M device/vr/openxr/openxr_render_loop.cc
- M device/vr/openxr/openxr_render_loop.h
- M device/vr/public/mojom/vr_service.mojom
- M third_party/blink/renderer/modules/xr/DEPS
- M third_party/blink/renderer/modules/xr/xr_composition_layer.cc
- M third_party/blink/renderer/modules/xr/xr_composition_layer.h
- M third_party/blink/renderer/modules/xr/xr_frame_provider.cc
- M third_party/blink/renderer/modules/xr/xr_frame_provider.h
- M third_party/blink/renderer/modules/xr/xr_gpu_binding.h
- M third_party/blink/renderer/modules/xr/xr_graphics_binding.h
- M third_party/blink/renderer/modules/xr/xr_id_hash_traits.h
- M third_party/blink/renderer/modules/xr/xr_layer.h
- M third_party/blink/renderer/modules/xr/xr_layer_shared_image_manager.cc
- M third_party/blink/renderer/modules/xr/xr_layer_shared_image_manager.h
- M third_party/blink/renderer/modules/xr/xr_render_state.cc
- M third_party/blink/renderer/modules/xr/xr_render_state.h
- 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_webgl_binding.h
- M third_party/blink/renderer/modules/xr/xr_webgl_layer.cc
- M third_party/blink/renderer/modules/xr/xr_webgl_layer.h
- M third_party/blink/renderer/platform/graphics/gpu/xr_frame_transport.cc
- M third_party/blink/renderer/platform/graphics/gpu/xr_frame_transport.h
- M third_party/blink/renderer/platform/graphics/gpu/xr_frame_transport_delegate.h
- M third_party/blink/renderer/platform/graphics/gpu/xr_gpu_frame_transport_delegate.cc
- M third_party/blink/renderer/platform/graphics/gpu/xr_gpu_frame_transport_delegate.h
- M third_party/blink/renderer/platform/graphics/gpu/xr_webgl_frame_transport_delegate.cc
- M third_party/blink/renderer/platform/graphics/gpu/xr_webgl_frame_transport_delegate.h
- M third_party/blink/web_tests/external/wpt/resources/chromium/webxr-test.js
Code-Review: +1 by Alexander Cooper, +1 by Brandon Jones
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |