Add beginning of FMat

[Polochon-street]

Hi,

I'm trying to add the beginning of matrices. (see https://aubio.org/doc/0.4.3/fmat_8h_source.html)

It's a bit difficult because the memory layout of matrices (just a big **data array) is not very Rust-friendly, so I've added some tests to make sure we go at least in the right direction :)

[EDIT] Still wip now

[Polochon-street]

@katyo should be ready for review now :)

I've also ran the rust formatter on the file and it changed some formatting, hope that's okay with you :grimacing:

[EDIT] Now with a non-owned version, for cases like filter_bank where it's convenient to have an FMat objet, but we don't want to Drop it ourselves

[katyo]

Excuse for long absence here.

The wrappers for data (like FVec) is a reference-like types by design. That types was introduced to simplify transfer data between aubio calls and Rust code. I don't sure the data-owning wrappers so necessary that we cannot play without it. Can you give me a use cases?

[Polochon-street]

Hi, no worries :)

I'm trying to make a non-owned version of it and still have a method to .into() a Vec<&mut [f32]> into an FMat (this is extremely useful when you want to deal with filterbanks and want to create the matrix of coefficients in Rust yourself).

Only problem I have is that I currently have this method https://github.com/katyo/aubio-rs/pull/7/files#diff-03ebbda4a6b1b68f4f6c547acada6610R449-R473 that does it, and you need to have a temporary Vec to store the data in the right shape for having a valid fmat_t, and without a Drop implementation, it leaks memory (since this Vec needs to survive the end of the function)

Would you have any suggestion? I'm a bit dry on that one :grimacing:

[EDIT] I've made a new commit with how it would like with merged stuff - see here for memory leaks: https://github.com/katyo/aubio-rs/pull/7/files#diff-03ebbda4a6b1b68f4f6c547acada6610R404-R410

[katyo]

It seems I found simple solution. I added extra type parameter to FMat which may holds owned data.

/**
 * Immutable matrix of real valued data.
 */
#[repr(C)]
pub struct FMat<'a, X> {
    fmat: ffi::fmat_t,
    _x: X,
    _pd: PhantomData<&'a ()>,
}

impl<'a, X> FMat<'a, X> {
    pub(crate) fn as_ptr(&'a self) -> *const ffi::fmat_t {
        &self.fmat
    }

    pub fn length(&self) -> usize {
        self.fmat.length as usize
    }

    pub fn height(&self) -> usize {
        self.fmat.height as usize
    }

    pub fn print(&self) {
        unsafe { ffi::fmat_print(self.as_ptr()) }
    }

    /// Read sample value in a buffer
    pub fn get_sample(&self, channel: usize, position: usize) -> Result<f32> {
        if channel >= self.height() || position >= self.length() {
            return Err(Error::InvalidArg);
        }
        Ok(unsafe {
            ffi::fmat_get_sample(
                self.as_ptr(),
                channel as ffi::uint_t,
                position as ffi::uint_t,
            )
        })
    }

    pub fn get_vec(&self) -> Vec<&mut [f32]> {
        let mut vec = Vec::with_capacity(self.height());
        let mut ptr = self.fmat.data;
        let end = self.fmat.data.wrapping_add(self.height());

        while ptr != end {
            vec.push(unsafe { slice::from_raw_parts_mut(*ptr, self.length()) });
            ptr = ptr.wrapping_add(1);
        }
        vec
    }
}

impl<'a> FMat<'a, ()> {
    /**
     * Create a matrix from an already existing `fmat_t` pointer.
     *
     * The matrix is non-owned; useful to avoid double-frees for `FilterBank`,
     * for instance.
     */
    pub unsafe fn from_raw_ptr(ptr: *const ffi::fmat_t) -> Self {
        FMat {
            fmat: *ptr,
            _x: (),
            _pd: PhantomData,
        }
    }
}

pub type FMatVecs = Vec<*const f32>;

impl<'a, T: AsRef<[&'a [f32]]>> From<T> for FMat<'a, FMatVecs> {
    /**
     * Create a matrix from a `FVecSet`
     *
     * Matrix's horizontal height is the `Vec`'s len, and
     * its vertical length the slice's len.
     */
    fn from(data: T) -> Self {
        let data = data.as_ref();

        #[cfg(feature = "check-size")]
        {
            let mut vecs = data.iter();
            if let Some(fst) = vecs.next() {
                let len = fst.len();
                if len == 0 {
                    panic!("No values in slice");
                }
                if vecs.any(|nxt| nxt.len() != len) {
                    panic!("Slices has different lengthes");
                }
            } else {
                panic!("No slices in set");
            }
        }

        let array = data.iter().map(|v| v.as_ptr()).collect::<Vec<_>>();

        Self {
            fmat: ffi::fmat_t {
                height: data.len() as _,
                length: data[0].len() as _,
                data: array.as_ptr() as _,
            },
            _x: array,
            _pd: PhantomData,
        }
    }
}

Also I modified tests:

#[cfg(test)]
mod test {
    use super::*;

    #[test]
    #[should_panic]
    fn test_from_fmat_wrong_size() {
        let x: &[&[f32]] = &[&[1.0, 2.0], &[4.0, 5.0, 6.0], &[7.0, 8.0, 9.0]];
        let _fmat: FMat<_> = x.into();
    }

    #[test]
    fn test_from_fmat() {
        let x: &[&[f32]] = &[&[1.0, 2.0], &[4.0, 5.0], &[7.0, 8.0]];
        let fmat: FMat<_> = x.into();
        assert_eq!(2, fmat.length());
        assert_eq!(3, fmat.height());

        assert_eq!(1., fmat.get_sample(0, 0).unwrap());
        assert_eq!(2., fmat.get_sample(0, 1).unwrap());
        assert_eq!(4., fmat.get_sample(1, 0).unwrap());
        assert_eq!(5., fmat.get_sample(1, 1).unwrap());
        assert_eq!(7., fmat.get_sample(2, 0).unwrap());
        assert_eq!(8., fmat.get_sample(2, 1).unwrap());
    }

    #[test]
    fn test_to_vec() {
        let x: &[&[f32]] = &[&[1.0, 2.0], &[4.0, 5.0], &[7.0, 8.0]];
        let fmat: FMat<_> = x.into();

        let matrix = fmat.get_vec();

        assert_eq!(matrix, vec![&[1.0, 2.0], &[4.0, 5.0], &[7.0, 8.0]]);
    }
}

As you can see we usually do not need Vec here.

[Polochon-street]

That works for me. Thanks very much for the help, I've pushed your suggestions, ran valgrind against the tests, and no memory leak has been detected. Nice!

[katyo]

LGTM. Can I merge this PR right now?

[Polochon-street]

Yup, if it looks good to you, LGTM :smile: Thanks for the feedback!