OpenGL surface via surfman #3151
Conversation
Adds a surfman backend to gfx-backend-gl and makes it the default backend for unix platforms.
zicklag
force-pushed
the
wgpu-gl-support
branch
from
d35f34b to
5a428ce
Compare
|
If surfman works do you think we remove the I'm asking because I think I might take out the glutin code just to help me think while understanding the code and avoid a bunch of feature gates, even if I add it in again afterwards. |
|
Another question: for the |
|
@zicklag I haven't investigated
I don't think there should be a headless instance type. Ideally, we'd expose it at the physical device level. One physical device would be "software", while another will be "hardware". When the user creates a surface, they don't know about a physical device, but the API clearly has means to express that different physical devices can work with different surfaces via supports_queue_family (each physical device would only expose one queue family, and you can make it so the headless physical device is only compatible with headless surfaces. I.e. the only point of divergence on the API level would be the way a surface is created: we could have |
Sounds good.
I think I get it. I'll see about that then. I'll try to get non-headless to work first. Also, what is |
|
WGL is basically window system interface in Windows for OpenGL. It's what you use to set up the GL context (internally by glutin, or by other means).
… On Feb 29, 2020, at 20:40, Zicklag ***@***.***> wrote:
So all in all, I would prefer the glutin path to stay, even if it's only partially usable today.
Sounds good.
Ideally, we'd expose it at the physical device level. One physical device would be "software", while another will be "hardware".
I think I get it. I'll see about that then. I'll try to get non-headless to work first.
Also, what is wgl? Is that for windows?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
So ideally would we want to support |
|
Overall, Windows is least of a priority for GL (so just because For your work, try to focus on Linux & Android first and foremost, then macOS/iOS, and only afterwards - Windows. No need to consider a separate WGL code path at this point :) |
|
I think I have the I managed to leave the Requires a |
|
Perhaps, we need to call |
|
Oops, I had that in there earlier! I lost it in a refactor. :) |
|
Do you have any idea what's wrong if I'm getting undefined symbols while linking X11: And more like that. I've got a weird setup right now because I'm not an admin on the system I'm using to build, but it doesn't have Apparently I did something wrong, but what does the undefined symbol mean? Does that mean I'm not linking X11 in? Like I'm missing a |
|
Yes definitely missing a link to x11 library. |
|
Ah, thanks. Got it to by adding I don't know why it didn't "Just Work". Maybe Now I've got a panic in |
|
Hey, we're getting close. You have any idea what this means? The panic is from here. And the entire trace log: |
|
There is a blit happening, and it complains that one of the framebuffers is incomplete. First of all, I would assume that your code path does not need a blit. With your surface implementation, rendering to a swapchain should be direct, no copies/blits involved. |
|
OK, I found out where the blit is: gfx/src/backend/gl/src/queue.rs Lines 219 to 234 in 106cb21 It is probably erroring because I do not have a swap buffers call for But, you did say that you don't think there should be any copying anyway. Is there a way around using the blit in this case? |
|
Here is how OpenGL allows one to render a frame: you draw framebuffer(0), and you render, done. Here is how gfx-rs allows one to render a frame: you get the next image, then you have/create a view into it, then you create a framebuffer that includes that view. That framebuffer can include other views as well, such as your manially depth texture. Then you execute a render pass that renders into that framebuffer. If we don't blit, we would require that if a framebuffer contains the swapchain image (in GL), it has to have nothing else. This is not what users expect... So instead, we create a render buffer internally, render into it, and then when it's time to present - we blit into the GL framebuffer(0). That blit is costy and highly unfortunate. However, Needless to say, you are unravelling one hack in GL after another, and it's sadly all connected. Requires heroic effort to defeat, but Rust community will be very happy :) |
|
Thanks for the explanation and the help. I'm glad we've gotten this far. :) I'll try to process that and look around and show you what I come up with. |
There was a problem hiding this comment.
As the default features being our last change, it would be great to try taking the quad example from hal-0.5 branch, moving it out, and compiling with "GL" feature against your modified code. This will require glutin to be enabled by default - it's fine for hal-0.5, since any user can pass --no-default-features. If it doesn't work at all, well, that's not the end of the world, we'll just make sure to release 0.6 sooner :)
- Re-order dependencies in examples Cargo.toml - Make sure that GL is tested in reftests - Remove cfg_aliases `use` from `build.rs` - Make surfman take precedence over glutin when both features are enabled. - Remove unused winit dependency - Only include winapi when using the `wgl` feature - Take out DEPTH and STENCIL flags from GL context - Remove unnecessary drop() call - Remove unneeded GL `cfg_attr`'s from warden
![monocodus[bot]](https://avatars.githubusercontent.com/in/28526?s=60&v=4){kind=small}
|
Unfortunately I just found that the reftests are segfaulting for GL and the makefile wasn't testing GL on CI properly. I'll have to look into that. I just pushed a commit that addresses all of the points of the review and additionally allows you to run with both the
I'll try that out. Edit: Oh, servo/surfman#172 just got merged, too, so I'll update the |
|
For some reason it is segfaulting right here: gfx/src/backend/gl/src/info.rs Line 126 in 99dc116 It is when it tries to get the vendor name from the GL context. There it isn't happening when running the Any clue what might cause that? |
|
Is SetCurrentContext being called?
… On Apr 12, 2020, at 15:54, Zicklag ***@***.***> wrote:
For some reason it is segfaulting right here:
https://github.com/gfx-rs/gfx/blob/99dc1164bd77bb56344dc79c2e510874ac8c9f34/src/backend/gl/src/info.rs#L126
It is when it tries to get the vendor name from the gl context. There it isn't happening when running the quad example, but it will happen if you re-order the creation of the surface and the enumeration of the adapters.
Any clue what might cause that?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Ah, thank you, that is definitely the cause. Apparently I was just getting lucky with the fact that it happened to be working with the order that the current The problem is that when I try to run |
Fixes a segfault that could happen when enumerating adapters and fixed the issue where only one adapter could be listed.
|
@kvark I found out where to put the gfx/src/backend/gl/src/window/surfman.rs Lines 314 to 317 in c4e2c2d It is the earliest spot that I could find after enumerating adapters that has access to the specific surface that needs to be made current. I tried sticking it in It seems like maybe we should be making the context current on de-reference, but I still don't know how to fix the black screen that happens when we do that, and I don't know how to go about debugging it. I feel like we can leave it for now and fix it later. The reftests aren't segfaulting anymore but they are hitting Also, CI isn't set to run the reftests, but there is a Makefile entry specifically for CI reftests. Does that need to be added to the Travis config? |
|
I just tested the following steps and got the
So as long as I'm not missing something other than the default features that makes this change breaking, I think we can safely backport it to 0.5! |
|
@zicklag what happens if the example tries to use the |
Re-adds some "unused" re-exports that were removed in the surfman update. Other than the default features, this makes the surfman update backward compatible with `hal-0.5`.
|
Ah, I get it now, we want to test the old examples to make sure that they still work as a validation of the fact that our changes are still backward compatible. 👍 I tested it and, good news! The examples work without changes after the latest commit I made to re-add some "unused" imports/exports in the That means that the only difference that will be necessary for the backport and the master commit will be to make sure the default features and examples stay unchanged from |
|
That's wonderful! |
|
Not that I know of. Some things such as when the context gets made current might not be technically correct or work in all cases, but that is kind of the story with the whole backend right now. 😉 I think its ready. If we notice anything else after we can make follow-up PRs. |
|
Build succeeded: |
|
@zicklag would you want to follow-up with a PR to hal-0.5 that has this code with the default features fixed? |
|
Yeah, I can probably have that PR ready by tonight. |
Fixes/Discussion: gfx-rs/wgpu#450
PR checklist:
makesucceeds (on *nix)make reftestssucceedsrustfmtrun on changed code