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.

Sign up for GitHub

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

Jump to bottom

Improve Alembic plugin to read assets with ArResolver to support other sources except local files #3302

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

Improve Alembic plugin to read assets with ArResolver to support other sources except local files #3302

wants to merge 5 commits into from

Conversation

roggiezhang-nv
Copy link
Contributor

@roggiezhang-nv roggiezhang-nv commented Sep 19, 2024 •
edited
Loading

Description of Change(s)

Alembic plugin only accepts assets that are from local paths. In order to fix this, we need to use ArResolver/ArAsset to read/write contents so customized resolver can read assets from other locations. However, interfaces of Alembic library have limitations that can only accept local urls or std::istream. This PR uses the following solution:

Open assets with ArResolver. Instead of reading them into the memory, we get the mapped file path with ArchGetFileName from ArAsset::GetFileUnsafe.

Fixes Issue(s)

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@roggiezhang-nv roggiezhang-nv changed the base branch from release to dev September 19, 2024 15:43
@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-10155

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@roggiezhang-nv roggiezhang-nv mentioned this pull request Oct 11, 2024
Merged
2 tasks
@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anwang2009
Copy link
Contributor

Hi @roggiezhang-nv, thanks for opening the discussion with an implementation! We took a step back and looked at the current design and came up with the following concerns:

  • _ReaderContext::_OpenAndGetMappedFilePath adds two file handle opens per path, even if the file paths are already supported in the original implementation. This adds bloat in a commonly used code path.

  • However, interfaces of Alembic library have limitations that only accept local paths or std::istreams ... Instead of reading them into the memory, we get the mapped file path with ArchGetFileName

    I want to clarify what is meant by "mapped file path". Could you give an example where having a mapped file path is necessary in linux, macos, and Windows? Is the mapping a separate process to the code path that has been changed here?

  • Related to the above points I'm wondering if there's an easy way at runtime to distinguish between paths that "already work" vs paths that need "special treatment"

We're considering two other designs as well:

  1. For your immediate needs, if there is a custom resolver you are using it's the least invasive to make the changes you need in your custom plugins. But if the desire is to solve this problem more generally:
  2. We can consider producing istreams in the filepath cases that aren't covered by the original implementation. I see that you looked into this in Improve ArchGetFileName for windows to return full path #3361, and I'm curious what specific istream flaws you ran into? We know that istream is less performant and requires deeper code changes, so we would hope to invoke the istream case only as necessary. The primary upside would be implementation clarity.

🤔 What do you think?

@roggiezhang-nv
Copy link
Contributor Author

roggiezhang-nv commented Nov 19, 2024 •
edited
Loading

Hi, An. Thanks for your reply. Here are my thoughts about your questions:

I want to clarify what is meant by "mapped file path". Could you give an example where having a mapped file path is necessary in linux, macos, and Windows? Is the mapping a separate process to the code path that has been changed here?

I may not describe it clearly. So Alembic library only accepts local URLs or override std::istream. In order to read assets that are not local, we'd have to do that in two ways:

  1. Cache remote assets to local drive, and get the local path (that's what we called "mapped file path", and that's what we have done in _OpenAndGetMappedFilePath). For default resolver in OpenUSD, it simply returns the original path since it's already local. We still need to mention that not all customized resolvers implement this kind of cache behavior. For those resolvers, we simply fail it as before in this PR. So the behavior in this PR can be summarized as:
    • For local files, it's the same behavior as before.
    • For customized resolver that caches file and ArAsset::GetFileUnsafe returns valid FILE descriptor, it can work now with this PR. (This is the main point of this PR)
    • For other resolvers, it simply fails as before.
  2. Override std::istream to read contents into a istream. However, this solution will break support for multi-layered ABC files. That's why we chose to use std::istream implementation in the beginning and gave up finally.

Related to the above points I'm wondering if there's an easy way at runtime to distinguish between paths that "already work" vs paths that need "special treatment"

We can simply check if the given urls exist so we can isolate the current work from the current PR. The only overhead for the current PR was in the _OpenAndGetMappedFilePath that opens the file and gets its mapped path, which still has no harm to performance. This step is not necessary for files that already exist in the local drive. This is why we thought it's the best solution currently.

@roggiezhang-nv roggiezhang-nv force-pushed the fix_abc_file_plugin branch 3 times, most recently from d18ce48 to 44c7afd Compare November 20, 2024 14:26
@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Pipelines were unable to run due to time out waiting for the pull request to finish merging.

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anwang2009
Copy link
Contributor

hi @roggiezhang-nv ! Are you planning any further changes before I start some performance testing and reviews?

@roggiezhang-nv
Copy link
Contributor Author

hi @roggiezhang-nv ! Are you planning any further changes before I start some performance testing and reviews?

@anwang2009 I added one strict checker to avoid open and release local asset twice so we treat local files the same as before without any side-effects. You can start evaluating the perf and review now.

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@roggiezhang-nv
Copy link
Contributor Author

@anwang2009 I added one more change per request to hold assets within the lifetime of _ReaderContext.

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

anwang2009
anwang2009 reviewed Jan 8, 2025
View reviewed changes
Copy link
Contributor

@anwang2009 anwang2009 left a comment

Choose a reason for hiding this comment

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

Thanks @roggiezhang-nv , lgtm besides some minor things!

pxr/usd/plugin/usdAbc/alembicReader.cpp Outdated
if (asset)
{
FILE* fileHandle = asset->GetFileUnsafe().first;
if (fileHandle)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also add the condition that the offset from GetFileUnsafe is not 0?

pxr/usd/plugin/usdAbc/alembicReader.cpp Outdated
_ReaderContext::_OpenAndGetMappedFilePath(const std::string& filePath)
{
// Return as it is if it's a local path and not a symlink.
if (!TfIsFile(filePath))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's resolve symlinks in this TfIsFile call

@roggiezhang-nv
Copy link
Contributor Author

@anwang2009 All comments addressed. Thanks.

anwang2009
anwang2009 reviewed Jan 8, 2025
View reviewed changes
pxr/usd/plugin/usdAbc/alembicReader.cpp Outdated
if (asset)
{
FILE* fileHandle = asset->GetFileUnsafe().first;
if (fileHandle && ftell(fileHandle) == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the offset that's returned by GetFileUnsafe().second instead of calling ftell?

It would be good to explain why the check against the file position is needed. I'm guessing it's to ensure that the .abc file we're looking at isn't embedded in a .usdz or other package, since the Alembic code itself won't know what to do with that.

  • sunya

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvmkuruc nvmkuruc nvmkuruc left review comments

@anwang2009 anwang2009 anwang2009 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@roggiezhang-nv @jesschimein @anwang2009 @nvmkuruc