Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ApiLayerInterface::LoadApiLayers() failing to load available and enabled API layer #155

Merged
merged 1 commit into from Jan 23, 2020

Conversation

ghost
Copy link

@ghost ghost commented Dec 20, 2019

In src/loader/api_layer_interface.cpp, when ApiLayerInterface::LoadApiLayers() is called, from api_layer_interface.cpp:197 up until api_layer_interface.cpp:234,
the OpenXR Loader constructs a list of API layers in the list enabled_api_layers.
I have not analyzed this section of code closely, but in my case, this section of code adds XR_APILAYER_LUNARG_core_validation to the enabled_api_layers list twice.

This results in the following bug when the below code is executed:

for (uint32_t layer = 0; layer < enabled_api_layers.size(); ++layer) {
    if (enabled_api_layers[layer] == manifest_file->LayerName()) {
    layer_found[layer] = true;
    enabled = true; 
    break;
  }
}

The break statement at the end of the for loop guarantees that a second API layer in the enabled_api_layers list of the exact same name as an API layer earlier in the list will never have its layer_found[layer] value set to "true", thus triggering the following error:

Error [GENERAL | xrCreateInstance | OpenXR-Loader] : ApiLayerInterface::LoadApiLayers - failed to find layer XR_APILAYER_LUNARG_core_validation

A hacky way of fixing this is by simply commenting out the break statement, and this seems to work fine with no side effects. (This pull request simply removes the break statement.)

A more proper fix might involve checking any API layer name against the current list of enabled_api_layers before adding it to the enabled_api_layers list.

… API layer is added to enabled_api_layers list twice
@claassistantio
Copy link

claassistantio commented Dec 20, 2019

CLA assistant check
All committers have signed the CLA.

@rpavlik
Copy link
Contributor

rpavlik commented Jan 22, 2020

Did you make any progress on figuring out the "correct" fix here?

@ghost
Copy link
Author

ghost commented Jan 23, 2020

@rpavlik No, I have not worked on or investigated this issue further.

@rpavlik
Copy link
Contributor

rpavlik commented Jan 23, 2020

OK. The issue was because it was being enabled in two places, right? I'm happy with merging this as long as we have an issue filed for the underlying problem.

@rpavlik rpavlik self-assigned this Jan 23, 2020
@ghost
Copy link
Author

ghost commented Jan 23, 2020

Not sure if there's an issue ticket for this opened anywhere separate from this PR,
but yes, the issue was a combination of:

  1. The API layer was enabled in two places, and
  2. The code for checking whether a layer is found (the for loop mentioned in this PR) will never check if the second copy of the enabled API layer is found due to the break statement.

Thus the second mention of the API layer's layer_found value defaults to false, generating an error about the API layer not being found.

@rpavlik
Copy link
Contributor

rpavlik commented Jan 23, 2020

Now there is one :) Thanks for the patch!

@rpavlik rpavlik merged commit d4a83bc into KhronosGroup:master Jan 23, 2020
rpavlik added a commit that referenced this pull request Jan 25, 2020
Patch release for the 1.0 series.

This release contains, among other things, a substantial simplification and
cleanup of the loader, which should fix a number of issues and also make it
forward compatible with extensions newer than the loader itself. As a part of
this change, the loader itself now only supports a single `XrInstance` active at
a time per process. If you attempt to create a new instance while an existing
one remains (such as in the case of application code leaking an `XrInstance`
handle), the loader will now return `XR_ERROR_LIMIT_REACHED`.

### GitHub Pull Requests

These had been integrated into the public repo incrementally.

- hello_xr
  - Initialize hand_scale to 1.0
    <#157>
  - Fix Vulkan CHECK_CBSTATE build under newer MSVC
    <#154>
  - Initialize hand_scale to 1.0 to still show controller cubes even if
    grabAction not available on startup.
    <#157>
- Loader
  - Single instance loader refactor with forward compatibility
    <#146> (and internal
    MRs 1599, 1621)
  - Fix bug in loading API layers that could result in not loading an available
    and enabled layer
    <#155>
- Build
  - Clean up linking, build loader and layers with all available
    platform/presentation support, fix pkg-config file, rename `runtime_list`
    test executable to `openxr_runtime_list`
    <#149>

### Internal issues

- Registry
  - Fix typo in visibility mesh enum comment.
  - Add `XR_EXT_win32_appcontainer_compatible` extension.
- Scripts
  - Fix comment typos.
  - Sync scripts with Vulkan. (internal MR 1625)
- Loader
  - Allow use of `/` in paths in FileSysUtils on Windows.
- Build
  - Improve messages
- hello_xr
  - Add D3D12 graphics plugin (internal MR 1616)
  - Fix comment typo.
rpavlik added a commit to KhronosGroup/OpenXR-SDK that referenced this pull request Jan 25, 2020
Patch release for the 1.0 series.

This release contains, among other things, a substantial simplification and
cleanup of the loader, which should fix a number of issues and also make it
forward compatible with extensions newer than the loader itself. As a part of
this change, the loader itself now only supports a single `XrInstance` active at
a time per process. If you attempt to create a new instance while an existing
one remains (such as in the case of application code leaking an `XrInstance`
handle), the loader will now return `XR_ERROR_LIMIT_REACHED`.

### GitHub Pull Requests

These had been integrated into the public repo incrementally.

- hello_xr
  - Initialize hand_scale to 1.0
    <KhronosGroup/OpenXR-SDK-Source#157>
  - Fix Vulkan CHECK_CBSTATE build under newer MSVC
    <KhronosGroup/OpenXR-SDK-Source#154>
  - Initialize hand_scale to 1.0 to still show controller cubes even if
    grabAction not available on startup.
    <KhronosGroup/OpenXR-SDK-Source#157>
- Loader
  - Single instance loader refactor with forward compatibility
    <KhronosGroup/OpenXR-SDK-Source#146> (and internal
    MRs 1599, 1621)
  - Fix bug in loading API layers that could result in not loading an available
    and enabled layer
    <KhronosGroup/OpenXR-SDK-Source#155>
- Build
  - Clean up linking, build loader and layers with all available
    platform/presentation support, fix pkg-config file, rename `runtime_list`
    test executable to `openxr_runtime_list`
    <KhronosGroup/OpenXR-SDK-Source#149>

### Internal issues

- Registry
  - Fix typo in visibility mesh enum comment.
  - Add `XR_EXT_win32_appcontainer_compatible` extension.
- Scripts
  - Fix comment typos.
  - Sync scripts with Vulkan. (internal MR 1625)
- Loader
  - Allow use of `/` in paths in FileSysUtils on Windows.
- Build
  - Improve messages
- hello_xr
  - Add D3D12 graphics plugin (internal MR 1616)
  - Fix comment typo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants