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 AVIF_ENABLE_COMPLIANCE_WARDEN #1611

Merged
merged 5 commits into from
Sep 21, 2023
Merged

Conversation

y-guyon
Copy link
Collaborator

@y-guyon y-guyon commented Sep 21, 2023

Use github.com/gpac/ComplianceWarden as a dependency to check all AVIF bitstreams output by avifEncoderFinish().

Introduce AVIF_USE_CXX in CMakeLists.txt to regroup the use cases requiring C++.

AVIF_ENABLE_COMPLIANCE_WARDEN is OFF in GitHub CI because many tests fail.
For example, the following build instructions

cmake .. -DAVIF_BUILD_APPS=ON  -DAVIF_BUILD_EXAMPLES=ON  -DAVIF_BUILD_TESTS=ON  -DAVIF_ENABLE_GTEST=ON  -DAVIF_ENABLE_COMPLIANCE_WARDEN=ON  -DAVIF_CODEC_AOM=ON  -DAVIF_LOCAL_AOM=ON  -DAVIF_LOCAL_LIBYUV=ON  -DAVIF_LOCAL_LIBSHARPYUV=ON  -DAVIF_LOCAL_GTEST=ON  -DAVIF_ENABLE_WERROR=OFF -DCMAKE_BUILD_TYPE=Release  -DBUILD_SHARED_LIBS=OFF  -DCMAKE_C_COMPILER=clang  -DCMAKE_CXX_COMPILER=clang++ && make -j 7 && ctest . -j 7

gives

The following tests FAILED:
avifchangesettingtest, avifclaptest, avifgridapitest, avifincrtest, avifiostatstest, aviflosslesstest, avifmetadatatest, avifprogressivetest, avifstreamtest, aviftilingtest, avifutilstest, test_cmd_animation, test_cmd_grid, test_cmd_lossless, test_cmd_metadata, test_cmd_progressive, test_cmd_targetsize

Some are due to warnings but there are also errors, such as for avifmetadatatest.cc > ExifButDefaultIrotImir:

[miaf][Rule #26] Error: Found 1 (ItemIds={1}) 'pixi' associated for 3 displayable (not hidden) images (ItemIds={1,2,3})

These should be investigated and ComplianceWarden or libavif should be fixed when relevant.

The LICENSE is fine because there is no "Redistributions of source code", "Redistributions in binary form" and "Neither the name of the copyright holder nor the names of its contributors" are used.

compliance_warden.sh is a bit hacky but it can be improved later on.

Use github.com/gpac/ComplianceWarden as a dependency to check all
AVIF bitstreams output by avifEncoderFinish().

Introduce AVIF_USE_CXX to regroup use cases requiring C++.
src/compliance.cc is only compiled if it is defined.
echo prints literal "\\n", printf appends '\n' correctly.
Copy link
Collaborator

@maryla-uc maryla-uc left a comment

Choose a reason for hiding this comment

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

Pretty cool!!

src/compliance.cc Show resolved Hide resolved
src/compliance.cc Show resolved Hide resolved
src/compliance.cc Show resolved Hide resolved
@y-guyon y-guyon merged commit ef6da2a into AOMediaCodec:main Sep 21, 2023
15 of 16 checks passed
@y-guyon y-guyon deleted the compliancewarden branch September 21, 2023 14:10
src/compliance.cc Show resolved Hide resolved
extern const SpecDesc * const globalSpecIsobmff;
extern const SpecDesc * const globalSpecMiaf;

avifResult avifIsCompliant(uint8_t * data, size_t size)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function should be marked as extern "C". We are currently relying on the extern "C" of this function's declaration in avif/internal.h, but it would be good to explicitly mark this function's definition as extern "C".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in #1616.

@@ -471,6 +471,10 @@ __attribute__((__format__(__printf__, 2, 3)))
#endif
void avifDiagnosticsPrintf(avifDiagnostics * diag, const char * format, ...);

#if defined(AVIF_ENABLE_COMPLIANCE_WARDEN)
avifResult avifIsCompliant(uint8_t * data, size_t size);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Can data be declared as const uint8_t *?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in #1616.

@@ -642,6 +692,11 @@ if(NOT SKIP_INSTALL_ALL)
endif()

if(AVIF_CODEC_LIBRARIES MATCHES vmaf)
# vmaf only impacts the avifenc and avifdec targets below, not the library avif target above.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Super nit: impacts => affects

library avif => avif library

This comment should also mention the examples, right? I.e., the avif library and examples targets.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in #1616.

parseFunc(&topReader);
// gpac/ComplianceWarden will print the formatted result page to stdout, warnings and errors inclusive.
AVIF_CHECKERR(!checkComplianceStd(topReader.myBox, topReader.specs[0]), AVIF_RESULT_BMFF_PARSE_FAILED);
return AVIF_RESULT_OK;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of reaching into the inside of ComplianceWarden, it seems better to invoke the cw program and parse its output or check its exit status. I am not up to date on the latest functions for doing this, but we can start with popen():
https://pubs.opengroup.org/onlinepubs/9699919799/functions/popen.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer the current approach but feel free to send a pull request.

@y-guyon y-guyon mentioned this pull request Dec 1, 2023
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.

3 participants