Skip to content

Commit

Permalink
Ignore descr props after transf props (#2571)
Browse files Browse the repository at this point in the history
Also reject non-essential transformative properties.
  • Loading branch information
y-guyon authored Jan 15, 2025
1 parent 2ea1f16 commit 49729e4
Show file tree
Hide file tree
Showing 7 changed files with 182 additions and 2 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ The changes are relative to the previous release, unless the baseline is specifi
* Deprecate avifCropRectConvertCleanApertureBox() and
avifCleanApertureBoxConvertCropRect(). Replace them with
avifCropRectFromCleanApertureBox() and avifCleanApertureBoxFromCropRect().
* Ignore descriptive properties associated after transformative ones.
* Reject non-essential transformative properties.

## [1.1.1] - 2024-07-30

Expand Down
50 changes: 48 additions & 2 deletions src/read.c
Original file line number Diff line number Diff line change
Expand Up @@ -2731,15 +2731,21 @@ static avifResult avifParseItemPropertyAssociation(avifMeta * meta, const uint8_

uint8_t associationCount;
AVIF_CHECKERR(avifROStreamRead(&s, &associationCount, 1), AVIF_RESULT_BMFF_PARSE_FAILED);
avifBool transformativePropertySeen = AVIF_FALSE;
for (uint8_t associationIndex = 0; associationIndex < associationCount; ++associationIndex) {
uint8_t essential;
AVIF_CHECKERR(avifROStreamReadBitsU8(&s, &essential, /*bitCount=*/1), AVIF_RESULT_BMFF_PARSE_FAILED); // bit(1) essential;
uint32_t propertyIndex;
AVIF_CHECKERR(avifROStreamReadBitsU32(&s, &propertyIndex, /*bitCount=*/propertyIndexIsU15 ? 15 : 7),
AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(7/15) property_index;

// ISO/IEC 14496-12 Section 8.11.14.3:
// 0 indicating that no property is associated (the essential indicator shall also be 0)
if (propertyIndex == 0) {
// Not associated with any item
if (essential) {
avifDiagnosticsPrintf(diag, "Box[ipma] for item ID [%u] contains an illegal essential property index 0", itemID);
return AVIF_RESULT_BMFF_PARSE_FAILED;
}
continue;
}
--propertyIndex; // 1-indexed
Expand All @@ -2756,6 +2762,23 @@ static avifResult avifParseItemPropertyAssociation(avifMeta * meta, const uint8_
// Copy property to item
const avifProperty * srcProp = &meta->properties.prop[propertyIndex];

// ISO/IEC 23000-22:2019/Amd. 2:2021 Section 7.3.9:
// All transformative properties associated with coded and derived images shall be marked as essential,
// and shall be from the set defined in 7.3.6.7 or the applicable MIAF profile. No other essential
// transformative property shall be associated with such images.
const avifBool isTransformative = !memcmp(srcProp->type, "clap", 4) || !memcmp(srcProp->type, "irot", 4) ||
!memcmp(srcProp->type, "imir", 4);
// ISO/IEC 23008-12:2022 Section 3.1.28:
// item property: descriptive or transformative information
const avifBool isDescriptive = !isTransformative;
// ISO/IEC 23008-12:2022 Section 6.5.1:
// Readers shall allow and ignore descriptive properties following the first transformative or
// unrecognized property, whichever is earlier, in the sequence associating properties with an item.
// No need to check for unrecognized properties as they cannot be transformative according to MIAF.
if (transformativePropertySeen && isDescriptive) {
continue;
}

// Some properties are supported and parsed by libavif.
// Other properties are forwarded to the user as opaque blobs.
const avifBool supportedType = !srcProp->isOpaque;
Expand Down Expand Up @@ -2790,7 +2813,16 @@ static avifResult avifParseItemPropertyAssociation(avifMeta * meta, const uint8_
"a1op",

// HEIF: Section 6.5.11.1: "essential shall be equal to 1 for an 'lsel' item property."
"lsel"
"lsel",

// MIAF 2019/Amd. 2:2021: Section 7.3.9:
// All transformative properties associated with coded and derived images shall be
// marked as essential
// It makes no sense to allow for non-essential crop/orientation associated with an item
// that is not a coded or derived image, so for simplicity 'item' is not checked here.
"clap",
"irot",
"imir"

};
size_t essentialTypesCount = sizeof(essentialTypes) / sizeof(essentialTypes[0]);
Expand All @@ -2811,6 +2843,15 @@ static avifResult avifParseItemPropertyAssociation(avifMeta * meta, const uint8_
*dstProp = *srcProp;
} else {
if (essential) {
// ISO/IEC 23008-12 Section 10.2.1:
// Under any brand, the primary item (or an alternative if alternative support is required)
// shall be processable by a reader implementing only the required features of that brand.
// Specifically, given that each brand has a set of properties that a reader is required to
// support: the item shall not have properties that are marked as essential and are outside
// this set.
// It is assumed that this rule also applies to items the primary item depends on (such as
// the cells of a grid).

// Discovered an essential item property that libavif doesn't support!
// Make a note to ignore this item later.
item->hasUnsupportedEssentialProperty = AVIF_TRUE;
Expand All @@ -2825,6 +2866,11 @@ static avifResult avifParseItemPropertyAssociation(avifMeta * meta, const uint8_
AVIF_CHECKRES(
avifRWDataSet(&dstProp->u.opaque.boxPayload, srcProp->u.opaque.boxPayload.data, srcProp->u.opaque.boxPayload.size));
}

if (isTransformative) {
AVIF_ASSERT_OR_RETURN(supportedType);
transformativePropertySeen = AVIF_TRUE;
}
}
}
return AVIF_RESULT_OK;
Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ if(AVIF_GTEST)
add_avif_gtest_with_data(avifsize0test)
add_avif_internal_gtest(avifstreamtest)
add_avif_internal_gtest(aviftilingtest)
add_avif_gtest_with_data(aviftransformtest)
add_avif_internal_gtest(avifutilstest)
add_avif_gtest(avify4mtest)

Expand Down
21 changes: 21 additions & 0 deletions tests/data/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,27 @@ License: [same as libavif](https://github.com/AOMediaCodec/libavif/blob/main/LIC
Source: `avifenc circle-trns-after-plte.png` with custom properties added in
`avifRWStreamWriteProperties()`: FullBox `1234`, Box `abcd` and Box `uuid`.

### File [clap_irot_imir_non_essential.avif](clap_irot_imir_non_essential.avif)

![](clap_irot_imir_non_essential.avif)

License: [same as libavif](https://github.com/AOMediaCodec/libavif/blob/main/LICENSE)

Source: File generated with `TransformTest.ClapIrotImir` from
`aviftransformtest.cc`, where `clap`, `irot` and `imir` are tagged as
non-essential.

### File [clop_irot_imor.avif](clop_irot_imor.avif)

![](clop_irot_imor.avif)

License: [same as libavif](https://github.com/AOMediaCodec/libavif/blob/main/LICENSE)

Source: File generated with `TransformTest.ClapIrotImir` from
`aviftransformtest.cc`, where essential `clap` property was replaced by made-up
non-essential `clop` property and essential `imir` property was replaced by
made-up non-essential `imor` property.

### File [draw_points.png](draw_points.png)

![](draw_points.png)
Expand Down
Binary file added tests/data/clap_irot_imir_non_essential.avif
Binary file not shown.
Binary file added tests/data/clop_irot_imor.avif
Binary file not shown.
110 changes: 110 additions & 0 deletions tests/gtest/aviftransformtest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
// Copyright 2025 Google LLC
// SPDX-License-Identifier: BSD-2-Clause

#include "avif/avif.h"
#include "aviftest_helpers.h"
#include "gtest/gtest.h"

namespace avif {
namespace {

// Used to pass the data folder path to the GoogleTest suites.
const char* data_path = nullptr;

//------------------------------------------------------------------------------

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

avifDiagnostics diag{};
ImagePtr image =
testutil::CreateImage(/*width=*/12, /*height=*/34, /*depth=*/10,
AVIF_PIXEL_FORMAT_YUV444, AVIF_PLANES_ALL);
ASSERT_NE(image, nullptr);
testutil::FillImageGradient(image.get()); // The pixels do not matter.

image->transformFlags |= AVIF_TRANSFORM_CLAP;
const avifCropRect rect{/*x=*/4, /*y=*/6, /*width=*/8, /*height=*/10};
ASSERT_TRUE(avifCleanApertureBoxFromCropRect(
&image->clap, &rect, image->width, image->height, &diag));

image->transformFlags |= AVIF_TRANSFORM_IROT;
image->irot.angle = 1;
image->transformFlags |= AVIF_TRANSFORM_IMIR;
image->imir.axis = 1;

// Encode.
EncoderPtr encoder(avifEncoderCreate());
ASSERT_NE(encoder, nullptr);
encoder->speed = AVIF_SPEED_FASTEST;
testutil::AvifRwData encoded;
ASSERT_EQ(avifEncoderWrite(encoder.get(), image.get(), &encoded),
AVIF_RESULT_OK);

// Decode.
ImagePtr decoded(avifImageCreateEmpty());
ASSERT_NE(decoded, nullptr);
DecoderPtr decoder(avifDecoderCreate());
ASSERT_NE(decoder, nullptr);
ASSERT_EQ(avifDecoderReadMemory(decoder.get(), decoded.get(), encoded.data,
encoded.size),
AVIF_RESULT_OK);

EXPECT_EQ(decoded->transformFlags, image->transformFlags);
EXPECT_EQ(decoded->clap.widthN, image->clap.widthN);
EXPECT_EQ(decoded->clap.widthD, image->clap.widthD);
EXPECT_EQ(decoded->clap.heightN, image->clap.heightN);
EXPECT_EQ(decoded->clap.heightD, image->clap.heightD);
EXPECT_EQ(decoded->clap.horizOffN, image->clap.horizOffN);
EXPECT_EQ(decoded->clap.horizOffD, image->clap.horizOffD);
EXPECT_EQ(decoded->clap.vertOffN, image->clap.vertOffN);
EXPECT_EQ(decoded->clap.vertOffD, image->clap.vertOffD);
EXPECT_EQ(decoded->irot.angle, image->irot.angle);
EXPECT_EQ(decoded->imir.axis, image->imir.axis);
}

TEST(TransformTest, ClapIrotImirNonEssential) {
// Invalid file with non-essential transformative properties.
const std::string path =
std::string(data_path) + "clap_irot_imir_non_essential.avif";
DecoderPtr decoder(avifDecoderCreate());
ASSERT_NE(decoder, nullptr);
ASSERT_EQ(avifDecoderSetIOFile(decoder.get(), path.c_str()), AVIF_RESULT_OK);
ASSERT_EQ(avifDecoderParse(decoder.get()), AVIF_RESULT_BMFF_PARSE_FAILED);
}

TEST(TransformTest, ClopIrotImor) {
// File with a non-essential unrecognized property 'clop', an essential
// transformation property 'irot', and a non-essential unrecognized property
// 'imor'.
const std::string path = std::string(data_path) + "clop_irot_imor.avif";
DecoderPtr decoder(avifDecoderCreate());
ASSERT_NE(decoder, nullptr);
ASSERT_EQ(avifDecoderSetIOFile(decoder.get(), path.c_str()), AVIF_RESULT_OK);
ASSERT_EQ(avifDecoderParse(decoder.get()), AVIF_RESULT_OK);

// 'imor' shall be ignored by libavif as it is after a transformative property
// in the 'ipma' association order.
ASSERT_EQ(decoder->image->numProperties, 1u);
const avifImageItemProperty& clop = decoder->image->properties[0];
EXPECT_EQ(std::string(clop.boxtype, clop.boxtype + 4), "clop");
}

//------------------------------------------------------------------------------

} // namespace
} // namespace avif

int main(int argc, char** argv) {
::testing::InitGoogleTest(&argc, argv);
if (argc != 2) {
std::cerr << "There must be exactly one argument containing the path to "
"the test data folder"
<< std::endl;
return 1;
}
avif::data_path = argv[1];
return RUN_ALL_TESTS();
}

0 comments on commit 49729e4

Please sign in to comment.