Skip to content

Tests: Fixes for backend tests when using Vulkan #9031

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

Closed
wants to merge 2 commits into from
Closed

Tests: Fixes for backend tests when using Vulkan #9031

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4ef165
Tests: Fixes for backend tests when using Vulkan
rafadevai Jul 22, 2025
36d12bb
Removing the finish calls.
rafadevai Aug 6, 2025
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
1 change: 1 addition & 0 deletions filament/backend/test/Lifetimes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ RenderFrame::RenderFrame(filament::backend::DriverApi& api): mApi(api) {

RenderFrame::~RenderFrame() {
mApi.endFrame(0);
mApi.finish();
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on the synchronization issues you were experiencing? Was it only with the Vulkan backend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can only saw it with the vulkan backend (the only one I tested) and it was just the report of the validation layers, the source of this is coming when doing readPixels which is not synchronized properly with vulkan, cant comment on metal.

So I just made the safe approach to make sure the work is completed before trying to read the data, which in the case of tests should be fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can't "fix the tests" to workaround bugs in the backends -- that's the whole point of having tests. readPixels should work without having to call finish() and it should work from within a renderpass. If the vk backend needs fixing, I prefer we file a buganizer, prioritize and fix rather an working around in the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the finish calls.

Will create a bug for the readPixels synchronization issue.

}

Cleanup::Cleanup(filament::backend::DriverApi& driverApi) : mDriverApi(driverApi) {}
Expand Down
4 changes: 1 addition & 3 deletions filament/backend/test/test_Blit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,6 @@ TEST_F(BlitTest, BlitRegion) {
}

TEST_F(BlitTest, BlitRegionToSwapChain) {
FAIL_IF(Backend::VULKAN, "Crashes due to not finding color attachment, see b/417481493");
auto& api = getDriverApi();
mCleanup.addPostCall([&]() { executeCommands(); });

Expand All @@ -498,11 +497,10 @@ TEST_F(BlitTest, BlitRegionToSwapChain) {
constexpr int kNumLevels = 3;

// Create a SwapChain and make it current.
Handle<HwRenderTarget> dstRenderTarget = mCleanup.add(api.createDefaultRenderTarget());
auto swapChain = mCleanup.add(createSwapChain());
api.makeCurrent(swapChain, swapChain);

Handle<HwRenderTarget> dstRenderTarget = mCleanup.add(api.createDefaultRenderTarget());

// Create a source texture.
Handle<HwTexture> srcTexture = mCleanup.add(api.createTexture(
SamplerType::SAMPLER_2D, kNumLevels, kSrcTexFormat, 1, kSrcTexWidth, kSrcTexHeight, 1,
Expand Down
26 changes: 12 additions & 14 deletions filament/backend/test/test_BufferUpdates.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,13 @@ TEST_F(BufferUpdatesTest, VertexBufferUpdate) {
Cleanup cleanup(api);

// Create a platform-specific SwapChain and make it current.
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));

auto swapChain = cleanup.add(createSwapChain());
api.makeCurrent(swapChain, swapChain);

Shader shader = createShader();

auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));

// To test large buffers (which exercise a different code path) create an extra large
// buffer. Only the first 3 vertices will be used.
TrianglePrimitive triangle(api, largeBuffers);
Expand Down Expand Up @@ -155,15 +155,14 @@ TEST_F(BufferUpdatesTest, VertexBufferUpdate) {
// This test renders two triangles in two separate draw calls. Between the draw calls, a uniform
// buffer object is partially updated.
TEST_F(BufferUpdatesTest, BufferObjectUpdateWithOffset) {
NONFATAL_FAIL_IF(SkipEnvironment(OperatingSystem::APPLE, Backend::VULKAN),
"All values including alpha are written as 0, see b/417254943");

auto& api = getDriverApi();
Cleanup cleanup(api);

const TrianglePrimitive triangle(api);

// Create a platform-specific SwapChain and make it current.
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));

auto swapChain = cleanup.add(createSwapChain());
api.makeCurrent(swapChain, swapChain);

Expand All @@ -179,9 +178,9 @@ TEST_F(BufferUpdatesTest, BufferObjectUpdateWithOffset) {
shader.bindUniform<SimpleMaterialParams>(api, ubuffer, kBindingConfig);

// Create a render target.
auto colorTexture =
cleanup.add(api.createTexture(SamplerType::SAMPLER_2D, 1, TextureFormat::RGBA8, 1,
screenWidth(), screenHeight(), 1, TextureUsage::COLOR_ATTACHMENT));
auto colorTexture = cleanup.add(
api.createTexture(SamplerType::SAMPLER_2D, 1, TextureFormat::RGBA8, 1, screenWidth(),
screenHeight(), 1, TextureUsage::COLOR_ATTACHMENT | TextureUsage::BLIT_SRC));
auto renderTarget = cleanup.add(api.createRenderTarget(TargetBufferFlags::COLOR0, screenWidth(),
screenHeight(), 1, 0, { { colorTexture } }, {}, {}));

Expand Down Expand Up @@ -234,16 +233,15 @@ TEST_F(BufferUpdatesTest, BufferObjectUpdateWithOffset) {
api.bindRenderPrimitive(triangle.getRenderPrimitive());
api.draw2(0, 3, 1);
api.endRenderPass();

api.flush();
api.commit(swapChain);
}


EXPECT_IMAGE(renderTarget, getExpectations(),
ScreenshotParams(screenWidth(), screenHeight(), "BufferObjectUpdateWithOffset",
2320747245));

api.flush();
api.commit(swapChain);
api.endFrame(0);
ScreenshotParams(screenWidth(), screenHeight(), "BufferObjectUpdateWithOffset",
2320747245));

// This ensures all driver commands have finished before exiting the test.
api.finish();
Expand Down
3 changes: 1 addition & 2 deletions filament/backend/test/test_Callbacks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,8 @@ TEST_F(BackendTest, FrameScheduledCallback) {
// Create a SwapChain.
// In order for the frameScheduledCallback to be called, this must be a real SwapChain (not
// headless) so we obtain a drawable.
auto swapChain = cleanup.add(createSwapChain());

Handle<HwRenderTarget> renderTarget = cleanup.add(api.createDefaultRenderTarget());
auto swapChain = cleanup.add(createSwapChain());

int callbackCountA = 0;
api.setFrameScheduledCallback(swapChain, nullptr, [&callbackCountA](PresentCallable callable) {
Expand Down
58 changes: 28 additions & 30 deletions filament/backend/test/test_LoadImage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ layout(location = 0) out vec4 fragColor;

// Filament's Vulkan backend requires a descriptor set index of 1 for all samplers.
// This parameter is ignored for other backends.
layout(location = 0, set = 0) uniform {samplerType} test_tex;
layout(binding = 0, set = 0) uniform {samplerType} test_tex;
Copy link
Collaborator

Choose a reason for hiding this comment

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

are we sure this doesn't break the other backends?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using location here is wrong GLSL code, the correct one is binding

Copy link
Contributor

Choose a reason for hiding this comment

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

so there's glsl for opengl and then glsl for spirv conversion. Could this be one of places where they differ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked here https://www.khronos.org/opengl/wiki/Layout_Qualifier_(GLSL)

The only thing extra in vulkan is the "set" parameter, but that one seems to be ignored in GL


void main() {
vec2 fbsize = vec2({texSize});
Expand All @@ -66,7 +66,7 @@ std::string fragmentUpdateImage3DTemplate (R"(#version 450 core
layout(location = 0) out vec4 fragColor;

// Filament's Vulkan backend requires a descriptor set index of 1 for all samplers.
layout(location = 0, set = 0) uniform {samplerType} test_tex;
layout(binding = 0, set = 0) uniform {samplerType} test_tex;

float getLayer(in sampler3D s) { return 2.5f / 4.0f; }
float getLayer(in sampler2DArray s) { return 2.0f; }
Expand All @@ -87,7 +87,7 @@ std::string fragmentUpdateImageMip (R"(#version 450 core
layout(location = 0) out vec4 fragColor;

// Filament's Vulkan backend requires a descriptor set index of 1 for all samplers.
layout(location = 0, set = 0) uniform sampler2D test_tex;
layout(binding = 0, set = 0) uniform sampler2D test_tex;

void main() {
vec2 fbsize = vec2({texSize});
Expand Down Expand Up @@ -314,9 +314,11 @@ TEST_F(LoadImageTest, UpdateImage2D) {
Cleanup cleanup(api);

// Create a platform-specific SwapChain and make it current.
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));
assert_invariant(defaultRenderTarget);

auto swapChain = cleanup.add(createSwapChain());
api.makeCurrent(swapChain, swapChain);
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));

// Create a program.
filament::SamplerInterfaceBlock::SamplerInfo samplerInfo { "test", "tex", 0,
Expand Down Expand Up @@ -374,21 +376,17 @@ TEST_F(LoadImageTest, UpdateImage2D) {
api.draw2(0, 3, 1);
api.endRenderPass();

EXPECT_IMAGE(defaultRenderTarget, getExpectations(),
ScreenshotParams(kTexSize, kTexSize, t.name, expectedHash));

api.commit(swapChain);
api.endFrame(0);

EXPECT_IMAGE(defaultRenderTarget, getExpectations(),
ScreenshotParams(kTexSize, kTexSize, t.name, expectedHash));
}

api.stopCapture();
}

TEST_F(LoadImageTest, UpdateImageSRGB) {
FAIL_IF(SkipEnvironment(OperatingSystem::APPLE, Backend::VULKAN),
"Crashing when reading pixels without a redundant call to makeCurrent right before the"
"render pass. b/422798473");

auto& api = getDriverApi();
Cleanup cleanup(api);
api.startCapture();
Expand All @@ -397,10 +395,13 @@ TEST_F(LoadImageTest, UpdateImageSRGB) {
PixelDataType const pixelType = PixelDataType::UBYTE;
TextureFormat const textureFormat = TextureFormat::SRGB8_A8;


// Create a platform-specific SwapChain and make it current.
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));
assert_invariant(defaultRenderTarget);

auto swapChain = cleanup.add(createSwapChain());
api.makeCurrent(swapChain, swapChain);
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));

// Create a program.
filament::SamplerInterfaceBlock::SamplerInfo samplerInfo { "test", "tex", 0,
Expand Down Expand Up @@ -463,20 +464,17 @@ TEST_F(LoadImageTest, UpdateImageSRGB) {
api.draw2(0, 3, 1);
api.endRenderPass();

EXPECT_IMAGE(defaultRenderTarget, getExpectations(),
ScreenshotParams(kTexSize, kTexSize, "UpdateImageSRGB", 3300305265));

api.commit(swapChain);
api.endFrame(0);
api.finish();

EXPECT_IMAGE(defaultRenderTarget, getExpectations(),
ScreenshotParams(kTexSize, kTexSize, "UpdateImageSRGB", 3300305265));

api.stopCapture();
}

TEST_F(LoadImageTest, UpdateImageMipLevel) {
FAIL_IF(SkipEnvironment(OperatingSystem::APPLE, Backend::VULKAN),
"Crashing when reading pixels without a redundant call to makeCurrent right before the"
"render pass. b/422798473");

auto& api = getDriverApi();
Cleanup cleanup(api);
api.startCapture();
Expand All @@ -486,9 +484,11 @@ TEST_F(LoadImageTest, UpdateImageMipLevel) {
TextureFormat textureFormat = TextureFormat::RGBA32F;

// Create a platform-specific SwapChain and make it current.
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));
assert_invariant(defaultRenderTarget);

auto swapChain = cleanup.add(createSwapChain());
api.makeCurrent(swapChain, swapChain);
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));

// Create a program.
filament::SamplerInterfaceBlock::SamplerInfo samplerInfo { "test", "tex", 0,
Expand Down Expand Up @@ -537,21 +537,17 @@ TEST_F(LoadImageTest, UpdateImageMipLevel) {
api.bindRenderPrimitive(mTriangle.getRenderPrimitive());
api.draw2(0, 3, 1);
api.endRenderPass();

api.commit(swapChain);
}

EXPECT_IMAGE(defaultRenderTarget, getExpectations(),
ScreenshotParams(kTexSize, kTexSize, "UpdateImageMipLevel", 1875922935));

api.commit(swapChain);
api.endFrame(0);

api.stopCapture();
}

TEST_F(LoadImageTest, UpdateImage3D) {
FAIL_IF(SkipEnvironment(OperatingSystem::APPLE, Backend::VULKAN),
"Crashing when reading pixels without a redundant call to makeCurrent right before the"
"render pass. b/422798473");
NONFATAL_FAIL_IF(SkipEnvironment(OperatingSystem::APPLE, Backend::VULKAN),
"Checkerboard not drawn, possibly due to using wrong z value of 3d texture, "
"see b/417254499");
Expand All @@ -566,9 +562,11 @@ TEST_F(LoadImageTest, UpdateImage3D) {
TextureUsage usage = TextureUsage::SAMPLEABLE | TextureUsage::UPLOADABLE;

// Create a platform-specific SwapChain and make it current.
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));
assert_invariant(defaultRenderTarget);

auto swapChain = cleanup.add(createSwapChain());
api.makeCurrent(swapChain, swapChain);
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));

// Create a program.
filament::SamplerInterfaceBlock::SamplerInfo samplerInfo { "test", "tex", 0,
Expand Down Expand Up @@ -624,11 +622,11 @@ TEST_F(LoadImageTest, UpdateImage3D) {
api.bindRenderPrimitive(mTriangle.getRenderPrimitive());
api.draw2(0, 3, 1);
api.endRenderPass();

EXPECT_IMAGE(defaultRenderTarget, getExpectations(),
ScreenshotParams(kTexSize, kTexSize, "UpdateImage3D", 1875922935));
}

EXPECT_IMAGE(defaultRenderTarget, getExpectations(),
ScreenshotParams(kTexSize, kTexSize, "UpdateImage3D", 1875922935));

api.stopCapture();
}

Expand Down
5 changes: 2 additions & 3 deletions filament/backend/test/test_MRT.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ TEST_F(BackendTest, MRT) {
// The test is executed within this block scope to force destructors to run before
// executeCommands().
{
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));

// Create a platform-specific SwapChain and make it current.
auto swapChain = cleanup.add(createSwapChain());
api.makeCurrent(swapChain, swapChain);
Expand All @@ -66,8 +68,6 @@ TEST_F(BackendTest, MRT) {

TrianglePrimitive triangle(api);

auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));

// Create two Textures.
auto usage = TextureUsage::COLOR_ATTACHMENT | TextureUsage::SAMPLEABLE;
Handle<HwTexture> textureA = cleanup.add(api.createTexture(
Expand Down Expand Up @@ -110,7 +110,6 @@ TEST_F(BackendTest, MRT) {

api.startCapture(0);

api.makeCurrent(swapChain, swapChain);
api.beginFrame(0, 0, 0);

// Draw a triangle.
Expand Down
7 changes: 4 additions & 3 deletions filament/backend/test/test_MipLevels.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ TEST_F(BackendTest, TextureViewLod) {
// The test is executed within this block scope to force destructors to run before
// executeCommands().
{
// Create a platform-specific SwapChain and make it current.
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));
assert_invariant(defaultRenderTarget);

// Create a SwapChain and make it current.
auto swapChain = cleanup.add(createSwapChain());
api.makeCurrent(swapChain, swapChain);
Expand Down Expand Up @@ -153,9 +157,6 @@ TEST_F(BackendTest, TextureViewLod) {
api.endRenderPass();
}

backend::Handle<HwRenderTarget> defaultRenderTarget =
cleanup.add(api.createDefaultRenderTarget(0));

PipelineState state = getColorWritePipelineState();
texturedShader.addProgramToPipelineState(state);

Expand Down
4 changes: 2 additions & 2 deletions filament/backend/test/test_MissingRequiredAttributes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ TEST_F(BackendTest, MissingRequiredAttributes) {
DriverApi& api = getDriverApi();
Cleanup cleanup(api);
// Create a platform-specific SwapChain and make it current.
auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));

auto swapChain = cleanup.add(createSwapChain());
api.makeCurrent(swapChain, swapChain);

Expand All @@ -75,8 +77,6 @@ TEST_F(BackendTest, MissingRequiredAttributes) {
ShaderUniformType::None),
});

auto defaultRenderTarget = cleanup.add(api.createDefaultRenderTarget(0));

TrianglePrimitive triangle(api);

PipelineState state = getColorWritePipelineState();
Expand Down
14 changes: 7 additions & 7 deletions filament/backend/test/test_PushConstants.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ TEST_F(BackendTest, PushConstants) {
// executeCommands().
{
// Create a SwapChain and make it current.
Handle<HwRenderTarget> renderTarget = cleanup.add(api.createDefaultRenderTarget());

auto swapChain = cleanup.add(createSwapChain());
api.makeCurrent(swapChain, swapChain);

Expand All @@ -126,8 +128,6 @@ TEST_F(BackendTest, PushConstants) {
shaderGen.getProgramWithPushConstants(api, { gVertConstants, gFragConstants, {} });
ProgramHandle program = cleanup.add(api.createProgram(std::move(p)));

Handle<HwRenderTarget> renderTarget = cleanup.add(api.createDefaultRenderTarget());

TrianglePrimitive triangle(api);

RenderPassParams params = getClearColorRenderPass();
Expand All @@ -139,7 +139,6 @@ TEST_F(BackendTest, PushConstants) {
ps.rasterState.colorWrite = true;
ps.rasterState.depthWrite = false;

api.makeCurrent(swapChain, swapChain);
api.beginFrame(0, 0, 0);

api.beginRenderPass(renderTarget, params);
Expand Down Expand Up @@ -178,12 +177,13 @@ TEST_F(BackendTest, PushConstants) {

api.endRenderPass();

EXPECT_IMAGE(renderTarget, getExpectations(),
ScreenshotParams(params.viewport.width, params.viewport.height, "pushConstants",
3575588741));

api.commit(swapChain);
api.endFrame(0);
api.finish();

EXPECT_IMAGE(renderTarget, getExpectations(),
ScreenshotParams(params.viewport.width, params.viewport.height, "pushConstants",
3575588741));
}

api.stopCapture(0);
Expand Down
Loading