AVBitStreamFilter

[FallingSnow]

I'm trying to implement AVBitStreamFilter in this library. I have a little code so far but I'm not sure if I'm working in the right direction. AVBitStreamFilter was easy but AVBSFContext has a bit of a strange allocation method and I couldn't find another example like it in your code.

Would you mind taking a look and letting me know if I'm doing anything wrong? This is what I have so far.

use std::{ffi::CStr, mem::MaybeUninit, ptr::NonNull};

use crate::{
    error::{Result, RsmpegError},
    ffi,
    shared::PointerUpgrade,
};

wrap!(AVBitStreamFilter: ffi::AVBitStreamFilter);

impl AVBitStreamFilter {
    /// Find a bitstream filter instance with it's short name.
    pub fn find_decoder_by_name(name: &CStr) -> Option<AVBitStreamFilter> {
        unsafe { ffi::av_bsf_get_by_name(name.as_ptr()) }
            .upgrade()
            .map(|x| unsafe { AVBitStreamFilter::from_raw(x) })
    }
}

wrap!(AVBSFContext: ffi::AVBSFContext);

impl AVBSFContext {
    /// Create a new [`AVBSFContext`] instance, allocate private data and
    /// initialize defaults for the given [`AVBitStreamFilter`].
    pub fn new(filter: &AVBitStreamFilter) -> Result<Self> {
        let mut bsfc_raw: MaybeUninit<ffi::AVBSFContext> = MaybeUninit::uninit();

        match unsafe { ffi::av_bsf_alloc(filter.as_ptr(), &mut bsfc_raw.as_mut_ptr()) } {
            0 => {
                let bsfc = unsafe {
                    AVBSFContext::from_raw(NonNull::new(&mut bsfc_raw.assume_init()).unwrap())
                };
                Ok(bsfc)
            }
            e => Err(RsmpegError::AVError(e)),
        }
    }
}

[ldm0]

Awesome! Thanks for contribution! Finally got someone understands how the macro works 😄 .

There are several small problems:

First, you can use wrap_ref!(AVBitStreamFilter: ffi::AVBitStreamFilter) and change the function signature of find bsfilter like this: pub fn find_decoder_by_name(name: &CStr) -> Option<AVBitStreamFilterRef>. Because this function doesn't return by value, but by static reference. Just like how the AVFilter is implemented:

Second, you shouldn't need the MaybeUninit<_>, the av_bsf_alloc takes the bsfc_raw as return value. So a ptr::null_mut() is enough.

Third, you can simply unwrap() after upgrade() the av_bsf_alloc's return value. We don't need to capture error here because this function only fails on no memory(AVERROR(NOMEM)). And in rsmpeg we don't need to care about the OOM error. Then the function signature can be changed to pub fn new(filter: &AVBitStreamFilter) -> Self. Get simpler!

Last but not least, don't forget to implement Drop 😄 .

Looking forward to your Pull Request.

[FallingSnow]

Thanks!

Second, you shouldn't need the MaybeUninit<_>, the av_bsf_alloc takes the bsfc_raw as return value. So a ptr::null_mut() is enough.

Ah I should have known better. I already knew that av_bsf_alloc was doing the alloc but for some reason I couldn't wrap my head around it.

And in rsmpeg we don't need to care about the OOM error.

Does this mean that the user doesn't know if an OOM error occured?

Here it is with the fixes you've suggested.

wrap_ref!(AVBitStreamFilterRef: ffi::AVBitStreamFilter);

impl AVBitStreamFilterRef {
    /// Find a bitstream filter instance with it's short name.
    pub fn find_by_name(name: &CStr) -> Option<AVBitStreamFilterRef> {
        unsafe { ffi::av_bsf_get_by_name(name.as_ptr()) }
            .upgrade()
            .map(|x| unsafe { AVBitStreamFilterRef::from_raw(x) })
    }
}

wrap!(AVBSFContext: ffi::AVBSFContext);

impl AVBSFContext {
    /// Create a new [`AVBSFContext`] instance, allocate private data and
    /// initialize defaults for the given [`AVBitStreamFilterRef`].
    pub fn new(filter: &AVBitStreamFilterRef) -> Self {
        let bsfc_raw = ptr::null_mut();

        unsafe {
            let filter_ptr = filter.as_ptr();
            ffi::av_bsf_alloc(filter_ptr, bsfc_raw);
            AVBSFContext::from_raw((*bsfc_raw).upgrade().unwrap())
        }
    }
}


impl Drop for AVBSFContext {
    fn drop(&mut self) {
        unsafe {
            ffi::av_bsf_free(&mut self.as_mut_ptr())
        }
    }
}

I'm gonna work a little bit more on it then I'll open a WIP pull request to fix any mistakes I might have made.

[ldm0]

Overall it looks good, here are some more suggestions:

You can change this line: pub fn new(filter: &AVBitStreamFilterRef) -> Self { to pub fn new(filter: &AVBitStreamFilter) -> Self {, because &AVBitStreamFilterRef can be autoderefed to &AVBitStreamFilter, but &AVBitStreamFilter cannot be autoderefed to &AVBitStreamFilterRef.

Overall it looks good. :-)

[ldm0]

Sorry for clicking the wrong button(

[FallingSnow]

See #23 for updates.