Ported VAS OT into OpenCV G-API#24224
Conversation
|
@AsyaPronina Thanks a lot for the patch. Couple of general notes:
|
Hello! Thank you a lot for the comments! I will fix conflict and add tests! |
modules/gapi/CMakeLists.txt
Outdated
| "${CMAKE_CURRENT_LIST_DIR}/include/opencv2/${name}/streaming/gstreamer/*.hpp" | ||
| "${CMAKE_CURRENT_LIST_DIR}/include/opencv2/${name}/streaming/onevpl/*.hpp" | ||
| "${CMAKE_CURRENT_LIST_DIR}/include/opencv2/${name}/plaidml/*.hpp" | ||
| "${CMAKE_CURRENT_LIST_DIR}/include/opencv2/${name}/ported_algorithms/ot/*.hpp" |
There was a problem hiding this comment.
Please don't name folders like that.
Note that public include headers is part of the public API. It is not user's concern if the OT algorithm is "ported" or not.
OT's domain is streaming, so I'd assume users to include opencv2/gapi/streaming/ot.hpp (or may be /ot/vasot.hpp, if multiple OT flavors are possible).
There was a problem hiding this comment.
Thanks a lot, fixed!
modules/gapi/CMakeLists.txt
Outdated
| src/backends/ov/bindings_ov.cpp | ||
| src/backends/python/gpythonbackend.cpp | ||
|
|
||
| # VAS Object Tracking ported algorithm: |
There was a problem hiding this comment.
Don't distinguish it as a "ported algorithm" please. src/streaming/ot would be enough.
There was a problem hiding this comment.
Thanks a lot, fixed!
ca0930b to
0562e76
Compare
|
Hello Dear Alexander (@asmorkalov)! Could we please ask for your advice on better place for VAS object tracking algorithm? Is it OpenCV video module or might be OpenCV g-api own 3rdparty component? CC: Dmitry (@dmatveev). |
|
Sounds interesting! Will take a look how can I help you. |
| @@ -0,0 +1,233 @@ | |||
| // This file is part of OpenCV project. | |||
There was a problem hiding this comment.
This header must be moved. ported_algorithms shouldn't be there
There was a problem hiding this comment.
Yes, I have already got rid from "ported_algorithms"! However, refactored implementation isn't on review yet, as I am transferring from common 3rdparty to G-API as a draft version.
0562e76 to
3da463e
Compare
82af648 to
53baf8f
Compare
modules/gapi/CMakeLists.txt
Outdated
| endif() | ||
| endif() | ||
|
|
||
| add_subdirectory(${CMAKE_CURRENT_LIST_DIR}/3rdparty/vasot ${CMAKE_CURRENT_BINARY_DIR}/3rdparty/vasot) |
There was a problem hiding this comment.
Why
${CMAKE_CURRENT_BINARY_DIR}/3rdparty/vasot
needs to be added?
There was a problem hiding this comment.
One IOS build is failing: https://github.com/opencv/opencv/actions/runs/6937152923/job/18870654308?pr=24224#step:7:297
CMake Error at modules/gapi/CMakeLists.txt:388 (add_subdirectory):
add_subdirectory not given a binary directory but the given source
directory
"/Users/opencv-cn/GHA-OCV-1/_work/opencv/opencv/opencv/modules/gapi/3rdparty/vasot"
is not a subdirectory of
"/Users/opencv-cn/GHA-OCV-1/_work/opencv/opencv/opencv/modules/world".
When specifying an out-of-tree source a binary directory must be explicitly
specified.
Call Stack (most recent call first):
modules/world/CMakeLists.txt:13 (include)
modules/world/CMakeLists.txt:32 (include_one_module)
There was a problem hiding this comment.
Basically it means the static build may be broken because of that.. but what makes OT different from any other source-level subdirectory in this case?
There was a problem hiding this comment.
What if..
- You move
gapi/3rdpartyundergapi/src/3rdparty? - You remove all the CMakeListst.txt from the VAS OT
- You just add
gapi/src/3rdparty/includeto the INCLUDE directories for theopencv_gapitarget?
In this case the OT code will be indistinguishable from the rest adn wouldn't require any special handling. There's literally no point in having a separate build target for that.
There was a problem hiding this comment.
Thanks a lot, fixed!
6f85ec4 to
89c275b
Compare
|
Hello Dear Maxim (@mshabunin), could you please help to understand what can be the issue here: |
|
@asmorkalov @mshabunin can we merge it? The issue with android build seems not related to this PR: |
89c275b to
93f9fe9
Compare
Dear Alexander (@asmorkalov), could you please take a look? |
|
Github actions are all passed, isnt it? |
93f9fe9 to
287ea3f
Compare
Hello Dear Dmitry! There was an issue with default build, but rebase fixed it! |
|
Dear Alexander (@asmorkalov), all builds are green and test is added, could I please ask you to merge? |
|
We try to add licenses for all 3rdparty components to the install set. For this you need to add license text file somewhere and call |
|
@mshabunin I believe it is published here under the Apache 2 license, same as OpenCV. Do we need to do anything in this case? |
|
I mean code in 3rdparty/vasot. As I understand it has been ported mostly unchanged from DLStreamer (MIT license). |
If build this If yes, should we change |
This is not necessary, for example we have SoftFloat library included into core module:
No, AFAIK MIT license doesn't forbid current usage form.
I'm not sure. It might depend on specific license requirements. I'd recommend consulting with legal expert. The MIT license says: The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software. 🤷 |
Thank you a lot!! |
287ea3f to
71afc30
Compare
|
@asmorkalov @mshabunin it is ready for merge now, is there anything else required? |
mshabunin
left a comment
There was a problem hiding this comment.
No objections from my side.
Just one note: resulting installed license file will be named vasot-vasot_LICENSE.txt because that cmake function prepends component name to the file name.
Thank you a lot, fixed! |
71afc30 to
e70c8e5
Compare
|
Dear Alexander (@asmorkalov) and Alexander (@opencv-alalek), could you please merge? |
Merge with extra: opencv/opencv_extra#1123
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.
! There is known flaw in the code: usage of
KalmanFilterNoOpencvclass. This should be reworked to use OpenCV version or just refactored. !! Link to the original implementation is: ttps://github.com/dlstreamer/dlstreamer/tree/master/src/monolithic/gst/elements/gvatrack/vas