Continue discussion on #239 (Can `Mat` be `Sync`?)

[jerry73204]

Sorry to bring the issue #239 back. I'd like to share some thoughts on this issue.

The roi() or range() is unsound because it introduces potential data races regardless of Sync. We can let roi() to borrow the owning Mat to solve this.

let owner: Mat = /* wthatever */;
let roi: MatRef<'_> = Mat::roi(&owner, rect)?;

I see this solution is unfriendly to multi-thread programming. The borrow checker complains lifetime not enough if you send the borrowing ROI to a thread. It can be worked around by special containers from ownref, owning_ref or detangled lifetime from qcell. Unfortunately, boilerplate code is unavoidable.

use ownref::ArcOwnedC;

let owner: ArcOwnedC<Mat> = ArcOwnedC::new(/* wthatever */);
let roi1: ArcOwnedC<Mat, MatRef<'_>> = owner.clone().try_map(|owner: Mat| Mat::roi(&owner, rect1))?;
let roi2: ArcOwnedC<Mat, MatRef<'_>> = owner.clone().try_map(|owner: Mat| Mat::roi(&owner, rect2))?;

Another solution is to have a read-only and auto reference-counted Mat. It can safely creating read-only owned ROIs. However, it's not possible to partition the Mat to mutable pieces and let threads to write data on each piece. This implementation is basically re-inventing what ownref::ArcOwnedC has done.

let owner: Mat = /* whatever */;
let ro_owner: MatReadOnly = owner.into_read_only();
let roi1 = ro.roi(rect1);
let roi2 = ro.roi(rect2);

To safely divide a Mat into exclusive and mutable ROIs, we may have a slice::split_mut-like method. The implementation involves non-trivial range checks.

My opinion is to make things simple: Mark the roi() and similar methods to be unsafe and provide .roi_ref() and .ref_mut() returning borrowed types. Let the user to choose to go unsafe or write boilerplate code, and we can give Sync back.

[twistedfall]

Thanks for the detailed proposal! It might be a way forward indeed. What's your thoughts on the following? After calling owner.into_read_only() I currently see no way to go back to the mutable Mat. You can't reliably call something like ro_owner.into_mutable() because you don't really know how many other read-only references were created from calling roi() or similar function.

[jerry73204]

owner.into_read_only() works similarly to Arc::new. It should has something like Arc::try_unwrap to return to mutable state. It would involves runtime check and we have to define an ArcMat with roi methods disabled. IMO, I would let roi() follows the safety rules and define the ArcMat based upon the safe API.

This is an implementation example of ArcMat using safe roi() and owning_ref.

use owning_ref::ArcRef;

struct ArcMat {
    arc: ArcRef<Mat>
}

impl Mat {
    pub fn into_arc(self) -> ArcMat {
        ArcMat { arc: ArcRef::new(self) }
    }

    // The roi() borrows the data
    pub fn roi<'a>(&'a self, rect: Rect) -> MatRef<'a> {
        unsafe {
            MatRef {
            mat: self.raw_roi(rect)
            _phantom: PhantomData::<&'a Mat>,
        }
    }
}

impl ArcMat {
    pub fn try_unwrap(arc: Self) -> Option<Mat> {
        Arc::try_unwrap(arc.into_owner())
    }

    // Override roi() so that it returns ref-counted Mat
    pub fn roi(&self) -> ArcMat {
        ArcMat {
            arc: self.arc.clone().map(|mat| &mat.roi().mat)
        }
    }
}

The usage goes like this.

let mat: Mat = whatever();
let roi: &Mat = mat.roi(rect);  // roi by borrowing

let arc: ArcMat = mat.into_arc();  // convert to ref-counted type

let _: &Mat = &*arc;  // dereference to Mat
let roi: ArcMat = arc.roi(rect);  // roi by ref-count

// return to mutable state
drop(roi);
let mat: Mat = ArcMat::try_unwrap(arc).unrwap();