Add new wrappers for image-rs by amyspark · Pull Request #446 · mozilla/mp4parse-rust

amyspark · 2026-04-09T19:22:54Z

Hi all,

This PR adds support for a few BMFF boxes that are required to speed up the AVIF decoder's setup in image-rs, avoiding having to decode the full image to query e.g. dimensions, bit depth, etc.

While at it, I've also added support for detecting and exposing the Exif and XMP metadata boxes.

All feedback is appreciated.

Fixes #441
Fixes #444

cc @Shnatsel

kinetiknz

I haven't reviewed this thoroughly yet, but I've included some initial feedback.

The iloc read loop is potentially very expensive after these changes. It should remain the way it was, but extended to read the required items.

The read_infe/read_string changes don't work correctly for multiple reasons, so I don't believe this code has been tested.

We'll need automated tests added to cover the changes in the core parser.

kinetiknz · 2026-04-14T05:04:16Z

+            .map(|item| self.item_as_slice(item))
+    }
+
+    pub fn spatial_extents(&self) -> Result<&ImageSpatialExtentsProperty> {


The duplication in these additions with the *_ptr() variants needs to be addressed.

I am not sure how to address it, since using the existing _ptr APIs requires unsafe blocks:

error[E0133]: dereference of raw pointer is unsafe and requires unsafe function or block --> src\codecs\avif\decoder.rs:99:21 | 99 | height: (*dimensions).image_height, | ^^^^^^^^^^^^^ dereference of raw pointer | = note: raw pointers may be null, dangling or unaligned; they can violate aliasing rules and cause data races: all of these are undefined behavior

As per #444 (comment) you've been relying on cbindgen to use this crate, so didn't want to modify them.

fn primary_property<'a, T>( &'a self, box_type: BoxType, extract: impl Fn(&'a ItemProperty) -> Option<&'a T>, ) -> Result<Option<&'a T>> { let primary = self.primary_item.as_ref().ok_or(Error::from(Status::PitmMissing))?; match self.item_properties.get(primary.id, box_type)? { Some(prop) => Ok(Some(extract(prop).expect("property key mismatch"))), None => Ok(None), } }

The safe and _ptr variants can then be rewritten as:

pub fn image_mirror(&self) -> Result<&ImageMirror> { self.primary_property(BoxType::ImageMirror, |p| match p { ItemProperty::Mirroring(v) => Some(v), _ => None, })? .ok_or_else(|| Status::ImirMissing.into()) } pub fn image_mirror_ptr(&self) -> Result<*const ImageMirror> { match self.image_mirror() { Ok(v) => Ok(v), Err(Error::InvalidData(Status::PitmMissing | Status::ImirMissing)) => Ok(std::ptr::null()), Err(e) => Err(e), } }

for example. The spatial_extents change needs to preserve the fail_with_status_if check.

kinetiknz · 2026-04-14T05:16:41Z

+        return Ok(None);
    }
+
+    let item_name = src.read_string(strictness)?;


Use the pattern in read_hdlr to read nul-terminated strings. Use correct error types per that pattern, not the generic InvalidUtf8 that read_string returned.

With skip_box_remain moved, this is now broken when parsing item_type == uri boxes. Also broken if the optional content_encoding field is not present, since these changes assume it is.

Use the pattern in read_hdlr to read nul-terminated strings.

There are four different instances of null-terminated UTF-8 strings, I would like them to not copy-paste code for each one. The next iteration turns it into a standalone function with two new error code enums, let me know if that's OK.

Also took care of the uri boxes.

if the optional content_encoding field is not present

In the AVIF files I have here with XMP metadata, the field is present but empty. I couldn't find in 14496:12-2020 how to detect missing-but-optional strings, could you confirm whether this is done by expecting the box to end upon read?

amyspark · 2026-04-20T22:27:40Z

Hi, is there anything I can do to help this forward?

kinetiknz · 2026-04-21T01:13:57Z

Hi, is there anything I can do to help this forward?

I think it'd be a good idea to separate this into two PRs. It's fairly easy to review and merge the "Add safe wrappers for AVIF metadata boxes" change, the only issue with that is the need to reduce the duplication between the safe and *_ptr() variants. That can be done with a safe helper for

"Read and expose Exif and XMP metadata in the AvifContext" needs tests for the infe parser changes (it's clear this code still has not been tested - the application/rdf+xml comparison fails because of untrimmed NULs, for example), the string reading should use read_into_try_vec to bound the read length (and the result can then be split on NULs into the appropriate fields). The uri_type reading path is taken for every non-mime type, causing parser failures in Normal/Strict mode on valid files.

kinetiknz · 2026-04-21T01:25:17Z

+            .map(|item| self.item_as_slice(item))
+    }
+
+    pub fn spatial_extents(&self) -> Result<&ImageSpatialExtentsProperty> {


fn primary_property<'a, T>( &'a self, box_type: BoxType, extract: impl Fn(&'a ItemProperty) -> Option<&'a T>, ) -> Result<Option<&'a T>> { let primary = self.primary_item.as_ref().ok_or(Error::from(Status::PitmMissing))?; match self.item_properties.get(primary.id, box_type)? { Some(prop) => Ok(Some(extract(prop).expect("property key mismatch"))), None => Ok(None), } }

The safe and _ptr variants can then be rewritten as:

pub fn image_mirror(&self) -> Result<&ImageMirror> { self.primary_property(BoxType::ImageMirror, |p| match p { ItemProperty::Mirroring(v) => Some(v), _ => None, })? .ok_or_else(|| Status::ImirMissing.into()) } pub fn image_mirror_ptr(&self) -> Result<*const ImageMirror> { match self.image_mirror() { Ok(v) => Ok(v), Err(Error::InvalidData(Status::PitmMissing | Status::ImirMissing)) => Ok(std::ptr::null()), Err(e) => Err(e), } }

for example. The spatial_extents change needs to preserve the fail_with_status_if check.

kinetiknz · 2026-04-21T01:27:57Z

        }
    }

+    pub fn colour_information(&self) -> Result<&ColourInformation> {


Don't add this as-is. item_properties.get() errors with Status::IprpConflict when multiple colr boxes are associated with the primary item but both an nclx and an ICC colr on one item is legal per HEIF (and common in real AVIFs that want to specify both display primaries and a profile). Add safe variants of the existing nclx_colour_information_ptr and use icc_colour_information directly.

kinetiknz · 2026-04-21T01:29:53Z

+            {
+                Some(ItemProperty::Mirroring(imir)) => Ok(imir),
+                Some(other_property) => panic!("property key mismatch: {:?}", other_property),
+                None => Err(Error::from(Status::ItemTypeMissing)),


Error type should be Status::ImirMissing.

kinetiknz · 2026-04-21T01:30:14Z

+            {
+                Some(ItemProperty::PixelAspectRatio(pasp)) => Ok(pasp),
+                Some(other_property) => panic!("property key mismatch: {:?}", other_property),
+                None => Err(Error::from(Status::ItemTypeMissing)),


Error type should be Status::PaspMissing.

kinetiknz · 2026-04-21T01:32:49Z

 struct PropertyIndex(u16);
 #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, PartialOrd)]
-struct ItemId(u32);
+pub struct ItemId(u32);


Why was this and ItemInfoEntry made pub?

kinetiknz · 2026-04-21T01:36:06Z

-    item_type: u32,
+pub struct ItemInfoEntry {
+    pub item_id: ItemId,
+    pub item_type: u32,


This should be FourCC.

kinetiknz · 2026-04-21T01:42:31Z

-        };
-
-        assert!(item.is_none());
+        let mut item: Option<AvifItem> = None;


Revert these changes entirely. This can be done more efficiently with:

Before the iloc_items loop:

let exif_item_id = item_infos.iter() .find(|i| i.item_type.to_be_bytes() == *b"Exif") .map(|i| i.item_id); let xmp_item_id = item_infos.iter() .find(|i| i.item_type.to_be_bytes() == *b"mime" && i.content_type.as_ref() .is_some_and(|ct| ct.as_slice() == b"application/rdf+xml")) .map(|i| i.item_id);

Inside the loop:

let item = if Some(item_id) == primary_item_id { &mut primary_item } else if Some(item_id) == alpha_item_id { &mut alpha_item } else if Some(item_id) == exif_item_id { &mut exif_metadata } else if Some(item_id) == xmp_item_id { &mut xmp_metadata } else { continue; };

Add safe wrappers for AVIF metadata boxes

ca7a660

amyspark mentioned this pull request Apr 9, 2026

Assorted optimizations for GStreamer's image-rs plugin image-rs/image#2907

Draft

linkmauve suggested changes Apr 10, 2026

View reviewed changes

Comment thread mp4parse/src/lib.rs Outdated

Comment thread mp4parse/src/lib.rs Outdated

amyspark force-pushed the add-image-rs-wrappers branch from 86a54c0 to fde99ab Compare April 10, 2026 14:22

amyspark requested a review from linkmauve April 13, 2026 15:58

kinetiknz requested changes Apr 14, 2026

View reviewed changes

amyspark force-pushed the add-image-rs-wrappers branch from fde99ab to 191fd55 Compare April 14, 2026 14:00

Read and expose Exif and XMP metadata in the AvifContext

4575afd

amyspark force-pushed the add-image-rs-wrappers branch from 191fd55 to 4575afd Compare April 14, 2026 16:11

kinetiknz requested changes Apr 21, 2026

View reviewed changes

Conversation

amyspark commented Apr 9, 2026

Uh oh!

Uh oh!

Uh oh!

kinetiknz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amyspark commented Apr 20, 2026

Uh oh!

kinetiknz commented Apr 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants