Mutability of Mat

[vadixidav]

It seems like some operations mutate Mat without requiring Mat to be mutable such as ellipse. This could result in a data race. These should be corrected to have correct mutability to ensure borrowing rules are fulfilled.

[Pzixel]

I don't see how drawing a figure from mat might mutate it. Could you elaborate?

[vadixidav]

Drawing on a figure changes the data inside of it, just like how slices are borrowed mutably when you get a mutable reference from one of their elements. For instance, if I call mix_channels on Mat in one thread while I am running ellipse in another, there is a data race because it isn't clear whether you will get a partially drawn on Mat, a complete one, or one without any drawing. Any result is possible, but it is clearly a data race. Drawing on the mat, resizing the mat, etc should all take it by mutable reference, otherwise a data race may occur.

[Pzixel]

I don't see any race here. mix_channels doesn't mutate self as well as ellipse. They share the same readonly data. What's your point?

[vadixidav]

@Pzixel See the documentation here for ellipse. It does not return a new Mat. It mutates the Mat it is called on. I used this in my SIFT example just recently to draw on a matrix. Despite the fact that it mutates the Mat, it is still taking it by an immutable reference. It seems you may have misremembered the way ellipse works.

[Pzixel]

It mutates the Mat it is called on

Why? I don't see how drawing method may mutate its owner. It's basically

for i in 0..width
   for j in 0.height
      drawPixel(self[i], self[j])

It seems you may have misremembered the way ellipse works.

I actually don't know. I assumed that drawing method doesn't mutate its owner becuase it doesn't make any sense.

[vadixidav]

See the code here. This is the macro in the line drawing portion of ellipse. It is complicated to follow, but before that you have the line here where the pointer is created as pointing directly to the buffer of the matrix. This is one of the places in the actual OpenCV code that mutates the matrix.

I am on a phone right now, but if you like I can show you precisely all the calls in the OpenCV code that cause mutation of the Mat being drawn on.

[Pzixel]

Ah, yes, They are indeed mutating self. I didn't see methods are void. Sorry, too much work with scala, where ellips whould be returnead instead :) You're right, it's an obvious bug.

[yuvallanger]

Some self mutating methods of Mat still don't have a &mut self first argument, making rustc complain warning: variable does not need to be mutable.

Considering #117 and the rewrite to a *-sys crate, should this still be changed or will it be dealt by the downstream crate adapting it to Rustlang?

From the background foreground patch I wrote I've learned that rustc is sometimes myopic about which variables from unsafe blocks need and which do not need to be mut. Here rustc will complain self is borrowed mutably unnecessarily. You'd have to mark it with a special pragma to make rustc stop complaining. Maybe change to &mut self and add this pragma?

[vadixidav]

@yuvallanger I intend to send a PR to fix it to have the correct mutability if I haven't already as part of the refactor. I don't want to modify the refactor branch anymore because I want it to merge so I don't have to keep merging other people's PRs into the refactor branch and making it more complicated. As soon as that is merged I will look at this. I am still waiting on a maintainer to look at the PR (ping @Pzixel).