Implement surface-to-surface copy in GL backend #2892
Conversation
| gl.bind_framebuffer(glow::DRAW_FRAMEBUFFER, Some(dst_fbo)); | ||
| gl.framebuffer_renderbuffer(glow::DRAW_FRAMEBUFFER, glow::COLOR_ATTACHMENT0, glow::RENDERBUFFER, Some(dst)); | ||
|
|
||
| gl.blit_framebuffer( |
There was a problem hiding this comment.
I was writing about 5 different comments on how to improve this, but at the end of the day I realized that blitting an FBO is just wrong here. Blitting is a graphics operation, it effectively samples a surface and writes into another surface. Those samples are writes do the format unpacking/packing, which is not what a copy needs. A copy doesn't respect any format conversions: you can copy from RGBA8Uint to R32Float, and it would still work in Vulkan and gfx-hal. But it wouldn't work with a blit operation, since it would only be able to write done one R component.
Unfortunately, GLES or GL Core don't have glCopyPixels or glDrawPixels, so I'm not entirely sure what to do here in general case... need to give it more thought.
We can still have this code path you wrote, but only when the formats of source and destination match exactly.
There was a problem hiding this comment.
I see. Thank you for the great explanation!
I had the idea of trying to circumvent this by implementing texture-to-texture copy in CopyImageToTexture instead of using surfaces. I tried changing create_image to create textures instead of render buffers when either TRANSFER_SRC or TRANSFER_DST usage flags are set, and then using the already implemented CopyTextureToBuffer and CopyBufferToTexture commands to perform the copy operation. However, CopyTextureToBuffer doesn't seem to work on WebGL. In any case, this solution would probably be very slow, right?
It's quite unfortunate that there is no easy way to implement this. Do you know if there is at least an easy way to check the surface format so we can keep this code as you advised? I haven't been able to find a way.
There was a problem hiding this comment.
However, CopyTextureToBuffer doesn't seem to work on WebGL.
Oh wow, this is sad. @grovesNL - heads up in case you have ideas :)
In any case, this solution would probably be very slow, right?
It's undesirable. Theoretically, GL renderbuffers could be more efficient than regular textures, but I don't recall any benchmarks confirming this :)
Do you know if there is at least an easy way to check the surface format
Let's just keep it in the Surface type that we have. We know it at creation time, at least.
src/backend/gl/src/queue.rs
Outdated
| data.dst_offset.x + data.extent.width as i32, | ||
| data.dst_offset.y + data.extent.height as i32, | ||
| glow::COLOR_BUFFER_BIT, | ||
| glow::LINEAR, |
src/backend/gl/src/queue.rs
Outdated
| glow::LINEAR, | ||
| ); | ||
|
|
||
| gl.bind_framebuffer(glow::READ_FRAMEBUFFER, None); |
There was a problem hiding this comment.
can just do glow::FRAMEBUFFER to cover both
2895: Fix WebGL always presenting latest bound FBO r=msiglreith a=hecrj When working on #2892, I noticed that when using WebGL with the GL backend, the latest bound FBO was being blitted to the default framebuffer, instead of the correct swapchain FBO. The changes in this PR fix this issue while: - Unifying swapchain creation and presentation - Removing related unused code - Removing a related unnecessary unsafe block PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [ ] tested examples with the following backends: - [ ] `rustfmt` run on changed code Co-authored-by: Héctor Ramón Jiménez <[email protected]>
hecrj
force-pushed
the
gl/surface-to-surface-copy
branch
from
00ec108 to
ee0a924
Compare
|
I have implemented the discussed changes. Let me know if there is anything that still needs to be addressed! |
2892: Implement surface-to-surface copy in GL backend r=kvark a=hecrj This is probably incomplete (and wrong). Now, it just always copies the color attachment. I imagine we should check the subresource layers in `data`. I would appreciate some guidance, is that easily implementable from here? PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [ ] tested examples with the following backends: - [ ] `rustfmt` run on changed code Co-authored-by: Héctor Ramón Jiménez <[email protected]>
Build failed |
|
bors retru |
|
bors retry |
2892: Implement surface-to-surface copy in GL backend r=kvark a=hecrj This is probably incomplete (and wrong). Now, it just always copies the color attachment. I imagine we should check the subresource layers in `data`. I would appreciate some guidance, is that easily implementable from here? PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [ ] tested examples with the following backends: - [ ] `rustfmt` run on changed code Co-authored-by: Héctor Ramón Jiménez <[email protected]>
Build failed |
|
Bors retry
… On Jul 29, 2019, at 13:32, bors[bot] ***@***.***> wrote:
Build failed
continuous-integration/travis-ci/push
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
2892: Implement surface-to-surface copy in GL backend r=kvark a=hecrj This is probably incomplete (and wrong). Now, it just always copies the color attachment. I imagine we should check the subresource layers in `data`. I would appreciate some guidance, is that easily implementable from here? PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [ ] tested examples with the following backends: - [ ] `rustfmt` run on changed code Co-authored-by: Héctor Ramón Jiménez <[email protected]>
Build succeeded |
This is probably incomplete (and wrong). Now, it just always copies the color attachment. I imagine we should check the subresource layers in
data. I would appreciate some guidance, is that easily implementable from here?PR checklist:
makesucceeds (on *nix)make reftestssucceedsrustfmtrun on changed code