Skip to content

Begin to refactor test_LoadImage to use SharedShaders #9092

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Begin to refactor test_LoadImage to use SharedShaders #9092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions filament/backend/test/ImageExpectations.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,16 @@
namespace test {

ScreenshotParams::ScreenshotParams(int width, int height, std::string fileName,
uint32_t expectedHash, bool isSrgb, int numAllowedDeviations, int pixelMatchThreshold)
uint32_t expectedHash, bool isSrgb, int numAllowedDeviations, int pixelMatchThreshold,
std::optional<float> forceAlphaValue)
: mWidth(width),
mHeight(height),
mIsSrgb(isSrgb),
mExpectedPixelHash(expectedHash),
mFileName(std::move(fileName)),
mAllowedPixelDeviations(numAllowedDeviations),
mPixelMatchThreshold(pixelMatchThreshold) {}
mPixelMatchThreshold(pixelMatchThreshold),
mForceAlphaValue(forceAlphaValue) {}
Copy link
Contributor

@poweifeng poweifeng Sep 17, 2025

Choose a reason for hiding this comment

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

why is forcing an alpha value necessary?


int ScreenshotParams::width() const {
return mWidth;
Expand All @@ -61,6 +63,10 @@ uint32_t ScreenshotParams::expectedHash() const {
return mExpectedPixelHash;
}

std::optional<float> ScreenshotParams::forceAlphaValue() const {
return mForceAlphaValue;
}

std::filesystem::path ScreenshotParams::actualDirectoryPath() {
return BackendTest::binaryDirectory().append("images/actual_images");
}
Expand Down Expand Up @@ -177,9 +183,29 @@ RenderTargetDump::RenderTargetDump(filament::backend::DriverApi& api,
const size_t size = mInternal->params.width() * mInternal->params.height() * 4;
mInternal->bytes.resize(size);

using filament::backend::PixelDataFormat;
using filament::backend::PixelDataType;
constexpr PixelDataFormat readPixelFormat = PixelDataFormat::RGBA;
constexpr PixelDataType readPixelType = PixelDataType::UBYTE;

auto cb = [](void* buffer, size_t size, void* user) {
auto* internal = static_cast<RenderTargetDump::Internal*>(user);
internal->bytesFilled = true;

// If requested, we overwrite the alpha value.
// If the read pixel format or type changes, this logic must be updated.
static_assert(readPixelFormat == PixelDataFormat::RGBA);
static_assert(readPixelType == PixelDataType::UBYTE);
if (auto alphaValue = internal->params.forceAlphaValue(); alphaValue) {
const int pixelCount = internal->params.width() * internal->params.height();
const int channels = 4;
constexpr int kAlphaChannel = 3;
uint8_t* pixelBuffer = static_cast<uint8_t*>(buffer);
for (int p = 0; p < pixelCount; p++) {
pixelBuffer[p * channels + kAlphaChannel] = *alphaValue * 255.0f;
}
}

#ifndef FILAMENT_IOS
image::LinearImage image;
if (internal->params.isSrgb()) {
Expand All @@ -203,9 +229,8 @@ RenderTargetDump::RenderTargetDump(filament::backend::DriverApi& api,
filePath);
#endif
};
filament::backend::PixelBufferDescriptor pb(mInternal->bytes.data(), size,
filament::backend::PixelDataFormat::RGBA, filament::backend::PixelDataType::UBYTE, cb,
(void*)mInternal.get());
filament::backend::PixelBufferDescriptor pb(mInternal->bytes.data(), size, readPixelFormat,
readPixelType, cb, (void*) mInternal.get());
api.readPixels(renderTarget, 0, 0, mInternal->params.width(), mInternal->params.height(),
std::move(pb));
}
Expand Down
64 changes: 63 additions & 1 deletion filament/backend/test/ImageExpectations.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include <filesystem>
#include <vector>
#include <optional>

#include "gtest/gtest.h"

Expand All @@ -45,12 +46,72 @@ class ScreenshotParams {
public:
// TODO(b/422804941): Add a set of environments where this test should use a different golden.
ScreenshotParams(int width, int height, std::string fileName, uint32_t expectedPixelHash,
bool isSrgb = false, int numAllowedDeviations = 0, int pixelMatchThreshold = 0);
bool isSrgb = false, int numAllowedDeviations = 0, int pixelMatchThreshold = 0,
std::optional<float> forceAlphaValue = std::nullopt);

class Builder {
public:
Builder& width(int width) {
mWidth = width;
return *this;
}

Builder& height(int height) {
mHeight = height;
return *this;
}

Builder& fileName(std::string fileName) {
mFileName = fileName;
return *this;
}

Builder& expectedPixelHash(uint32_t pixelHash) {
mExpectedPixelHash = pixelHash;
return *this;
}

Builder& isSrgb(bool isSrgb) {
mIsSrgb = isSrgb;
return *this;
}

Builder& numAllowedDeviations(int numAllowedDeviations) {
mNumAllowedDeviations = numAllowedDeviations;
return *this;
}

Builder& pixelMatchTheshold(int pixelMatchTheshold) {
mPixelMatchThreshold = pixelMatchTheshold;
return *this;
}

Builder& forceAlphaValue(float alphaValue) {
mForceAlphaValue = alphaValue;
return *this;
}

ScreenshotParams build() {
return ScreenshotParams(mWidth, mHeight, mFileName, mExpectedPixelHash, mIsSrgb,
mNumAllowedDeviations, mPixelMatchThreshold, mForceAlphaValue);
}

private:
int mWidth;
int mHeight;
std::string mFileName;
uint32_t mExpectedPixelHash;
bool mIsSrgb;
int mNumAllowedDeviations;
int mPixelMatchThreshold;
std::optional<float> mForceAlphaValue;
};

int width() const;
int height() const;
bool isSrgb() const;
uint32_t expectedHash() const;
std::optional<float> forceAlphaValue() const;

static std::filesystem::path actualDirectoryPath();
std::string actualFileName() const;
Expand All @@ -70,6 +131,7 @@ class ScreenshotParams {
std::string mFileName;
int mAllowedPixelDeviations;
int mPixelMatchThreshold;
std::optional<float> mForceAlphaValue;
};

/**
Expand Down
32 changes: 29 additions & 3 deletions filament/backend/test/SharedShaders.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,19 @@ layout(binding = 0, set = 0) uniform Params {
} params;
)";
}
case ShaderUniformType::Sampler: {
case ShaderUniformType::Sampler2D: {
Copy link
Contributor

@AndyHovingh AndyHovingh Aug 20, 2025

Choose a reason for hiding this comment

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

Note that WebGPU/WGSL does not support combined image samplers. We need to define and bind both the texture and the sampler for it and use both in the call to sample the color from the texture.

In pure WGSL it is:

...
@group(0) @binding(0) var mySampler : sampler;
@group(0) @binding(1) var myTexture : texture_2d<f32>;
...
let color = textureSample(myTexture, mySampler, uv);

textureLoad can be used to lookup data directly from a texture without a sampler.

We deal with this in our shader generation code using a SPIRV-OPT pass that splits combined image samplers (see (libs/filamat/src/GLSLPostProcessor.cpp ~line 574)[https://github.com/google/filament/blob/5daa0cfe4b9d74a489117d8df393d01540af1a0a/libs/filamat/src/GLSLPostProcessor.cpp#L574]: optimizer->RegisterPass(CreateSplitCombinedImageSamplerPass()); (thanks to a lot of help from David Neto). We also, do some tricks to remap the bindings for them, as can be seen from the rebindImageSamplerForWGSL(*spirv); call below the pass. We account for this when creating bind group layouts and bind groups on the driver/backend side (see filament/backend/src/webgpu/WebGPUDescriptorSetLayout.cpp ~lines 89-94):

// In WebGPU, textures and samplers are separate bindings.
/ We map the Filament binding index to two WebGPU binding indices:
// - texture: binding * 2
 // - sampler: binding * 2 + 1
wEntry.binding = fEntry.binding * 2;
entryInfo.binding = wEntry.binding;

I could be wrong, as I am not familiar with this side of the code, but I don't see where this is accounted for in the test shader generation. I think we need to modify the shader GLSL for the WebGPU case to handle this.

return R"(
layout(location = 0, set = 0) uniform sampler2D test_tex;
)";
}
case ShaderUniformType::ISampler2D: {
return R"(
layout(location = 0, set = 0) uniform isampler2D test_tex;
)";
}
case ShaderUniformType::USampler2D: {
return R"(
layout(location = 0, set = 0) uniform usampler2D test_tex;
)";
}
default:
Expand All @@ -179,14 +189,30 @@ std::vector<UniformConfig> GetUniformConfig(ShaderUniformType type) {
case ShaderUniformType::SimpleWithPadding: {
return {{ "Params" }};
}
case ShaderUniformType::Sampler: {
case ShaderUniformType::Sampler2D: {
filament::SamplerInterfaceBlock::SamplerInfo samplerInfo{
"backend_test", "test_tex", 0,
SamplerType::SAMPLER_2D, SamplerFormat::FLOAT, Precision::HIGH, false };
return {{
"test_tex", DescriptorType::SAMPLER_2D_FLOAT, samplerInfo
}};
}
case ShaderUniformType::ISampler2D: {
filament::SamplerInterfaceBlock::SamplerInfo samplerInfo{
"backend_test", "test_tex", 0,
SamplerType::SAMPLER_2D, SamplerFormat::INT, Precision::HIGH, false };
return {{
"test_tex", DescriptorType::SAMPLER_2D_INT, samplerInfo
}};
}
case ShaderUniformType::USampler2D: {
filament::SamplerInterfaceBlock::SamplerInfo samplerInfo{
"backend_test", "test_tex", 0,
SamplerType::SAMPLER_2D, SamplerFormat::UINT, Precision::HIGH, false };
return {{
"test_tex", DescriptorType::SAMPLER_2D_INT, samplerInfo
}};
}
default:
abort();
}
Expand Down Expand Up @@ -252,4 +278,4 @@ std::string SharedShaders::getFragmentShaderText(FragmentShaderType fragment,
return fragmentText->withUniform(*uniformText);
}

} // namespace test
} // namespace test
4 changes: 3 additions & 1 deletion filament/backend/test/SharedShadersConstants.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ enum class ShaderUniformType : uint8_t {
None,
Simple,
SimpleWithPadding,
Sampler,
Sampler2D,
ISampler2D,
USampler2D,
};

struct SimpleMaterialParams {
Expand Down
40 changes: 27 additions & 13 deletions filament/backend/test/test_LoadImage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,9 +287,9 @@ TEST_F(LoadImageTest, UpdateImage2D) {
// Test integer format uploads.
// TODO: These cases fail on OpenGL and Vulkan.
// TODO: These cases now also fail on Metal, but at some point previously worked.
testCases.emplace_back("RGB_INTEGER UBYTE to RGB8UI", PixelDataFormat::RGB_INTEGER, PixelDataType::UBYTE, TextureFormat::RGB8UI);
testCases.emplace_back("RGB_INTEGER USHORT to RGB16UI", PixelDataFormat::RGB_INTEGER, PixelDataType::USHORT, TextureFormat::RGB16UI);
testCases.emplace_back("RGB_INTEGER INT to RGB32I", PixelDataFormat::RGB_INTEGER, PixelDataType::INT, TextureFormat::RGB32I);
testCases.emplace_back("RGB_INTEGER UBYTE to RGB8UI", PixelDataFormat::RGB_INTEGER, PixelDataType::UBYTE, TextureFormat::RGB8UI);
testCases.emplace_back("RGB_INTEGER USHORT to RGB16UI", PixelDataFormat::RGB_INTEGER, PixelDataType::USHORT, TextureFormat::RGB16UI);
testCases.emplace_back("RGB_INTEGER INT to RGB32I", PixelDataFormat::RGB_INTEGER, PixelDataType::INT, TextureFormat::RGB32I);

// Test uploads with buffer padding.
// TODO: Vulkan crashes with "Assertion failed: (offset + size <= allocationSize)"
Expand Down Expand Up @@ -319,14 +319,23 @@ TEST_F(LoadImageTest, UpdateImage2D) {
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));

// Create a program.
filament::SamplerInterfaceBlock::SamplerInfo samplerInfo { "test", "tex", 0,
SamplerType::SAMPLER_2D, getSamplerFormat(t.textureFormat), Precision::HIGH, false };

std::string const fragment = getFormattedFragment(fragmentTemplate, t.textureFormat);
Shader shader(api, cleanup, ShaderConfig{
.vertexShader = mVertexShader,
.fragmentShader= fragment,
.uniforms = {{"test_tex", DescriptorType::SAMPLER_2D_FLOAT, samplerInfo}}
ShaderUniformType uniformType;
switch (getSamplerFormat(t.textureFormat)) {
case SamplerFormat::FLOAT:
case SamplerFormat::SHADOW: // not used for tests, but must cover all cases
uniformType = ShaderUniformType::Sampler2D;
break;
case SamplerFormat::INT:
uniformType = ShaderUniformType::ISampler2D;
break;
case SamplerFormat::UINT:
uniformType = ShaderUniformType::USampler2D;
break;
}
Shader shader = SharedShaders::makeShader(api, cleanup, {
.mVertexType = VertexShaderType::Textured,
.mFragmentType = FragmentShaderType::Textured,
.mUniformType = uniformType,
});

// Create a Texture.
Expand Down Expand Up @@ -374,8 +383,13 @@ TEST_F(LoadImageTest, UpdateImage2D) {
api.draw2(0, 3, 1);
api.endRenderPass();

EXPECT_IMAGE(defaultRenderTarget,
ScreenshotParams(kTexSize, kTexSize, t.name, expectedHash));
EXPECT_IMAGE(defaultRenderTarget, ScreenshotParams::Builder()
.width(kTexSize)
.height(kTexSize)
.fileName(t.name)
.expectedPixelHash(expectedHash)
.forceAlphaValue(1.0f)
.build());

api.commit(swapChain);
api.endFrame(0);
Expand Down
4 changes: 2 additions & 2 deletions filament/backend/test/test_MipLevels.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,12 @@ TEST_F(BackendTest, TextureViewLod) {
Shader whiteShader = SharedShaders::makeShader(api, cleanup, ShaderRequest {
.mVertexType = VertexShaderType::Textured,
.mFragmentType = FragmentShaderType::White,
.mUniformType = ShaderUniformType::Sampler
.mUniformType = ShaderUniformType::Sampler2D
});

// Create a program that samples a texture.
std::string vertexShader = SharedShaders::getVertexShaderText(
VertexShaderType::Textured, ShaderUniformType::Sampler);
VertexShaderType::Textured, ShaderUniformType::Sampler2D);
filament::SamplerInterfaceBlock::SamplerInfo samplerInfo {
"backend_test", "sib_tex", 0,
SamplerType::SAMPLER_2D, SamplerFormat::FLOAT, Precision::HIGH, false };
Expand Down
2 changes: 1 addition & 1 deletion filament/backend/test/test_RenderExternalImage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ Shader createShader(DriverApi& api, Cleanup& cleanup, Backend backend) {
return SharedShaders::makeShader(api, cleanup, ShaderRequest{
.mVertexType = VertexShaderType::Textured,
.mFragmentType = FragmentShaderType::Textured,
.mUniformType = ShaderUniformType::Sampler
.mUniformType = ShaderUniformType::Sampler2D
});
}

Expand Down
Loading