props: write out any properties set on the image

[bradh]

This complements the opaque property reading done under #2420

There are a couple of issues I'd like guidance on:

• the AV2 CI test failure
• whether it is OK to carry over unsupported properties when writing out a new file

An example of the unsupported property might be a udes that applied to the original image, but might not be appropriate in a modified version of that image. I'm unsure of the best way to handle this.

[y-guyon]

Thank you for the pull request and the tests.

the AV2 CI test failure

This is expected. AV2 is disabled by default and must be explicitly chosen as a codec through both the encoding and decoding API, even if enabled at libavif compile time. When only AV2 is available as a codec, encoding fails by default.
It can be solved by skipping the whole test file, example:

libavif/tests/CMakeLists.txt

Lines 369 to 371 in 03738d5

	avifrangetest
	avify4mtest
	PROPERTIES DISABLED True

or just the test unit, example:

libavif/tests/gtest/avifavmminitest.cc

Lines 89 to 91 in 03738d5

	if (!testutil::Av1EncoderAvailable() || !testutil::Av1DecoderAvailable()) {
	GTEST_SKIP() << "AV1 codec unavailable, skip test.";
	}

whether it is OK to carry over unsupported properties when writing out a new file

From a libavif API point of view, the only way to have custom props in an avifImage is to manually add them or to decode an AVIF file (for now). On top of that, I guess it is quite rare for a user of libavif to encode to AVIF an avifImage that came from decoding an AVIF file, so carrying over custom props should not really happen in practice.

[y-guyon]

Sorry for the churn. All these open questions are part of why I did not write the encoder part at the same time as the decoder part.

[wantehchang]

Brad: Please consider my review as supplemental. Yannis is the primary reviewer of this pull request.

[wantehchang]

Optional: The integer literal 4 is already used in several places (e.g., line 9 and line 34) in the pull request, so it doesn't seem necessary to define this constant.

[bradh]

I thought it made the code a little easier to read.

The other usages were in array bounds, which is only supported by GCC as an extension.

I can change it to be a #define if that would be preferred.

[bradh]

I believe I have addressed all the comments.

I'd particularly like review on the fuzzer and the gain map related code.

I'd like feedback on the ordering issue, see #2510 (comment)

[y-guyon]

Thank you for adding the gainmap test and fuzz test! The PR looks good, just a few nits.

[bradh]

@y-guyon Thanks for all the help on this one - much appreciated.

[y-guyon]

@y-guyon Thanks for all the help on this one - much appreciated.

At least as many thanks to you then!