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

[gl] replace Surfman by EGL #3470

Merged
merged 1 commit into from Nov 9, 2020
Merged

[gl] replace Surfman by EGL #3470

merged 1 commit into from Nov 9, 2020

Conversation

kvark
Copy link
Member

@kvark kvark commented Nov 9, 2020

Fixes #3468

This is a BIG change. It re-architects WSI of the GL backend to unconditionally use EGL (via khronos-egl crate atm, but we can change that). This means the backend is only supported on Linux/BSD/Android, it's no longer supported on Apple and Microsoft platforms.

One of the consequences - we throw Surfman away. There was too much complication and too little benefit from it anyway.

Another consequence - we are locked to GLES, given that I experienced issues trying to get a desktop GL context via EGL. We can revisit this, but really I think going with GLES 3.1 makes total sense for the backend, unconditionally. if the target platform is powerful above what GLES 3.1 offers, it should just support Vulkan.

Most importantly, it solves GL context sharing, so presentation is much more robust. However, it doesn't solve the extra copy - #3469. For this, we'll need to follow-up by directly using platform extensions, such as EGL_WL_create_wayland_buffer_from_image.

PR checklist:

  • make succeeds (on *nix)
  • make reftests succeeds
  • tested examples with the following backends: Linux

@kvark kvark requested a review from grovesNL November 9, 2020 06:35
Copy link
Contributor

@grovesNL grovesNL left a comment

Choose a reason for hiding this comment

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

This looks great, thank you!

I think this is a good direction 👍 I think we can probably use a similar path for WebGL by creating contexts ahead of time and later reusing them.

Only supporting OpenGL ES sounds fine for now, we can always look into the issues with desktop OpenGL later if it's needed by anyone.

bors r+

bors bot added a commit that referenced this pull request Nov 9, 2020
3470: [gl] replace Surfman by EGL r=grovesNL a=kvark

Fixes #3468

This is a BIG change. It re-architects WSI of the GL backend to unconditionally use EGL (via `khronos-egl` crate atm, but we can change that). This means the backend is *only* supported on Linux/BSD/Android, it's no longer supported on Apple and Microsoft platforms.

One of the consequences - we throw Surfman away. There was too much complication and too little benefit from it anyway.

Another consequence - we are locked to GLES, given that I experienced issues trying to get a desktop GL context via EGL. We can revisit this, but really I think going with GLES 3.1 makes total sense for the backend, unconditionally. if the target platform is powerful above what GLES 3.1 offers, it should just support Vulkan.

Most importantly, it solves GL context sharing, so presentation is much more robust. However, it doesn't solve the extra copy - #3469. For this, we'll need to follow-up by directly using platform extensions, such as [EGL_WL_create_wayland_buffer_from_image](https://www.khronos.org/registry/EGL/extensions/WL/EGL_WL_create_wayland_buffer_from_image.txt).

PR checklist:
- [x] `make` succeeds (on *nix)
- [x] `make reftests` succeeds
- [x] tested examples with the following backends: Linux


Co-authored-by: Dzmitry Malyshau <kvark@fastmail.com>
@bors
Copy link
Contributor

bors bot commented Nov 9, 2020

Build failed:

@grovesNL
Copy link
Contributor

grovesNL commented Nov 9, 2020

Oh I didn't notice the CI failures, we need to add the egl package on Linux and disable gl on macOS.

I'm not sure about the Android failure but maybe we could disable CI on gl there temporarily:

thread 'main' panicked at 'called Result::unwrap() on an Err value: CrossCompilation', /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/khronos-egl-2.1.1/build.rs:7:10

https://github.com/gfx-rs/gfx/pull/3470/checks?check_run_id=1372587404#step:5:83

@kvark
Copy link
Member Author

kvark commented Nov 9, 2020

Filed timothee-haudebourg/khronos-egl#6, will disable Android here for now

@kvark kvark merged commit 94b71d5 into gfx-rs:master Nov 9, 2020
@kvark kvark deleted the egl branch November 9, 2020 16:00
@VincentJousse
Copy link

This looks great, thank you!

I think this is a good direction 👍 I think we can probably use a similar path for WebGL by creating contexts ahead of time and later reusing them.

Only supporting OpenGL ES sounds fine for now, we can always look into the issues with desktop OpenGL later if it's needed by anyone.

bors r+

It would be very useful here to retrieve WebGL support. I have no dev background in neither WebGL nor OpenGL, but I may help somehow.

@kvark
Copy link
Member Author

kvark commented Nov 12, 2020

We are definitely going to keep WebGL support, although I haven't tested it in a while. You can try following the instructions in ccf789d on master to see where we stand now? I hope it's not broken, or otherwise it should have been caught by CI.

@VincentJousse
Copy link

gfx-backend-gl doesn’t build with wasm32-unknown-unknown target since Surface struct doesn’t implement present method called here

@kvark
Copy link
Member Author

kvark commented Nov 13, 2020

Ah ok. So you can definitely help by fixing this and re-enabling the WASM build on CI!

@MarcAntoine-Arnaud MarcAntoine-Arnaud mentioned this pull request Nov 14, 2020
4 tasks
bors bot added a commit that referenced this pull request Nov 21, 2020
3475: re-enable WebGL support r=grovesNL a=MarcAntoine-Arnaud

Mentionned in #3470
Maybe help issue #2749

PR checklist:
- [x] re-enable WebGL rendering
- [x] use published version of `spirv_cross` dependency
- [x] re-enable CI
- [x] check and update examples



Co-authored-by: Marc-Antoine Arnaud <maarnaud@media-io.com>
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.

Idiomatic OpenGL context directly on EGL
3 participants