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
Enable GAPI VASOT in Python #24576
Enable GAPI VASOT in Python #24576
Conversation
651280a
to
ce4267f
Compare
Various build issues in the last build.. Also failures detected |
6e25e93
to
50a7c5e
Compare
50a7c5e
to
4521894
Compare
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.
The changes looks good but invasive. I'm not sure that OpaqueKind
was ever designed to contain such specific detail as TrackingStatus
.
Perhaps we can avoid exposing TrackingStatus
into python and just use int
instead. For improving readability we still can define TrackingStatus
enum but on python side (__init__.py
) and use it like that:
rois, labels, ids, status = cv.gapi.ot.track(img, rect, labels, delta)
if status == TrackingStatus.NEW.value:
pass
In that case the status
is int
but you may compare it with enum
:
class TrackingStatus(Enum):
NEW = 1,
TRACKED = 2,
LOST = 3
@@ -11,6 +11,7 @@ | |||
#include <opencv2/gapi/garg.hpp> | |||
#include <opencv2/gapi/gopaque.hpp> | |||
#include <opencv2/gapi/render/render_types.hpp> // Prim | |||
#include <opencv2/gapi/ot_tracking_status.hpp> // TrackingStatus |
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.
Should it be just a part of ot.hpp
?
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.
+1
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.
It couldn't be that time, as was included in gcommon.hpp
. Now, it is removed from gcommon,hpp
and returned back to ot.hpp
Need to discuss this, since it's not the better solution and in some extent even worse :) |
@@ -299,3 +332,10 @@ def kernel_with_params(cls): | |||
|
|||
|
|||
cv.gapi.wip.GStreamerPipeline = cv.gapi_wip_gst_GStreamerPipeline | |||
|
|||
class TrackingStatus: |
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.
(Optional)
What type is used for NEW
, LOST
, TRACKED
? I assume int
, will there be just 0
,1
,2
when we print it into console?
Perhaps we can leverage python enum
module to improve that.
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.
@AsyaPronina Could you add a couple of python test cases, please?
@@ -366,6 +366,17 @@ IIStream& operator>> (IIStream& is, cv::gapi::wip::draw::Line &l) { | |||
return is >> l.color >> l.lt >> l.pt1 >> l.pt2 >> l.shift >> l.thick; | |||
} | |||
|
|||
IIStream& operator>> (IIStream& is, cv::gapi::ot::TrackingStatus& status) { |
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.
Why not just static_cast<int>
?
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.
+1
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.
Removed!
CV_SCALAR, // cv::Scalar user G-API data | ||
CV_MAT, // cv::Mat user G-API data | ||
CV_DRAW_PRIM, // cv::gapi::wip::draw::Prim user G-API data | ||
CV_OT_TRACKING_STATUS // cv::gapi::ot::TrackingStatus user G-API data |
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.
I doubt if we need to know about this type at the Opaque level.
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.
Fixed!
@@ -11,6 +11,7 @@ | |||
#include <opencv2/gapi/garg.hpp> | |||
#include <opencv2/gapi/gopaque.hpp> | |||
#include <opencv2/gapi/render/render_types.hpp> // Prim | |||
#include <opencv2/gapi/ot_tracking_status.hpp> // TrackingStatus |
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.
+1
@@ -366,6 +366,17 @@ IIStream& operator>> (IIStream& is, cv::gapi::wip::draw::Line &l) { | |||
return is >> l.color >> l.lt >> l.pt1 >> l.pt2 >> l.shift >> l.thick; | |||
} | |||
|
|||
IIStream& operator>> (IIStream& is, cv::gapi::ot::TrackingStatus& status) { |
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.
+1
std::string statusStr; | ||
is >> statusStr; | ||
status = cv::gapi::ot::from_string(statusStr); | ||
return is; |
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.
Please don't do this. It is binary serialization, no the formatted input/output
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.
Removed!
GAPI_EXPORTS IOStream& operator<< (IOStream& os, const cv::gapi::ot::TrackingStatus& status); | ||
GAPI_EXPORTS IIStream& operator>> (IIStream& is, cv::gapi::ot::TrackingStatus& status); |
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.
Is it required now for status? I believe s11n can be omitted for the tracking status.
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.
Fixed!
@@ -223,7 +206,7 @@ TEST(VASObjectTracker, PipelineTest) | |||
reference_trackings_det_ids[frame_no].push_back(fn_tracked_box["id"]); | |||
reference_trackings_tr_ids[frame_no].push_back(int(fn_tracked_box["tracking_id"])); | |||
reference_trackings_tr_statuses[frame_no].push_back( | |||
from_string(fn_tracked_box["tracking_status"])); | |||
cv::gapi::ot::from_string(fn_tracked_box["tracking_status"])); |
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.
it shouldn't be an API function.
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.
Fixed!
return false; | ||
return value != (uint64_t)-1 || !PyErr_Occurred(); | ||
} | ||
}; |
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.
Are there tests on that?
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.
Added!
1a52653
to
bf5c877
Compare
Fixed! |
Fixed! |
ab17bed
to
87b6096
Compare
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.
Great! Looks much better and less invasive. There a couple of thoughts how it can be improved:
- Do we really need to use
uint64_t
as return value forot::track
? Can we just useint
? - Since both
int64_t
anduint64_t
are used only as return values we don't need to expose them into pythonGArray
&GOpaque
types. (e.gcv.GArray.Int64()
- never used)
@@ -24,7 +24,7 @@ namespace ot { | |||
* | |||
* Tracking status twin for vas::ot::TrackingStatus | |||
*/ | |||
enum class TrackingStatus: int32_t |
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.
It looked fine, why we decided to change this?
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.
to be automatically convertable to int
def test_skip(): | ||
pass | ||
|
||
pass |
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.
Why this one is needed there?
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.
+1
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.
Fixed
float delta); | ||
GAPI_EXPORTS_W std::tuple<cv::GArray<cv::Rect>, | ||
cv::GArray<int>, | ||
cv::GArray<uint64_t>, |
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.
I actually doubt if we even need uint64_t
there...
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 is what inner VAS function operates with. I don't think that reinterpret_cast
or copying are the best options to deal with it to return another type.
@@ -56,6 +56,14 @@ class Int(): | |||
def __new__(self): | |||
return cv.GOpaqueT(cv.gapi.CV_INT) | |||
|
|||
class Int64(): |
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.
I don't see any usage of both Int64()
and UInt64()
propose to remove this unless it's really needed
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.
One of the return types of OT I guess.
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.
UInt64
will be used if we want to create custom kernel in python and accept output from track
operation.
@@ -205,6 +225,8 @@ def op(op_id, in_types, out_types): | |||
type2str = { | |||
cv.gapi.CV_BOOL: 'cv.gapi.CV_BOOL' , | |||
cv.gapi.CV_INT: 'cv.gapi.CV_INT' , | |||
cv.gapi.CV_INT64: 'cv.gapi.CV_INT64' , |
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.
Where do we use int64_t
? I can only see uint64_t
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.
It was exposed in the bindings already. I decided to just add it here also.
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.
👍
GAPI_EXPORTS_W std::tuple<cv::GArray<cv::Rect>, | ||
cv::GArray<int>, | ||
cv::GArray<uint64_t>, | ||
cv::GArray<int>> track(const cv::GMat& mat, |
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.
minor: please put track()
on a new line after its monstrous return value type
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.
Fixed
@@ -56,6 +56,14 @@ class Int(): | |||
def __new__(self): | |||
return cv.GOpaqueT(cv.gapi.CV_INT) | |||
|
|||
class Int64(): |
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.
One of the return types of OT I guess.
def test_skip(): | ||
pass | ||
|
||
pass |
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.
+1
res = comp.apply(cv.gin(in_image, in_rects, in_rects_cls), | ||
args=cv.gapi.compile_args(cv.gapi.ot.cpu.kernels())) |
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.
Should the output be tested somehow? How the LOST/NEW/TRACK status look in Python now?
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.
Fixed!
be67f3c
to
9911ac9
Compare
Hello Dear Alexander (@asmorkalov), Dear Alexander (@opencv-alalek), Dear Maksim (@mshabunin), could you please merge this PR? |
@asmorkalov @mshabunin can we merge it today pretty please? |
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.
Looks good to me.
9911ac9
to
d20727a
Compare
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.