refactor: mp4 #442
refactor: mp4 #442
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #442 +/- ##
==========================================
- Coverage 61.48% 57.17% -4.31%
==========================================
Files 385 359 -26
Lines 31171 30376 -795
==========================================
- Hits 19165 17368 -1797
- Misses 12006 13008 +1002
... and 58 files with indirect coverage changes
🚀 New features to boost your workflow:
|
lennartkloock
force-pushed
the
lennart/CLOUD-31
branch
4 times, most recently
from
b641b8c to
f76aaa9
Compare
| /// | ||
| /// The caller must ensure that the length of the bytes is less than or equal to 8, | ||
| /// otherwise this function will panic. | ||
| pub fn pad_to_u64_be(&self) -> u64 { |
There was a problem hiding this comment.
Why should these functions be here?
There was a problem hiding this comment.
They are needed to pad an unknown number of bytes to an u32/u64. I thought they are useful so I made them part of the bytes-util crate.
| use crate::{BytesCow, StringCow}; | ||
|
|
||
| /// A trait that should be implemented by types that can contain other deserializable types. | ||
| pub trait Container { |
There was a problem hiding this comment.
Can you explain what this is used for?
There was a problem hiding this comment.
It's trait which is implemented for all containers of boxes (Option and Vec). It is used in the derive macro to figure out the contained type.
|
|
||
| impl<'a> Deserialize<'a> for f32 { | ||
| fn deserialize<R: ZeroCopyReader<'a>>(mut reader: R) -> io::Result<Self> { | ||
| reader.as_std().read_f32::<byteorder::BigEndian>() |
There was a problem hiding this comment.
I am not really a fan of deciding that all these number types should always be BigEndian.
I think, a better approach is to add a new type that is BigEndian<T> and LittleEndian<T>.
There was a problem hiding this comment.
Does that apply to other number types too?
|
|
||
| /// A 24-bit signed integer in big-endian byte order. | ||
| #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] | ||
| pub struct I24Be(pub i32); |
There was a problem hiding this comment.
I think this is a bad type, i think if you want to create an i24 type we should first reach for something like this i24. This should be feature gated as i24
|
|
||
| /// A 48-bit signed integer in big-endian byte order. | ||
| #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] | ||
| pub struct I48Be(pub i64); |
There was a problem hiding this comment.
Similar to i24 we should also use i48. This should be feature gated as i48
|
|
||
| /// A 24-bit unsigned integer in big-endian byte order. | ||
| #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] | ||
| pub struct U24Be(pub u32); |
There was a problem hiding this comment.
There doesnt seem to be a unsized version of a 24 bit integer crate in the wild.
I created this issue jmg049/i24#14.
|
|
||
| /// A 48-bit unsigned integer in big-endian byte order. | ||
| #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] | ||
| pub struct U48Be(pub u64); |
There was a problem hiding this comment.
Like the u24, no seemingly premade u48 number type. Chubercik/i48#5
| 111, | ||
| 109, | ||
| ], | ||
| UnknownBox { |
There was a problem hiding this comment.
Firstly, this seems to not find the ftyp box which is strange, and more alarmingly it stops after failing to decode that box. There are other boxes here which are not decoded.
| #[iso_box(nested_box)] | ||
| pub config: AVCConfigurationBox<'a>, | ||
| /// The optional MPEG-4 extension descriptors box contained in this box. | ||
| #[iso_box(nested_box(collect))] |
There was a problem hiding this comment.
not a fan, ideally we should just be able to say this is a nested_box without adding the collect part, likewise we should be able to do this for vecs without collect.
| match nested { | ||
| IsoBoxFieldNestedBox::Single => { | ||
| fields_in_self.push(quote! { | ||
| #field_name: ::std::option::Option::ok_or(#field_name, ::std::io::Error::new(::std::io::ErrorKind::InvalidData, format!("{} not found", #field_name_str)))? | ||
| }); | ||
| field_serializers.push(quote! { | ||
| #crate_path::reexports::scuffle_bytes_util::zero_copy::Serialize::serialize(&self.#field_name, &mut writer)?; | ||
| }); | ||
| } | ||
| IsoBoxFieldNestedBox::Collect | IsoBoxFieldNestedBox::CollectUnknown => { | ||
| fields_in_self.push(field_name.to_token_stream()); | ||
| field_serializers.push(quote! { | ||
| #[allow(for_loops_over_fallibles)] | ||
| for item in &self.#field_name { | ||
| #crate_path::reexports::scuffle_bytes_util::zero_copy::Serialize::serialize(item, &mut writer)?; | ||
| } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
I suspect this can be simplified to be handled by the trait objects instead of generating an iterator.
| /// | ||
| /// ISO/IEC 14496-12 - 12.2.2 | ||
| #[derive(IsoBox, Debug, PartialEq, Eq, Default)] | ||
| #[iso_box(box_type = b"smhd", skip_impl(deserialize_seed, serialize, sized), crate_path = crate)] |
There was a problem hiding this comment.
Why even bother with the box impl anyways why cant this box be done with macro?
There was a problem hiding this comment.
Serialize and Deserialize are not implemented for the fixed point numbers. They should be.
| pub loudness_bases: Vec<LoudnessBase>, | ||
| } | ||
|
|
||
| impl<'a> DeserializeSeed<'a, &FullBoxHeader> for LoudnessBaseBox { |
There was a problem hiding this comment.
Why does this take the FullBoxHeader by reference?
There was a problem hiding this comment.
It's the FullBoxHeader of the parent box. Some values in this box depend on the version that is stored in the FullBoxHeader.
| /// | ||
| /// ISO/IEC 14996-12 - 8.13.2 | ||
| #[derive(IsoBox, Debug, PartialEq, Eq)] | ||
| #[iso_box(box_type = b"fiin", skip_impl(deserialize_seed, serialize), crate_path = crate)] |
There was a problem hiding this comment.
What can we do here to make this implementable with the macro?
There was a problem hiding this comment.
The number of items in the partition_entries vec depends on the value of entry_count. I don't think it's easily possible to model that in the macro.
| #[iso_box(box_type = b"fiin", skip_impl(deserialize_seed, serialize), crate_path = crate)] | ||
| pub struct FDItemInformationBox { | ||
| pub full_header: FullBoxHeader, | ||
| pub entry_count: u16, |
There was a problem hiding this comment.
hmm, i dont really like this field.
I understand its part of the "spec", but its really just a length delimiter for the vector below. Perhaps we can remove it and infer it from the vector instead.
Perhaps a
#[iso_box(repeated(u16))]
pub partition_entries: Vec<PartitionEntry>,annotation which then tells the decoder to first decode a u16 which then tells it how many iterations to perform when doing decode.
codegen could look something like this
let tmp_length = <$ty as Deserialize>::deserialize(r)? as usize;
let partition_entries = (0..tmp_length).map(|_| Deserialize::deserialize(r)).collect::<Result<_, _>>()?;that way we generalize over anything that implements collect...
| where | ||
| R: scuffle_bytes_util::zero_copy::ZeroCopyReader<'a>, | ||
| { | ||
| let full_header = FullBoxHeader::deserialize(&mut reader)?; |
There was a problem hiding this comment.
This seems wrong, why wouldnt the fullbox header first take the box header?
There was a problem hiding this comment.
It's documented here: https://pr-442.scuffle-docrs.pages.dev/isobmff/struct.FullBoxHeader
| #[derive(IsoBox, PartialEq, Eq)] | ||
| #[iso_box(box_type = b"mdat", crate_path = crate)] | ||
| pub struct MediaDataBox<'a> { | ||
| pub data: BytesCow<'a>, | ||
| } |
There was a problem hiding this comment.
doesnt the mdat box have a box header?
There was a problem hiding this comment.
It does, as every other box. The macro takes care of it.
TroyKomodo
force-pushed
the
lennart/CLOUD-31
branch
6 times, most recently
from
7174e79 to
a8422c5
Compare
lennartkloock
force-pushed
the
lennart/CLOUD-31
branch
from
53b6b4e to
502ed6c
Compare
🚀 Preview Deployments
|
lennartkloock
force-pushed
the
lennart/CLOUD-31
branch
from
8db2832 to
6af1d1d
Compare
lennartkloock
force-pushed
the
lennart/CLOUD-31
branch
from
6af1d1d to
af8d0fa
Compare
This splits the original
scuffle-mp4crate into multiple crates:isobmffwhich contains everything described by ISO/IEC 14496-12scuffle-h264now contains the AVC boxes described by ISO/IEC 14496-15scuffle-h265now contains the HEVC boxes described by ISO/IEC 14496-15scuffle-av1now contains the AV1 boxes described by AV1 Codec ISO Media File Format Bindingscuffle-opuswhich contains the Opus boxes described by Encapsulation of Opus in ISO Base Media File Formatscuffle-mp4crate now only contains the actual MP4 boxes (not ISOBMFF) described by ISO/IEC 14496-14Other changes:
isobmff::IsoBox, zero-copyDeserializeand zero-copySerializetraitsNote for reviewers:
isobmff::boxescontains a lot of code that can only be reviewed by going through the spec step by step. Maybe start with other modules first.CLOUD-31 CLOUD-88