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

Refactor format_info.ts and related texture functions #4181

Open
greggman opened this issue Feb 7, 2025 · 3 comments
Open

Refactor format_info.ts and related texture functions #4181

greggman opened this issue Feb 7, 2025 · 3 comments
Labels
bug Something isn't working

Comments

@greggman
Copy link
Contributor

greggman commented Feb 7, 2025

The CTS has the issue that there's a global table of texture data kAllTextureFormatInfo and it's often used to filter/select formats. Example

      .combine('colorFormat', kEncodableTextureFormats)
      // Filter out any non-renderable formats
      .filter(({ colorFormat }) => !!kTextureFormatInfo[colorFormat].colorRender)

But this is wrong. rg11b10ufloat is conditionally renderable so as defined it will never be tested here.

Similarly there is code like this

export function isMultisampledTextureFormat(
  format: GPUTextureFormat,
  isCompatibilityMode: boolean
): boolean {
  if (isCompatibilityMode) {
    if (kCompatModeUnsupportedMultisampledTextureFormats.indexOf(format) >= 0) {
      return false;
    }
  }
  return kAllTextureFormatInfo[format].multisample;
}

This is also wrong. rg11b10ufloat is marked as multisample: false but enabling rg11b10ufloat-renderable is supposed to make it support multisampling which means, every test using this filter will not be testing this format so we'd have no idea if it actually works.

It feels like this should be solved by making kAllTextureFormatInfo private and refactoring any of these queries about formats to require enough info to be useful, and/or, be renamed to reflect what they're returning. So for example, something that wants to know all renderable formats can not use

export function canUseAsRenderTarget(format: GPUTextureFormat) {
  return !!kTextureFormatInfo[format].colorRender || isDepthOrStencilTextureFormat(format);
}

   .combine('format', kAllTextureFormats)
   .filter(t => canUseAsRenderTarget(t.format))

because, you can't know if a format is renderable until you know if you have the feature. That particular function could be renamed to canPotentiallyUseAsRenderTarget in which case it should return true for rg11b10ufloat. Similarly isMultisampledTextureFormat would need to be isPotentiallyMultisampledTextureFormat.

But ultimately, with all of these special exceptions, it doesn't feel like tests should be rolling their own checks by accessing kAllTextureFormatInfo directly. They should all be gated on the data needed to give the correct answer, forcing you to get the larger list and then use skipIf... and/or selectDeviceForXOrSkipTest

I think I'd also suggest that most texture format filtering in general should be moved to some helpers that tests all share the same filtering where appropriate.

@shrekshao, @kainino0x

@greggman greggman added the bug Something isn't working label Feb 7, 2025
@greggman
Copy link
Contributor Author

  • isFilterableAsTextureF32 -> isPossiblyFilterableAsTextureF32
  • canUseAsRenderTarget -> canPossiblyUseAsRenderTarget
  • check filterFormatsByFeature
  • check getFeaturesForFormats
  • make private
    • kTextureFormatInfo
    • kRegularTextureFormatInfo
    • kSizedDepthStencilFormatInfo
    • kUnsizedDepthStencilFormatInfo
    • kBCTextureFormatInfo
    • kETC2TextureFormatInfo
    • kASTCTextureFormatInfo

@greggman
Copy link
Contributor Author

  • fix every usage of skipIfTextureFormatNotUsableAsStorageTexture

    This assumes the list of textures usable as storage textures is fixed, gated
    only on compat/non-compat. that seems unlikely to stay true. For one, it doesn't
    handle bgra8unorm-storage

@kainino0x
Copy link
Collaborator

I was planning to split the format info into two separate tables, one for static info (like block sizes) and one for dynamic info (capabilities) that could depend on the device, but I never got around to that after doing the first step of the refactor. I think #3848 was the issue tracking that.

greggman added a commit to greggman/cts that referenced this issue Feb 20, 2025
greggman added a commit to greggman/cts that referenced this issue Feb 20, 2025
Also expanded the multisample depth test and add
multisample stencil placeholder.

Issue: gpuweb#4181
greggman added a commit to greggman/cts that referenced this issue Feb 20, 2025
Also expanded the multisample depth test and add
multisample stencil placeholder.

Issue: gpuweb#4181
greggman added a commit that referenced this issue Feb 20, 2025
Also expanded the multisample depth test and add
multisample stencil placeholder.

Issue: #4181
greggman added a commit to greggman/cts that referenced this issue Feb 20, 2025
Also expanded the multisample depth test and add
multisample stencil placeholder.

Issue: gpuweb#4181
greggman added a commit to greggman/cts that referenced this issue Feb 20, 2025
greggman added a commit to greggman/cts that referenced this issue Feb 20, 2025
greggman added a commit to greggman/cts that referenced this issue Feb 20, 2025
greggman added a commit to greggman/cts that referenced this issue Feb 21, 2025
greggman added a commit to greggman/cts that referenced this issue Feb 21, 2025
greggman added a commit to greggman/cts that referenced this issue Feb 21, 2025
greggman added a commit to greggman/cts that referenced this issue Feb 21, 2025
greggman added a commit to greggman/cts that referenced this issue Feb 21, 2025
greggman added a commit to greggman/cts that referenced this issue Feb 21, 2025
greggman added a commit to greggman/cts that referenced this issue Feb 21, 2025
greggman added a commit to greggman/cts that referenced this issue Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants