Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ags to convert tool #1211

Closed

Conversation

feimao-gfxr
Copy link
Contributor

Problem

  1. AGS support is already in GFXR. The convert output file should also include the AGS calls.
  2. The index values in the convert output file doesn't match anything in the replay consumer. It makes it very hard to find the matching call in the convert file when debugging an issue in the replayer.

Solution

  1. Add AGS ascii consumer to support AGS in the convert tool.
  2. Remove api_call_count_ from the convert consumer and use block index for index values in the convert output file. Block index values provides consistent matches between different decode consumers.

Result
AGS calls are included in the convert output file. Indices are block index values now.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 10482.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3015 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3015 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 10561.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3017 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3017 passed.

@andrew-lunarg
Copy link
Contributor

It looks like the PR needs a simple clang-format.exe -i tools/convert/main.cpp to get through our style check.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 13613.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3027 running.

@feimao-gfxr
Copy link
Contributor Author

It looks like the PR needs a simple clang-format.exe -i tools/convert/main.cpp to get through our style check.

Updated.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3027 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 13964.

@lunarpapillo
Copy link
Contributor

CI build #3027 failed because the Pixel 3 needs a factory reset. I've restarted your run.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3031 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3031 passed.

Copy link
Contributor

@andrew-lunarg andrew-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me with a few tweaks noted inline as separate comments.

I think ideally we'd prefer:

  • JSON output to use the nlohmann library in external\nlohmann\include\nlohmann\json.hpp,
  • to use Json and json in names of types and files instead of Ascii and ascii,
  • and for the JSON output to be structured more like the Vulkan schema.

That ideal outcome would give us simpler code and consistency in output,
but this should still slot in next to the other JSON Consumers and could still be useful.

I agree about the change to block indexes, and good catch deleting the dead code counting calls but never using the count.

framework/decode/dx12_consumer_base.h Outdated Show resolved Hide resolved
framework/decode/dx12_consumer_base.h Outdated Show resolved Hide resolved
tools/convert/main.cpp Outdated Show resolved Hide resolved
@feimao-gfxr
Copy link
Contributor Author

This seems fine to me with a few tweaks noted inline as separate comments.

I think ideally we'd prefer:

  • JSON output to use the nlohmann library in external\nlohmann\include\nlohmann\json.hpp,
  • to use Json and json in names of types and files instead of Ascii and ascii,
  • and for the JSON output to be structured more like the Vulkan schema.
  • JSON output to use the nlohmann library in external\nlohmann\include\nlohmann\json.hpp,
  • to use Json and json in names of types and files instead of Ascii and ascii,
  • and for the JSON output to be structured more like the Vulkan schema.

These are general efforts. Is there any work going on in parallel for the gfxrecon-convert project at LunarG? Will the future work on gfxrecon-convert be based on this PR? We don't want duplicated efforts.

@andrew-lunarg
Copy link
Contributor

There are ongoing efforts to work on Convert. The Vulkan support in Convert all uses the nlohmann library. The DX12 support is being moved to it. There is no internal plan to move AGS to it.

For the purpose of this PR, I think the important part of my previous review comment was:

This seems fine to me with a few tweaks noted inline as separate comments.

I.e. we can get this in to dev now with a couple of tweaks and worry about bringing it more in-line with the JSON output for the other APIs later if there is desire to do so.

If you would rather wait and see what the changes to DX12 look like before merging this, that is also an option.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 50726.

@feimao-gfxr
Copy link
Contributor Author

There are ongoing efforts to work on Convert. The Vulkan support in Convert all uses the nlohmann library. The DX12 support is being moved to it. There is no internal plan to move AGS to it.

For the purpose of this PR, I think the important part of my previous review comment was:

This seems fine to me with a few tweaks noted inline as separate comments.

I.e. we can get this in to dev now with a couple of tweaks and worry about bringing it more in-line with the JSON output for the other APIs later if there is desire to do so.

If you would rather wait and see what the changes to DX12 look like before merging this, that is also an option.

For moving to using nlohmann library and related file renaming, I think it would be better to do it together with (or after) the changes to DX12.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3277 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3277 failed.

Copy link
Contributor

@andrew-lunarg andrew-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I think the ToString functions in util mentioned by David could be used though.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 64155.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 64158.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3391 failed.

Copy link
Contributor

@andrew-lunarg andrew-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking my approval off the PR and requesting the duplicate DX12 enum to string functions are removed in favour of our existing generated_dx12_enum_to_string.h, unless there is something missing from them that these ones in the PR provide. In particular the changes requested by David in comments.

@bradgrantham-lunarg bradgrantham-lunarg added the P2 A high-priority code maintenance issue or a functional problem that is recoverable or not a crash. label Oct 30, 2023
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 84181.

Copy link
Contributor

@davidd-lunarg davidd-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM although this PR branch is more than 100 commits behind dev so it might be a good idea to rebase manually before merging.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3504 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3504 failed.

@feimao-gfxr
Copy link
Contributor Author

Closed, because it is replaced by PR #1360.

@feimao-gfxr feimao-gfxr deleted the dev-d3d12-add-ags-to-convert branch November 17, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 A high-priority code maintenance issue or a functional problem that is recoverable or not a crash.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants