usdValidation: usdUtils: CompressionValidator #3449

beersandrew · 2024-12-03T03:12:51Z

Description of Change(s)

Link to proposal (if applicable)

Fixes Issue(s)

https://github.com/orgs/PixarAnimationStudios/projects/3/views/1?pane=issue&itemId=65425103

Note: This PR is a replacement for #3400

Checklist

[X] I have created this PR based on the dev branch

[X] I have followed the coding conventions

[X] I have added unit tests that exercise this functionality (Reference:
testing guidelines)

[X] I have verified that all unit tests pass with the proposed changes

[X] I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)

beersandrew · 2024-12-03T03:14:40Z

pxr/usdValidation/usdUtilsValidators/validators.cpp

@@ -97,12 +98,55 @@ _PackageEncapsulationValidator(const UsdStagePtr &usdStage)
    return errors;
 }

+static UsdValidationErrorVector
+_CompressionValidator(const UsdStagePtr& usdStage) {


@tallytalwar I've come across something in this validator that's not clear to me.

My expectation from reading the description from the existing CompressionChecker is that this test should make sure that files within the USDZ file itself were not compressed. So I've created a sample file like this (in this repo as compressedUsdaInside.usdz). However this file fails here, before it can get to the usdchecker code.

If I try a different approach of compressing the usdz itself, it also fails early here.

Do you know how to reproduce the nicer errors from usdchecker itself? (Similar situation with ByteAlignmentChecker as well)

Why its failing on parsing the "compressed" "usda" file is because I think usda extension makes it load the usda file plugin, which when parsing tries to see if the file header has the usda magic cookie and if not it fails. If you stick with this approach only maybe use a different extension for this file.

usdz is an archival format, so the code checks if it has any compression on it or not.

Maybe add a compressed asset to usdz file like a compressed jpeg/png?

I've tried compressing a jpg and adding it to the usdz, and I've tried including a zip file as an asset and then including it in the usdz as well. I'm still unable to trigger the CompressionChecker in the existing complianceChecker.py usdchecker version

jesschimein · 2024-12-03T17:45:57Z

Filed as internal issue #USD-10492

jesschimein · 2024-12-03T17:45:59Z

/AzurePipelines run

azure-pipelines · 2024-12-03T17:46:10Z

Azure Pipelines successfully started running 1 pipeline(s).

tallytalwar

Logic is fine, but I was thinking we can split this up into a UsdzPackageValidator and a UsdzRootPackageValidator with appropriate keywords.

Both validators are stage level validators:

RootPackageValidator: will do compression error logic, byle alignment logic and extension validation logic just for the rootLayer, if the rootLayer is a package.
UsdzPackageValidator: will do all the same validation checks for all packages found in the stage.

A lot of the actual validation code itself can be shared between the 2 validator methods.

This way we can do this: https://github.com/PixarAnimationStudios/OpenUSD/blob/dev/pxr/usdValidation/bin/usdchecker/usdchecker.cpp#L572-L575

Let me know what you think about this and feel free to ping on aswf slack if you want to discuss this more?

Thanks

tallytalwar · 2024-12-10T00:23:50Z

pxr/usdValidation/usdUtilsValidators/validators.cpp

+    if (!zipFile)
+    {
+        return {};
+    }


You can move this check above doing the ArSplitPackageRelativePathOuter

tallytalwar · 2024-12-10T00:33:40Z

pxr/usdValidation/usdUtilsValidators/validators.cpp

+                            rootLayer, SdfPath(fileName))
+                },
+                TfStringPrintf(
+                ("File '%s' in package '%s' has "


Extra "()" here? TfStringPrintf(("File .... instead of TfStringPrintf("File..

- removed unnecessary parenthesis on error messages - move early exit condition earlier

- update the validation logic to always check for both compression & byteAlignment, either only at the root level or at all levels

- added a test to test a referenced usdz file that has internal byte alignment and compression errors - added example test usd files

beersandrew · 2024-12-16T01:35:46Z

pxr/usdValidation/usdUtilsValidators/validators.cpp

+_GetUsdzPackageErrors(const SdfLayerHandle &rootLayer)
+{
+    const UsdZipFile zipFile = UsdZipFile::Open(rootLayer->GetRealPath().c_str());
+    if (!zipFile)


@tallytalwar I noticed testing locally that when passing a .usda file in here, this if statement does not filter out the .usda layer, which I anticipated that it would. All of the errors are still coming back correctly but I wanted to ask if there was some other condition to put here to exit out early as I couldn't find something on the UsdZipFile class that was applicable

Yeah looks like from the code it works even if the layer is a usda layer, as it goes through ArAsset. I will confirm internally if thats anticipated.

beersandrew · 2024-12-16T01:42:58Z

pxr/usdValidation/usdUtilsValidators/testenv/testUsdUtilsValidators.cpp

+        // Verify internal compression and byte misalignment errors occur
+        TF_AXIOM(errors.size() == 3);
+
+        const std::string fullErrorPath = ArchGetCwd() +


@tallytalwar Two things about this line.

It seems in the validator itself, if I pass in a root level myFile.usdz, the ArSplitPackageRelativePathOuter will get the package path that i want, as in, only myFile.usdz however passing in parentFile.usda that references myFile.usdz the error message has the full real path in the error message

If the above is okay as is, I wanted to join these paths here in a platform agnostic way and I thought there was some library function to join these paths so it would work everywhere as this as written seems it's the wrong direction for Windows, and it seemed wrong to write some helper function here to deal with the correct slashes. I did find other tests that used only this slash, but let me know if this should be changed.

I am not sure I fully understand, 1.

But for 2, there is: https://github.com/PixarAnimationStudios/OpenUSD/blob/dev/pxr/base/tf/stringUtils.h#L677-L689 which you can use to do a platform agnostic way of concatenating paths, etc.

To rephrase 1, this line: https://github.com/PixarAnimationStudios/OpenUSD/pull/3449/files#diff-d9adee7401f280af5ec4cc8aa666b4fb0d767ea269bedc2750dcc333503af6aaR156 behaves differently whether it's the root package or an internal package.

When it's a root package the value will be : "rootPackage.usdz" (or whatever the name of the file is)

When it's an internal package the value will be : "/fully/qualified/path/of/package/internalPackage.usdz" (where this is the path on disk or would be the path on disk of the internal package.

I think this is less of an issue now that you've reminded me of 2. (Thanks!) but I wanted to point it out as the error messages are different in these cases where sometimes the fully qualified path is there and other times it's not

I see, yeah I think I can reason about that (will also confirm internally), but the reference path to internal package gets fully resolved relative to its referenced location, in order to locate the asset, and that causes the paths to be absolute.

jesschimein · 2024-12-17T22:22:37Z

/AzurePipelines run

azure-pipelines · 2024-12-17T22:22:46Z

Azure Pipelines successfully started running 1 pipeline(s).

pxr/usdValidation/usdUtilsValidators/validators.cpp

jesschimein · 2024-12-18T18:14:26Z

/AzurePipelines run

azure-pipelines · 2024-12-18T18:14:36Z

Azure Pipelines successfully started running 1 pipeline(s).

tallytalwar

Just couple of more small notes to be addressed; will sync in when done. Thanks Andy

- don't unnecessarily capture assets & unresolvedPaths for this validator - fixed a missing const

beersandrew commented Dec 3, 2024

View reviewed changes

beersandrew mentioned this pull request Dec 3, 2024

usdUtils: CompressionValidator #3400

Closed

tallytalwar reviewed Dec 10, 2024

View reviewed changes

beersandrew force-pushed the validation-usdUtils-CompressionValidator branch from b325222 to 14c4f8f Compare December 12, 2024 19:20

beersandrew added 6 commits December 15, 2024 20:10

usdValidation: usdUtils: CompressionValidator

c56ef13

fix: addressing PR comments

5778b58

- removed unnecessary parenthesis on error messages - move early exit condition earlier

fix: change validator from compression to Root & UsdzPackageValidators

ad33967

- update the validation logic to always check for both compression & byteAlignment, either only at the root level or at all levels

test: add tests for rootPackageValidator

0023923

test: adding a test for UsdzPackageValidator

71aafa0

- added a test to test a referenced usdz file that has internal byte alignment and compression errors - added example test usd files

fix: fix rebase errors

8229bd9

beersandrew force-pushed the validation-usdUtils-CompressionValidator branch from 55acc80 to 8229bd9 Compare December 16, 2024 01:32

beersandrew marked this pull request as ready for review December 16, 2024 01:32

beersandrew commented Dec 16, 2024

View reviewed changes

beersandrew requested a review from tallytalwar December 16, 2024 01:43

fix: use proper path concat function

d73a3f8

tallytalwar reviewed Dec 18, 2024

View reviewed changes

pxr/usdValidation/usdUtilsValidators/validators.cpp Outdated Show resolved Hide resolved

tallytalwar reviewed Dec 18, 2024

View reviewed changes

pxr/usdValidation/usdUtilsValidators/validators.cpp Outdated Show resolved Hide resolved

tallytalwar reviewed Dec 18, 2024

View reviewed changes

beersandrew added 2 commits December 18, 2024 13:37

fix: removed unnecessary include

322e04b

fix: clean up

ea205ee

- don't unnecessarily capture assets & unresolvedPaths for this validator - fixed a missing const

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

usdValidation: usdUtils: CompressionValidator #3449

usdValidation: usdUtils: CompressionValidator #3449

beersandrew commented Dec 3, 2024 •

edited

Loading

beersandrew Dec 3, 2024

tallytalwar Dec 7, 2024

beersandrew Dec 12, 2024

jesschimein commented Dec 3, 2024

jesschimein commented Dec 3, 2024

azure-pipelines bot commented Dec 3, 2024

tallytalwar left a comment •

edited

Loading

tallytalwar Dec 10, 2024

tallytalwar Dec 10, 2024

beersandrew Dec 16, 2024

tallytalwar Dec 18, 2024

beersandrew Dec 16, 2024

tallytalwar Dec 18, 2024

beersandrew Dec 18, 2024

tallytalwar Dec 18, 2024

jesschimein commented Dec 17, 2024

azure-pipelines bot commented Dec 17, 2024

jesschimein commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

tallytalwar left a comment

usdValidation: usdUtils: CompressionValidator #3449

Are you sure you want to change the base?

usdValidation: usdUtils: CompressionValidator #3449

Conversation

beersandrew commented Dec 3, 2024 • edited Loading

Description of Change(s)

Link to proposal (if applicable)

Fixes Issue(s)

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesschimein commented Dec 3, 2024

jesschimein commented Dec 3, 2024

azure-pipelines bot commented Dec 3, 2024

tallytalwar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesschimein commented Dec 17, 2024

azure-pipelines bot commented Dec 17, 2024

jesschimein commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

tallytalwar left a comment

Choose a reason for hiding this comment

beersandrew commented Dec 3, 2024 •

edited

Loading

tallytalwar left a comment •

edited

Loading