Call avifImageFixXMP() during jpg,png reading
Strip one null character from XMP chunks extracted from JPG and PNG
files. Add regression test in avifmetadatatest.
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 40d9311..8e63627 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -69,6 +69,8 @@
* At decoding, avifIOStats now returns the same values as at encoding.
* avifRGBImageAllocatePixels() now returns avifResult instead of void to report
memory allocation failures.
+* avifReadImage(), avifJPEGRead() and avifPNGRead() now remove the trailing zero
+ byte from read XMP chunks, if any. See avifImageFixXMP().
## [0.11.1] - 2022-10-19
diff --git a/apps/shared/avifjpeg.c b/apps/shared/avifjpeg.c
index b599b3e..1e4d668 100644
--- a/apps/shared/avifjpeg.c
+++ b/apps/shared/avifjpeg.c
@@ -551,6 +551,7 @@
} else if (standardXMPData) {
avifImageSetMetadataXMP(avif, standardXMPData, standardXMPSize);
}
+ avifImageFixXMP(avif); // Remove one trailing null character if any.
}
jpeg_finish_decompress(&cinfo);
ret = AVIF_TRUE;
diff --git a/apps/shared/avifpng.c b/apps/shared/avifpng.c
index 3b00618..5c4614b 100644
--- a/apps/shared/avifpng.c
+++ b/apps/shared/avifpng.c
@@ -193,6 +193,10 @@
*ignoreXMP = AVIF_TRUE; // Ignore any other XMP chunk.
}
}
+ // The iTXt XMP payload may not contain a zero byte according to section 4.2.3.3 of
+ // the PNG specification, version 1.2. Still remove one trailing null character if any,
+ // in case libpng does not strictly enforce that at decoding.
+ avifImageFixXMP(avif);
return AVIF_TRUE;
}
@@ -515,26 +519,23 @@
if (avif->xmp.data && (avif->xmp.size > 0)) {
// The iTXt XMP payload may not contain a zero byte according to section 4.2.3.3 of
// the PNG specification, version 1.2.
- if (memchr(avif->xmp.data, '\0', avif->xmp.size)) {
- fprintf(stderr, "Error writing PNG: XMP metadata contains an invalid null character\n");
+ // The chunk is given to libpng as is. Bytes after a zero byte may be stripped.
+
+ // Providing the length through png_text.itxt_length does not work.
+ // The given png_text.text string must end with a zero byte.
+ if (avif->xmp.size >= SIZE_MAX) {
+ fprintf(stderr, "Error writing PNG: XMP metadata is too big\n");
goto cleanup;
- } else {
- // Providing the length through png_text.itxt_length does not work.
- // The given png_text.text string must end with a zero byte.
- if (avif->xmp.size >= SIZE_MAX) {
- fprintf(stderr, "Error writing PNG: XMP metadata is too big\n");
- goto cleanup;
- }
- avifRWDataRealloc(&xmp, avif->xmp.size + 1);
- memcpy(xmp.data, avif->xmp.data, avif->xmp.size);
- xmp.data[avif->xmp.size] = '\0';
- png_text * text = &texts[numTextMetadataChunks++];
- memset(text, 0, sizeof(*text));
- text->compression = PNG_ITXT_COMPRESSION_NONE;
- text->key = "XML:com.adobe.xmp";
- text->text = (char *)xmp.data;
- text->itxt_length = xmp.size;
}
+ avifRWDataRealloc(&xmp, avif->xmp.size + 1);
+ memcpy(xmp.data, avif->xmp.data, avif->xmp.size);
+ xmp.data[avif->xmp.size] = '\0';
+ png_text * text = &texts[numTextMetadataChunks++];
+ memset(text, 0, sizeof(*text));
+ text->compression = PNG_ITXT_COMPRESSION_NONE;
+ text->key = "XML:com.adobe.xmp";
+ text->text = (char *)xmp.data;
+ text->itxt_length = xmp.size;
}
if (numTextMetadataChunks != 0) {
png_set_text(png, info, texts, numTextMetadataChunks);
diff --git a/apps/shared/avifutil.c b/apps/shared/avifutil.c
index 8e20090..0f8381f 100644
--- a/apps/shared/avifutil.c
+++ b/apps/shared/avifutil.c
@@ -277,6 +277,22 @@
return format;
}
+void avifImageFixXMP(avifImage * image)
+{
+ // Zero bytes are forbidden in UTF-8 XML: https://en.wikipedia.org/wiki/Valid_characters_in_XML
+ // Keeping zero bytes in XMP may lead to issues at encoding or decoding.
+ // For example, the PNG specification forbids null characters in XMP. See avifPNGWrite().
+ // The XMP Specification Part 3 says "When XMP is encoded as UTF-8,
+ // there are no zero bytes in the XMP packet" for GIF.
+
+ // Consider a single trailing null character following a non-null character
+ // as a programming error. Leave other null characters as is.
+ // See the discussion at https://github.com/AOMediaCodec/libavif/issues/1333.
+ if (image->xmp.size >= 2 && image->xmp.data[image->xmp.size - 1] == '\0' && image->xmp.data[image->xmp.size - 2] != '\0') {
+ --image->xmp.size;
+ }
+}
+
void avifDumpDiagnostics(const avifDiagnostics * diag)
{
if (!*diag->error) {
diff --git a/apps/shared/avifutil.h b/apps/shared/avifutil.h
index be0b2b8..16d7a53 100644
--- a/apps/shared/avifutil.h
+++ b/apps/shared/avifutil.h
@@ -72,6 +72,9 @@
avifAppSourceTiming * sourceTiming,
struct y4mFrameIterator ** frameIter);
+// Removes a single trailing null character from the image->xmp, if there is exactly one.
+void avifImageFixXMP(avifImage * image);
+
// Used by image decoders when the user doesn't explicitly choose a format with --yuv
// This must match the cited fallback for "--yuv auto" in avifenc.c's syntax() function.
#define AVIF_APP_DEFAULT_PIXEL_FORMAT AVIF_PIXEL_FORMAT_YUV444
diff --git a/tests/data/README.md b/tests/data/README.md
index 8755fae..768292f 100644
--- a/tests/data/README.md
+++ b/tests/data/README.md
@@ -106,6 +106,22 @@
| 136627 | 0xffe1 APP1 | 4791 | http://ns.adobe.com/xmp/extensio |
| | | | ... |
+### File [paris_xmp_trailing_null.jpg](paris_xmp_trailing_null.jpg)
+
+
+
+License: [same as libavif](https://github.com/AOMediaCodec/libavif/blob/main/LICENSE)
+
+Source: `paris_exif_xmp_icc.jpg` loaded with `avifReadImage()`, stripped of ICC and Exif, a zero byte appended to XMP,
+then written with `avifJPEGWrite()` with quality 0 (without calling `avifImageFixXMP()`).
+
+| address | marker | length | data |
+|--------:|-------------|-------:|----------------------------------------------|
+| 0 | 0xffd8 SOI | | |
+| 2 | 0xffe0 APP0 | 16 | `JFIF.........` |
+| 20 | 0xffe1 APP1 | 3930 | `http://ns.adobe.com/xap/1.0/.<?x` |
+| | | | ... |
+
### File [paris_icc_exif_xmp.png](paris_icc_exif_xmp.png)
@@ -148,7 +164,7 @@
License: [same as libavif](https://github.com/AOMediaCodec/libavif/blob/main/LICENSE)
-Source: `paris_exif_xmp_icc.jpg` stripped from all metadata with `exiftool -all=` and Exif orientation added
+Source: `paris_exif_xmp_icc.jpg` stripped of all metadata with `exiftool -all=` and Exif orientation added
with `exiv2 -k -M "set Exif.Image.Orientation 5"`
| address | marker | length | data |
diff --git a/tests/data/paris_xmp_trailing_null.jpg b/tests/data/paris_xmp_trailing_null.jpg
new file mode 100644
index 0000000..2d46824
--- /dev/null
+++ b/tests/data/paris_xmp_trailing_null.jpg
Binary files differ
diff --git a/tests/gtest/avifmetadatatest.cc b/tests/gtest/avifmetadatatest.cc
index 507b573..bb6c7b7 100644
--- a/tests/gtest/avifmetadatatest.cc
+++ b/tests/gtest/avifmetadatatest.cc
@@ -2,6 +2,7 @@
// SPDX-License-Identifier: BSD-2-Clause
#include <array>
+#include <cstring>
#include <tuple>
#include "avif/avif.h"
@@ -413,6 +414,32 @@
//------------------------------------------------------------------------------
+// Regression test for https://github.com/AOMediaCodec/libavif/issues/1333.
+// Coverage for avifImageFixXMP().
+TEST(MetadataTest, XMPWithTrailingNullCharacter) {
+ testutil::AvifImagePtr jpg =
+ testutil::ReadImage(data_path, "paris_xmp_trailing_null.jpg");
+ ASSERT_NE(jpg, nullptr);
+ ASSERT_NE(jpg->xmp.size, 0u);
+ // avifJPEGRead() should strip the trailing null character.
+ ASSERT_EQ(std::memchr(jpg->xmp.data, '\0', jpg->xmp.size), nullptr);
+
+ // Append a zero byte to see what happens when encoded with libpng.
+ avifRWDataRealloc(&jpg->xmp, jpg->xmp.size + 1);
+ jpg->xmp.data[jpg->xmp.size - 1] = '\0';
+ testutil::WriteImage(jpg.get(),
+ (testing::TempDir() + "xmp_trailing_null.png").c_str());
+ const testutil::AvifImagePtr png =
+ testutil::ReadImage(testing::TempDir().c_str(), "xmp_trailing_null.png");
+ ASSERT_NE(png, nullptr);
+ ASSERT_NE(png->xmp.size, 0u);
+ // avifPNGRead() should strip the trailing null character, but the libpng
+ // export during testutil::WriteImage() probably took care of that anyway.
+ ASSERT_EQ(std::memchr(png->xmp.data, '\0', png->xmp.size), nullptr);
+}
+
+//------------------------------------------------------------------------------
+
} // namespace
} // namespace libavif
