diff --git a/dolby_vision/CHANGELOG.md b/dolby_vision/CHANGELOG.md index df1d071..0931fd9 100644 --- a/dolby_vision/CHANGELOG.md +++ b/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. diff --git a/dolby_vision/src/rpu/extension_metadata/blocks/mod.rs b/dolby_vision/src/rpu/extension_metadata/blocks/mod.rs index eac6e35..f51a0ea 100644 --- a/dolby_vision/src/rpu/extension_metadata/blocks/mod.rs +++ b/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") ); } diff --git a/dolby_vision/src/rpu/extension_metadata/cmv29.rs b/dolby_vision/src/rpu/extension_metadata/cmv29.rs index 16b1707..0fcce27 100644 --- a/dolby_vision/src/rpu/extension_metadata/cmv29.rs +++ b/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(()) diff --git a/dolby_vision/src/rpu/extension_metadata/cmv40.rs b/dolby_vision/src/rpu/extension_metadata/cmv40.rs index f0a78aa..91f098b 100644 --- a/dolby_vision/src/rpu/extension_metadata/cmv40.rs +++ b/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(()) diff --git a/dolby_vision/src/rpu/extension_metadata/mod.rs b/dolby_vision/src/rpu/extension_metadata/mod.rs index 5da6b12..86940ac 100644 --- a/dolby_vision/src/rpu/extension_metadata/mod.rs +++ b/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 { diff --git a/dolby_vision/src/rpu/generate.rs b/dolby_vision/src/rpu/generate.rs index 0aab85a..8ab5f06 100644 --- a/dolby_vision/src/rpu/generate.rs +++ b/dolby_vision/src/rpu/generate.rs @@ -185,19 +185,21 @@ impl GenerateConfig { Ok(list) } - pub fn encode_option_rpus(rpus: &mut [Option]) -> Vec> { - 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]) -> impl Iterator>> { + rpus.iter() + .filter_map(|rpu| rpu.as_ref().map(|e| e.write_hevc_unspec62_nalu())) } - pub fn encode_rpus(rpus: &mut [DoviRpu]) -> Vec> { - 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>> { + 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>>) -> Vec> { + rpus.filter_map(Result::ok).collect() } pub fn write_rpus>(&self, path: P) -> Result<()> { diff --git a/dolby_vision/src/rpu/utils.rs b/dolby_vision/src/rpu/utils.rs index 48e6d22..03fc171 100644 --- a/dolby_vision/src/rpu/utils.rs +++ b/dolby_vision/src/rpu/utils.rs @@ -93,7 +93,7 @@ pub fn parse_rpu_file>(input: P) -> Result> { .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>(input: P) -> Result> { } 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 {}", diff --git a/dolby_vision/src/rpu/vdr_dm_data.rs b/dolby_vision/src/rpu/vdr_dm_data.rs index 03df8f2..29a0158 100644 --- a/dolby_vision/src/rpu/vdr_dm_data.rs +++ b/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::(reader)? { + if let Some(cmv29_dm_data) = + DmData::parse::(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::(reader)? { + if let Some(cmv40_dm_data) = + DmData::parse::(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(()) diff --git a/src/dovi/editor.rs b/src/dovi/editor.rs index afe3421..24eedb4 100644 --- a/src/dovi/editor.rs +++ b/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); diff --git a/src/tests/rpu.rs b/src/tests/rpu.rs index 2b6d197..28cedf8 100644 --- a/src/tests/rpu.rs +++ b/src/tests/rpu.rs @@ -464,10 +464,10 @@ fn cmv40_full_rpu() -> Result<()> { ..Default::default() }); - let mut rpus = config.generate_rpu_list()?; + let rpus = config.generate_rpu_list()?; assert_eq!(rpus.len(), config.length); - let encoded_rpus = GenerateConfig::encode_rpus(&mut rpus); + let encoded_rpus = GenerateConfig::collect_encoded_rpus(GenerateConfig::encode_rpus(&rpus)); assert_eq!(encoded_rpus.len(), config.length); let vdr_dm_data = rpus[0].vdr_dm_data.as_ref().unwrap(); @@ -939,10 +939,10 @@ fn cmv40_full_l8_l9_l10() -> Result<()> { ..Default::default() }); - let mut rpus = config.generate_rpu_list()?; + let rpus = config.generate_rpu_list()?; assert_eq!(rpus.len(), config.length); - let encoded_rpus = GenerateConfig::encode_rpus(&mut rpus); + let encoded_rpus = GenerateConfig::collect_encoded_rpus(GenerateConfig::encode_rpus(&rpus)); assert_eq!(encoded_rpus.len(), config.length); let vdr_dm_data = rpus[0].vdr_dm_data.as_ref().unwrap(); diff --git a/tests/rpu/info.rs b/tests/rpu/info.rs index d29f0e3..abed39b 100644 --- a/tests/rpu/info.rs +++ b/tests/rpu/info.rs @@ -93,9 +93,11 @@ fn invalid_l3_error() -> Result<()> { let assert = cmd.arg(SUBCOMMAND).arg(input_rpu).arg("-s").assert(); - assert.failure().stderr(predicate::str::contains( - "Error: Found invalid RPU: Index 0, error: Invalid block level 3 for CM v2.9 RPU", - )); + assert.failure().stderr( + predicate::str::contains("Error: Found invalid RPU: Index 0").and( + predicate::str::contains("CM v2.9: Disallowed block level 3"), + ), + ); Ok(()) }