Multithreaded detection/tracking

[ayberkozgur]

Solves issue #63.

Adds two new detection triggers to the API:

• BACKGROUND_DETECT_PERIODICALLY runs detection in a background thread. Detection is triggered only when mCallsBeforeDetection many calls to find() are made, just like in DETECT_PERIODICALLY. As soon as the detection ends, tracked tags are updated. Calls to find() only run tracking.
• BACKGROUND_DETECT_ALWAYS runs detection in a background thread as above, but detection is triggered as soon as the previous detection ends.

There are a handful of TODOs to address and more commits will follow. For the meantime, I await your reviews.

[qbonnard]

Finally, I compiled and played quickly with the sample, it's pretty cool ! Nice work !

[ayberkozgur]

Thanks! More commits to follow today.

[ayberkozgur]

By the way, what kind of tests would you have in mind to test an async thing like this?

[ayberkozgur]

This is about all the changes I wanted to make.

[ayberkozgur]

I'm posting the mail I wrote yesterday:

I think to test async stuff we really need to be able to do video tests for it to make sense. Video tests will be extremely beneficial for tracking tests as well since we can't really test tracking without detecting stuff first and tracking across more than one frame.

The same goes for Kalman filtering: I would shoot a video where I show multiple tags head on so without Kf the z axis shakes heavily. I would then go about calculating the average variance of the Z axis.

So I propose merging the multithreaded stuff for now and open an issue to do a sample video decoding test.

[qbonnard]

Regarding video tests, I answer here: #50
Regarding, this PR, I'll need a bit more time to do a proper review, since it's not trivial ;)

[qbonnard]

So, here is a small review. There are more or less important stuff, and stuff that we already discussed but apparently did not agree on:

• I really think there should be a WITH_ASYNC option in cmake to disable pthread at build time. I know that Windows support is not an argument for you, but emscripten should be...
• why is resizedInput a static variable rather than a member variable again ?
• I'm not convinced getLatestAsyncDetectionIdleMillis and getLatestAsyncDetectionWorkMillis are a worthy addition to the API... To be honnest I didn't really understand what they do. They sound more like test instrumentation to me. If they're used to tweak the number of frames before async detection happens, so that it corresponds to a period (in millisecond), why not directly add a setAsyncDetectionPeriod(millis) ?
• you forgot an #include <iostream> in Detect.hpp ;)
• you seem to prefer the Type const& declarations better than the const Type &... I don't really care, but for coherency reasons it would be nice to stick to the existing one (or change it everywhere, but I'm not sure it's worth it)
• Detect does not seem as simple to use as it could be, but it's not a big issue as long as it work, we can always refactor later, since it's not exposed to the user. I can't really pinpoint what I would change either, so nevermind ;) Maybe the frame counter before an actual detection could be inside Detect for example ?

[ayberkozgur]

Here are my responses:

1. Ok ok I'll do it (javascript is still a thing I presume)
2. Ok ok I'll do it
3. Then please propose a way to implement the background-detection sample which is essential for me. I think these APIs are in the same line as getCameraMatrix(); not essential in theory but makes life so much easier when user implements code.
4. Damnit :D
5. So this is one of the things that everyone prefers differently and cannot convince the other person otherwise. I really don't care about const type& var vs. type const& var since it makes no difference and I don't believe it affects readability. As a counterexample, I cared and adapted to the mVar style of member variable naming (which I don't prefer) because not doing this in parts of the code would result in serious readability issues in my opinion.
6. Please believe me when I say this is as simple as it gets with c++. To make it simpler, you write a helper class and carry the concurrency over there, which might release some strain over Detect but will have the same mutex/cond complexity. We could do this in the future.

[ayberkozgur]

Well, you asked for it, and here it is with all the grotesque glory of #ifdefs.

[severin-lemaignan]

I would sed 's/WITH_PTHREADS/WITH_MULTITHREADING/ because it gives the wrong impression that if you do not define WITH_PTHREADS, another multi-threading API may be used instead.

[severin-lemaignan]

btw, you may want to use CMake's FindThreads to check if pthreads are available and automatically use them (or at least, automatically select a sensible default value for WITH_PTHREADS).

[ayberkozgur]

I don't think WITH_PTHREADS give the impression of multithreading support with another lib; we would have WITH_ANOTHER_LIB in that case. E.g OpenCV has a long list of WITH_LIB style options.

Good point about FindThreads. Coming up.

[severin-lemaignan]

@ayberkozgur Still, your intention is to tell people if they are going to use multi-threading or not. Using pthread is an implementation detail that may well be replaced at some point by std::thread.

[ayberkozgur]

Honestly, my intention is to ask them whether they have pthreads or not. It might be an implementation detail but it affects the build process heavily at this point.

Note: I believe README would be the adequate place to note "pthreads or no multithreading as of now folks"

[ayberkozgur]

FindThreads looks like cmake 3.0 only, I'm setting default values for WITH_PTHREADS for now.

[severin-lemaignan]

@ayberkozgur My point with WITH_MULTITHREADING vs WITH_PTHREADS was not for the CMake file, but the the C++ defines. I do agree that the CMake variable can correctly be called WITH_PTHREADS.

[ayberkozgur]

@severin-lemaignan Yes, WITH_MULTITHREADING would make more sense in the source. What would make more sense is HAS_PTHREADS or HAS_MULTITHREADING_SUPPORT.

[severin-lemaignan]

@ayberkozgur +1 for HAS_MULTITHREADING

[qbonnard]

I think it should be HAS_MULTITHREADING_WITH_PTHREAD. Just kidding, don't start another war ;)

My response to yours:

1. I think they use it in this internet thing, but we don't use that around here.
2. I was asking a honest question: what is the (end) goal of these two methods/the sample ? I had a quick look at the sample, but I didn't really get what was computed/measured. It's not a way of saying it's useless, I'm really asking what is your purpose (since you seem to use multithreading in a context that I don't, i.e. on android). Is it for tuning the resource usage/performance trade off?
3. yeah, sorry about that... as you say, I guess this kind of issue is unavoidable when sharing code. I mostly care about the coherency, if you want to change a "convention" and are willing to propagate the changes, I'm open for discussion (althogh for the member thing, I do like to distinguish members from other variables, and my small fingers don't like _)
4. yeah as I said, nevermind. I was just probing whether you had thought about a possible refactoring already ;)

[qbonnard]

ping @ayberkozgur for 3. : can you kindly explain for my slow brain the purpose of the sample (other than showing how to use multithreaded detection) ?

[ayberkozgur]

There is pretty much no other reason, but why shouldn't this be a valid reason? Isn't a sample something that illustrates some functionality of a library? If we assume this is true, and that there is only timing information that you can show about multithreadedness without having to resort to external tools such as top then we have to show this timing information for the user not to go like "how is this any different than DETECT_PERIODICALLY?". There is literally nothing else that defines multithreadedness than timing (from a user point of view). If we assume this is false, where a multithreaded sample is supposed to do something else than illustrating multithread functionality, then I'm out of, but still open to, ideas. I'm more open to ideas that can accomplish exporting timing info (still tied to timing but without timestamps: the order of operations?) on a thread other than the user's main thread.

Your brain isn't slow, it's just too preoccupied with creating the flawless user API XD

[qbonnard]

Your brain isn't slow, it's just too preoccupied with creating the flawless user API XD

That's what my therapist says. I say it's all Severin's fault for not letting the old API be, with its 21024 undocumented classes. My therapist says I won't get any more pills, so I'm not sure I can deal with the addition of two methods whose sole purpose is to illustrate some internal value ;)

I think it's enough to allow the user to play with background detection in real time in the sample. The user will "feel" that detection has a low latency, and is more regular than with DetectPeriodically. If the users wants to see precisely, they can use tools like top or gnome-system-monitor as you say. Displaying values seems a bit abstract, not to mention the fact that they are not accurate because of the smoothing... What would be cool though, would be to allow to switch between async and sync to feel the difference. No ?

[ayberkozgur]

OK let's go with that for now.

[qbonnard]

I didn't find anything to complain, so this PR would impress even my therapist ;)
Many thanks again for this solid contribution !