Ported VAS OT into OpenCV G-API #24224
Conversation
AsyaPronina
requested review from
TolyaTalamanov,
dmatveev,
smirnov-alexey and
sivanov-work
|
@AsyaPronina Thanks a lot for the patch. Couple of general notes:
|
asmorkalov
added
pr: needs test
Hello! Thank you a lot for the comments! I will fix conflict and add tests! |
modules/gapi/CMakeLists.txt
Outdated
| @@ -65,6 +65,7 @@ file(GLOB gapi_ext_hdrs | |||
| "${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
| @@ -190,6 +191,19 @@ set(gapi_srcs | |||
| 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!
AsyaPronina
force-pushed
the
asyadev/port_vas_ot_to_opencv
branch
14 times, most recently
from
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.
AsyaPronina
force-pushed
the
asyadev/port_vas_ot_to_opencv
branch
from
0562e76
to
3da463e
Compare
AsyaPronina
force-pushed
the
asyadev/port_vas_ot_to_opencv
branch
2 times, most recently
from
82af648
to
53baf8f
Compare
modules/gapi/CMakeLists.txt
Outdated
| @@ -383,10 +385,12 @@ if(HAVE_ONNX) | |||
| 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!
AsyaPronina
force-pushed
the
asyadev/port_vas_ot_to_opencv
branch
2 times, most recently
from
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: |
AsyaPronina
force-pushed
the
asyadev/port_vas_ot_to_opencv
branch
from
89c275b
to
93f9fe9
Compare
Dear Alexander (@asmorkalov), could you please take a look? |
|
Github actions are all passed, isnt it? |
AsyaPronina
force-pushed
the
asyadev/port_vas_ot_to_opencv
branch
from
93f9fe9
to
287ea3f
Compare
Hello Dear Dmitry! There was an issue with default build, but rebase fixed it! |
AsyaPronina
removed
the
pr: needs test
|
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!! |
AsyaPronina
force-pushed
the
asyadev/port_vas_ot_to_opencv
branch
from
287ea3f
to
71afc30
Compare
|
@asmorkalov @mshabunin it is ready for merge now, is there anything else required? |
Thank you a lot, fixed! |
AsyaPronina
force-pushed
the
asyadev/port_vas_ot_to_opencv
branch
from
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