Skip to content
Snippets Groups Projects

vkd3d-dxbc: Add options to fix a DXBC shader checksum.

Merged Giovanni Mascellani requested to merge giomasce/vkd3d:olympus into master
2 unresolved threads

Merge request reports

Loading
Loading

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

    • -        {"colour",    no_argument,       NULL, OPTION_COLOUR},
      -        {"help",      no_argument,       NULL, OPTION_HELP},
      -        {"list",      no_argument,       NULL, OPTION_LIST},
      -        {"list-data", no_argument,       NULL, OPTION_LIST_DATA},
      -        {"no-colour", no_argument,       NULL, OPTION_NO_COLOUR},
      -        {"version",   no_argument,       NULL, OPTION_VERSION},
      -        {NULL,        0,                 NULL, 0},
      +        {"colour",          no_argument,       NULL, OPTION_COLOUR},
      +        {"help",            no_argument,       NULL, OPTION_HELP},
      +        {"list",            no_argument,       NULL, OPTION_LIST},
      +        {"list-data",       no_argument,       NULL, OPTION_LIST_DATA},
      +        {"no-colour",       no_argument,       NULL, OPTION_NO_COLOUR},
      +        {"version",         no_argument,       NULL, OPTION_VERSION},
      +        {"ignore-checksum", no_argument,       NULL, OPTION_IGNORE_CHECKSUM},
      +        {NULL,              0,                 NULL, 0},

      No big deal, but these were ordered before.

      @@ -173,6 +180,7 @@ static void print_usage(const char *program_name)
               "  --list-data              List the data contained in the DXBC sections.\n"
               "  --no-colour              Disable colour, even when supported by the output.\n"
               "  -V, --version            Display version information and exit.\n"
      +        "  --ignore-checksum        Do not validate the checksum when parsing the DXBC blob.\n"
               "  --                       Stop option processing. Any subsequent argument is\n"
               "                           interpreted as a filename.\n"
               "\n"

      Likewise.

      -    ret = vkd3d_shader_parse_dxbc(&dxbc, 0, &dxbc_desc, &messages);
      +    flags = 0;
      +    if (options.ignore_checksum)
      +        flags |= VKD3D_SHADER_PARSE_DXBC_IGNORE_CHECKSUM;
      +
      +    ret = vkd3d_shader_parse_dxbc(&dxbc, flags, &dxbc_desc, &messages);

      There may be some value in trying without VKD3D_SHADER_PARSE_DXBC_IGNORE_CHECKSUM first, to determine whether the checksum was valid or not.

      +        "  -o, --output=<file>      Re-serialize the DXBC shader to <file>. This is useful\n"
      +        "                           in combination with --ignore-checksum to fix bad or\n"
      +        "                           missing checksums.\n"

      I'm a little worried about painting ourselves into a corner with this. In particular, there are a couple of features I'd like to add in the future:

      • Updating the contents of DXBC sections.
      • Appending DXBC sections.
      • Extracting the contents of DXBC sections.

      That last feature in particular potentially implies having multiple output files. You could e.g. imagine syntax like this:

      # Extract the ISGN, OSGN, and SHDR sections to the corresponding files.
      vkd3d-dxbc -o shader.ISGN -x t:ISGN -o shader.OSGN -x t:OSGN -o shader.SHDR -x t:SHDR shader.dxbc
      # Extract the SHEX section, pipe to hexdump.
      vkd3d-dxbc -x t:SHEX shader.dxbc | hexdump -C
      # Update the SHDR section from shader.SHDR, write the new DXBC to shader2.dxbc.
      vkd3d-dxbc -i shader.SHDR -u t:SHDR -o shader2.dxbc shader.dxbc
      # Update the SHDR section from shader.SHDR, pipe to xxd.
      vkd3d-dxbc -u t:SHDR shader.dxbc < shader.SHDR | xxd -i

      I.e., -i specifies the input file for subsequent operations, -o specifies the output file for subsequent operations, when unspecified stdin/stdout is used, and there's an implied "write output if modified" at the end. The question then becomes how rewriting the checksum fits in. One option would be to make that an explicit operation. E.g.:

      vkd3d-dxbc --ignore-checksum --recalculate-checksum shader.dxbc > shader2.dxbc
      vkd3d-dxbc --ignore-checksum -o shader2.dxbc --recalculate-checksum shader.dxbc

      Thoughts?

    • There may be some value in trying without VKD3D_SHADER_PARSE_DXBC_IGNORE_CHECKSUM first, to determine whether the checksum was valid or not.

      And what would happen depending on whether it's valid or not?

      I'm a little worried about painting ourselves into a corner with this. In particular, there are a couple of features I'd like to add in the future:

      • Updating the contents of DXBC sections.
      • Appending DXBC sections.
      • Extracting the contents of DXBC sections.

      That last feature in particular potentially implies having multiple output files. You could e.g. imagine syntax like this:

      # Extract the ISGN, OSGN, and SHDR sections to the corresponding files.
      vkd3d-dxbc -o shader.ISGN -x t:ISGN -o shader.OSGN -x t:OSGN -o shader.SHDR -x t:SHDR shader.dxbc
      # Extract the SHEX section, pipe to hexdump.
      vkd3d-dxbc -x t:SHEX shader.dxbc | hexdump -C
      # Update the SHDR section from shader.SHDR, write the new DXBC to shader2.dxbc.
      vkd3d-dxbc -i shader.SHDR -u t:SHDR -o shader2.dxbc shader.dxbc
      # Update the SHDR section from shader.SHDR, pipe to xxd.
      vkd3d-dxbc -u t:SHDR shader.dxbc < shader.SHDR | xxd -i

      I.e., -i specifies the input file for subsequent operations, -o specifies the output file for subsequent operations, when unspecified stdin/stdout is used, and there's an implied "write output if modified" at the end. The question then becomes how rewriting the checksum fits in. One option would be to make that an explicit operation. E.g.:

      vkd3d-dxbc --ignore-checksum --recalculate-checksum shader.dxbc > shader2.dxbc
      vkd3d-dxbc --ignore-checksum -o shader2.dxbc --recalculate-checksum shader.dxbc

      Thoughts?

      • I don't completely understand how you perform input: you seem to have the concept of a "main input" (using the positional argument at the end), which is interpreted as the beginning of a pipeline on which a few filters operate, possibly using auxiliary inputs (using -o). Is that the intention? The main input is allowed to be the stdin too?
      • You seem the consider -o an option (changing the command line parsing state but not performing actions themselves) and the various filter (-x, -u) as the actions, i.e., points at which the program does something for real. Maybe it might be more expressive to reverse the point of view? I.e., -x and -u merely configure a filter on the pipeline (with the possibility to chain many, or zero) and -o takes the current pipeline state, executes it and writes to the specified file. My proposal rather fits this model (except that no filter is currently defined, i.e., you can only write the same shader you just read), which looks more flexible to me. For example you can chain or otherwise compose filters and operators, which seems harder if filters immediately trigger an action.
      • --recalculate-checksum doesn't sound right to me: in my model the checksum is recalculated merely by virtue of the fact that each time we emit a DXBC file we put the right checksum. There is no explicit action to do so. To me vkd3d-dxbc --ignore-checksum -o shader2.dxbc shader.dxbc means: "Read shader.dxbc and rewrite it to shader2.dxbc without changing nothing, and without caring if when reading the checksum is not correct". The write will naturally put the correct checksum, because it's the only thing it's able to do.
    • Please register or sign in to reply
  • There may be some value in trying without VKD3D_SHADER_PARSE_DXBC_IGNORE_CHECKSUM first, to determine whether the checksum was valid or not.

    And what would happen depending on whether it's valid or not?

    We could print a "(valid)" or "(invalid)" next to the checksum in the --list output, I guess.

    • I don't completely understand how you perform input: you seem to have the concept of a "main input" (using the positional argument at the end), which is interpreted as the beginning of a pipeline on which a few filters operate, possibly using auxiliary inputs (using -o). Is that the intention? The main input is allowed to be the stdin too?

    I'm not sure pipeline/filters is the quite the right model, but essentially, yes. The current input/output start out as stdin/stdout, and -i/-o change them. Add/update/extract etc. just use the current input output file, and after parsing the command line we'd have a list like this:

        struct
        {
            enum operation op;
            const char *section;
            const char *file;
        }
        operations[] =
        {
            {EXTRACT, "t:SHDR", "shader.SHDR"},
            {UPDATE,  "t:ISGN", "shader.ISGN"},
            {DELETE,  "t:SHEX", NULL},
            {ADD,     "t:OSGN", "shader.OSGN"},
        };
    • You seem the consider -o an option (changing the command line parsing state but not performing actions themselves) and the various filter (-x, -u) as the actions, i.e., points at which the program does something for real. Maybe it might be more expressive to reverse the point of view? I.e., -x and -u merely configure a filter on the pipeline (with the possibility to chain many, or zero) and -o takes the current pipeline state, executes it and writes to the specified file. My proposal rather fits this model (except that no filter is currently defined, i.e., you can only write the same shader you just read), which looks more flexible to me. For example you can chain or otherwise compose filters and operators, which seems harder if filters immediately trigger an action.

    I think you're essentially arguing for something like

    # Extract the SHDR section to shader.SHDR
    vkd3d-dxbc -x t:SHDR -o shader.SHDR shader.dxbc
    # Update the SHDR section from shader.SHDR, write the new DXBC to shader2.dxbc.
    vkd3d-dxbc -i shader.SHDR -u t:SHDR -o shader2.dxbc shader.dxbc

    right?

    I don't necessarily have an issue with that, but I guess the question then becomes what e.g. these do:

    vkd3d-dxbc -i shader.SHDR -u t:SHDR -i shader.ISGN -u t:SHDR -o shader2.dxbc shader.dxbc
    vkd3d-dxbc -i shader.SHDR -u t:SHDR -i shader.ISGN -u t:ISGN -x t:ISGN -o shader2.dxbc shader.dxbc
    vkd3d-dxbc -i shader.SHDR -u t:SHDR -i shader.ISGN -o shader2.dxbc -u t:ISGN -x t:ISGN shader.dxbc
    vkd3d-dxbc -x t:ISGN -o shader.ISGN -x t:OSGN -o shader.OSGN shader.dxbc
    vkd3d-dxbc -o shader.txt shader.dxbc
    vkd3d-dxbc shader.dxbc > shader.txt

    and whether that's better than the alternative.

  • Giovanni Mascellani added 317 commits

    added 317 commits

    • 065d0a42...500b61c3 - 314 commits from branch wine:master
    • 1b726ce7 - vkd3d-shader/dxbc: Add flag to ignore the DXBC checksum.
    • e4b75324 - vkd3d-dxbc: Add an option to ignore checksum.
    • 6fabcd6c - vkd3d-dxbc: Add an option to re-emit the shader with the correct checksum.

    Compare with previous version

  • added 2 commits

    • f344a82d - vkd3d-dxbc: Add an option to ignore checksum.
    • c7ecda85 - vkd3d-dxbc: Add an option to re-emit the shader with the correct checksum.

    Compare with previous version

  • Is this better?

  • added 12 commits

    • c7ecda85...f866fb95 - 9 commits from branch wine:master
    • 67dd80df - vkd3d-shader/dxbc: Add flag to ignore the DXBC checksum.
    • a39f7547 - vkd3d-dxbc: Add an option to ignore checksum.
    • c3d1b33c - vkd3d-dxbc: Add an option to re-emit the shader with the correct checksum.

    Compare with previous version

  • added 3 commits

    • 1ddf3807 - vkd3d-shader/dxbc: Add flag to ignore the DXBC checksum.
    • db2a0632 - vkd3d-dxbc: Add an option to ignore checksum.
    • defb1f90 - vkd3d-dxbc: Add an option to re-emit the shader with the correct checksum.

    Compare with previous version

  •      vkd3d_compute_dxbc_checksum(data, data_size, calculated_checksum);
    -    if (memcmp(checksum, calculated_checksum, sizeof(checksum)))
    +    if (memcmp(checksum, calculated_checksum, sizeof(checksum))
    +            && !(flags & VKD3D_SHADER_PARSE_DXBC_IGNORE_CHECKSUM))

    It probably doesn't matter too much, but it seems like a bit of a waste to do the memcmp() if we're going to ignore the checksum anyway. In principle that's true for the vkd3d_compute_dxbc_checksum() call as well.

    +            case 'e':
    +            case OPTION_EMIT:
    +                action = action_push(options, ACTION_TYPE_EMIT);
    +                if (!options->output_filename)
    +                {
    +                    fprintf(stderr, "No output filename specified.\n");
    +                    return false;
    +                }

    Is that a problem? We could just write to stdout, right? In fact, I'd argue for just outputting to stdout in the initial commit, and then adding --output support in a separate commit.

    +        "  -o, --output=<file>      Re-serialize the DXBC shader to <file>. This is useful\n"
    +        "                           in combination with --ignore-checksum to fix bad or\n"
    +        "                           missing checksums.\n"

    That doesn't quite match what the code does now.

  • added 2 commits

    • 92402312 - vkd3d-dxbc: Add an option to re-emit the shader with the correct checksum.
    • c5f7994a - vkd3d-dxbc: Add an option to choose the output filename.

    Compare with previous version

    • -        if ((option = getopt_long(argc, argv, "htV", long_options, NULL)) == -1)
      +        if ((option = getopt_long(argc, argv, "ehtVo:", long_options, NULL)) == -1)

      "o:" belongs in the next commit.

      +            case 'e':
      +            case OPTION_EMIT:
      +                action_push(options, ACTION_TYPE_EMIT);
      +                if (isatty(fileno(stdout)))
      +                {
      +                    fprintf(stderr, "Output is a tty and output format is binary, exiting.\n"
      +                            "If this is really what you intended, specify the output explicitly.\n");
      +                    return false;
      +                }
      +                break;

      Relatedly, we can't specify the output explicitly yet in this commit.

      +        "Currently this tool can only re-emit the same DXBC blob it loaded. However it\n"
      +        "freshly recompute the checksum while doing so, so --emit can be useful\n"
      +        "together with --ignore-checksum to fix the checksum for a blob.\n";

      I think the wording is a bit awkward here. I'd propose "However, it will recompute ..." or possibly "However, it recomputes ..."

      Note that multiple --emit options will cause vkd3d-dxbc to output the DXBC blob multiple times. I think that's fine, perhaps even desirable, but it may be worth explicitly pointing out.

      -                if (!write_output(stdout, &serialized))
      +                if (!(output = open_output(options.output_filename, &close_output)))
      +                {
      +                    vkd3d_shader_free_shader_code(&serialized);
      +                    goto done;
      +                }
      +
      +                if (!write_output(output, &serialized))

      That should use "action->u.output_filename" instead of "options.output_filename", right?

    • Done, together with another couple of minor details I discovered in the meantime.

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

    • c1b61c77 - vkd3d-dxbc: Add an option to re-emit the shader with the correct checksum.
    • c6cb6f75 - vkd3d-dxbc: Add an option to choose the output filename.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard added 58 commits

    added 58 commits

    • c6cb6f75...23259263 - 54 commits from branch wine:master
    • 75bc6896 - vkd3d-shader/dxbc: Add flag to ignore the DXBC checksum.
    • 4b80b06f - vkd3d-dxbc: Add an option to ignore checksum.
    • b72ad78a - vkd3d-dxbc: Add an option to re-emit the shader with the correct checksum.
    • 7273d7f5 - vkd3d-dxbc: Add an option to choose the output filename.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading