props: write out any properties set on the image #2510

bradh · 2024-11-27T10:55:10Z

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 · 2024-11-27T13:00:30Z

src/write.c

+            avifItemPropertyDedupStart(dedup);
+            avifImageItemProperty * prop = &(itemMetadata->properties[i]);
+            avifBoxMarker propMarker;
+            AVIF_CHECKRES(avifRWStreamWriteBox(&dedup->s, (const char *)prop->boxtype, AVIF_BOX_SIZE_TBD, &propMarker));


I wonder if we should check for custom box types (for example forbid meta). Otherwise it would be possible to generate invalid files with the libavif API.

With or without this check, we should fuzz this new API. I think a new dedicated fuzz target would be easy. It needs to check that encoding any API-compliant custom prop garbage is a success, and that decoding the encoded output is a success when expected (that is the hard part to determine) or a graceful error.
Maybe an approach similar to ArbitraryAvifImageWithGainMap() would be a good fit.

I added a note to the API documentation about not adding known properties.

The other parts are still to be done.

I added a note to the API documentation about not adding known properties.

"Known properties" is likely too vague as a criterion.

I would still prefer the encoder to check for validity. I can think of ways:

Check that there is no forbidden box type (an approximate list should be rather quick to gather, for example here up to level 2)

Check that the box type makes sense ([a-zA-Z0-9! ]{4} or something)

Check that the uuid makes sense (is that possible?)

Check that there is no property with the same box type assigned twice to the same item (assuming nobody needs that besides colr which is set by libavif)

Call avifParse() within avifEncode() when custom properties are set (a bit too much of a change maybe)

@wantehchang What do you think?

The other parts are still to be done.

You meant in this PR or in another one?

The other parts are still to be done.

You meant in this PR or in another one?

I mean the other parts of this comment.

I'm doubtful about the value of checking for known boxes on encode, but will try it. My concern is that we're trying to protect programmers from themselves. They can always make junk files (e.g. by changing the results of the encode before writing it out) and we're potentially making the encode slower for limited protection.

I would like clarification on why you only wanted up to level 2 though - if we're going to check properties, why not check for adding known properties? Possibly that would be caught by the fourth point (no duplicates), but I'm -0 on that, because there are likely uses for that (e.g. udes with different languages is supported in 23008-12, and maybe similar use of that pattern in other documents).

There are pretty limited options for checking a UUID, but I could see how much hassle that is.

Instead of requiring avifParse(), we could suggest that as an option for users. Again, trying to limit the impact on encode performance.

I've added argument checks and associated test code. The checks are at set time, so the failure result is closer to the problem.

I still need to add documentation about using avifParse.

include/avif/avif.h

tests/gtest/avifpropertytest.cc

y-guyon · 2024-11-27T13:41:32Z

tests/gtest/avifpropertytest.cc

+      std::vector<uint8_t>(uuidProp.boxPayload.data,
+                           uuidProp.boxPayload.data + uuidProp.boxPayload.size),
+      uuid_data);
+}


Missing blank line

It would be good to test custom props on gainmaps too.

I added the blank line.

I don't have enough spec information or understanding to do the gainmap work without more guidance. Should it work just the same? Just set it on altImageMetadata ?

avifImageAddOpaqueProperty(image->gainMap->image) should work for encoding.
image->gainMap can be created this way:

libavif/tests/gtest/avifgainmaptest.cc

Line 113 in 378c646

ImagePtr image = CreateTestImageWithGainMap(/*base_rendition_is_hdr=*/false);

At decoding decoder->image->gainMap->image should be tested too.

y-guyon · 2024-11-29T10:01:25Z

src/write.c

+            avifItemPropertyDedupStart(dedup);
+            avifImageItemProperty * prop = &(itemMetadata->properties[i]);
+            avifBoxMarker propMarker;
+            AVIF_CHECKRES(avifRWStreamWriteBox(&dedup->s, (const char *)prop->boxtype, AVIF_BOX_SIZE_TBD, &propMarker));


I added a note to the API documentation about not adding known properties.

"Known properties" is likely too vague as a criterion.

I would still prefer the encoder to check for validity. I can think of ways:

Check that there is no forbidden box type (an approximate list should be rather quick to gather, for example here up to level 2)

Check that the box type makes sense ([a-zA-Z0-9! ]{4} or something)

Check that the uuid makes sense (is that possible?)

Check that there is no property with the same box type assigned twice to the same item (assuming nobody needs that besides colr which is set by libavif)

Call avifParse() within avifEncode() when custom properties are set (a bit too much of a change maybe)

@wantehchang What do you think?

The other parts are still to be done.

You meant in this PR or in another one?

y-guyon · 2024-11-29T10:03:46Z

include/avif/avif.h

+// the user extension (uuid) mechanism, see ISO/IEC 14496-12:2022 Section 4.2. The box type is set to 'uuid' and
+// does not need to be specified.


// The box type is set to 'uuid' ~~and does not need to be specified~~

y-guyon · 2024-11-29T10:09:23Z

tests/gtest/avifpropertytest.cc

+  result = avifDecoderReadMemory(decoder.get(), decoded.get(), encoded.data,
+                                 encoded.size);


avifDecoderParse() would be enough here.

If avifDecoderReadMemory() is kept, it would be cleaner to check decoded than decoder->image.

y-guyon · 2024-11-29T10:11:03Z

tests/gtest/avifpropertytest.cc

+      std::vector<uint8_t>(uuidProp.boxPayload.data,
+                           uuidProp.boxPayload.data + uuidProp.boxPayload.size),
+      uuid_data);
+}


avifImageAddOpaqueProperty(image->gainMap->image) should work for encoding.
image->gainMap can be created this way:

libavif/tests/gtest/avifgainmaptest.cc

Line 113 in 378c646

ImagePtr image = CreateTestImageWithGainMap(/*base_rendition_is_hdr=*/false);

At decoding decoder->image->gainMap->image should be tested too.

props: write out any properties set on the image

d0492d2

y-guyon requested changes Nov 27, 2024

View reviewed changes

properties: clean up test suite for review comments

6641371

bradh force-pushed the prop_write_2024-11-27 branch from 510480a to 6641371 Compare November 27, 2024 21:15

bradh added 2 commits November 28, 2024 08:25

properties: do not test property encoding on AVM

1d193cf

properties: add extra usage documentation

2f4fe06

bradh force-pushed the prop_write_2024-11-27 branch from 07614a3 to 2f4fe06 Compare November 27, 2024 22:58

y-guyon requested changes Nov 29, 2024

View reviewed changes

properties: sanity check boxtype and UUID values

f2cc919

bradh force-pushed the prop_write_2024-11-27 branch from 7d366cf to f2cc919 Compare November 30, 2024 09:00

properties: update documentation for review comment

4deb7ac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

props: write out any properties set on the image #2510

props: write out any properties set on the image #2510

bradh commented Nov 27, 2024

y-guyon left a comment

y-guyon Nov 27, 2024

bradh Nov 27, 2024

y-guyon Nov 29, 2024

bradh Nov 30, 2024

bradh Nov 30, 2024

y-guyon Nov 27, 2024

bradh Nov 27, 2024

y-guyon Nov 29, 2024

y-guyon Nov 29, 2024

y-guyon Nov 29, 2024

bradh Nov 30, 2024

y-guyon Nov 29, 2024

y-guyon Nov 29, 2024

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

		// the user extension (uuid) mechanism, see ISO/IEC 14496-12:2022 Section 4.2. The box type is set to 'uuid' and
		// does not need to be specified.

		result = avifDecoderReadMemory(decoder.get(), decoded.get(), encoded.data,
		encoded.size);

props: write out any properties set on the image #2510

Are you sure you want to change the base?

props: write out any properties set on the image #2510

Conversation

bradh commented Nov 27, 2024

y-guyon left a 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

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