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

Asset file for trimmed captures #1644

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

panos-lunarg
Copy link
Contributor

No description provided.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 226269.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 226350.

@panos-lunarg panos-lunarg force-pushed the asset_file_for_trimmed_captures branch from f5cd9a6 to 79ee258 Compare July 30, 2024 14:27
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 226412.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4484 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4484 failed.

@panos-lunarg panos-lunarg force-pushed the asset_file_for_trimmed_captures branch from 79ee258 to 60bc814 Compare August 1, 2024 08:36
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 227571.

@panos-lunarg panos-lunarg force-pushed the asset_file_for_trimmed_captures branch from 60bc814 to 54585d4 Compare August 1, 2024 09:19
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 227583.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4500 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4500 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 228412.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4514 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4514 failed.

@panos-lunarg panos-lunarg force-pushed the asset_file_for_trimmed_captures branch from e21b66f to 1cc22f0 Compare August 3, 2024 04:29
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 228788.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4516 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4516 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 228847.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4518 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4518 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 229187.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4522 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4522 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 229275.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4524 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4524 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 229670.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4527 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5113 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5113 failed.

{
success = ProcessFileHeader();
size_t last_slash_pos = filename.find_last_of("\\/");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use gfxrecon::util::filepath::GetBasedir for this?


std::string FileProcessor::ApplyAbsolutePath(const std::string& file)
{
if (absolute_path_.empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use gfxrecon::util::filepath::Join for this? I don't think you'd even need to check absoluate_path_.empty().

@@ -127,8 +131,6 @@ class ApiCaptureManager

bool IsTrimHotkeyPressed() { return common_manager_->IsTrimHotkeyPressed(); }

CaptureSettings::RuntimeTriggerState GetRuntimeTriggerState() { return common_manager_->GetRuntimeTriggerState(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this deleted in this PR just for cleanup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More or less yes. Since now there are 2 runtime android options GetRuntimeTriggerState didn't serve any great purpose returning just the trigger state. Now GetRuntimeTriggerEnabled, GetRuntimeTriggerDisabled and the new RuntimeWriteAssetsEnabled and RuntimeWriteAssetsDisabled fetch the runtime values themselves

CaptureSettings::LoadRunTimeEnvVarSettings(&settings);
bool write_assets = settings.GetTraceSettings().runtime_write_assets;

if (previous_write_assets_ != write_assets)
Copy link
Contributor

@bradgrantham-lunarg bradgrantham-lunarg Oct 21, 2024

Choose a reason for hiding this comment

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

I expected write_assets_ to be set from write_assets and for previous_write_assets_ to be the previous value of write_assets_. I don't have a good suggestion except it looks like write_assets_ is a one-shot, so could write_assets be write_assets_requested and previous_write_assets_ be write_assets_already_requested_? (Or some such)

Copy link
Contributor Author

@panos-lunarg panos-lunarg Oct 21, 2024

Choose a reason for hiding this comment

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

The initial idea with this was that an asset dump could be triggered by setting an Android property to true.
Then I realized that this would require that once the dump is finished the property is turned back to false so that the next request can be tracked again. Since the user can not be in position to know when the dump is finished, the resetting of the property must be done by the layer, but this requires elevated permissions.
So instead of that the dump is triggered when the property changes value. So:

  • write_assets is the current android property value
  • previous_write_assets_ is the value of the android property the last time a dump was triggered.
    Comparing those two we can catch changes.
  • write_assets_ is the internal boolean that triggers a dump. Apart from the android property this can also be set to true also by calling the exposed callback function

{
std::string asset_filename = base_filename;

size_t dot_pos = base_filename.rfind('.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see if the functions GetFilenameStem and GetFilenameExtension in gfxrecon::util::filepath can be used for parsing out the extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't aware of that utility. I will try to use it wherever seems fit.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 283131.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5125 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5125 passed.

@panos-lunarg panos-lunarg force-pushed the asset_file_for_trimmed_captures branch from 890f135 to 6777d64 Compare October 22, 2024 05:24
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 283925.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5140 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5140 passed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 284021.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5141 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5141 passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants