From 57b6e284556e8849396c33666fdec64c89b513b4 Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Fri, 12 Jan 2024 12:21:19 +0800 Subject: [PATCH] Make `AVFormatContext*::streams` return slice rather than iterator (#141) * Add attributes support for `wrap_*` macros; Make `*Mut` & `*Ref` transparent * Make `AVFormatContext*::streams` return slice rather than iterator --- .github/workflows/ci.yml | 2 + src/avformat/avformat.rs | 126 +++++++++++++++------------------------ src/macros.rs | 13 +++- tests/avio_writing.rs | 6 +- tests/remux.rs | 10 +--- tests/transcoding.rs | 6 +- 6 files changed, 65 insertions(+), 98 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 42db95f..a501f37 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -132,7 +132,9 @@ jobs: override: true - name: Install valgrind dependencies + # On Debian and Ubuntu, libc6-dbg is required for valgrind run: | + sudo apt-get update -qq sudo apt-get -y install libc6-dbg cargo install cargo-valgrind --version 2.0.2 diff --git a/src/avformat/avformat.rs b/src/avformat/avformat.rs index 3ac4b63..000e40a 100644 --- a/src/avformat/avformat.rs +++ b/src/avformat/avformat.rs @@ -1,6 +1,5 @@ use std::{ ffi::CStr, - marker::PhantomData, ops::Drop, ptr::{self, NonNull}, }; @@ -182,13 +181,31 @@ impl AVFormatContextInput { } impl<'stream> AVFormatContextInput { - /// Get Iterator of all [`AVStream`]s in the [`ffi::AVFormatContext`]. - pub fn streams(&'stream self) -> AVStreamRefs<'stream> { - AVStreamRefs { - stream_head: NonNull::new(self.streams as *mut _).unwrap(), - len: self.nb_streams, - _marker: PhantomData, + /// Return slice of [`AVStreamRef`]. + pub fn streams(&'stream self) -> &'stream [AVStreamRef<'stream>] { + // #define `<->` as "has the same layout due to repr(transparent)" + // ``` + // NonNull <-> *const ffi::AVStream + // AVStream <-> NonNull + // AVStreamRef <-> AVStream + // ``` + // indicates: AVStreamRef <-> *const ffi::AVStream + let streams = self.streams as *const *const ffi::AVStream as *const AVStreamRef<'stream>; + // u32 to usize, safe + let len = self.nb_streams as usize; + + // I trust that FFmpeg won't give me null pointers :-( + #[cfg(debug_assertions)] + { + let arr = unsafe { + std::slice::from_raw_parts(self.streams as *const *const ffi::AVStream, len) + }; + for ptr in arr { + assert!(!ptr.is_null()); + } } + + unsafe { std::slice::from_raw_parts(streams, len) } } /// Get [`AVInputFormatRef`] in the [`AVFormatContextInput`]. @@ -344,13 +361,31 @@ impl AVFormatContextOutput { } impl<'stream> AVFormatContextOutput { - /// Return Iterator of [`AVStreamRef`]. - pub fn streams(&'stream self) -> AVStreamRefs<'stream> { - AVStreamRefs { - stream_head: NonNull::new(self.streams as *mut _).unwrap(), - len: self.nb_streams, - _marker: PhantomData, + /// Return slice of [`AVStreamRef`]. + pub fn streams(&'stream self) -> &'stream [AVStreamRef<'stream>] { + // #define `<->` as "has the same layout due to repr(transparent)" + // ``` + // NonNull <-> *const ffi::AVStream + // AVStream <-> NonNull + // AVStreamRef <-> AVStream + // ``` + // indicates: AVStreamRef <-> *const ffi::AVStream + let streams = self.streams as *const *const ffi::AVStream as *const AVStreamRef<'stream>; + // u32 to usize, safe + let len = self.nb_streams as usize; + + // I trust that FFmpeg won't give me null pointers :-( + #[cfg(debug_assertions)] + { + let arr = unsafe { + std::slice::from_raw_parts(self.streams as *const *const ffi::AVStream, len) + }; + for ptr in arr { + assert!(!ptr.is_null()); + } } + + unsafe { std::slice::from_raw_parts(streams, len) } } /// Get [`AVOutputFormat`] from the [`AVFormatContextOutput`]. @@ -429,7 +464,7 @@ impl AVOutputFormat { } } -wrap_ref_mut!(AVStream: ffi::AVStream); +wrap_ref_mut!(#[repr(transparent)] AVStream: ffi::AVStream); settable!(AVStream { time_base: AVRational, }); @@ -505,69 +540,6 @@ impl<'stream> AVStream { } } -/// Iterator on reference to raw AVStream `satellite` array. -pub struct AVStreamRefsIter<'stream> { - ptr: NonNull>, - end: NonNull>, - _marker: PhantomData<&'stream ffi::AVStream>, -} - -impl<'stream> std::iter::Iterator for AVStreamRefsIter<'stream> { - type Item = AVStreamRef<'stream>; - fn next(&mut self) -> Option { - if self.ptr == self.end { - None - } else { - let old = self.ptr; - unsafe { - self.ptr = NonNull::new_unchecked(self.ptr.as_ptr().offset(1)); - } - Some(unsafe { AVStreamRef::from_raw(*old.as_ref()) }) - } - } -} - -// ATTENTION Consider add macro for this when similar pattern occurs again. -/// A reference to raw AVStream `satellite` array, cannot be directly constructed. Using -/// this for safety concerns. -pub struct AVStreamRefs<'stream> { - stream_head: NonNull>, - len: u32, - _marker: PhantomData<&'stream ffi::AVStream>, -} - -impl<'stream> std::iter::IntoIterator for AVStreamRefs<'stream> { - type Item = AVStreamRef<'stream>; - type IntoIter = AVStreamRefsIter<'stream>; - fn into_iter(self) -> Self::IntoIter { - let end = - NonNull::new(unsafe { self.stream_head.as_ptr().add(self.len as usize) }).unwrap(); - AVStreamRefsIter { - ptr: self.stream_head, - end, - _marker: PhantomData, - } - } -} - -impl<'stream> AVStreamRefs<'stream> { - /// Get `streams[`index`]`. - pub fn get(&self, index: usize) -> Option> { - if index < self.num() { - let stream_ptr = unsafe { *self.stream_head.as_ptr().add(index) }; - Some(unsafe { AVStreamRef::from_raw(stream_ptr) }) - } else { - None - } - } - - /// Get `streams.len()`. - pub fn num(&self) -> usize { - // From u32 to usize, safe. - self.len as usize - } -} - #[cfg(test)] mod test { use super::*; diff --git a/src/macros.rs b/src/macros.rs index c1ef316..855c685 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -4,9 +4,11 @@ /// Wrapping with XXX -> XXX mapping. macro_rules! wrap_pure { ( + $(#[$meta:meta])* ($wrapped_type: ident): $ffi_type: ty $(,$attach: ident: $attach_type: ty = $attach_default: expr)* ) => { + $(#[$meta])* pub struct $wrapped_type { something_should_not_be_touched_directly: std::ptr::NonNull<$ffi_type>, // Publicize the attachment, can be directly changed without deref_mut() @@ -72,6 +74,7 @@ macro_rules! wrap_pure { macro_rules! wrap_ref_pure { (($wrapped_type: ident, $wrapped_ref: ident): $ffi_type: ty) => { // This is needed for wrapping reference owned value from ffi + #[repr(transparent)] pub struct $wrapped_ref<'a> { inner: std::mem::ManuallyDrop<$wrapped_type>, _marker: std::marker::PhantomData<&'a $wrapped_type>, @@ -109,6 +112,7 @@ macro_rules! wrap_ref_pure { macro_rules! wrap_mut_pure { (($wrapped_type: ident, $wrapped_mut: ident): $ffi_type: ty) => { // This is needed for wrapping mutable reference owned value from ffi + #[repr(transparent)] pub struct $wrapped_mut<'a> { inner: std::mem::ManuallyDrop<$wrapped_type>, _marker: std::marker::PhantomData<&'a $wrapped_type>, @@ -151,11 +155,12 @@ macro_rules! wrap_mut_pure { /// Wrapping with XXXRef, XXXMut, XXX -> XXX. macro_rules! wrap_ref_mut { ( + $(#[$meta:meta])* $name: ident: $ffi_type: ty $(,$attach: ident: $attach_type: ty = $attach_default: expr)* $(,)? ) => { paste::paste! { - wrap_pure!(($name): $ffi_type $(,$attach: $attach_type = $attach_default)*); + wrap_pure!($(#[$meta])* ($name): $ffi_type $(,$attach: $attach_type = $attach_default)*); wrap_ref_pure!(($name, [<$name Ref>]): $ffi_type); wrap_mut_pure!(($name, [<$name Mut>]): $ffi_type); } @@ -165,11 +170,12 @@ macro_rules! wrap_ref_mut { /// Wrapping with XXXRef, XXX -> XXX. macro_rules! wrap_ref { ( + $(#[$meta:meta])* $name: ident: $ffi_type: ty $(,$attach: ident: $attach_type: ty = $attach_default: expr)* $(,)? ) => { paste::paste! { - wrap_pure!(($name): $ffi_type $(,$attach: $attach_type = $attach_default)*); + wrap_pure!($(#[$meta])* ($name): $ffi_type $(,$attach: $attach_type = $attach_default)*); wrap_ref_pure!(($name, [<$name Ref>]): $ffi_type); } }; @@ -178,11 +184,12 @@ macro_rules! wrap_ref { /// Wrapping with XXXMut, XXX -> XXX. macro_rules! wrap_mut { ( + $(#[$meta:meta])* $name: ident: $ffi_type: ty $(,$attach: ident: $attach_type: ty = $attach_default: expr)* $(,)? ) => { paste::paste! { - wrap_pure!(($name): $ffi_type $(,$attach: $attach_type = $attach_default)*); + wrap_pure!($(#[$meta])* ($name): $ffi_type $(,$attach: $attach_type = $attach_default)*); wrap_mut_pure!(($name, [<$name Mut>]): $ffi_type); } }; diff --git a/tests/avio_writing.rs b/tests/avio_writing.rs index e26e76c..731d9e2 100644 --- a/tests/avio_writing.rs +++ b/tests/avio_writing.rs @@ -146,11 +146,7 @@ fn encode_write_frame( packet.set_stream_index(out_stream_index as i32); packet.rescale_ts( encode_context.time_base, - output_format_context - .streams() - .get(out_stream_index) - .unwrap() - .time_base, + output_format_context.streams()[out_stream_index].time_base, ); match output_format_context.interleaved_write_frame(&mut packet) { diff --git a/tests/remux.rs b/tests/remux.rs index 8bd3b23..dc96cdb 100644 --- a/tests/remux.rs +++ b/tests/remux.rs @@ -67,14 +67,8 @@ fn remux(input_path: &CStr, output_path: &CStr) -> Result<()> { continue; }; { - let input_stream = input_format_context - .streams() - .get(input_stream_index) - .unwrap(); - let output_stream = output_format_context - .streams() - .get(output_stream_index) - .unwrap(); + let input_stream = &input_format_context.streams()[input_stream_index]; + let output_stream = &output_format_context.streams()[output_stream_index]; log_packet(input_stream.time_base, &packet, "in"); packet.rescale_ts(input_stream.time_base, output_stream.time_base); packet.set_stream_index(output_stream_index as i32); diff --git a/tests/transcoding.rs b/tests/transcoding.rs index 44cbfe7..a102390 100644 --- a/tests/transcoding.rs +++ b/tests/transcoding.rs @@ -309,11 +309,7 @@ fn encode_write_frame( packet.set_stream_index(out_stream_index as i32); packet.rescale_ts( encode_context.time_base, - output_format_context - .streams() - .get(out_stream_index) - .unwrap() - .time_base, + output_format_context.streams()[out_stream_index].time_base, ); match output_format_context.interleaved_write_frame(&mut packet) {