Skip to content

Remove unnecessary payload data_size duplication #2575

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
marius-pelegrin-arm wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Remove unnecessary payload data_size duplication #2575

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
4 changes: 1 addition & 3 deletions framework/decode/api_payload.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,8 +429,6 @@ struct InitSubresourceArgs
struct InitDx12AccelerationStructureArgs
{
format::MetaDataId meta_data_id; // Needed by DispatchVisitor, but not ApiDecoder
// Note: the command header has uniquely named size field... so duplicate this info for visitor use
size_t data_size; // Needed for deferred decompression, but not ApiDecoder

format::InitDx12AccelerationStructureCommandHeader command_header;
std::vector<format::InitDx12AccelerationStructureGeometryDesc> geometry_descs;
Expand Down Expand Up @@ -548,7 +546,7 @@ struct AnnotationArgs

// NOTE: The string name is intentionally *not* data to differ from the "data" fields that are uint8_t *
// parameter data for the next level Decode operations, simplifying DispatchHasData logic
std::string annotation_data;
std::string annotation_data;

auto GetTuple() const { return std::tie(block_index, type, label, annotation_data); }
};
Expand Down
11 changes: 4 additions & 7 deletions framework/decode/block_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1223,7 +1223,7 @@ ParsedBlock BlockParser::ParseMetaData(BlockBuffer& block_buffer)
success = success && block_buffer.Read(header.inputs_flags);
success = success && block_buffer.Read(header.inputs_num_instance_descs);
success = success && block_buffer.Read(header.inputs_num_geometry_descs);
success = success && block_buffer.Read(header.inputs_data_size);
success = success && block_buffer.Read(header.data_size);

// Parse geometry descs.
std::vector<format::InitDx12AccelerationStructureGeometryDesc> geom_descs;
Expand All @@ -1250,15 +1250,12 @@ ParsedBlock BlockParser::ParseMetaData(BlockBuffer& block_buffer)
if (success)
{
const char* label = "init DX12 acceleration structure meta-data block";
ParameterReadResult read_result = ReadParameterBuffer(label, block_buffer, header.inputs_data_size);
ParameterReadResult read_result = ReadParameterBuffer(label, block_buffer, header.data_size);
Copy link
Contributor

@davidd-lunarg davidd-lunarg Dec 19, 2025

Choose a reason for hiding this comment

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

The third argument uncompressed_size to BlockParser::ReadParameterBuffer is ignored when compression is off, so header.data_size (previously header.inputs_data_size) is not used to set result.uncompressed_size.

gfxreconstruct/framework/decode/block_parser.cpp

Lines 259 to 263 in aae6ac7

else
{
// For uncompressed blocks the data size is the parameter buffer size
result.uncompressed_size = block_buffer.Remainder();
}

Looking at how block_buffer.Remainder() is calculated, I think uncompressed_size == block_buffer.Remainder() when the uncompressed_size argument is non-default. @jzulauf-lunarg Should an assert be added in ReadParameterBuffer to enforce/verify this? For example:

    else
    {
        // For uncompressed blocks the data size is the parameter buffer size
+       GFXRECON_ASSERT(uncompressed_size == kReadSizeFromBuffer || uncompressed_size == block_buffer.Remainder());
        result.uncompressed_size = block_buffer.Remainder();
    }

if (read_result.success)
{
return ParsedBlock(block_buffer.ReleaseData(),
InitDx12AccelerationStructureArgs{ meta_data_id,
read_result.uncompressed_size,
header,
geom_descs,
read_result.buffer.GetDataAs<uint8_t>() },
InitDx12AccelerationStructureArgs{
meta_data_id, header, geom_descs, read_result.buffer.GetDataAs<uint8_t>() },
read_result.is_compressed);
}
}
Expand Down
13 changes: 6 additions & 7 deletions framework/decode/dx12_acceleration_structure_builder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,8 @@ void Dx12AccelerationStructureBuilder::SetupBuild(

// Map GPU VAs in instance desc input data.
temp_instance_desc_input_data_.clear();
temp_instance_desc_input_data_.insert(temp_instance_desc_input_data_.end(),
build_inputs_data,
build_inputs_data + command_header.inputs_data_size);
temp_instance_desc_input_data_.insert(
temp_instance_desc_input_data_.end(), build_inputs_data, build_inputs_data + command_header.data_size);
constexpr auto address_stride = sizeof(D3D12_RAYTRACING_INSTANCE_DESC);
constexpr auto address_offset = offsetof(D3D12_RAYTRACING_INSTANCE_DESC, AccelerationStructure);
for (UINT i = 0; i < inputs_desc.NumDescs; ++i)
Expand All @@ -260,7 +259,7 @@ void Dx12AccelerationStructureBuilder::SetupBuild(
graphics::dx12::GetAccelerationStructureInputsBufferEntries(
inputs_desc, temp_geometry_descs_.data(), inputs_buffer_size, inputs_buffer_entries);

GFXRECON_ASSERT(inputs_buffer_size == command_header.inputs_data_size);
GFXRECON_ASSERT(inputs_buffer_size == command_header.data_size);

// Get required sizes and update buffers.
D3D12_RAYTRACING_ACCELERATION_STRUCTURE_PREBUILD_INFO prebuild_info;
Expand All @@ -275,7 +274,7 @@ void Dx12AccelerationStructureBuilder::SetupBuild(
UpdateBufferSize(device5_,
inputs_buffer_,
inputs_buffer_size_,
command_header.inputs_data_size,
command_header.data_size,
D3D12_HEAP_TYPE_UPLOAD,
D3D12_RESOURCE_STATE_GENERIC_READ,
D3D12_RESOURCE_FLAG_NONE);
Expand All @@ -292,9 +291,9 @@ void Dx12AccelerationStructureBuilder::SetupBuild(
}

// Write inputs data to resources.
GFXRECON_CHECK_CONVERSION_DATA_LOSS(size_t, command_header.inputs_data_size);
GFXRECON_CHECK_CONVERSION_DATA_LOSS(size_t, command_header.data_size);
HRESULT hr =
MapSubresourceAndWriteData(inputs_buffer_, 0, static_cast<size_t>(command_header.inputs_data_size), final_data);
MapSubresourceAndWriteData(inputs_buffer_, 0, static_cast<size_t>(command_header.data_size), final_data);
GFXRECON_ASSERT(SUCCEEDED(hr));

// Fix GPU VAs that point into buffers.
Expand Down
4 changes: 2 additions & 2 deletions framework/decode/dx12_json_consumer_base.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,11 @@ void Dx12JsonConsumerBase::ProcessInitDx12AccelerationStructureCommand(
json_options);
FieldToJson(jdata["inputs_num_instance_descs"], command_header.inputs_num_instance_descs, json_options);
FieldToJson(jdata["inputs_num_geometry_descs"], command_header.inputs_num_geometry_descs, json_options);
FieldToJson(jdata["inputs_data_size"], command_header.inputs_data_size, json_options);
FieldToJson(jdata["data_size"], command_header.data_size, json_options);
RepresentBinaryFile(*(this->writer_),
jdata[format::kNameData],
"initdx12accelerationstructurecommand.bin",
command_header.inputs_data_size,
command_header.data_size,
build_inputs_data);
FieldToJson(jdata["geometry_descs"], geometry_descs.data(), geometry_descs.size(), json_options);
writer_->WriteBlockEnd();
Expand Down
21 changes: 11 additions & 10 deletions framework/encode/dx12_state_writer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ Dx12StateWriter::Dx12StateWriter(util::FileOutputStream* output_stream,
util::Compressor* compressor,
format::ThreadId thread_id,
util::FileOutputStream* asset_file_stream) :
output_stream_(output_stream), compressor_(compressor), thread_id_(thread_id), encoder_(&parameter_stream_)
output_stream_(output_stream),
compressor_(compressor), thread_id_(thread_id), encoder_(&parameter_stream_)
{
assert(output_stream != nullptr);
}
Expand Down Expand Up @@ -790,7 +791,7 @@ void Dx12StateWriter::WriteMetaCommandCreationState(const Dx12StateTable& state_
for (auto wrapper : metacommand_wrappers)
{
// Write the meta command init call.
auto wrapper_info = wrapper->GetObjectInfo();
auto wrapper_info = wrapper->GetObjectInfo();
if (wrapper_info->was_initialized == true)
{
format::InitializeMetaCommand init_meta_command;
Expand Down Expand Up @@ -846,13 +847,13 @@ void Dx12StateWriter::WriteResourceSnapshots(
begin_cmd.meta_header.block_header.type = format::kMetaDataBlock;
begin_cmd.meta_header.meta_data_id = format::MakeMetaDataId(
format::ApiFamilyId::ApiFamily_D3D12, format::MetaDataType::kBeginResourceInitCommand);
begin_cmd.thread_id = thread_id_;
begin_cmd.device_id = device_id;
begin_cmd.thread_id = thread_id_;
begin_cmd.device_id = device_id;

// TODO: adjust to hold sum of resource-sizes
begin_cmd.total_copy_size = max_resource_size;

begin_cmd.max_copy_size = max_resource_size;
begin_cmd.max_copy_size = max_resource_size;

output_stream_->Write(&begin_cmd, sizeof(begin_cmd));

Expand Down Expand Up @@ -1748,12 +1749,12 @@ void Dx12StateWriter::WriteAccelerationStructuresState(

// Get NumDescs and data sizes.
size_t inputs_data_ptr_file_size = 0;
cmd.inputs_data_size = 0;
cmd.data_size = 0;
cmd.inputs_num_instance_descs = 0;
cmd.inputs_num_geometry_descs = 0;
if (write_build_data)
{
cmd.inputs_data_size = as_build.input_data_size - as_build.input_data_header_size;
cmd.data_size = as_build.input_data_size - as_build.input_data_header_size;
if (as_build.inputs.Type == D3D12_RAYTRACING_ACCELERATION_STRUCTURE_TYPE_BOTTOM_LEVEL)
{
cmd.inputs_num_geometry_descs = as_build.inputs.NumDescs;
Expand All @@ -1767,15 +1768,15 @@ void Dx12StateWriter::WriteAccelerationStructuresState(
GFXRECON_ASSERT(false && "Invalid D3D12_RAYTRACING_ACCELERATION_STRUCTURE_TYPE.");
}

GFXRECON_CHECK_CONVERSION_DATA_LOSS(size_t, cmd.inputs_data_size);
inputs_data_ptr_file_size = static_cast<size_t>(cmd.inputs_data_size);
GFXRECON_CHECK_CONVERSION_DATA_LOSS(size_t, cmd.data_size);
inputs_data_ptr_file_size = static_cast<size_t>(cmd.data_size);
if (compressor_ != nullptr)
{
// Compress block data.
size_t compressed_size =
compressor_->Compress(inputs_data_ptr_file_size, inputs_data_ptr, &compressed_parameter_buffer_, 0);

if ((compressed_size > 0) && (compressed_size < cmd.inputs_data_size))
if ((compressed_size > 0) && (compressed_size < cmd.data_size))
{
cmd.meta_header.block_header.type = format::BlockType::kCompressedMetaDataBlock;

Expand Down
2 changes: 1 addition & 1 deletion framework/format/format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@ struct InitDx12AccelerationStructureCommandHeader
uint32_t inputs_flags{ 0 };
uint32_t inputs_num_instance_descs{ 0 }; ///< NumDescs for TLAS
uint32_t inputs_num_geometry_descs{ 0 }; ///< NumDescs for BLAS
uint64_t inputs_data_size{ 0 };
uint64_t data_size{ 0 };

// In the capture file, accel struct data is written in the following order:
// InitDx12AccelerationStructureCommandHeader
Expand Down
6 changes: 3 additions & 3 deletions tools/compress/compression_converter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ decode::FileTransformer::VisitResult CompressionConverter::WriteMetaData(const d
if (init_cmd.data_size > 0)
{
GFXRECON_CHECK_CONVERSION_DATA_LOSS(size_t, init_cmd.data_size);
size_t data_size = static_cast<size_t>(init_cmd.data_size);
size_t data_size = static_cast<size_t>(init_cmd.data_size);
const uint8_t* data_address = args.data;

PrepMetadataBlock(init_cmd.meta_header, args.meta_data_id, data_address, data_size);
Expand Down Expand Up @@ -501,8 +501,8 @@ CompressionConverter::WriteMetaData(const decode::InitDx12AccelerationStructureA
const std::vector<format::InitDx12AccelerationStructureGeometryDesc>& geom_descs = args.geometry_descs;
GFXRECON_ASSERT(args.geometry_descs.size() == init_cmd.inputs_num_geometry_descs);

GFXRECON_CHECK_CONVERSION_DATA_LOSS(size_t, init_cmd.inputs_data_size);
size_t data_size = static_cast<size_t>(init_cmd.inputs_data_size);
GFXRECON_CHECK_CONVERSION_DATA_LOSS(size_t, init_cmd.data_size);
size_t data_size = static_cast<size_t>(init_cmd.data_size);
const uint8_t* data_address = args.data;

PrepMetadataBlock(init_cmd.meta_header, args.meta_data_id, data_address, data_size);
Expand Down