Merge pull request #328 from mozilla/unsupported-detail

Communicate more detail about what specifically is unsupported across FFI

Author: baumanj

mp4parse/av1-avif

@@ -126,6 +126,9 @@ box_database!(
     CleanApertureBox                  0x636c_6170, // "clap"
     ImageRotation                     0x6972_6f74, // "irot"
     ImageMirror                       0x696d_6972, // "imir"
+    OperatingPointSelectorProperty    0x6131_6f70, // "a1op"
+    AV1LayeredImageIndexingProperty   0x6131_6c78, // "a1lx"
+    LayerSelectorProperty             0x6c73_656c, // "lsel"
     SampleTableBox                    0x7374_626c, // "stbl"
     SampleDescriptionBox              0x7374_7364, // "stsd"
     TimeToSampleBox                   0x7374_7473, // "stts"

mp4parse/src/boxes.rs

@@ -148,6 +148,87 @@ struct HashMap;
 #[allow(dead_code)]
 struct String;
 
+/// The return value to the C API
+/// Any detail that needs to be communicated to the caller must be encoded here
+/// since the [`Error`] type's associated data is part of the FFI.
+#[repr(C)]
+#[derive(PartialEq, Debug)]
+pub enum Status {
+    Ok = 0,
+    BadArg = 1,
+    Invalid = 2,
+    Unsupported = 3,
+    Eof = 4,
+    Io = 5,
+    Oom = 6,
+    UnsupportedA1lx,
+    UnsupportedA1op,
+    UnsupportedClap,
+    UnsupportedGrid,
+    UnsupportedIpro,
+    UnsupportedLsel,
+}
+
+/// For convenience of creating an error for an unsupported feature which we
+/// want to communicate the specific feature back to the C API caller
+impl From<Status> for Error {
+    fn from(parse_status: Status) -> Self {
+        let msg = match parse_status {
+            Status::Ok
+            | Status::BadArg
+            | Status::Invalid
+            | Status::Unsupported
+            | Status::Eof
+            | Status::Io
+            | Status::Oom => {
+                panic!("Status -> Error is only for Status:UnsupportedXXX errors")
+            }
+
+            Status::UnsupportedA1lx => "AV1 layered image indexing (a1lx) is unsupported",
+            Status::UnsupportedA1op => "Operating point selection (a1op) is unsupported",
+            Status::UnsupportedClap => "Clean aperture (clap) transform is unsupported",
+            Status::UnsupportedGrid => "Grid-based images are unsupported",
+            Status::UnsupportedIpro => "Item protection (ipro) is unsupported",
+            Status::UnsupportedLsel => "Layer selection (lsel) is unsupported",
+        };
+        Self::UnsupportedDetail(parse_status, msg)
+    }
+}
+
+impl From<Error> for Status {
+    fn from(error: Error) -> Self {
+        match error {
+            Error::NoMoov | Error::InvalidData(_) => Self::Invalid,
+            Error::Unsupported(_) => Self::Unsupported,
+            Error::UnsupportedDetail(parse_status, _msg) => parse_status,
+            Error::UnexpectedEOF => Self::Eof,
+            Error::Io(_) => {
+                // Getting std::io::ErrorKind::UnexpectedEof is normal
+                // but our From trait implementation should have converted
+                // those to our Error::UnexpectedEOF variant.
+                Self::Io
+            }
+            Error::OutOfMemory => Self::Oom,
+        }
+    }
+}
+
+impl From<Result<(), Status>> for Status {
+    fn from(result: Result<(), Status>) -> Self {
+        match result {
+            Ok(()) => Status::Ok,
+            Err(Status::Ok) => unreachable!(),
+            Err(e) => e,
+        }
+    }
+}
+
+impl From<fallible_collections::TryReserveError> for Status {
+    fn from(_: fallible_collections::TryReserveError) -> Self {
+        Status::Oom
+    }
+}
+
 /// Describes parser failures.
 ///
 /// This enum wraps the standard `io::Error` type, unified with
@@ -158,6 +239,10 @@ pub enum Error {
     InvalidData(&'static str),
     /// Parse error caused by limited parser support rather than invalid data.
     Unsupported(&'static str),
+    /// Similar to [`Self::Unsupported`], but for errors that have a specific
+    /// [`Status`] variant for communicating the detail across FFI.
+    /// See the helper [`From<Status> for Error`](enum.Error.html#impl-From<Status>)
+    UnsupportedDetail(Status, &'static str),
     /// Reflect `std::io::ErrorKind::UnexpectedEof` for short data.
     UnexpectedEOF,
     /// Propagate underlying errors from `std::io`.
@@ -1857,9 +1942,15 @@ fn read_avif_meta<T: Read + Offset>(
     let item_infos = item_infos.ok_or(Error::InvalidData("iinf missing"))?;
 
     if let Some(item_info) = item_infos.iter().find(|x| x.item_id == primary_item_id) {
-        if &item_info.item_type.to_be_bytes() != b"av01" {
-            warn!("primary_item_id type: {}", U32BE(item_info.item_type));
-            return Err(Error::InvalidData("primary_item_id type is not av01"));
+        debug!("primary_item_id type: {}", U32BE(item_info.item_type));
+        match &item_info.item_type.to_be_bytes() {
+            b"av01" => {}
+            b"grid" => return Err(Error::from(Status::UnsupportedGrid)),
+            _ => {
+                return Err(Error::InvalidData(
+                    "primary_item_id type is neither 'av01' nor 'grid'",
+                ))
+            }
         }
     } else {
         return Err(Error::InvalidData(
@@ -1964,9 +2055,7 @@ fn read_infe<T: Read>(src: &mut BMFFBox<T>, strictness: ParseStrictness) -> Resu
     let item_protection_index = be_u16(src)?;
 
     if item_protection_index != 0 {
-        return Err(Error::Unsupported(
-            "protected items (infe.item_protection_index != 0) are not supported",
-        ));
+        return Err(Error::from(Status::UnsupportedIpro));
     }
 
     let item_type = be_u32(src)?;
@@ -2109,6 +2198,9 @@ fn read_iprp<T: Read>(
                         | ItemProperty::Rotation(_) => {
                             if !a.essential {
                                 warn!("{:?} is invalid", property);
+                                // This is a "shall", but it is likely to change, so only
+                                // fail if using strict parsing.
+                                // See https://github.com/mozilla/mp4parse-rust/issues/284
                                 fail_if(
                                     strictness == ParseStrictness::Strict,
                                     "All transformative properties associated with coded and \
@@ -2532,6 +2624,11 @@ fn read_ipma<T: Read>(
 
 /// Parse an ItemPropertyContainerBox
 ///
+/// For unsupported properties that we know about, return specific
+/// [`Status`] UnsupportedXXXX variants. Unless running in
+/// [`ParseStrictness::Permissive`] mode, in which case, unsupported properties
+/// will be ignored.
+///
 /// See ISOBMFF (ISO 14496-12:2020 § 8.11.14.1
 fn read_ipco<T: Read>(
     src: &mut BMFFBox<T>,
@@ -2545,6 +2642,14 @@ fn read_ipco<T: Read>(
         if let Some(property) = match b.head.name {
             BoxType::AuxiliaryTypeProperty => Some(ItemProperty::AuxiliaryType(read_auxc(&mut b)?)),
             BoxType::AV1CodecConfigurationBox => Some(ItemProperty::AV1Config(read_av1c(&mut b)?)),
+            BoxType::AV1LayeredImageIndexingProperty
+                if strictness != ParseStrictness::Permissive =>
+            {
+                return Err(Error::from(Status::UnsupportedA1lx))
+            }
+            BoxType::CleanApertureBox if strictness != ParseStrictness::Permissive => {
+                return Err(Error::from(Status::UnsupportedClap))
+            }
             BoxType::ColourInformationBox => {
                 Some(ItemProperty::Colour(read_colr(&mut b, strictness)?))
             }
@@ -2553,6 +2658,14 @@ fn read_ipco<T: Read>(
             BoxType::ImageSpatialExtentsProperty => {
                 Some(ItemProperty::ImageSpatialExtents(read_ispe(&mut b)?))
             }
+            BoxType::LayerSelectorProperty if strictness != ParseStrictness::Permissive => {
+                return Err(Error::from(Status::UnsupportedLsel))
+            }
+            BoxType::OperatingPointSelectorProperty
+                if strictness != ParseStrictness::Permissive =>
+            {
+                return Err(Error::from(Status::UnsupportedA1op))
+            }
             BoxType::PixelInformationBox => Some(ItemProperty::Channels(read_pixi(&mut b)?)),
             other_box_type => {
                 // Though we don't do anything with other property types, we still store