GPU: Texture Type Refactor #130671

New Issue

Clément Foucault · 2024-11-21T13:04:16+01:00

Clément Foucault commented

2024-11-21 13:04:16 +01:00

For the same reason as vertex format outlined in #130632 we want to improve the texture API safety.

Currently all texture types are allows to be created and used freely. Some usage require correct usage flags that are bug prone and not enforced on all backend. Type checks are done by the backend at runtime and are not even given to create errors on misusage. Which makes error detection much more complex.

Proposal

This proposal enforce correct usage through different classes with different capabilities. They should all be derived from the same class for easier management in the GPU module.

Conceptually these classes replace the need for eGPUTextureUsage and add data type safety.

Texture

Can only be read with a sampler

Formats: All of them

TextureTarget

Can be attached as framebuffer and be read with a sampler

Formats: All of them except, compressed formats, SNORM formats, 3 component formats, shared exponent format.

TextureMemoryless

Can only be attached as framebuffer

Formats: Same as target.

TextureWrite

Can be used as image and be read with a sampler

Formats: Same as target but without depth formats and with signed formats.

TextureAtomic

Can be used as image with atomics operations and be read with a sampler

Formats: Only 1 component integer formats.
Note: Is only a workaround for Metal, but might be good to enforce correct usage somehow.

Implementation

This can be done in a similar fashion as vertex format in #130632:

namespace blender::gpu {
/**
 * Enum that contains data formats that are compatible with atomic texture.
 * Only 32 bit integer formats are supported.
 */
enum class TextureAtomicFormat : uint8_t {
  Invalid = 0,

  SINT_32 = uint8_t(DataType::SINT_32),
  UINT_32 = uint8_t(DataType::UINT_32),
};

// Repeat and adapt for each texture type ...

}  // namespace blender::gpu

And for creation:

namespace blender::gpu {
class Texture  {
    /* Static constructor overloaded for each type. */
    Texture create(TextureFormat format);
    TextureTarget create(TextureTargetFormat format);
    TextureMemoryless create(TextureMemorylessFormat format);
    TextureWrite create(TextureWriteFormat format);
    TextureAtomic create(TextureAtomicFormat format);
};
}  // namespace blender::gpu

The GPU and DRW api can be then adjusted to only accept certain class for certain function. For instance, only TextureTarget can be attached to framebuffers.

Questions

What about if we want to have a texture attached to a framebuffer but then written using image write?

Proposal, add TextureTargetWrite. To avoid templates, have explicit conversion functions as_texture_target() and as_texture_write(). This way all types can all convert implicitly to Read only Texture, except TextureMemoryLess, but we can just delete the constructor to prevent casting.

graph TD;
    TextureTargetWrite-.->|Explicit|TextureTarget;
    Texture-->TextureWrite;
    Texture-->TextureAtomic;
    Texture-->TextureTarget;
    Texture-->TextureTargetWrite;
    TextureWrite-->TextureAtomic;
	TextureTarget-->TextureMemoryless;
    TextureTargetWrite-.->|Explicit|TextureWrite;
    Texture-.-|Deleted|TextureMemoryless;

Another solution is to forbid such usage. It is very likely to be suboptimal because of layout transition. The issue is that it is already used by EEVEE for gbuffer using texture views.

What about texture views?

Texture views should inherit the parent type.

For the same reason as vertex format outlined in #130632 we want to improve the texture API safety. Currently all texture types are allows to be created and used freely. Some usage require correct usage flags that are bug prone and not enforced on all backend. Type checks are done by the backend at runtime and are not even given to create errors on misusage. Which makes error detection much more complex. ### Proposal This proposal enforce correct usage through different classes with different capabilities. They should all be derived from the same class for easier management in the GPU module. Conceptually these classes replace the need for `eGPUTextureUsage` and add data type safety. #### Texture Can only be read with a sampler - Formats: All of them #### TextureTarget Can be attached as framebuffer and be read with a sampler - Formats: All of them except, compressed formats, SNORM formats, 3 component formats, shared exponent format. #### TextureMemoryless Can only be attached as framebuffer - Formats: Same as target. #### TextureWrite Can be used as image and be read with a sampler - Formats: Same as target but without depth formats and with signed formats. #### TextureAtomic Can be used as image with atomics operations and be read with a sampler - Formats: Only 1 component integer formats. - Note: Is only a workaround for Metal, but might be good to enforce correct usage somehow. ### Implementation This can be done in a similar fashion as vertex format in #130632: ```cpp namespace blender::gpu { /** * Enum that contains data formats that are compatible with atomic texture. * Only 32 bit integer formats are supported. */ enum class TextureAtomicFormat : uint8_t { Invalid = 0, SINT_32 = uint8_t(DataType::SINT_32), UINT_32 = uint8_t(DataType::UINT_32), }; // Repeat and adapt for each texture type ... } // namespace blender::gpu ``` And for creation: ```cpp namespace blender::gpu { class Texture { /* Static constructor overloaded for each type. */ Texture create(TextureFormat format); TextureTarget create(TextureTargetFormat format); TextureMemoryless create(TextureMemorylessFormat format); TextureWrite create(TextureWriteFormat format); TextureAtomic create(TextureAtomicFormat format); }; } // namespace blender::gpu ``` The GPU and DRW api can be then adjusted to only accept certain class for certain function. For instance, only `TextureTarget` can be attached to framebuffers. ### Questions #### What about if we want to have a texture attached to a framebuffer but then written using image write? Proposal, add `TextureTargetWrite`. To avoid templates, have explicit conversion functions `as_texture_target()` and `as_texture_write()`. This way all types can all convert implicitly to Read only Texture, except `TextureMemoryLess`, but we can just delete the constructor to prevent casting. ```mermaid graph TD; TextureTargetWrite-.->|Explicit|TextureTarget; Texture-->TextureWrite; Texture-->TextureAtomic; Texture-->TextureTarget; Texture-->TextureTargetWrite; TextureWrite-->TextureAtomic; TextureTarget-->TextureMemoryless; TextureTargetWrite-.->|Explicit|TextureWrite; Texture-.-|Deleted|TextureMemoryless; ``` Another solution is to forbid such usage. It is very likely to be suboptimal because of layout transition. The issue is that it is already used by EEVEE for gbuffer using texture views. #### What about texture views? Texture views should inherit the parent type.

Clément Foucault added the

Type

Design

label 2024-11-21 13:04:16 +01:00

Clément Foucault changed title from ~~GPU:~~ to GPU: Texture Type Refactor

2024-11-21 13:04:34 +01:00

Clément Foucault added the

Module

Viewport & EEVEE

label 2024-11-21 13:04:38 +01:00

Miguel Pozo commented

2024-11-22 15:53:17 +01:00

I like this in theory, but I can see it exploding into too much complexity in practice.
I wouldn't like to end up using templates for each function taking a Texture.
And we are not even taking texture dimensions or data types into account, so we won't have full compile-time safety anyway.

I think having the (most common) types themselves would be nice, as long as they can be optionally constructible from a base Texture (or GPUTexture) doing appropriate runtime checks.
That way we can improve readability and safety, and use static checking where possible, but we don't trap ourselves into having to make the compiler happy at any cost.

I like this in theory, but I can see it exploding into too much complexity in practice. I wouldn't like to end up using templates for each function taking a Texture. And we are not even taking texture dimensions or data types into account, so we won't have full compile-time safety anyway. I think having the (most common) types themselves would be nice, as long as they can be optionally constructible from a base Texture (or GPUTexture) doing appropriate runtime checks. That way we can improve readability and safety, and use static checking where possible, but we don't trap ourselves into having to make the compiler happy at any cost.

Clément Foucault commented

2024-11-22 17:14:55 +01:00

And we are not even taking texture dimensions or data types into account, so we won't have full compile-time safety anyway.

We are not aiming to put the everything in the type info. There is no benefit to put things like texture dimensions or the data type inside the texture type (since none of the binding function are shader interface aware). This task is only to move the usage flags into the type info and forbid invalid data types in constructors.

I think having the (most common) types themselves would be nice, as long as they can be optionally constructible from a base Texture (or GPUTexture) doing appropriate runtime checks.

That can be a nice solution for complex usage. But appart from the weird case of EEVEE that uses TextureWrite and TextureTarget on the same texture, I don't see the need for it. And like I said, this should be discouraged.

Note that the goal of the task is to make the general usage safe. Custom usage should be very explicit.

Also note that many combination of the usage flags are invalid (can't have GPU_TEXTURE_USAGE_MEMORYLESS and not GPU_TEXTURE_USAGE_ATTACHMENT, can't have GPU_TEXTURE_USAGE_ATOMIC with GPU_TEXTURE_USAGE_ATTACHMENT). So enforcing it at the type level seems to be also a win.

Another option to avoid templates, is to have explicit conversion functions TextureTargetWrite().as_texture_target() and TextureTargetWrite().as_texture_write() for these use case. This way all types can all convert to Read Texture (except TextureMemoryLess, but we can just delete the constructor to prevent casting).

So we would have:

graph TD;
    TextureTargetWrite-.->|Explicit|TextureTarget;
    Texture-->TextureWrite;
    Texture-->TextureWriteAtomic;
    Texture-->TextureTarget;
    Texture-->TextureTargetWrite;
    TextureWrite-->TextureWriteAtomic;
	TextureTarget-->TextureMemoryLess;
    TextureTargetWrite-.->|Explicit|TextureWrite;
    Texture-.-|Deleted|TextureMemoryLess;

GPU_TEXTURE_USAGE_HOST_READ

About GPU_TEXTURE_USAGE_HOST_READ I a bit on the fence. To me this sounds like it should be a backend detail that we shouldn't have to care about. Proposal for this is to just be an extra argument (or different constructor) for TextureTarget and TextureWrite.

GPU_TEXTURE_USAGE_FORMAT_VIEW

Given how niche this is, I would rather have some special format tricks to allow this. Like
declaring the texture with a special UNORM_24_DEPTH_UINT_8_STENCIL_READ with a view set to UNORM_24_DEPTH_UINT_8 to read the depth bits. Or simply have a runtime switch like for GPU_TEXTURE_USAGE_HOST_READ.

Again, I am not sure statically protecting format view creation is necessary.

> And we are not even taking texture dimensions or data types into account, so we won't have full compile-time safety anyway. We are not aiming to put the everything in the type info. There is no benefit to put things like texture dimensions or the data type inside the texture type (since none of the binding function are shader interface aware). This task is only to move the usage flags into the type info and forbid invalid data types in constructors. > I think having the (most common) types themselves would be nice, as long as they can be optionally constructible from a base Texture (or GPUTexture) doing appropriate runtime checks. That can be a nice solution for complex usage. But appart from the weird case of EEVEE that uses `TextureWrite` and `TextureTarget` on the same texture, I don't see the need for it. And like I said, this should be discouraged. Note that the goal of the task is to make the general usage safe. Custom usage should be very explicit. Also note that many combination of the usage flags are invalid (can't have `GPU_TEXTURE_USAGE_MEMORYLESS` and not `GPU_TEXTURE_USAGE_ATTACHMENT`, can't have `GPU_TEXTURE_USAGE_ATOMIC` with `GPU_TEXTURE_USAGE_ATTACHMENT`). So enforcing it at the type level seems to be also a win. Another option to avoid templates, is to have explicit conversion functions `TextureTargetWrite().as_texture_target()` and `TextureTargetWrite().as_texture_write()` for these use case. This way all types can all convert to Read Texture (except `TextureMemoryLess`, but we can just delete the constructor to prevent casting). So we would have: ```mermaid graph TD; TextureTargetWrite-.->|Explicit|TextureTarget; Texture-->TextureWrite; Texture-->TextureWriteAtomic; Texture-->TextureTarget; Texture-->TextureTargetWrite; TextureWrite-->TextureWriteAtomic; TextureTarget-->TextureMemoryLess; TextureTargetWrite-.->|Explicit|TextureWrite; Texture-.-|Deleted|TextureMemoryLess; ``` #### GPU_TEXTURE_USAGE_HOST_READ About `GPU_TEXTURE_USAGE_HOST_READ` I a bit on the fence. To me this sounds like it should be a backend detail that we shouldn't have to care about. Proposal for this is to just be an extra argument (or different constructor) for `TextureTarget` and `TextureWrite`. #### GPU_TEXTURE_USAGE_FORMAT_VIEW Given how niche this is, I would rather have some special format tricks to allow this. Like declaring the texture with a special `UNORM_24_DEPTH_UINT_8_STENCIL_READ` with a view set to `UNORM_24_DEPTH_UINT_8` to read the depth bits. Or simply have a runtime switch like for `GPU_TEXTURE_USAGE_HOST_READ`. Again, I am not sure statically protecting format view creation is necessary.

Clément Foucault commented

2024-11-23 00:26:30 +01:00

@pragma37 I think I misunderstood your comment. I think your concern is also about texture creation functions and their variants.

In this case we also want to be very explicit as there are also requirements:

TextureAtomic cannot be 1D or 1DArray (Not implemented in Metal)
TextureTarget cannot be 1D or 1DArray
TextureMemoryless can only be 2D or 2DArray

@pragma37 I think I misunderstood your comment. I think your concern is also about texture creation functions and their variants. In this case we also want to be very explicit as there are also requirements: - `TextureAtomic` cannot be 1D or 1DArray (Not implemented in Metal) - `TextureTarget` cannot be 1D or 1DArray - `TextureMemoryless` can only be 2D or 2DArray

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

GPU: Texture Type Refactor #130671

Proposal

Texture

TextureTarget

TextureMemoryless

TextureWrite

TextureAtomic

Implementation

Questions

What about if we want to have a texture attached to a framebuffer but then written using image write?

What about texture views?

GPU_TEXTURE_USAGE_HOST_READ

GPU_TEXTURE_USAGE_FORMAT_VIEW