Comments

When reviewing #135, I was like "wait, why is this test actually passing?". The test was about flipping a Mat into a brand new Mat, both passed to a function that looks roughly like this:

funcFlip(srcMat, dstMat, flipCodeint) {}

In Go, parameters passed by value (default like above) should not be modified (and in any case, it will never change the value in the parent block). In gocv, this would works though because we manipulate a C pointer under the hood but this is a bit misleading.

Question is: could we agree on a "convention" stating that all arguments modified should be passed by pointer?

If we take the Flip() function as an example:

src:= ...
dst:= gocv.NewMat()
// wait, how does `dst` can be updated? No return-value either? Hmmm.
gocv.Flip(src, dst, 0);
// versus// oh right so `Flip` takes a `src` Mat and puts the result in `dst`. Lovely <3
gocv.Flip(src, &dst, 0);

This comment has been minimized.

There was a discussion about this at some point on slack or something like that. The question is, should we be more like the C++ API (the current way) or more like the Python API (return a new Mat in function that has a 'dest' that the dev needs to close).

I am still not pulled either way with great conviction, the C++ style seemed the best at the time, but we could decide to change the interface. Seems like doing so sooner would be best if we are going to do so.

This comment has been minimized.

As I recall it was @221bytes that I was discussing this with on the Golang Slack channel.

In any case, we can decide to follow one of 2 approaches here:

continue using the dst Mat assigned pattern as currently implemented. It does make it more like the C++ code, so the C++ docs would match up, but is a little different than the typical Golang code.

switch to returning a new gocv.Mat for any returned values, more like how the Python code handles. It makes it a little less of a match to the C++ docs, but is certainly a little more like what go programmers expect.

I don't like to break the API, but this conversation keeps coming up for good reason. Seems like it would be better to be a little more idiomatic for Go.

To use the example above:

src := ...
// dst is now return-value is all cases that there is a Mat that is modified.
dst := gocv.Flip(src, 0);

There is, however, a use case that is handled by OpenCV's C++ interface that this would not accommodate, which is when the src and dst are the same, what openCV calls "inplace". There are a number of OpenCV functions which allow this, for example dilate. This is rather inconsistent, as only some OpenCV functions support this.

I am starting to get the feeling we should probably just ignore the inplace return assignment option and always return new Mat values from any functions that have an output. This would eliminate an entire category for potential confusion for the consumers of this package.

Ideally, I would like to hear from a few more community members before revamping the current API. But so far everyone has been in favor of changing to a more idiomatic Go approach of returning new values.

This comment has been minimized.

edited

continue using the dst Mat assigned pattern as currently implemented. It does make it more like the C++ code, so the C++ docs would match up, but is a little different than the typical Golang code.

I'd be OK with this if we could add pointers for arguments that are modified.

I don't like to break the API, but this conversation keeps coming up for good reason. Seems like it would be better to be a little more idiomatic for Go.

The good thing is 1.0.0 has not been released yet, so we can still BC break for good reasons.

I am starting to get the feeling we should probably just ignore the inplace return assignment option and always return new Mat values from any functions that have an output. This would eliminate an entire category for potential confusion for the consumers of this package.

I like this proposal a lot! I believe a package like gocv should provide a uniform/Go-oriented API on top of opencv. The current package is already good but this suggestion would make it even better.

(my 2cents)

deadprogram
changed the title from
Pass mutable parameters by pointer
to
Use Go-style return values for gocv functions that have result MatMar 19, 2018

This comment has been minimized.

I remember this talk, you said it was easier to stick to the C++ API for documentation and ease of implementation. Indeed, as a total newbie, it was for me easy to contribute (very small contribution though) to the project.
Except the reason, we wanna do it the gopher way, what are the other pros of refactoring now?

This comment has been minimized.

Go passes variables by value (copy) not by reference to functions. In Gocv, we should not expect dst to be modified after having called gocv.Flip(src, dst, 0) since dst is passed by value. Yet, it works because Mat only attribute is a C pointer. This is very misleading.

This comment has been minimized.

Hum, the name his dest and every functions as a nice comment associated with it. I can't see how someone can be misguided to be honest. I am not a super user and it was clear for me. Refactoring all the library only for this purpose sounds a bit overkill to me.
Ron took this decision at the beginning and it's working for us. Sticking to the c++ api is nice for documenting and for moving code from c++ to golang so for me we should keep the way it.
Please remember that I was a supporter of a more golang style at first ^^

This comment has been minimized.

Since I was paged in, I'll state my opinion. On the other hand I haven't used the API in any significant way so I'm probably not the best person to weight in, so I'll state in from the prospect of an API designer, which I happen to have experience in.

I use two goals when designing an API:

Reading a snippet of code should make things obvious to an unfamiliar reader, without having to resort to documentation.

It shall be designed for one of speed, simplicity of use or consistency with an external API. Know what you optimize for first.

About 1), nobody reads the doc unless they get stuck, you don't want users to get stuck. They'll prefer to copy paste a snippet and start from there. Having an input being mutated in Go does happen in some cases, io.Reader is a notorious example as @deadprogram noted above. In practice, it is important to note that the method Read() accepts a slice, not an struct instance. What's confusing here is that Mat is an struct instance, not an interface. Take as an example io.Copy(), what would be io.Copy(&bytes.Buffer{}, ...) is simply Read(gocv.Mat{}), a pass by value instead of a pointer. I think it's the primary source of confusion.

About 2), it really depends. People in the thread, including Ron himself, noted the value of sticking as close as possible to the C API for documentation purposes. If you want to aim for speed, you want to reduce heap allocations, so you want to accept outputs as arguments, instead of returning them (like io.Reader). If you aim for simplicity, returning a new gocov.Mat instance is likely the most idiomatic way. (I personally value speed, otherwise why using cgo at all?)

Then there's the trade off of breaking the API. In my projects, I "fixed" it by immediately releasing major version 1 and being quite open to releasing new major versions, around once per year or so. I try to batch the breaking API changes into one short amount of time (a ~3 months window), and each breakage is done for a real purpose that users can understand why.

I'm in the camp that trying to be "as close to the C API" when writing a C API wrapper in Go is counter-productive, but the cost of breaking current gocv users may not make doing structural changes like the one I describe below be worth it.

I'll still give my opinion since I was asked about it but feel free to dump it to /dev/null. :)

Examples of things I'd change:

I'd make a MatFloat64, MatFloat32, MatInt32, MatUint16 (and whatever other types) so that the methods GetXXX() would not be a mess. I'm using image.Image.At() as an example here; https://golang.org/pkg/image/#Image. Then, Mat could be an interface! You get strong typing for free. As a matter of fact, it would enable you to make the MatXX to implement https://golang.org/pkg/image/draw/#Drawer, which then would make it possible to decode a jpeg into a gocv.Mat or things like that.

Make most functions that "returns" a Mat be methods of Mat, so that AbsDiff(src1, src2, dst Mat) would become a method Mat.AbsDiff(src1, src2 Mat). Same for stuff like PutText(). That would make it really more obvious IMHO. Right now https://godoc.org/gocv.io/x/gocv is a mess (sorry) and bucketting the functions into methods would make the API tremendously more readable. Example: https://golang.org/pkg/time/

I'd change bool return values to be error, in particular Read().

I'd move UI stuff (Window) into a subpackage gocvui to make it clear it's for ui, not algorithm.

Same for I/O (IMRead/IMWrite/VideoCapture) into gocvio.

Remove the implicit dependency on the C API documentation. Make yours be self-standing and be free! :)

Then I think the API would be much more idiomatic, more welcoming to new users by being more focused, at the cost of being less C-like.

Is it worth? Is it desirable? I can't make the call but I it's worth taking a deep look and experimenting on this.

This comment has been minimized.

Thanks for the comments @maruel however some of these items are counter the design of not trying to re-architect the internals of OpenCV itself. For example, the UI stuff is in the OpenCV highgui module in the main package, so as to allow people to find where to add things within GoCV. I think we discussed this via email early on.

In any case, lots of ideas in your thoughtful response. So there is a possible new option. To restate all the possibilities:

Current code:

dst:= gocv.NewMat()
gocv.Flip(src, dst, 0);

Pass dst as a pointer to avoid "surprise" to Go programmers:

dst:= gocv.NewMat()
gocv.Flip(src, &dst, 0);

Return a new Mat as dst:

dst:= gocv.Flip(src, 0);

Change functions that mutate dst into methods off of the Mat they change:

This comment has been minimized.

At the moment, I'm starting to get the feeling that the @willdurand original suggestion of passing Mat* to indicate output param might be the best balance between Go-like idiom (anything passed by pointer is subject to mutation) and performance (we do not need to allocate/de-allocate extra cv::Mat objects within loops).

This would also probably be the smallest change to the API needed for consumers of the package (adding & in front of any dst params) vs. the other proposed changes.

This comment has been minimized.

Thanks for working on this, let me know if you need help, I have some spare time for this lib.

I skipped @maruel's comment about documentation, I also think that it would be better for end users to have the documentation as part of the godoc, not links to opencv documentation. What if the urls change for some reason?

This comment has been minimized.

edited

The OpenCV docs have hashes that do not change between versions, and now that we adopted earlier @berak suggestion as discussed in #109 to just link to master branch version, the links will always be the latest.

I like linking to the OpenCV docs as it gives us the full benefit of the OpenCV community's docs. But I do not like requiring devs to look at both places to do anything useful.

We ideally should have "good enuf'" GoDocs that do not require always going to the OpenCV docs. That will require a lot more effort on our parts to do so but for sure will be worth it. To achieve this we need to include a little more info for each function, what would the ideal function look like?

Regarding the @maruel comment on moving things to be methods of Mat. The problem is that since nearly everything uses Mat it would not really help much as far as "unflattening" the docs. I do not want to use sub-packages that do not match the OpenCV modules layout for reasons already expressed. Hence our options are limited on that.

This comment has been minimized.

edited

In addition to helping newcomers to this project but who know OpenCV, or people who are looking to add some specific unimplemented functionality, the current structure mapping directly to OpenCV's modules works well. Are those the designs patterns we might otherwwise choose? IDK but OpenCV is the target, so we want to reduce any cognitive load needed to find things.

Also, CGo does not work well with subpackages in all my previous experiments.