Allocate exact-sized interleaved (strided) buffers by mhansen · Pull Request #10017 · google/filament

mhansen · 2026-05-19T00:41:58Z

pixelflinger · 2026-05-19T06:33:35Z

@mhansen the unit tests seem to fail under ASAN.

mhansen · 2026-05-19T07:11:48Z

Whoops, sorry for the noise. I'll look at the tests tomorrow.

z3moon · 2026-05-19T16:24:05Z

        const uint8_t slot = mAttributes[attributeIndex].buffer;
-        const size_t end = offset + mVertexCount * stride;
+        const size_t elementSize = Driver::getElementTypeSize(mAttributes[attributeIndex].type);
+        const size_t end = offset + (mVertexCount - 1) * stride + elementSize;


This has an underflow issue. e.g., if mVertexCount == 0, mVertexCount - 1 will underflow for the type uint32_t.

On #10009 I asked about this:

I see there's a check that mVertexCount > 0 here:

filament/filament/src/details/VertexBuffer.cpp

Line 170 in 2526357

FILAMENT_CHECK_PRECONDITION(mImpl->mVertexCount > 0) << "vertexCount cannot be 0";

. Can we rely on that? Or is it stripped out in some builds?

Ah yeah we can rely on it then. Could you add comments about why it's safe?

pixelflinger · 2026-05-19T17:09:54Z

        const uint8_t slot = mAttributes[attributeIndex].buffer;
-        const size_t end = offset + mVertexCount * stride;
+        const size_t elementSize = Driver::getElementTypeSize(mAttributes[attributeIndex].type);
+        const size_t end = offset + (mVertexCount - 1) * stride + elementSize;


The correct calculation is mVertexCount * stride.

Indeed the existing code was wrong because it added the "offset" to the size calculation, the offset should be irrelevant. If we compare this to a std::vector<Foo>, stride plays the role of sizeof(Foo) and mVertexCount plays the role of .size(). offset plays the role of offsetof(Foo, field), it shouldn't be part of the size calculation.

D'oh. You're right, I completely bamboozled myself here.

The simplification is offset + elementSize = stride, so offset + (mVertexCount - 1) * stride + elementSize collapses to stride + (mVertexCount - 1) * stride collapses to mVertexCount * stride. Sorry, I did this in an accretive not simplififying way.

Wait, no, I remember why I did this - I was worried about if attributes are struct-of-array. While in this case (array of struct), mVertexCount * stride is correct, filament also seems to support vertices that are offset, e.g. here we have position at bytes 0-23 and color at bytes 23-35:

filament/samples/vbotest.cpp

Lines 72 to 75 in 180e91f

app.vb = VertexBuffer::Builder().vertexCount(3).bufferCount(1)

.attribute(VertexAttribute::POSITION, 0, VertexBuffer::AttributeType::FLOAT2, 0, 8)

.attribute(VertexAttribute::COLOR, 0, VertexBuffer::AttributeType::UBYTE4, 24, 4)

.normalized(VertexAttribute::COLOR).build(*engine);

If we used mVertexCount * stride, this would ignore colour's starting offset of 24. And the max-sized attribute would be computed as 24, so we'd allocate a 24-byte buffer.

I've added a test demonstrating this. And also another test for what happens with 32-byte-padded rows (ensuring that we allocate for the padding too)

good catch! I completely agree.

This demonstrates the issue: oversized buffer. In the next commit, we will fix this. Towards google#10009

pixelflinger · 2026-05-22T17:17:13Z

        const uint8_t slot = mAttributes[attributeIndex].buffer;
-        const size_t end = offset + mVertexCount * stride;
+        const size_t elementSize = Driver::getElementTypeSize(mAttributes[attributeIndex].type);
+        const size_t end = offset + (mVertexCount - 1) * stride + elementSize;


good catch! I completely agree.

pixelflinger · 2026-05-22T17:17:45Z

-        const size_t end = offset + mVertexCount * stride;
+        const size_t elementSize = Driver::getElementTypeSize(mAttributes[attributeIndex].type);
+        const size_t end = offset + (mVertexCount - 1) * stride + elementSize;
+        const size_t rounded = ((end + stride - 1) / stride) * stride;


yes perfect, this works.

mhansen changed the title ~~Allocate exact-sized strided buffers~~ Allocate exact-sized interleaved (strided) buffers May 19, 2026

show50726 added the internal Issue/PR does not affect clients label May 19, 2026

mhansen force-pushed the exact-size branch 6 times, most recently from a35232c to eac2a19 Compare May 19, 2026 03:24

pixelflinger requested review from pixelflinger and z3moon May 19, 2026 06:30

z3moon reviewed May 19, 2026

View reviewed changes

pixelflinger requested changes May 19, 2026

View reviewed changes

mhansen force-pushed the exact-size branch from eac2a19 to 84cfaa4 Compare May 20, 2026 00:46

mhansen added 2 commits May 20, 2026 11:42

Add test for interleaved buffer sizing (expects buggy size)

138fd71

This demonstrates the issue: oversized buffer. In the next commit, we will fix this. Towards google#10009

Fix interleaved buffer size calculation

edb49e9

mhansen force-pushed the exact-size branch from 84cfaa4 to edb49e9 Compare May 20, 2026 01:45

pixelflinger approved these changes May 22, 2026

View reviewed changes

Merge branch 'main' into exact-size

6b270bc

	app.vb = VertexBuffer::Builder().vertexCount(3).bufferCount(1)
	.attribute(VertexAttribute::POSITION, 0, VertexBuffer::AttributeType::FLOAT2, 0, 8)
	.attribute(VertexAttribute::COLOR, 0, VertexBuffer::AttributeType::UBYTE4, 24, 4)
	.normalized(VertexAttribute::COLOR).build(*engine);

Conversation

mhansen commented May 19, 2026

Uh oh!

pixelflinger commented May 19, 2026

Uh oh!

mhansen commented May 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants