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

Add COPY_SRC to Metal's surface usage bits. #4852

Merged
merged 1 commit into from
Dec 10, 2023
Merged

Add COPY_SRC to Metal's surface usage bits. #4852

merged 1 commit into from
Dec 10, 2023

Conversation

Toqozz
Copy link
Contributor

@Toqozz Toqozz commented Dec 9, 2023

Description
On machines running Metal, we could not configure surfaces with the COPY_SRC usage flag. This flag is used a pretty often on platforms that do support it, and often you'll have macOS users submitting a bug to the root project, which requires them to implement a backbuffer or some other workaround.

See "Discussion".

Testing
The simplest way to test is to add a wgpu::TextureUsages::COPY_SRC usage flag to hello_triangle example's surface configuration. Prior to this change (and given you're on macOS), you'll get a panic Error in Surface::configure: requested usage is not supported. This change fixes the panic.

Discussion
I can't find anything that says why this shouldn't be/isn't possible. I've tested the change on an M1 Pro Macbook which works without any issues, and I've even run it with Metal API validation enabled -- no errors. I found this pull request from Feb 2022, which added the COPY_DST flag with similar concerns, but no real answers.

From reading the Metal documentation (I'm a novice, to be clear), it seems that the flag that really matters is isFrameBufferOnly, which we already set appropriately depending on whether the texture usage is only RENDER_ATTACHMENT.

Textures obtained from a CAMetalDrawable object may only be usable as attachments, depending on the value of framebufferOnly passed to their parent CAMetalLayer object. Textures created directly by the app do not have such restrictions.
https://developer.apple.com/documentation/metal/mtltexture/1515749-isframebufferonly

https://developer.apple.com/documentation/metal/onscreen_presentation/reading_pixel_data_from_a_drawable_texture

So everything seems fine. We could probably allow more flags, but this at least brings things in line with DX12.

@Toqozz Toqozz requested a review from a team as a code owner December 9, 2023 01:22
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!
Digged around a tiny bit as well and also couldn't find any reason why the texture shouldn't be copyable.

Could you please add a brief changelog line about this bugfix?

@Toqozz
Copy link
Contributor Author

Toqozz commented Dec 10, 2023

Sure thing.

@Wumpf Wumpf enabled auto-merge (squash) December 10, 2023 08:23
@Wumpf Wumpf merged commit eff9a36 into gfx-rs:trunk Dec 10, 2023
27 checks passed
@Toqozz Toqozz deleted the Toqozz-copysrc branch December 10, 2023 08:40
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