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

Remove checks for skipIfColorRenderableNotSupportedForFormat #4179

Conversation

greggman
Copy link
Contributor

@greggman greggman commented Feb 6, 2025

This was added when compat was expected to make rendering to f16 and f32 to be optional but it's no longer optional and so the check is not needed.

There's also a ton of changes added to render to rgba32uint via bitcast instead of rendering directly to rgba32float. I don't think I'll remove them but I will at least go update the comments in another PR.

@greggman greggman requested a review from shrekshao February 6, 2025 22:55
@shrekshao
Copy link
Contributor

  • (I just found out my previous skipIfColorRenderableNotSupportedForFormat is using .color instead of .colorRender for if colorRenderable which is incorrect...)
  • should we care about rg11b10ufloat coverage? There's a rg11b10ufloat-renderable feature. (This seems go back to the old float16-renderable and float32-renderable case where we need to selectDeviceOrSkip)
    • This also reminds me now that GL_EXT_color_buffer_float is required for compat, in dawn we should always enable the RG11B10UfloatRenderable feature

@greggman
Copy link
Contributor Author

greggman commented Feb 6, 2025

That's a good point. Maybe we should keep these checks and just rename it to selectDeviceForRenderingToFormatOrSkipTest

I'll do that. I'm curious if adding rg11b10ufloat to the list of valid formats will break anything.

@shrekshao
Copy link
Contributor

kRegularTextureFormatInfo already includes rg11b10ufloat

I think adding the selectDevice for that will skip tests on compat (now the feature is never turned on in dawn compat)

But all of our bots (linux desktop, android arm64 pixel 6) running compat cts actually support GL_EXT_color_buffer_float so they used to pass.

@greggman
Copy link
Contributor Author

greggman commented Feb 7, 2025

oh man, so many issues 😭

EXT_color_buffer_float doesn't allow rg11b10ufloat to be mulitsampled but rg10b10ufloat-renderable requires it to support multisampling. It also requires blending but EXT_color_buffer_float has this comment

     10   03/26/15 markc      Clarify blending restriction so it can't
                              be interpreted to include R11F_G11F_B10F.

So ... as it stands, I guess we can't enable rg11b10ufloat-renderable on compat or at least no OpenGL ES 3.1 backed compat.

There's a further problem. I feel like kAllTextureFormatInfo was designed to assume everything is static. Lots filtering that happens. Lots of tests take that to make their parameters, which happens before the tests are run, using things in kAllTextureFormatInfo

But it doesn't work. For example, we want to test rg11b10ufloat in every test it can be used. But you can't pre-filter statically because whether its renderable and whether its multisampleable depends on the extensions.

That means functions like

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

Are just plain wrong.

So is

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

Those questions can't be answered at from static info.

I feel like we should make all of the data in format_info.ts private and only provide access through APIs that require you to pass in the correct data. Rename things as appropriate like getAllPossiblyRenderableFormats instead of having lots of code do it's own filtering

For example this

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

Is just wrong given some formats are conditionally renderable

...sigh...

@greggman
Copy link
Contributor Author

greggman commented Feb 7, 2025

I feel like we should do #4178 sooner rather than later

@shrekshao
Copy link
Contributor

shrekshao commented Feb 7, 2025

Facepalm. I'm always too naive facing these formats issue.
Then for this commit let's just simply remove and leave rg11b10ufloat as non renderable

This was added when compat was expected to make rendering to
f16 and f32 to be optional but it's no longer optional and
so the check is not needed.
@greggman greggman force-pushed the remove-skipIfColorRenderableNotSupportedForFormat branch from a0065fb to 6758337 Compare February 10, 2025 18:26
@greggman greggman enabled auto-merge (squash) February 10, 2025 18:28
@greggman greggman merged commit 769869f into gpuweb:main Feb 10, 2025
1 check passed
@greggman greggman deleted the remove-skipIfColorRenderableNotSupportedForFormat branch February 10, 2025 18:46
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