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 logic in egl Device::query_device #1686

Merged
merged 11 commits into from
Jul 21, 2024
Merged

Conversation

quaternic
Copy link
Contributor

@quaternic quaternic commented Jul 1, 2024

The previous test would mistakenly return an error if the extensions contain "EGL_EXT_device_base" but not the two others. As stated by the comment, and in https://registry.khronos.org/EGL/extensions/EXT/EGL_EXT_device_base.txt , it is supposed to contain the other two.

The cases that should not return an error are:

  • contains EGL_EXT_device_base (B)
  • contains both EGL_EXT_device_enumeration (E) and EGL_EXT_device_query (Q)

The overall effects of this patch in terms of those three extensions are:

  • !B & E & !Q leads to a different error message than before
  • B & !E & !Q no longer errors
  • !B & E & Q no longer errors

@kchibisov
Copy link
Member

cc @i509VCB I think the test was similar to the one in smithay?

@quaternic
Copy link
Contributor Author

Huh, looks like smithay is requiring all three are present: https://github.com/Smithay/smithay/blob/41712470abede74dab8a039ce048c70d56115ce6/src/backend/egl/device.rs#L31

Based on the existing comment in the glutin source, the check was intended to do the correct thing, but just got the operators mixed up in the code. The minimal diff would have been just substituting && for || and vice versa, but I also reordered it to check for the combined extension first.

Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add a CHANGELOG entry.

@i509VCB
Copy link
Contributor

i509VCB commented Jul 3, 2024

The previous test would mistakenly return an error if the extensions contain "EGL_EXT_device_base" but not the two others. As stated by the comment, and in https://registry.khronos.org/EGL/extensions/EXT/EGL_EXT_device_base.txt , it is supposed to contain the other two.

The cases that should not return an error are:

  • contains EGL_EXT_device_base
  • contains both EGL_EXT_device_enumeration and EGL_EXT_device_query

The changes look correct the specification for EGL_EXT_device_base states the following:

Add to section "3.2 Devices"

    "EGL_EXT_device_base is equivalent to the combination of the
    functionality defined by EGL_EXT_device_query and
    EGL_EXT_device_enumeration."

@quaternic quaternic requested a review from kchibisov July 3, 2024 23:24
Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, the original logic doesn't match what's described in the comment :)

glutin/src/api/egl/device.rs Outdated Show resolved Hide resolved
glutin/src/api/egl/device.rs Outdated Show resolved Hide resolved
Co-authored-by: Marijn Suijten <[email protected]>
glutin/src/api/egl/device.rs Outdated Show resolved Hide resolved
glutin/src/api/egl/device.rs Outdated Show resolved Hide resolved
@kchibisov kchibisov requested a review from MarijnS95 July 15, 2024 12:09
@kchibisov kchibisov merged commit 90ebeb5 into rust-windowing:master Jul 21, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants