Disable Sample Transform decoding by default

[y-guyon]

Add AVIF_IMAGE_CONTENT_SAMPLE_TRANSFORMS.
16-bit avifImage may not be gracefully handled by all current avifDecoder users, so it is safer to disable the feature by default.

[maryla-uc]

I think that the addition of AVIF_IMAGE_CONTENT_SAMPLE_TRANSFORMS is a great change, but I'm not convinced about the avifdec flag.
Although libavif only supports "recipes" for using sato for bit depth extension, I think that the decoding code supports other use cases. Tying sample transforms decoding to bit depth seems not very future proof. Just add a separate flag?

[wantehchang]

I agree that returning a 16-bit output image is an incompatible change, so users need to opt in to this new feature. Thank you for thinking of this issue!

[wantehchang]

I have two comments on this.

1. It may be better to control this with a new boolean field in the avifDecoder struct. Since sample transforms also produce color and alpha, sample transforms are not a new content type.
2. It may be better to restrict this control to bit depth extensions only rather than all sample transforms, if we can determine the output bit depth without applying the sample transforms. (Maryla also said this in her review comment.) I think we can get the bit depth from the 'pixi' box of the 'sato' derived image item, right?

In summary, please consider exposing this control as a new avifBool enableExtendedBitDepths field in the avifDecoder struct

[y-guyon]

Thank you for the review.

1. I disagree. Gain maps complement the primary color image but adding color information. Bit depth extensions, and thus Sample Transforms, also extend the color information in a similar way. The dimension is just bit depth instead of brightness, if I may say so.
   Also avifImageContentTypeFlags is a bit mask rather than an enum, meaning the different avifImageContentTypeFlag values are more orthogonal concepts that can each be toggled on or off than distinct types.
   Each avifImageContentTypeFlag maps to a separate image item in AVIFv1 (primary color item, aux alpha, derived gain map, derived sample transform).
2. As of today, libavif applies any Sample Transform formula with at most 2 input items. Also, there could be plenty of different formulas for extending the bit depth, not only the few that are available at encoding right now. I do not think it is easy to precisely detect whether the decoded formula is a bit depth extension, and only that.
   Maryla's comment "Tying sample transforms decoding to bit depth" was specifically about avifdec's --depth flag. I am not sure this is what you meant.

[wantehchang]

I am commenting on the change to the public header include/avif/avif.h.

If a 'sato' derived image item has one 8-bit AV1 image item and adds the constant 0 to the input image item, the 'sato' derived image item should have a 'pixi' box that indicates bit depth 8. Current clients of libavif can handle this output image. This is why we only need to disable by default the 'sato' derived image items whose 'pixi' box indicates a bit depth other than 8, 10, 12.

Since the outut image is still represented with the avifImage struct, it seems that the only field in avifImage that may catch existing clients by surprise is the depth field. Are there other kinds of 'sato' derived image items that produce an output avifImage that current clients of libavif cannot handle?

[y-guyon]

Current clients of libavif can handle this output image.

Yes. The question is: should it still be applied?
With this specific example (noop 0 addition), it is obviously better NOT to enable Sample Transforms, as it would just make decoding slower for the same result.

Are there other kinds of 'sato' derived image items that produce an output avifImage that current clients of libavif cannot handle?

I do not think so. But it does not mean it is beneficial to do so.
libavif only supports Sample Transform derived image items that are NOT the primary item, and in the same altr group as the primary item. So there is always an alternative, and the primary item should be the default output of libavif in my opinion.
Advanced users can enable Sample Transform decoding to extract more information when needed.

[wantehchang]

I thought the only concern was breaking the clients unprepared to handle new bit depths. I did not realize the cost incurred by applying sample transforms was also a concern.

We need to provide guidance on whether libavif clients should enable sample transforms. Besides the two concerns mentioned above, are there other concerns?

I think we should constrain the new bit depths that libavif supports and document what they are. My code analysis shows that libavif malfunctions if decoder->data->meta->sampleTransformDepth > 16, so we need to at least disallow bit depths > 16.

---

Note: You can ignore the rest of this comment.

Re: AVIF_IMAGE_CONTENT_SAMPLE_TRANSFORMS vs. enableSampleTransforms: In your interpretation of avifImageContentTypeFlags based on the image item type, AVIF_IMAGE_CONTENT_SAMPLE_TRANSFORMS makes sense. I have a different interpretation of avifImageContentTypeFlags based on the output destination:

• AVIF_IMAGE_CONTENT_COLOR_AND_ALPHA: the output goes to decoder->image
• AVIF_IMAGE_CONTENT_GAIN_MAP: the output goes to decoder->image->gainMap->image

When sample transforms are enabled, the output still goes to decoder->image. Therefore sample transforms, unlike gain maps, do not behave like a new image content type.

[y-guyon]

I thought the only concern was breaking the clients unprepared to handle new bit depths. I did not realize the cost incurred by applying sample transforms was also a concern.

That was not explicit, sorry about that.

We need to provide guidance on whether libavif clients should enable sample transforms. Besides the two concerns mentioned above, are there other concerns?

Security? More features expose more attack surfaces in general.

we need to at least disallow bit depths > 16.

I think 16-bit is already the maximum libavif can output:

libavif/src/avif.c

Lines 143 to 146 in ec2ee83

	avifImage * avifImageCreate(uint32_t width, uint32_t height, uint32_t depth, avifPixelFormat yuvFormat)
	{
	// width and height are checked when actually used, for example by avifImageAllocatePlanes().
	AVIF_CHECKERR(depth <= 16, NULL); // avifImage only supports up to 16 bits per sample. See avifImageUsesU16().

This is verified in #3038 (although the error code was wrong).

I think we should constrain the new bit depths that libavif supports and document what they are.

I agree they should be documented.

My code analysis shows that libavif malfunctions if decoder->data->meta->sampleTransformDepth > 16, so we need to at least disallow bit depths > 16.

libavif "malfunctions" in the sense that 1. if >16-bit were to be allowed, some parts of libavif would need further changes for proper support, or 2. you can craft AVIF files that make libavif crash or incorrectly decode as of today?

When sample transforms are enabled, the output still goes to decoder->image. Therefore sample transforms, unlike gain maps, do not behave like a new image content type.

For me the name imageContentToDecode refers more to what is inside the file (to decode) than where to output it into.

[wantehchang]

Re: libavif malfunctions if decoder->data->meta->sampleTransformDepth > 16

Yannis: I didn't do a complete code analysis, but what I found concerns me. (You can do a similar analysis by inspecting the code related to sampleTransformDepth.)

In avifDecoderReset(), we assign a value to sampleTransformDepth:

            data->meta->sampleTransformDepth = pixiProp->u.pixi.planeDepths[0];

Code Search shows this is the only place where we assign a value to sampleTransformDepth. We do not restrict pixiProp->u.pixi.planeDepths[0] in avifDecoderSampleTransformItemValidateProperties().

Possible solution: We can change avifDecoderSampleTransformItemValidateProperties() to disallow pixiProp->u.pixi.planeDepths[0] > 16.

Later in avifDecoderReset(), we copy data->meta->sampleTransformDepth to decoder->image->depth:

    if (decoder->data->meta->sampleTransformExpression.count > 0) {
        AVIF_ASSERT_OR_RETURN(decoder->data->meta->sampleTransformDepth != 0);
        decoder->image->depth = decoder->data->meta->sampleTransformDepth;
    }

This bypasses the bit depth check in avifImageCreate() that you mentioned.

decoder->image is passed as the dstImage argument to avifDecoderApplySampleTransform().

    if (decoder->data->tileInfos[AVIF_ITEM_COLOR].tileCount != 0 && decoder->data->meta->sampleTransformExpression.count > 0) {
        AVIF_CHECKRES(avifDecoderApplySampleTransform(decoder, decoder->image));
    }

Eventually we reach avifImageApplyExpression32b(), which uses dstImage as follows:

                if (avifImageUsesU16(dstImage)) {
                    ((uint16_t *)row)[x] = (uint16_t)stack[0];
                } else {
                    row[x] = (uint8_t)stack[0];
                }

The cast to uint16_t * for any bit depth > 8 is incorrect for bit depths > 16. That's what I meant by "libavif malfunctions".

[wantehchang]

One more thing: avifDecoderSampleTransformItemValidateProperties() performs the following check:

    for (uint8_t i = 0; i < pixiProp->u.pixi.planeCount; ++i) {
        if (pixiProp->u.pixi.planeDepths[i] != pixiProp->u.pixi.planeDepths[0]) {
            avifDiagnosticsPrintf(diag,
                                  "Item ID %u of type '%.4s' has different depths specified by pixi property [%u, %u], this is not supported",
                                  item->id,
                                  (const char *)item->type,
                                  pixiProp->u.pixi.planeDepths[0],
                                  pixiProp->u.pixi.planeDepths[i]);
            return AVIF_RESULT_NOT_IMPLEMENTED;
        }
    }

This check is redundant because avifParsePixelInformationProperty() already performs the same check.

[wantehchang]

I wrote:

Possible solution: We can change avifDecoderSampleTransformItemValidateProperties() to disallow pixiProp->u.pixi.planeDepths[0] > 16.

Your PR #3038 disallows bit depths > 16 in avifParsePixelInformationProperty(). That is stricter and also works.