vkd3d-dxbc: Add options to fix a DXBC shader checksum.
Merge request reports
Activity
- {"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 mevkd3d-dxbc --ignore-checksum -o shader2.dxbc shader.dxbc
means: "Readshader.dxbc
and rewrite it toshader2.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.
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.
- 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
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.
Toggle commit list-
065d0a42...500b61c3 - 314 commits from branch
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.
Toggle commit list-
c7ecda85...f866fb95 - 9 commits from branch
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.
- 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?
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.
Toggle commit list-
c6cb6f75...23259263 - 54 commits from branch