Improve validation errors and editor feedback

dolby_vision/CHANGELOG.md

@@ -1,6 +1,10 @@
 ## Unreleased
 
 - `ReservedExtMetadataBlock.data` was replaced by a regular `Vec`.
+- Improved DM data error contexts to include the CM version and block levels.
+- `GenerateConfig`: list encoding helpers now return iterators instead of a `Vec`.
+  - The iterators do not filter out errors anymore.  
+    `collect_encoded_rpus` was added for convenience to reproduce previous behaviour.
 
 ## 3.3.2
 - `rpu`: fix `write_rpu_data` allocated capacity. Now static and 512 bytes.

dolby_vision/src/rpu/extension_metadata/blocks/mod.rs

@@ -185,9 +185,8 @@ impl ExtMetadataBlock {
 
         ensure!(
             T::ALLOWED_BLOCK_LEVELS.contains(&level),
-            "Metadata block level {} is invalid for {}",
-            &level,
-            T::VERSION
+            "Metadata block level {} is not allowed",
+            &level
         );
 
         Ok(())
@@ -203,8 +202,7 @@ impl ExtMetadataBlock {
         ensure!(
             block_length == self.length_bytes(),
             format!(
-                "{}: Invalid metadata block. Block level {} should have length {}",
-                T::VERSION,
+                "Invalid metadata block. Block level {} should have length {}",
                 level,
                 self.length_bytes()
             )
@@ -217,7 +215,7 @@ impl ExtMetadataBlock {
         for _ in 0..ext_block_use_bits {
             ensure!(
                 !reader.read_bit()?,
-                format!("{}: ext_dm_alignment_zero_bit != 0", T::VERSION)
+                format!("ext_dm_alignment_zero_bit != 0")
             );
         }
 

dolby_vision/src/rpu/extension_metadata/cmv29.rs

@@ -52,19 +52,14 @@ impl WithExtMetadataBlocks for CmV29DmData {
             5 => level5::ExtMetadataBlockLevel5::parse(reader)?,
             6 => level6::ExtMetadataBlockLevel6::parse(reader)?,
             255 => level255::ExtMetadataBlockLevel255::parse(reader)?,
-            3 | 8 | 9 | 10 | 11 | 254 => bail!(
-                "Invalid block level {} for {} RPU",
-                ext_block_level,
-                Self::VERSION,
-            ),
+            3 | 8 | 9 | 10 | 11 | 254 => bail!("Disallowed block level {}", ext_block_level),
             _ => {
+                // FIXME: This returns early so the parsing doesn't actually take place
                 ensure!(
                     false,
                     format!(
-                        "{} - Unknown metadata block found: Level {}, length {}, please open an issue.",
-                        Self::VERSION,
-                        ext_block_level,
-                        ext_block_length
+                        "Unknown metadata block found: Level {}, length {}, please open an issue.",
+                        ext_block_level, ext_block_length
                     )
                 );
 
@@ -128,53 +123,32 @@ impl CmV29DmData {
 
         ensure!(
             invalid_blocks_count == 0,
-            format!(
-                "{}: Only allowed blocks level 1, 2, 4, 5, 6, and 255",
-                Self::VERSION
-            )
+            "Only allowed blocks level 1, 2, 4, 5, 6, and 255"
         );
 
         ensure!(
             level1_count <= 1,
-            format!(
-                "{}: There must be at most one L1 metadata block",
-                Self::VERSION
-            )
+            "There must be at most one L1 metadata block"
         );
         ensure!(
             level2_count <= 8,
-            format!(
-                "{}: There must be at most 8 L2 metadata blocks",
-                Self::VERSION
-            )
+            "There must be at most 8 L2 metadata blocks"
         );
         ensure!(
             level255_count <= 1,
-            format!(
-                "{}: There must be at most one L255 metadata block",
-                Self::VERSION
-            )
+            "There must be at most one L255 metadata block"
         );
         ensure!(
             level4_count <= 1,
-            format!(
-                "{}: There must be at most one L4 metadata block",
-                Self::VERSION
-            )
+            "There must be at most one L4 metadata block"
         );
         ensure!(
             level5_count <= 1,
-            format!(
-                "{}: There must be at most one L5 metadata block",
-                Self::VERSION
-            )
+            "There must be at most one L5 metadata block"
         );
         ensure!(
             level6_count <= 1,
-            format!(
-                "{}: There must be at most one L6 metadata block",
-                Self::VERSION
-            )
+            "There must be at most one L6 metadata block"
         );
 
         Ok(())

dolby_vision/src/rpu/extension_metadata/cmv40.rs

@@ -52,19 +52,14 @@ impl WithExtMetadataBlocks for CmV40DmData {
             10 => level10::ExtMetadataBlockLevel10::parse(reader, ext_block_length)?,
             11 => level11::ExtMetadataBlockLevel11::parse(reader)?,
             254 => level254::ExtMetadataBlockLevel254::parse(reader)?,
-            1 | 2 | 4 | 5 | 6 | 255 => bail!(
-                "Invalid block level {} for {} RPU",
-                ext_block_level,
-                Self::VERSION,
-            ),
+            1 | 2 | 4 | 5 | 6 | 255 => bail!("Disallowed block level {}", ext_block_level),
             _ => {
+                // FIXME: This returns early so the parsing doesn't actually take place
                 ensure!(
                     false,
                     format!(
-                        "{} - Unknown metadata block found: Level {}, length {}, please open an issue.",
-                        Self::VERSION,
-                        ext_block_level,
-                        ext_block_length
+                        "Unknown metadata block found: Level {}, length {}, please open an issue.",
+                        ext_block_level, ext_block_length
                     )
                 );
 
@@ -145,51 +140,30 @@ impl CmV40DmData {
 
         ensure!(
             invalid_blocks_count == 0,
-            format!(
-                "{}: Only allowed blocks level 3, 8, 9, 10, 11 and 254",
-                Self::VERSION
-            )
+            "Only allowed blocks level 3, 8, 9, 10, 11 and 254"
         );
 
-        ensure!(
-            level254_count == 1,
-            format!("{}: There must be one L254 metadata block", Self::VERSION)
-        );
+        ensure!(level254_count == 1, "There must be one L254 metadata block");
 
         ensure!(
             level3_count <= 1,
-            format!(
-                "{}: There must be at most one L3 metadata block",
-                Self::VERSION
-            )
+            "There must be at most one L3 metadata block"
         );
         ensure!(
             level8_count <= 5,
-            format!(
-                "{}: There must be at most 5 L8 metadata blocks",
-                Self::VERSION
-            )
+            "There must be at most 5 L8 metadata blocks"
         );
         ensure!(
             level9_count <= 1,
-            format!(
-                "{}: There must be at most one L9 metadata block",
-                Self::VERSION
-            )
+            "There must be at most one L9 metadata block"
         );
         ensure!(
             level10_count <= 4,
-            format!(
-                "{}: There must be at most 4 L10 metadata blocks",
-                Self::VERSION
-            )
+            "There must be at most 4 L10 metadata blocks"
         );
         ensure!(
             level11_count <= 1,
-            format!(
-                "{}: There must be at most one L11 metadata block",
-                Self::VERSION
-            )
+            "There must be at most one L11 metadata block"
         );
 
         Ok(())

dolby_vision/src/rpu/extension_metadata/mod.rs

@@ -1,4 +1,4 @@
-use anyhow::{Result, ensure};
+use anyhow::{Context, Result, ensure};
 use bitvec_helpers::{
     bitstream_io_reader::BsIoSliceReader, bitstream_io_writer::BitstreamIoWriter,
 };
@@ -59,9 +59,7 @@ pub trait WithExtMetadataBlocks {
 
         ensure!(
             Self::ALLOWED_BLOCK_LEVELS.contains(&level),
-            "Metadata block level {} is invalid for {}",
-            &level,
-            Self::VERSION
+            "Metadata block level {level} is not allowed"
         );
 
         let blocks = self.blocks_mut();
@@ -90,13 +88,16 @@ pub trait WithExtMetadataBlocks {
         let ext_metadata_blocks = self.blocks_ref();
 
         for ext_metadata_block in ext_metadata_blocks {
+            let level = ext_metadata_block.level();
             let remaining_bits =
                 (ext_metadata_block.length_bits() - ext_metadata_block.required_bits()) as u32;
 
             writer.write_ue(ext_metadata_block.length_bytes())?;
-            writer.write::<8, u8>(ext_metadata_block.level())?;
+            writer.write::<8, u8>(level)?;
 
-            ext_metadata_block.write(writer)?;
+            ext_metadata_block
+                .write(writer)
+                .with_context(|| format!("Level {level}"))?;
 
             // ext_dm_alignment_zero_bit
             writer.pad(remaining_bits)?;
@@ -116,10 +117,7 @@ impl DmData {
         meta.set_num_ext_blocks(num_ext_blocks);
 
         while !reader.byte_aligned() {
-            ensure!(
-                !reader.read_bit()?,
-                format!("{}: dm_alignment_zero_bit != 0", T::VERSION)
-            );
+            ensure!(!reader.read_bit()?, "dm_alignment_zero_bit != 0");
         }
 
         for _ in 0..num_ext_blocks {

dolby_vision/src/rpu/generate.rs

@@ -185,19 +185,21 @@ impl GenerateConfig {
         Ok(list)
     }
 
-    pub fn encode_option_rpus(rpus: &mut [Option<DoviRpu>]) -> Vec<Vec<u8>> {
-        rpus.iter_mut()
-            .filter_map(|e| e.as_mut())
-            .map(|e| e.write_hevc_unspec62_nalu())
-            .filter_map(Result::ok)
-            .collect()
+    /// Encodes the RPUs for binary file format (HEVC UNSPEC62)
+    pub fn encode_option_rpus(rpus: &[Option<DoviRpu>]) -> impl Iterator<Item = Result<Vec<u8>>> {
+        rpus.iter()
+            .filter_map(|rpu| rpu.as_ref().map(|e| e.write_hevc_unspec62_nalu()))
     }
 
-    pub fn encode_rpus(rpus: &mut [DoviRpu]) -> Vec<Vec<u8>> {
-        rpus.iter_mut()
-            .map(|e| e.write_hevc_unspec62_nalu())
-            .filter_map(Result::ok)
-            .collect()
+    /// Encodes the RPUs for binary file format (HEVC UNSPEC62)
+    pub fn encode_rpus(rpus: &[DoviRpu]) -> impl Iterator<Item = Result<Vec<u8>>> {
+        rpus.iter().map(|e| e.write_hevc_unspec62_nalu())
+    }
+
+    /// Collects all valid results into a single list
+    /// Helper for use with list encoding functions
+    pub fn collect_encoded_rpus(rpus: impl Iterator<Item = Result<Vec<u8>>>) -> Vec<Vec<u8>> {
+        rpus.filter_map(Result::ok).collect()
     }
 
     pub fn write_rpus<P: AsRef<Path>>(&self, path: P) -> Result<()> {

dolby_vision/src/rpu/utils.rs

@@ -93,7 +93,7 @@ pub fn parse_rpu_file<P: AsRef<Path>>(input: P) -> Result<Vec<DoviRpu>> {
             .filter_map(|(i, res)| {
                 if let Err(e) = &res {
                     if warning_error.is_none() {
-                        warning_error = Some(format!("Found invalid RPU: Index {i}, error: {e}"))
+                        warning_error = Some(format!("Found invalid RPU: Index {i}\n  {e:#}"))
                     }
                 }
 
@@ -126,7 +126,7 @@ pub fn parse_rpu_file<P: AsRef<Path>>(input: P) -> Result<Vec<DoviRpu>> {
     } else if offsets_count == 0 {
         bail!("No RPU found");
     } else if let Some(error) = warning_error {
-        bail!("{}", error);
+        bail!("{error}");
     } else {
         bail!(
             "Number of valid RPUs different from total: expected {} got {}",

dolby_vision/src/rpu/vdr_dm_data.rs

@@ -1,4 +1,4 @@
-use anyhow::{Result, bail, ensure};
+use anyhow::{Context, Result, bail, ensure};
 use bitvec_helpers::{
     bitstream_io_reader::BsIoSliceReader, bitstream_io_writer::BitstreamIoWriter,
 };
@@ -96,12 +96,16 @@ pub(crate) fn vdr_dm_data_payload(
         VdrDmData::parse(reader)?
     };
 
-    if let Some(cmv29_dm_data) = DmData::parse::<CmV29DmData>(reader)? {
+    if let Some(cmv29_dm_data) =
+        DmData::parse::<CmV29DmData>(reader).with_context(|| CmV29DmData::VERSION)?
+    {
         vdr_dm_data.cmv29_metadata = Some(DmData::V29(cmv29_dm_data));
     }
 
     if reader.available()? >= DM_DATA_PAYLOAD2_MIN_BITS {
-        if let Some(cmv40_dm_data) = DmData::parse::<CmV40DmData>(reader)? {
+        if let Some(cmv40_dm_data) =
+            DmData::parse::<CmV40DmData>(reader).with_context(|| CmV40DmData::VERSION)?
+        {
             vdr_dm_data.cmv40_metadata = Some(DmData::V40(cmv40_dm_data));
         }
     }
@@ -178,11 +182,11 @@ impl VdrDmData {
         }
 
         if let Some(cmv29) = &self.cmv29_metadata {
-            cmv29.validate()?;
+            cmv29.validate().with_context(|| CmV29DmData::VERSION)?;
         }
 
         if let Some(cmv40) = &self.cmv40_metadata {
-            cmv40.validate()?;
+            cmv40.validate().with_context(|| CmV40DmData::VERSION)?;
         }
 
         Ok(())
@@ -234,11 +238,11 @@ impl VdrDmData {
         }
 
         if let Some(cmv29) = &self.cmv29_metadata {
-            cmv29.write(writer)?;
+            cmv29.write(writer).with_context(|| CmV29DmData::VERSION)?;
         }
 
         if let Some(cmv40) = &self.cmv40_metadata {
-            cmv40.write(writer)?;
+            cmv40.write(writer).with_context(|| CmV40DmData::VERSION)?;
         }
 
         Ok(())
@@ -315,9 +319,9 @@ impl VdrDmData {
 
         if let Some(dm_data) = self.extension_metadata_for_level_mut(level) {
             match dm_data {
-                DmData::V29(meta) => meta.add_block(block)?,
-                DmData::V40(meta) => meta.add_block(block)?,
-            }
+                DmData::V29(meta) => meta.add_block(block).with_context(|| CmV29DmData::VERSION),
+                DmData::V40(meta) => meta.add_block(block).with_context(|| CmV40DmData::VERSION),
+            }?
         }
 
         Ok(())

src/dovi/editor.rs

@@ -151,7 +151,18 @@ impl Editor {
 
         config.execute(&mut rpus)?;
 
-        let mut data = GenerateConfig::encode_option_rpus(&mut rpus);
+        let mut warned = false;
+        let mut data = GenerateConfig::encode_option_rpus(&rpus)
+            .enumerate()
+            .filter_map(|(i, res)| {
+                if !warned && let Err(err) = &res {
+                    warned = true;
+                    println!("Failed writing invalid RPU: Index {i}\n  {err:#}");
+                }
+
+                res.ok()
+            })
+            .collect();
 
         if let Some(to_duplicate) = config.duplicate.as_mut() {
             to_duplicate.sort_by_key(|meta| meta.offset);