New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
3 New OpenCV Effects (Tracker, Stabilization, and Object Detection) #585
Conversation
Added map instead of vector in the Stabilizer, removed unnecessary code and improved documentation
Also integrated a basic the tracker Effect with the UI, changed the effect creation to use Json messages, and added the ProcessingController class to handle thread communication
Also included kalman filter functions and code for tracking the output boxes from DNN model
Also added tests for ObjectDetection and several bug fixes
Co-authored-by: Frank Dana <ferdnyc@gmail.com>
CMake: Adjustments to build config with OpenCV/Protobuf/Boost
std::shared_ptr<QImage> imgIn = std::make_shared<QImage>(qimg.copy()); | ||
|
||
// Always convert to RGBA8888 (if different) | ||
if (imgIn->format() != QImage::Format_RGBA8888_Premultiplied) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is kind of silly. We know the image isn't QImage::Format_RGBA8888_Premultiplied
, you explicitly set it to QImage::Format_RGBA888
three lines previous! 😆
Also, would it be better to do the conversion before creating the shared pointer? Right now, you're converting from BGR to RGB, creating a new QImage from the conversion buffer, copying the QImage (to free the buffer, I assume) and wrapping it in a shared pointer, and then quite probably copying it again to do the format-conversion, and finally updating the shared pointer to the new copy.
You can probably do more of that in one go, something like this:
QImage qimg((uchar*) img.data, img.cols, img.rows, img.step, QImage::Format_RGB888);
auto imgIn = std::make_shared<QImage>(
qimg.convertToFormat(QImage::Format_RGBA8888_Premultiplied));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Unfortunately, Qt didn't add support for QImage::Format_BGR888
until Qt 5.14, or you really could do it all in one go.)
Co-authored-by: Frank Dana <ferdnyc@gmail.com>
- Moved TimeUtil from header to src - Fixed issue with Qimage -> Mat conversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BrennoCaldato
This'll get rid of my Error:
messages. (Still not sure how you're not seeing those!)
If you have a chance you may want to look over the Codacy scan report (that's its current URL, it may update with changes to the PR), it tends to flag a lot of stuff that's not fatal, and easy to overlook, but worth addressing if possible. |
Re: Codacy, I see it's still flagging some stuff in |
Hi @ferdnyc I opted for leaving the If you prefer to remove this file from codacy it's also fine for me. |
|
||
/* initialization */ | ||
*cost = 0; | ||
for (row = 0; row < nOfRows; row++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BrennoCaldato Regarding Codacy's feedback on things like the scope of loop variables — I see it as a purely stylistic choice, ultimately, and in practice there will be no appreciable difference in performance between the two. However, I tend to side with the voices in this discussion who say that, if anything, multiple for loops that each use a separate, locally-scoped variable may run slightly faster than reusing the same variable allocated once at the beginning.
That's mostly because, if we're talking about a POD variable (like a loop counter), the compiler can, and will, just store that counter in a register. Which means it won't require any allocations at all. It's probably smart enough that it's going to do that regardless, at least when optimization is turned on, but the version with locally-scoped variables gives it a better chance of optimizing away the allocations even at lower optimization levels.
See also Matt Godbolt's talk from CppCon 2017, where he shows off Compiler Explorer and uses it to examine how various styles of C++ loop code get translated into assembly instructions at different optimization levels:
(The entire talk is really worth watching if you have time, it's all amazing. At around 35 minutes in, there's a segment where he demonstrates some of the tricks the compiler uses to optimize adds and multiplies, as well as to avoid divide operations — that really blew me away.)
So, long story short, take these two bits of otherwise-identical code:
// Version 1
int row;
for (row = 0; row < ...; ++row) {
...
}
for (row = 0; ...) {
...
}
for (row = 0; ...) {
...
}
// Version 2
for (int row = 0; row < ...; ++row) {
...
}
for (int row = 0; ...) {
...
}
for (int row = 0; ...) {
...
}
In practice, all other things being equal:
- The difference in terms of real-world performance will be too small to measure, in basically all scenarios.
- With optimization turned on, both versions will most likely require zero memory allocations for
row
. - Therefore there's no practical advantage to Version 1 over the (IMHO) cleaner and more readable Version 2
- Codacy is therefore (again IMHO) correct to recommend rewriting instances of Version 1 as Version 2, where it can catch them.
- The same holds for instances that Codacy can't catch. (It recommended reducing the scope of your
col
, but notrow
, even thoughrow
is never used outside of the loops either.)
But, as I said, ultimately I consider it a stylistic choice and I'm not hung up on imposing one or the other in the libopenshot code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ferdnyc This is very interesting. I didn't have time to watch the video yet but I'll definitely do. Thanks for sending it.
@BrennoCaldato @ferdnyc I'm getting ready to merge this into libopenshot. I think we have the basic integration very well done at this point, and it opens up a tremendous amount of future effects and features we can based off of OpenCV functions. I still think we have a few things to tweak on the openshot-qt side... UI wise, and I might hide the "Object Detector" effect initially, since it is mostly useless until future Keyframe improvements. But this is a great start!! |
libopenshot welcomes OpenCV and all the potential it brings! 👏 |
The idea is that processing frame by frame data using algorithms or AI is slow on most consumer hardware, and would be unusably slow in OpenShot's real-time preview. To counter this, we pre-process the image data before the effect is dropped onto the clip (for example, determine the amount of stabilization required per frame, or determine where an object moves on each frame). The user get's a progress bar, and it takes as long as it takes. Once done, we save the data using Protobuf files (for compression and speed), and our Effect reads the data from protobuf in real-time. Thus, the real-time previews are super responsive and very usable.
Related UI for this code: OpenShot/openshot-qt#3806
Tracker:
Object Detection:
Stabilization: