twistedfall / opencv-rust

Rust bindings for OpenCV 3 & 4
MIT License
1.96k stars 157 forks source link

Unable to cvt_color in-place #571

Closed BTOdell closed 4 months ago

BTOdell commented 5 months ago

The cvtColor function permits passing in the same Mat for both input and output. When doing so, OpenCV can efficiently convert from RGB to BGR and back without reallocating.

However, the Rust aliasing rules prevent passing in the same Mat to the cvt_color function. Is there a workaround for this limitation in the bindings?

twistedfall commented 5 months ago

I'd say this is the "limitation" of Rust itself, but yeah, there is no way to do that safely. For one of my projects I'm using the following function as a workaround:

#[inline(always)]
unsafe fn op_inplace<T>(m: &mut Mat, f: impl FnOnce(&Mat, &mut Mat) -> Result<T>) -> Result<T> {
    let mut m_alias = Mat::from_raw(m.as_raw_mut());
    let out = f(m, &mut m_alias);
    let _ = m_alias.into_raw();
    out
}

It takes a Mat and a function that receives the same Mat as both & and &mut that allows passing it to OpenCV functions that can operate in-place, e.g.:

unsafe {
    op_inplace(&mut im, |im, out| {
        imgproc::threshold(im, out, BINARIZATION_THRESHOLD, 255., imgproc::THRESH_BINARY)
    })?
};
nsabovic commented 5 months ago

I am hitting the same issue with other imgproc functions, so maybe the title of this issue should be changed into something like "Using the same Mat as source and destination".

I'd vote for including a similar helper in the library proper. In my particular scenario I'm not using a Mat but a BoxedRefMut, so this particular function wouldn't work. I am using this:

#[inline(always)]
pub unsafe fn op_inplace<M: MatTrait, R>(
    m: &mut M,
    f: impl FnOnce(&M, &mut Mat) -> Result<R>,
) -> Result<R> {
    let mut m_alias = Mat::new_rows_cols_with_data_unsafe(
        m.rows(),
        m.cols(),
        m.typ(),
        m.data_mut().cast(),
        m.mat_step().get(0),
    )?;
    f(m, &mut m_alias)
}

This comes with a limitation that the matrix must be 2D. I also like the unsafe marker because not all OCV functions can be used like this, and the docs don't help so some amount of bravery is required.

A quick question about the m_alias.into_raw() call. This is because you constructed an owning Mat from a copy and you don't want it dropped twice. But in my case I am using non-owning constructor and so I don't need to do that, right?

twistedfall commented 5 months ago

I'd vote for including a similar helper in the library proper.

That's a good idea, I'll try to implement it.

This is because you constructed an owning Mat from a copy and you don't want it dropped twice.

Exactly for this reason, yes. the second Mat has the same pointer so it's literally the same Mat on the C++ side.

But in my case I am using non-owning constructor and so I don't need to do that, right?

That's right, in your case the second Mat is a separate object (with its own header and stuff) that happens to reference the same data as the first Mat so dropping it won't affect the first Mat. The downside of your implementation that I see is that should the OpenCV function that's called from f need to reallocate the Mat's data for some reason then this new Mat will be completely lost and initial Mat in m will be left untouched.

twistedfall commented 4 months ago

You can now call an unsafe modify_inplace() method on a Mat to get the desired behavior in v0.92.0