vkd3d-shader: Make dumped shaders more comfortable to work with.
Merge request reports
Activity
+void vkd3d_compute_checksum(const void *data, size_t size, uint32_t checksum[4]) +{ + const uint8_t *ptr = data; + struct md5_ctx ctx; + + md5_init(&ctx); + md5_update(&ctx, ptr, size); + dxbc_checksum_final(&ctx); + + memcpy(checksum, ctx.digest, sizeof(ctx.digest)); +}
Should we use regular MD5 (or even SHA1; it's not much more complicated) for this instead of the DXBC variant? If we're going with the DXBC variant, I think we can easily implement vkd3d_compute_dxbc_checksum() on top of vkd3d_compute_checksum(), and probably move it to dxbc.c.
How much do we care that source and target shaders use the same base name? I think it makes the code a bit more awkward in a few spots, but perhaps it's worth it?
+static const char *shader_get_target_type_suffix(enum vkd3d_shader_target_type type) +{ + switch (type) + { + case VKD3D_SHADER_TARGET_SPIRV_BINARY: + return "spirv"; + case VKD3D_SHADER_TARGET_SPIRV_TEXT: + return "spirv.txt"; + case VKD3D_SHADER_TARGET_D3D_ASM: + return "asm"; + case VKD3D_SHADER_TARGET_D3D_BYTECODE: + return "d3dbc"; + case VKD3D_SHADER_TARGET_DXBC_TPF: + return "dxbc"; + case VKD3D_SHADER_TARGET_FX: + return "fx"; + default: + vkd3d_unreachable(); } }
That doesn't seem to handle VKD3D_SHADER_TARGET_GLSL.
Should we use regular MD5 (or even SHA1; it's not much more complicated) for this instead of the DXBC variant? If we're going with the DXBC variant, I think we can easily implement vkd3d_compute_dxbc_checksum() on top of vkd3d_compute_checksum(), and probably move it to dxbc.c.
My reasoning was that we're using whatever hash function just to generate unique filenames, we don't particularly care that it is cryptographically strong or standard. So I reused the same function to avoid adding a lot of code for a convenient but minor debugging feature. If we're adding another hash function, then I'd say the default choice would be SHA256. But ultimately I don't feel strongly about any choice.
How much do we care that source and target shaders use the same base name? I think it makes the code a bit more awkward in a few spots, but perhaps it's worth it?
I think it's useful to quickly pair the input and output files. Otherwise one has to look for two log lines, one telling the input and another one telling the output file.
My reasoning was that we're using whatever hash function just to generate unique filenames, we don't particularly care that it is cryptographically strong or standard. So I reused the same function to avoid adding a lot of code for a convenient but minor debugging feature. If we're adding another hash function, then I'd say the default choice would be SHA256. But ultimately I don't feel strongly about any choice.
Thinking again, do you know precisely what we should change to that checksum function to make the standard MD5? Looking at the function names it might be just the final algorithm. It might be easy to use a standard (if old) hash function while at the same time avoiding adding an entire new hash implementation to the code.
Thinking again, do you know precisely what we should change to that checksum function to make the standard MD5? Looking at the function names it might be just the final algorithm. It might be easy to use a standard (if old) hash function while at the same time avoiding adding an entire new hash implementation to the code.
Essentially, yes. See commit 49e55dd6.
added 45 commits
-
b0671c2c...32dc7ff4 - 43 commits from branch
wine:master
- 8a0f7514 - vkd3d-shader: Use a hash to build the filename when dumping shaders.
- 312003fd - vkd3d-shader: Dump the converted shader too.
-
b0671c2c...32dc7ff4 - 43 commits from branch
added 6 commits
-
2b00110d...0e72aba0 - 4 commits from branch
wine:master
- 15cba575 - vkd3d-shader: Use a hash to build the filename when dumping shaders.
- 2f31ad46 - vkd3d-shader: Dump the converted shader too.
-
2b00110d...0e72aba0 - 4 commits from branch
458 vkd3d_compute_md5(data, size, (uint32_t *)checksum, VKD3D_MD5_STANDARD); 459 460 for (i = 0; i < ARRAY_SIZE(checksum); ++i) 461 { 462 str_checksum[2 * i] = hexadecimal_digits[checksum[i] >> 4]; 463 str_checksum[2 * i + 1] = hexadecimal_digits[checksum[i] & 0xf]; 464 } 465 str_checksum[32] = '\0'; 457 466 458 467 if (profile) 459 snprintf(filename, ARRAY_SIZE(filename), "%s/vkd3d-shader-%u-%s.%s", path, id, profile, suffix); 468 snprintf(filename, ARRAY_SIZE(filename), "%s/vkd3d-shader-%s-%s.%s", path, str_checksum, profile, suffix); 460 469 else 461 snprintf(filename, ARRAY_SIZE(filename), "%s/vkd3d-shader-%u.%s", path, id, suffix); 470 snprintf(filename, ARRAY_SIZE(filename), "%s/vkd3d-shader-%s.%s", path, str_checksum, suffix); 471 TRACE("Dumping shader to %s.\n", filename); I guess it's on you to take the final decision, but the thing I don't like about trailing dots is that they make it harder to quickly select (usually to copy and paste) whatever they're next to. For example, on my terminal emulator I can double click on a character and it will automatically select all the contiguous non-whitespace character, including the dot, which is usually not what I want. This means that I have to spend more time clicking precisely the first and last character of the selection, or edit the pasted text.
changed this line in version 6 of the diff
I guess it's on you to take the final decision, but the thing I don't like about trailing dots is that they make it harder to quickly select (usually to copy and paste) whatever they're next to. For example, on my terminal emulator I can double click on a character and it will automatically select all the contiguous non-whitespace character, including the dot, which is usually not what I want. This means that I have to spend more time clicking precisely the first and last character of the selection, or edit the pasted text.
True. I think adding quotes around the filename would help though, right?