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

Strange usage of TextureFormatFeatureFlags::STORAGE_READ_WRITE #6599

Closed
atlv24 opened this issue Nov 24, 2024 · 10 comments · Fixed by #6642
Closed

Strange usage of TextureFormatFeatureFlags::STORAGE_READ_WRITE #6599

atlv24 opened this issue Nov 24, 2024 · 10 comments · Fixed by #6642
Labels
area: validation Issues related to validation, diagnostics, and error handling type: bug Something isn't working

Comments

@atlv24
Copy link
Contributor

atlv24 commented Nov 24, 2024

idk whats going on here but it looks wrong. if its not wrong it needs docs. why is read_write needed for read_only? why is write_only never checked?

wgt::StorageTextureAccess::WriteOnly => hal::TextureUses::STORAGE_READ_WRITE,
wgt::StorageTextureAccess::ReadOnly => {
if !view
.format_features
.flags
.contains(wgt::TextureFormatFeatureFlags::STORAGE_READ_WRITE)
{
return Err(Error::StorageReadNotSupported(view.desc.format));
}
hal::TextureUses::STORAGE_READ
}

@teoxoy
Copy link
Member

teoxoy commented Nov 25, 2024

That does look wrong, we need a new TextureFormatFeatureFlags::STORAGE_READ flag and TextureFormatFeatureFlags::STORAGE_READ_WRITE should be renamed to TextureFormatFeatureFlags::STORAGE_WRITE.

@teoxoy teoxoy added type: bug Something isn't working area: validation Issues related to validation, diagnostics, and error handling labels Nov 25, 2024
@ErichDonGubler
Copy link
Member

ErichDonGubler commented Nov 26, 2024

Question: Looking at the spec. section on Plain color formats in Texture format capabilities, it seems that STORAGE_READ and STORAGE_WRITE should be called STORAGE_READ_ONLY and STORAGE_WRITE_ONLY, respectively, with a separate STORAGE_READ_WRITE flag being necessary for determining if a flag can actually be used in a read/write scenario. Should this not be the final shape of feature bits?

@nical
Copy link
Contributor

nical commented Nov 26, 2024

it seems that STORAGE_READ and STORAGE_WRITE should be called STORAGE_READ_ONLY and STORAGE_WRITE_ONLY, respectively, with a separate STORAGE_READ_WRITE flag being necessary for determining if a flag can actually be used in a read/write scenario. Should this not be the final shape of feature bits?

That sounds correct to me.

@teoxoy
Copy link
Member

teoxoy commented Nov 26, 2024

Ah yes, you are right, I forgot we can't treat the presence of both read & write as read_write.

@teoxoy
Copy link
Member

teoxoy commented Nov 26, 2024

Apparently I forgot to click "comment" 😅

@atlv24
Copy link
Contributor Author

atlv24 commented Nov 26, 2024

Yeah im aware, thats what i raised here: #6601

i thought that we could choose the semantics though, as long as their consistent. im not aware how much of this is spec defined. using 3 bits for this when 2 is enough to represent all states just feels a bit weird.

what happens when you set both READ_ONLY and READ_WRITE? or other invalid combinations?

i feel like read_write should be represented by READ | WRITE, write_only should be represented by only WRITE being set, same with read_only.

@teoxoy
Copy link
Member

teoxoy commented Nov 26, 2024

The problem is that a format can support read-only & write-only but not read-write (see table). We probably need another enum to encode this and it will depend on the context if we need to use the new enum or the bitflags.

@atlv24
Copy link
Contributor Author

atlv24 commented Nov 26, 2024

The problem is that a format can support read-only & write-only but not read-write (see table). We probably need another enum to encode this and it will depend on the context if we need to use the new enum or the bitflags.

isnt that the same as not setting read or write?

read + write = read_write
read = read_only
write = write_only
none = read_only + write_only

@teoxoy
Copy link
Member

teoxoy commented Nov 27, 2024

No flag should mean that it doesn't support/use storage usage at all.

@atlv24
Copy link
Contributor Author

atlv24 commented Nov 28, 2024

ahh okay. i am convinced that we should mirror the spec as closely as possible then, i'll get some PRs up this weekend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: validation Issues related to validation, diagnostics, and error handling type: bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants