KhronosGroup / KTX-Software

KTX (Khronos Texture) Library and Tools
Other
854 stars 226 forks source link

Merge ktx compare to main #868

Closed aqnuep closed 4 months ago

aqnuep commented 5 months ago

This PR contains all the code changes for the ktx compare tool, alongside other changes related to fixes needed to be done in order for ktx compare to work as expected (including the fixes related to the handling of the D24S8 format).

@MarkCallow, @lexaknyazev, FYI.

aqnuep commented 5 months ago

@MarkCallow could you prioritize reviewing this and the related CTS PR this week?

It is practically the combined PR of all the previously reviewed PRs plus rebasing/bug fixing, so it should be fairly straightforward.

MarkCallow commented 5 months ago

@MarkCallow could you prioritize reviewing this and the related CTS PR this week?

I'll get to it as soon as I can. I have to get 4.3.2 out before I can merge this to main.

aqnuep commented 5 months ago

ktx help compare produces

ktx help fatal: Invalid command specified: "compare". See 'ktx help --help'.

Thanks for the feedback, I'll check what's going on.

I think we need header lines (at least in text output) like

--- input-file1
+++ input-file2

to easily see what the minuses and pluses refer to.

How much work will it be to add color-coding like git diff?

I would like an option to print the name of the file being processed here when the files differ and in validate when a file is invalid. It is useful when running a script over multiple files. See grep -H for example. How would such be handled in the JSON output and how much work will it be. I realize this is outside the current scope but I want to propose it to the TSG.

The text output of ktx compare is not supposed to be an actual diff-like output, as it wouldn't work in general for all the types of differences the tool can output. Not to mention that it would require significantly more effort to include context, etc.

Outputting the filenames for every difference would be possible, but it would make things extremely verbose as well. Also, per above, the output is "context-free", and does not output matching parts, hence every single diff would have to be wrapped with the filename headers, which would be highly undesirable.

Color coding would be possible, but you'd have to consider output support for ansi-color (terminal support and not including colors when output is captured in a file stream).

Everything is doable, but the output format has been out there for review for months, and at this point in time we do not have sufficient time to implement these additional requests, even if we'd agree to do them.

MarkCallow commented 4 months ago

The text output of ktx compare is not supposed to be an actual diff-like output, as it wouldn't work in general for all the types of differences the tool can output. Not to mention that it would require significantly more effort to include context, etc.

Maybe I confused you by using 3 pluses and minuses. I know it is not actual diff-like output. I am not looking for context. I am looking for an indication of which file corresponds to the - and which to the +. Basically just printing 2 lines at the top of the output.

Outputting the filenames for every difference would be possible, but it would make things extremely verbose as well. Also, per above, the output is "context-free", and does not output matching parts, hence every single diff would have to be wrapped with the filename headers, which would be highly undesirable.

On reflection I think having a list of pairs of files to compare by way of a shell script loop is unlikely so this is not needed. To be clear, I was not looking for a filename at every difference, just once at the start of the output.

Color coding would be possible, but you'd have to consider output support for ansi-color (terminal support and not including colors when output is captured in a file stream).

Indeed. I am not asking you to add color-coding. I am asking for an estimate of the work so I can decide if it is something we can pursue.

Everything is doable, but the output format has been out there for review for months, and at this point in time we do not have sufficient time to implement these additional requests, even if we'd agree to do them.

Sometimes things only become apparent once you start using a tool.

MarkCallow commented 4 months ago

The man page needs an explanation of the texel bock diff output. The following is from a comparison of 2 R8G8B8A8 cubemaps. My questions, which should be answered in the documentation, follow:

+Mismatch in level 0 layer 0 face 0
  Coordinates: 2, 0, 0
    Image byte offset: 0x8
    File byte offset: 0x1100
-    Packed: 0x08, 0x09, 0x14, 0x03
+    Packed: 0x08, 0x09, 0x0a, 0x0b
-    Channels: 8, 9, 20, 3
+    Channels: 8, 9, 10, 11
  1. Coordinates presumably is image coordinates. Are they based on KTXorientation or do they assume top down orientation?
  2. What is the meaning of "Packed" here?
  3. What are the Channels? According to the DFDs both files have channel ids of 0, 1, 2 and 15 (RGBA).
aqnuep commented 4 months ago

The man page needs an explanation of the texel bock diff output. The following is from a comparison of 2 R8G8B8A8 cubemaps. My questions, which should be answered in the documentation, follow:

+Mismatch in level 0 layer 0 face 0
  Coordinates: 2, 0, 0
    Image byte offset: 0x8
    File byte offset: 0x1100
-    Packed: 0x08, 0x09, 0x14, 0x03
+    Packed: 0x08, 0x09, 0x0a, 0x0b
-    Channels: 8, 9, 20, 3
+    Channels: 8, 9, 10, 11
  1. Coordinates presumably is image coordinates. Are they based on KTXorientation or do they assume top down orientation?
  2. What is the meaning of "Packed" here?
  3. What are the Channels? According to the DFDs both files have channel ids of 0, 1, 2 and 15 (RGBA).
  1. Coordinates are in texel coordinates. They are not affected by KTXorientation, they are the "raw" coordinates.
  2. Packed is the array of individual packed data elements (each element has a size of typeSize)
  3. Channels is the array of channel values in integer (for INT and NORM formats) or float (for FLOAT formats) format, and they are output in the same order as the channels appear in the DFD, as discussed and agreed over email (i.e. not always in RGBA order) in order to work as expected for more exotic formats like YUYV which have multiple components of the same type
aqnuep commented 4 months ago

I've updated the man page to include information above about the output format.

MarkCallow commented 4 months ago

I've merged https://github.com/KhronosGroup/KTX-Software-CTS/pull/22 though I haven't yet deleted the branch. Please update the reference here, fix the conflicts and mark this ready so I can merge it today.

aqnuep commented 4 months ago

I've rebased the PR.

I also had to apply a fix in the dfd2vk code to handle the weird convention the KTX 2.0 spec defines for D24_UNORM_S8_UINT that uses OpenGL conventions.

aqnuep commented 4 months ago

Thank you, @MarkCallow for helping getting this through!