Skip to content
Snippets Groups Projects

vkd3d-shader: Make dumped shaders more comfortable to work with.

Merged Giovanni Mascellani requested to merge giomasce/vkd3d:flegrei into master
4 unresolved threads

Merge request reports

Merge request pipeline #31044 skipped

Merge request pipeline skipped for 52796658

Approved by

Merged by Henri VerbeetHenri Verbeet 7 months ago (Aug 29, 2024 5:26pm UTC)

Merge details

  • Changes merged into master with 52796658.
  • Did not delete the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Giovanni Mascellani approved this merge request

    approved this merge request

    • +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.

    • Please register or sign in to reply
  • added 2 commits

    • b587fcd5 - vkd3d-shader: Use a hash to build the filename when dumping shaders.
    • b0671c2c - vkd3d-shader: Dump the converted shader too.

    Compare with previous version

    • 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.

    • Ok, now it uses standard MD5.

    • Please register or sign in to reply
  • 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.

    Compare with previous version

  • added 2 commits

    • 7e3f5d46 - vkd3d-shader: Use a hash to build the filename when dumping shaders.
    • 2b00110d - vkd3d-shader: Dump the converted shader too.

    Compare with previous version

  • Henri Verbeet added 6 commits

    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.

    Compare with previous version

  • How do you feel about the modified version I just pushed?

  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit 15cba575
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.

  • Henri Verbeet changed this line in version 6 of the diff

    changed this line in version 6 of the diff

  • Please register or sign in to reply
    • 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?

    • Yeah, it seems so.

    • Please register or sign in to reply
  • Henri Verbeet approved this merge request

    approved this merge request

  • Henri Verbeet added 2 commits

    added 2 commits

    • 1a0d6a4d - vkd3d-shader: Use a hash to build the filename when dumping shaders.
    • 52796658 - vkd3d-shader: Dump the converted shader too.

    Compare with previous version

  • Please register or sign in to reply
    Loading