VDR: Fix vertex offset in indirect draw calls

[panos-lunarg]

No description provided.

[ci-tester-lunarg]

CI gfxreconstruct build queued with queue ID 354749.

[ci-tester-lunarg]

CI gfxreconstruct build # 5917 running.

[ci-tester-lunarg]

CI gfxreconstruct build # 5917 passed.

[hongSARC]

So if I understood what the code is doing correctly, in FindMinMaxVertexIndices function, you are processing the index buffer to find the minimum and maximum indices used in the range of the index data.
This function takes into account the index count and the first index, and computes the min and max of the indices.

Then, you are adding the vertex offset to the min and max values for the index range.

---

When you add different offsets to the min and max, you essentially expand the range incorrectly, as you are applying two different offsets. You should be applying a single vertex offset to both the min and max values.

The offsets you add should be uniform for both min and max, based on the range of indices you are processing.

I think it should calculate the min and max indices first then apply the vertex offset for that specific draw call to the indices. Then track the overall min and max indices across all draw calls comparing the adjusted values.

[panos-lunarg]

So if I understood what the code is doing correctly, in FindMinMaxVertexIndices function, you are processing the index buffer to find the minimum and maximum indices used in the range of the index data. This function takes into account the index count and the first index, and computes the min and max of the indices.

Correct.

Then, you are adding the vertex offset to the min and max values for the index range.

When you add different offsets to the min and max, you essentially expand the range incorrectly, as you are applying two different offsets. You should be applying a single vertex offset to both the min and max values.

The offsets you add should be uniform for both min and max, based on the range of indices you are processing.

I think it should calculate the min and max indices first then apply the vertex offset for that specific draw call to the indices. Then track the overall min and max indices across all draw calls comparing the adjusted values.

I think I understand what you mean and I think you are right. I updated the patch accordingly

[ci-tester-lunarg]

CI gfxreconstruct build queued with queue ID 359227.

[ci-tester-lunarg]

CI gfxreconstruct build # 5961 running.

[ci-tester-lunarg]

CI gfxreconstruct build # 5961 passed.

[hongSARC]

Could you also pass the original binary file offset? (bufferOffset)

The vertexOffset is relative to the buffer segment for that draw, but the actual data in binary file may be in different location.
I'll need that bufferOffset to correctly subtract or add it when reading vertex data, ensuring that the offsets match the actual memory layout. Then I can properly adjust the vertex positions when parsing.

[panos-lunarg]

Could you also pass the original binary file offset? (bufferOffset)

What do you mean with bufferOffset? Do you mean the offsets used in vkCmdBindVertexBuffers, vkCmdBindIndexBuffer and co?

[hongSARC]

No, I'm looking for the absolute offset within the original binary file where the vertex buffer data starts. (I just referred as "bufferOffset")
This would allow me to correctly calculate the memory layout and adjust the vertex positions when parsing. The vertexOffset provided is relative to the buffer segment, but I need to know the starting point within the binary file to properly read the data.
Otherwise, I'll need to reconstruct it using buffer bindings (I think), which is more complex..

Maybe within the "parameters" or inside "vertexBuffers in api json? (fileOffset is what I meant. We need to have for both index buffer and vertex buffer file offset to handle indirect draw calls since it contains multiple draws I need to parse.)
example)

"indexBuffer": {
  "bufferId": 2145,
  "file": "indirect_indexBuffer_qs_2167_bcb_2094_dc_2103_uint16.bin",
  "fileOffset": 4
},
"vertexBuffers": [
  {
    "bufferId": 2144,
    "vertexBufferBinding": 0,
    "file": "indirect_vertexBuffers_qs_2167_bcb_2094_dc_2103_binding_0.bin",
    "fileOffset": 20
  },

---

Here are the steps of my parsing, (FYI)
-Use firstIndex and indexCount to get the indices array from the index buffer.
-Use the vertexOffset (relative to the buffer segment) along with the "bufferOffset" (absolute offset within the binary file) to calculate the correct starting point for the vertex buffer.
With the correct starting point, I can then parse the vertex buffer using the calculated offset.

[panos-lunarg]

Here are the steps of my parsing, (FYI)
-Use firstIndex and indexCount to get the indices array from the index buffer

This looks correct

-Use the vertexOffset (relative to the buffer segment) along with the "bufferOffset" (absolute offset within the binary file) to calculate the correct starting point for the vertex buffer.

I think this is not correct.
With #1915 we included the vertexOffset when the buffer is dumped. The bufferOffset was included from the beginning.
When dumping the buffer gfxr does:

const uint32_t offset = vb_entry->second.offset + (min_max_vertex_indices.first * binding_stride);

• vb_entry->second.offset is the offset used in vkCmdBindVertexBuffers
• (min_max_vertex_indices.first * binding_stride) is the offset added by the vertex offset (added by Vulkan dump resources: Fix in vertex buffer size calculation #1915)

Also in order to correctly detect the memory layout you will also need the information in VkPipelineVertexInputStateCreateInfo provided by the graphics pipeline (each binding's stride and format and offset for each attribute) but I assume you already take these into consideration.

[panos-lunarg]

The buffer is dumped starting from that offset. I don't think it makes sense to add it again in your calculations

[hongSARC]

(Accidently deleted this comment so adding it back..)

I think this is not correct. With #1915 we included the vertexOffset when the buffer is dumped. The bufferOffset was included from the beginning. When dumping the buffer gfxr does:

const uint32_t offset = vb_entry->second.offset + (min_max_vertex_indices.first * binding_stride);

Yes, this is the offset for the start of the buffer right? This is the value I would like to have passed into part of the parameters like this example. (as fileOffset)

"indexBuffer": {
  "bufferId": 2145,
  "file": "indirect_indexBuffer_qs_2167_bcb_2094_dc_2103_uint16.bin",
  "fileOffset": 4
},
"vertexBuffers": [
  {
    "bufferId": 2144,
    "vertexBufferBinding": 0,
    "file": "indirect_vertexBuffers_qs_2167_bcb_2094_dc_2103_binding_0.bin",
    "fileOffset": 20
  },

Since I don't have direct access to the min indices and binding_offset values, could you provide the pre-calculated "offset" value? This would allow me to directly access the buffer data in the binary file instead of doing the calculation again.

[panos-lunarg]

Yes, this is the offset for the start of the buffer right? This is the value I would like to have passed into part of the parameters like this example. (as fileOffset)

The buffer is dumped starting from that offset. I don't think it makes sense to add it again in your calculations

[hongSARC]

I understand what you mean by 'The buffer is dumped starting from that offset', but I still think I need the that offset value to correctly parse the vertex data (for like indirect calls). The vertexOffset value in the indirectParams is relative to the start of the buffer, but I don't know where the buffer starts in the binary file. Without the fileOffset value, I won't be able to correctly calculate the absolute offset within the binary file and parse the data accurately.

Since I would need to search each draw's data from the vertexOffset given from indirectParams, I would need the absolute offset of the file to correctly locate. Unless the vertexOffset is already accounting with the file offset, I would have to calculate the offset of the draw call based on the given file, which is already applied with another offset. Does this make sense or am I misunderstanding something?

[dfriederich]

The issue is that for indirect draws, there can be multiple VkDrawIndexedIndirectCommand/VkDrawIndirectCommand setups. While the offset used is correct for the one setup with the lowest vertex, it may be incorrect for others, each has its own vertexOffset, only index, etc. So we cannot just relay on gfxr to start at the proper offset, we have to adapt it to each one individually, hence for this we do need the offset.

Also from a data point of view, I think the json file should contain all the data, and the offset used for the file should be conceptually be part of that data. Right now we just have a file with content (hence the file size), but the json does not state the offset it was started for, I think it really should.

[panos-lunarg]

The issue is that for indirect draws, there can be multiple VkDrawIndexedIndirectCommand/VkDrawIndirectCommand setups. While the offset used is correct for the one setup with the lowest vertex, it may be incorrect for others, each has its own vertexOffset, only index, etc. So we cannot just relay on gfxr to start at the proper offset, we have to adapt it to each one individually, hence for this we do need the offset.

I think it wasn't a very good idea that in #1915 we added the vertexOffset in buffer's offset calculation because of indirect draw calls and it would be more sane to dump the buffers starting at the offset it was bound with vkCmdBindVertexBuffers. That's the buffer the application used in the first place.

Indirect parameters are read from the buffers and are dumped in the output json (an example of a vkCmdDrawIndexedIndirect: https://pastebin.com/raw/5G5t6TKW) so that information is available. I think the are two missing pieces of information from the json that is needed to parse the dumped buffers: One is the current pipeline's VkPipelineVertexInputStateCreateInfo and the other is the VkIndexType of the bound index buffer.

In any case I can try to add the offset in the buffer's json entries although that won't be very straightforward as the buffers are dumped in a different place than where the json is generated and the calculated offset contributed from the vertexOffset (which requires parsing the index buffer) is lost by that time.

[ci-tester-lunarg]

CI gfxreconstruct build queued with queue ID 375110.

[panos-lunarg]

Pushed more commits in this PR.
Adds the offset in the dumped buffer. Also adds the vertex state input information per draw call and the index type of the bound index buffer

[ci-tester-lunarg]

CI gfxreconstruct build # 6096 running.

[ci-tester-lunarg]

CI gfxreconstruct build # 6096 failed.

[hongSARC]

I think it wasn't a very good idea that in #1915 we added the vertexOffset in buffer's offset calculation because of indirect draw calls and it would be more sane to dump the buffers starting at the offset it was bound with vkCmdBindVertexBuffers. That's the buffer the application used in the first place.

I understand the perspective, but I'm not sure which would be the "correct" way to handle this. While it's true that including it might provide more context about the original buffer layout, it would also result in larger files with potentially unnecessary data. On the other hand, having the vertexOffset would allow us to trim the amount of unused data sent back, regardless of the type of draw call. It's a trade-off between preserving buffer context and minimizing data overhead. I think both options could make sense, but in context of my use cases, the additional data is not needed. As for indirect call, I would need to caculate min index again

In any case I can try to add the offset in the buffer's json entries although that won't be very straightforward as the buffers are dumped in a different place than where the json is generated and the calculated offset contributed from the vertexOffset (which requires parsing the index buffer) is lost by that time.

I see. I thought this would be straight forward since other parameters were also being outputted to json and the file offset is basically starting point of the binary buffer dumped.