Closed ayberkozgur closed 9 years ago
Finally, I compiled and played quickly with the sample, it's pretty cool ! Nice work !
Thanks! More commits to follow today.
By the way, what kind of tests would you have in mind to test an async thing like this?
This is about all the changes I wanted to make.
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.
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 ;)
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:
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...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)
?#include <iostream>
in Detect.hpp
;)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)Here are my responses:
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.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.Detect
but will have the same mutex/cond complexity. We could do this in the future.Well, you asked for it, and here it is with all the grotesque glory of #ifdef
s.
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.
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
).
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.
@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
.
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"
FindThreads
looks like cmake 3.0 only, I'm setting default values for WITH_PTHREADS
for now.
@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
.
@severin-lemaignan Yes, WITH_MULTITHREADING
would make more sense in the source. What would make more sense is HAS_PTHREADS
or HAS_MULTITHREADING_SUPPORT
.
@ayberkozgur +1 for HAS_MULTITHREADING
I think it should be HAS_MULTITHREADING_WITH_PTHREAD
. Just kidding, don't start another war ;)
My response to yours:
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) ?
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
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 ?
OK let's go with that for now.
I didn't find anything to complain, so this PR would impress even my therapist ;) Many thanks again for this solid contribution !
Solves issue #63.
Adds two new detection triggers to the API:
BACKGROUND_DETECT_PERIODICALLY
runs detection in a background thread. Detection is triggered only whenmCallsBeforeDetection
many calls tofind()
are made, just like inDETECT_PERIODICALLY
. As soon as the detection ends, tracked tags are updated. Calls tofind()
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
TODO
s to address and more commits will follow. For the meantime, I await your reviews.