Begin building OpenXr on Android [chromium/src : main]
Alexander Cooper (Gerrit)
Attention is currently required from: Kenneth Russell.
Alexander Cooper would like Kenneth Russell to review this change.
Begin building OpenXr on Android
Refactors windows-specific code that has crept into the OpenXr classes
by currently wrapping most of them in ifdefs. In a few cases I've
extracted some classes to use temporarily to allow key methods to remain
visible for Android (BeginFrame and InitSession the most critical).
Some of these classes will change slightly with other refactor work that
was identified and some would be good candidates for future cleanup.
At the moment, OpenXr is not added as a supported runtime on Android,
this simply begins it building on Android. Future changes will hook up
the rest of the stack to consider/query for an OpenXr runtime.
Bug: 1435548
Change-Id: I9bdfe5938f3f08efbc62c8d40fe0ec0dc6a0754f
---
M DEPS
M components/webxr/DEPS
M content/browser/xr/service/vr_service_impl.cc
M device/vr/BUILD.gn
M device/vr/DEPS
M device/vr/android/DEPS
M device/vr/buildflags/buildflags.gni
A device/vr/openxr/android/openxr_graphics_binding_open_gles.cc
A device/vr/openxr/android/openxr_graphics_binding_open_gles.h
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
A device/vr/openxr/openxr_graphics_binding.h
M device/vr/openxr/openxr_platform.h
M device/vr/openxr/openxr_render_loop.cc
M device/vr/openxr/openxr_render_loop.h
M device/vr/openxr/openxr_statics.cc
M device/vr/openxr/openxr_statics.h
M device/vr/openxr/openxr_util.cc
M device/vr/openxr/openxr_util.h
M device/vr/openxr/test/fake_openxr_impl_api.cc
M device/vr/openxr/test/openxr_negotiate.h
M device/vr/openxr/test/openxr_test_helper.h
A device/vr/openxr/windows/openxr_graphics_binding_d3d11.cc
A device/vr/openxr/windows/openxr_graphics_binding_d3d11.h
M device/vr/windows/compositor_base.cc
M device/vr/windows/compositor_base.h
M third_party/openxr/BUILD.gn
29 files changed, 464 insertions(+), 160 deletions(-)
To view, visit change 4461233. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Cooper (Gerrit)
Attention is currently required from: Kenneth Russell.
2 comments:
Patchset:
Ken, Hoping you can take a look at this open question/device/vr/DEPS before I send for review by my team.
File device/vr/openxr/openxr_platform.h:
Patch Set #1, Line 23:
#include "third_party/khronos/EGL/egl.h"I'm not entirely sure that this is the right header to include here. @k...@chromium.org, do you have any guidance on this? When I try to include it as <EGL/egl.h> like the openxr-sdk-source does, I'm told that that path doesn't exist; and while it does seem to find it as "EGL/egl.h", the DEPs file is still unhappy and we don't seem to have that directory, so I'm unsure who I would add to look at it...
To view, visit change 4461233. To unsubscribe, or for help writing mail filters, visit settings.
Kenneth Russell (Gerrit)
Attention is currently required from: Alexander Cooper, Brandon Jones.
Kenneth Russell would like Brandon Jones to review this change authored by Alexander Cooper.
To view, visit change 4461233. To unsubscribe, or for help writing mail filters, visit settings.
Kenneth Russell (Gerrit)
Attention is currently required from: Alexander Cooper, Brandon Jones.
1 comment:
Patchset:
Brandon, would you be able to do the primary review of this CL?
To view, visit change 4461233. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Cooper (Gerrit)
Attention is currently required from: Brandon Jones, Kenneth Russell.
1 comment:
Patchset:
Brandon, would you be able to do the primary review of this CL?
I've chatted a bit with Brandon offline and we weren't sure if using the Dependency this way was expected. Essentially the third_party openxr_platform file requires EGL types to be defined and I'm unsure of the right way to add that inclusion since <EGL/egl.h> seems to not exist. See the comment in device/vr/openxr/openxr_platform.h for more context.
To view, visit change 4461233. To unsubscribe, or for help writing mail filters, visit settings.
Kenneth Russell (Gerrit)
File device/vr/openxr/openxr_platform.h:
Patch Set #1, Line 23:
#include "third_party/khronos/EGL/egl.h"
I'm not entirely sure that this is the right header to include here. @kbr@chromium. […]
Have you investigated adding the `khronos_headers` config from third_party/khronos/BUILD.gn into the appropriate build target here in `device/vr`? I think that's how several other portions of Chromium gain access to these headers explicitly.
To view, visit change 4461233. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Cooper (Gerrit)
Attention is currently required from: Brandon Jones, Kenneth Russell.
2 comments:
Patchset:
I've chatted a bit with Brandon offline and we weren't sure if using the Dependency this way was exp […]
Brandon can certainly review the rest of the CL (as a WebXR Owner); but the part I actually needed your review on was device/vr/DEPS (note that there's already a matching DEP being removed in device/vr/android and it is essentially being moved up a level). Usage is in openxr_render_loop (though not really fully used yet), and there was the question about how best to address the need to have EGL defined in device/vr/openxr/openxr_platform.h, which is currently using the third_party/khronos:khronos_header configs. (I attempted to use ui/gl/gl_bindings.h, but got redefinition errors in some of the other files).
File device/vr/openxr/openxr_platform.h:
Patch Set #1, Line 23:
#include "third_party/khronos/EGL/egl.h"
Have you investigated adding the `khronos_headers` config from third_party/khronos/BUILD. […]
That seemed to work; I tried to get them via ui/gl/gl_bindings instead, but hit some redefinition errors in some of the other files.
To view, visit change 4461233. To unsubscribe, or for help writing mail filters, visit settings.
Findit (Gerrit)
Attention is currently required from: Brandon Jones, Kenneth Russell.
This change meets the code coverage requirements.
Patch set 5:Code-Coverage +1
Brandon Jones (Gerrit)
Attention is currently required from: Alexander Cooper, Kenneth Russell.
9 comments:
Patchset:
Going forward I'd like to see a lot less #ifs sprinkled throughout the code, but as a first step of a larger refactor it's fine.
File device/vr/openxr/android/openxr_graphics_binding_open_gles.cc:
Patch Set #6, Line 45:
color_swapchain_images.emplace_back();Shouldn't this be emplacing... something? Like the texture handle in the D3D version of the class?
File device/vr/openxr/openxr_api_wrapper.h:
Patch Set #6, Line 53:
// and D3D11Fence for each D3D11 texture in the vector.Nit: Might want to remove/relocate the D3D references in comments like these as well.
Feels like this is an good opportunity to have an abstract SwapChainInfo class with platform-specific inherited types. In fact, might even be something that gets defined in openxr_graphics_binding.h, since it seems like it's going to be tightly associated with that.
File device/vr/openxr/openxr_api_wrapper.cc:
Patch Set #6, Line 749:
#endifIs this code that could live in the OpenXrGraphicsBinding? Feels like trying to shift a fair amount of the platform-specific graphics code into that class would be a good trend.
Patch Set #6, Line 771:
#endifSimilarly, feels like this logic could comfortably reside in the OpenXrGraphicsBinding too. (Probably applies to several other code snippets in this file as well.)
File device/vr/openxr/openxr_graphics_binding.h:
Patch Set #6, Line 22:
virtual const void* get() const = 0;Having this method just named `get()` threw me off a little, especially because it just returns a void pointer. I think the intent is to treat it like a smart pointer, the class as a whole doesn't quite act that way.
Naming it something like `graphics_binding()` or `GetGraphicsBinding()` would probably help readability a bit.
File device/vr/openxr/openxr_render_loop.cc:
Patch Set #6, Line 67:
texture_helper_.SetBackbuffer(swap_chain_info->d3d11_texture.get());It's a bit unfortunate that these types of calls are sprinkled all throughout the code, but I understand the refactoring is an ongoing process.
Do you think that Android will end up having it's own `texture_helper_`? If not, I think it would be appropriate to rename texture_helper_ to something like d3d_texture_helper_ to help emphasize that it's a windows-only utility.
File device/vr/windows/compositor_base.cc:
Patch Set #6, Line 604:
#endifCan we reduce the number of #ifs here by structuring it like so?
```
#if win
if(no_submit) {
...
} else {
copy_successful = ...
}
#endifif(copy_successful) {
...
}
```
To view, visit change 4461233. To unsubscribe, or for help writing mail filters, visit settings.
Piotr Bialecki (Gerrit)
Attention is currently required from: Alexander Cooper, Kenneth Russell.
6 comments:
Patchset:
Drive-by with nits (I may do a 2nd pass over it).
File DEPS:
Patch Set #6, Line 152:
only on WindowsNit: update.
File device/vr/openxr/android/openxr_graphics_binding_open_gles.h:
Patch Set #6, Line 26:
XrGraphicsBindingOpenGLESAndroidKHRAlso initialize pointers to nullptr here? I'm not sure what c++ will do with them if we aren't explicit...
File device/vr/openxr/openxr_util.cc:
// Since the OpenXR backend only knows how to draw with D3D11 at the moment,
// the XR_KHR_D3D11_ENABLE_EXTENSION_NAME is required.Nit: update.
File device/vr/openxr/windows/openxr_graphics_binding_d3d11.h:
Patch Set #6, Line 33:
XR_TYPE_GRAPHICS_BINDING_D3D11_KHRAlso initialize pointers to nullptr here?
File device/vr/openxr/windows/openxr_graphics_binding_d3d11.cc:
Patch Set #6, Line 35:
typelssNit: typo.
To view, visit change 4461233. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Cooper (Gerrit)
Attention is currently required from: Brandon Jones, Kenneth Russell, Piotr Bialecki.
16 comments:
Patchset:
Going forward I'd like to see a lot less #ifs sprinkled throughout the code, but as a first step of […]
Yeah, agreed. The ultimate goal as we progress through this work will be to identify places where the #ifs can be abstracted to a platform-specific implementation of some type. Partially this is an attempt at doing the basic chunks of work and then creating areas where I could maybe parallelize some of the other work.
Patchset:
Some good feedback on reducing ifdefs by consolidating some of the render logic. I'd like to tackle that as part of a more wholistic rendering-focused refactor/pass at implementing the Android renderer so that I can get a clearer idea of what a good API shape would be.
File DEPS:
Patch Set #6, Line 152:
only on WindowsNit: update.
Done
File device/vr/openxr/android/openxr_graphics_binding_open_gles.h:
Patch Set #6, Line 26:
XrGraphicsBindingOpenGLESAndroidKHR
Also initialize pointers to nullptr here? I'm not sure what c++ will do with them if we aren't expli […]
Done
File device/vr/openxr/android/openxr_graphics_binding_open_gles.cc:
Patch Set #6, Line 45:
color_swapchain_images.emplace_back();
Shouldn't this be emplacing... […]
In a future change it likely will. For now it's just invoking the default constructor on SwapChainInfo.
File device/vr/openxr/openxr_api_wrapper.h:
Patch Set #6, Line 53:
// and D3D11Fence for each D3D11 texture in the vector.Nit: Might want to remove/relocate the D3D references in comments like these as well.
I moved the comment down to the members.
Feels like this is an good opportunity to have an abstract SwapChainInfo class with platform-specifi […]
The biggest issue that I have with that vs. if-def'd types on a concrete class (though maybe it shouldn't be) is the additional boilerplate to do the typecast.
At any rate; though I haven't filed a bug on this, I'd like to defer this to the TODO at the top of the struct 😊 LMK if you agree and I can file a bug to capture this as an option during that refactor.
File device/vr/openxr/openxr_api_wrapper.cc:
Patch Set #6, Line 749:
#endif
Is this code that could live in the OpenXrGraphicsBinding? Feels like trying to shift a fair amount […]
That seems reasonable, we also don't seem to always forward it. I may defer that to a later change if you don't object.
Patch Set #6, Line 771:
#endif
Similarly, feels like this logic could comfortably reside in the OpenXrGraphicsBinding too. […]
Hmm, I could certainly move the "WaitOnFence" stuff that calls this into the OpenXrGraphicsBinding which would allow that to move, but there's some trickiness with storing the fence if we want to keep it somewhere (though we don't actually guarantee that the fence is associated with the same image here...) I think I'd like to take a pass at all of the rendering stuff separately and more wholistically as part of a future change. My goal with this change was essentially to allow building for Android and to abstract just the things that were trivially easy to "implement" on Android that were required to call the various methods so that they *could* all theoretically be called.
File device/vr/openxr/openxr_graphics_binding.h:
Patch Set #6, Line 22:
virtual const void* get() const = 0;
Having this method just named `get()` threw me off a little, especially because it just returns a vo […]
Yeah, I think that was my original intent (e.g. it was just a way to abstract out the struct we need to pass to XrSessionCreateInfo.next); but even before your suggestions to abstract more work into here (which I'll likely do) it grew a little beyond that. I've renamed it to `GetSessionCreateInfo()`
File device/vr/openxr/openxr_render_loop.cc:
Patch Set #6, Line 67:
texture_helper_.SetBackbuffer(swap_chain_info->d3d11_texture.get());
It's a bit unfortunate that these types of calls are sprinkled all throughout the code, but I unders […]
I'd like to defer a decision on texture_helper_ for the time being. I'm not sure if I want the graphics binding to become graphics helper or if we want a separate texture_helper, and I think I'll have more clarity once I try to get things rendering on Android.
File device/vr/openxr/openxr_util.cc:
// Since the OpenXR backend only knows how to draw with D3D11 at the moment,
// the XR_KHR_D3D11_ENABLE_EXTENSION_NAME is required.Nit: update.
Moved. Leaving open because there's another change I need to check this on lower down too.
File device/vr/openxr/windows/openxr_graphics_binding_d3d11.h:
Patch Set #6, Line 33:
XR_TYPE_GRAPHICS_BINDING_D3D11_KHRAlso initialize pointers to nullptr here?
Done
File device/vr/openxr/windows/openxr_graphics_binding_d3d11.cc:
Patch Set #6, Line 35:
typelssNit: typo.
Probably existing. but Fixed.
Patch Set #6, Line 45:
CHECK(color_swapchain == XR_NULL_HANDLE);Pretty certain this mistaken CHECK is the cause of the test failures, switched to !=
File device/vr/windows/compositor_base.cc:
Patch Set #6, Line 604:
#endif
Can we reduce the number of #ifs here by structuring it like so? […]
The yak would like to thank you for it's nice new trim.
To view, visit change 4461233. To unsubscribe, or for help writing mail filters, visit settings.
Brandon Jones (Gerrit)
Attention is currently required from: Alexander Cooper, Kenneth Russell, Piotr Bialecki.
Patch set 7:Code-Review +1
5 comments:
Patchset:
Thanks for the responses!
File device/vr/openxr/openxr_api_wrapper.h:
The biggest issue that I have with that vs. […]
That's fine. Let's get it captured in a bug and defer this and other larger refactors below to a follow up.
File device/vr/openxr/openxr_api_wrapper.cc:
Patch Set #6, Line 749:
#endif
That seems reasonable, we also don't seem to always forward it. […]
Acknowledged
Patch Set #6, Line 771:
#endif
Hmm, I could certainly move the "WaitOnFence" stuff that calls this into the OpenXrGraphicsBinding w […]
Acknowledged
File device/vr/openxr/openxr_render_loop.cc:
Patch Set #6, Line 67:
texture_helper_.SetBackbuffer(swap_chain_info->d3d11_texture.get());
I'd like to defer a decision on texture_helper_ for the time being. […]
Acknowledged
To view, visit change 4461233. To unsubscribe, or for help writing mail filters, visit settings.
Viatcheslav Ostapenko (Gerrit)
Attention is currently required from: Alexander Cooper, Kenneth Russell, Piotr Bialecki.
1 comment:
Patchset:
I have working WIP implementation here: https://chromium-review.googlesource.com/c/chromium/src/+/4379532
PTAL, may be you find anything useful there.
To view, visit change 4461233. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Cooper (Gerrit)
Attention is currently required from: Brandon Jones, Geoff Lang, Kenneth Russell, Piotr Bialecki.
Alexander Cooper would like Geoff Lang to review this change.
29 files changed, 551 insertions(+), 214 deletions(-)
Alexander Cooper (Gerrit)
Attention is currently required from: Brandon Jones, Geoff Lang, Kenneth Russell, Piotr Bialecki.
Patch set 9:Auto-Submit +1
1 comment:
Patchset:
Geoff could I get a +1 for the (moved) ui/gl DEP (from device/vr/android to device/vr)? I think Ken had intended to just stamp after Brandon's +1, but he looks to be OOO today.
To view, visit change 4461233. To unsubscribe, or for help writing mail filters, visit settings.
Geoff Lang (Gerrit)
Attention is currently required from: Alexander Cooper, Brandon Jones, Kenneth Russell, Piotr Bialecki.
Patch set 9:Code-Review +1Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Begin building OpenXr on Android
Refactors windows-specific code that has crept into the OpenXr classes
by currently wrapping most of them in ifdefs. In a few cases I've
extracted some classes to use temporarily to allow key methods to remain
visible for Android (BeginFrame and InitSession the most critical).
Some of these classes will change slightly with other refactor work that
was identified and some would be good candidates for future cleanup.
At the moment, OpenXr is not added as a supported runtime on Android,
this simply begins it building on Android. Future changes will hook up
the rest of the stack to consider/query for an OpenXr runtime.
Bug: 1435548
Change-Id: I9bdfe5938f3f08efbc62c8d40fe0ec0dc6a0754f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4461233
Auto-Submit: Alexander Cooper <alco...@chromium.org>
Reviewed-by: Brandon Jones <baj...@chromium.org>
Commit-Queue: Geoff Lang <geof...@chromium.org>
Reviewed-by: Geoff Lang <geof...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1137896}