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

Disable tests that may fail if the codec is not aom #1176

Merged
merged 4 commits into from
Oct 19, 2022

Conversation

y-guyon
Copy link
Collaborator

@y-guyon y-guyon commented Oct 18, 2022

For example, the SVT-AV1 encoder requires 4:2:0 images with even dimensions of at least 64x64 px.

The first commit is just for convenience. Hide it for better diffs.

Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

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

Yannis: Thank you very much for writing this pull request. There is a typo in tests/test_cmd_lossless.sh (marked with "IMPORTANT").

tests/test_cmd_metadata.sh Outdated Show resolved Hide resolved
tests/test_cmd_lossless.sh Outdated Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
tests/CMakeLists.txt Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
# These tests are supported with aom being the encoder and decoder. If aom is unavailable,
# these tests are disabled because other codecs may not implement all the necessary features.
# For example, SVT-AV1 requires 8-bit, 4:2:0 images with even dimensions of at least 64x64 px.
set_tests_properties(avifallocationtest avifgridapitest avifmetadatatest avifincrtest PROPERTIES DISABLED True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Just a comment, no change requested.] Disabling these tests when libaom is not available is a drastic measure. This is appropriate as a simple solution in the v0.11.1 patch release.

Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

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

LGTM with a change.

tests/test_cmd_lossless.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

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

Still LGTM.


# Lossless test. The decoded pixels should be the same as the original image.
echo "Testing basic lossless"
# TODO(yguyon): Make this test pass with INPUT_PNG instead of DECODED_FILE.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Just FYI. No change requested.] I debugged this. The reason this test fails with INPUT_PNG is that avifdec does not write the Exif and XMP metadata to the output PNG file, but the are_images_equal command also compares the metadata and ICC profiles.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I thought I remembered the PNG was read with an opaque alpha channel, the AVIF was decoded without an alpha channel, and are_images_equal does not consider them equal because of that.

# Cleanup
cleanup() {
pushd ${TMP_DIR}
rm -- "${ENCODED_FILE}" "${DECODED_FILE}" "${DECODED_FILE_LOSSLESS}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Just FYI. No change requested.] In these scripts, we should pass -f to the rm command, because some of these files may not exist when the test fails.

DECODED_FILE_LOSSLESS="avif_test_cmd_decoded_lossless.png"
ENCODED_FILE="avif_test_cmd_lossless_encoded.avif"
DECODED_FILE="avif_test_cmd_lossless_decoded.png"
DECODED_FILE_LOSSLESS="avif_test_cmd_lossless_decoded_lossless.png"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yannis: These tmp files are all created in ${TMP_DIR}, which is unique. So the tmp file names should not need to be unique, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, I confused the behavior with the .cc ones. I guess /tmp might still be given as an argument for ${TMP_DIR} on some build systems so it does not hurt.

@wantehchang wantehchang merged commit b9366f0 into AOMediaCodec:main Oct 19, 2022
@y-guyon y-guyon deleted the disable_tests_if_not_aom branch October 24, 2022 08:29
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.

2 participants