Enable GAPI VASOT in Python by AsyaPronina · Pull Request #24576 · opencv/opencv

AsyaPronina · 2023-11-22T14:58:37Z

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
The PR is proposed to the proper branch
There is a reference to the original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

dmatveev · 2023-12-01T09:18:15Z

Various build issues in the last build.. Also failures detected

TolyaTalamanov

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

TolyaTalamanov · 2023-12-12T09:03:27Z

modules/gapi/misc/python/python_bridge.hpp

 #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


Should it be just a part of ot.hpp?

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

TolyaTalamanov · 2023-12-12T09:24:35Z

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

Need to discuss this, since it's not the better solution and in some extent even worse :)

TolyaTalamanov · 2023-12-12T09:29:32Z

modules/gapi/misc/python/package/gapi/__init__.py


 cv.gapi.wip.GStreamerPipeline = cv.gapi_wip_gst_GStreamerPipeline
+
+class TrackingStatus:


(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.

TolyaTalamanov

@AsyaPronina Could you add a couple of python test cases, please?

TolyaTalamanov · 2023-12-13T09:24:45Z

modules/gapi/src/backends/common/serialization.cpp

    return is >> l.color >> l.lt >> l.pt1 >> l.pt2 >> l.shift >> l.thick;
 }

+IIStream& operator>> (IIStream& is, cv::gapi::ot::TrackingStatus& status) {


Why not just static_cast<int>?

dmatveev · 2023-12-13T15:01:06Z

modules/gapi/include/opencv2/gapi/gcommon.hpp

+        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


I doubt if we need to know about this type at the Opaque level.

dmatveev · 2023-12-13T15:09:37Z

modules/gapi/misc/python/python_bridge.hpp

 #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


dmatveev · 2023-12-13T15:10:17Z

modules/gapi/src/backends/common/serialization.cpp

    return is >> l.color >> l.lt >> l.pt1 >> l.pt2 >> l.shift >> l.thick;
 }

+IIStream& operator>> (IIStream& is, cv::gapi::ot::TrackingStatus& status) {


dmatveev · 2023-12-13T15:10:44Z

modules/gapi/src/backends/common/serialization.cpp

+    std::string statusStr;
+    is >> statusStr;
+    status = cv::gapi::ot::from_string(statusStr);
+    return is;


Please don't do this. It is binary serialization, no the formatted input/output

dmatveev · 2023-12-13T15:11:19Z

modules/gapi/src/backends/common/serialization.hpp

+GAPI_EXPORTS IOStream& operator<< (IOStream& os, const cv::gapi::ot::TrackingStatus& status);
+GAPI_EXPORTS IIStream& operator>> (IIStream& is,       cv::gapi::ot::TrackingStatus& status);


Is it required now for status? I believe s11n can be omitted for the tracking status.

dmatveev · 2023-12-13T15:11:47Z

modules/gapi/test/cpu/gapi_ot_tests_cpu.cpp

            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"]));


it shouldn't be an API function.

dmatveev · 2023-12-13T15:12:16Z

modules/python/src2/cv2_convert.hpp

+            return false;
+        return value != (uint64_t)-1 || !PyErr_Occurred();
+    }
+};


Are there tests on that?

AsyaPronina · 2023-12-14T01:28:19Z

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

Fixed!

AsyaPronina · 2023-12-14T01:28:34Z

@AsyaPronina Could you add a couple of python test cases, please?

Fixed!

TolyaTalamanov

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 for ot::track? Can we just use int?
Since both int64_t and uint64_t are used only as return values we don't need to expose them into python GArray & GOpaque types. (e.g cv.GArray.Int64() - never used)

TolyaTalamanov · 2023-12-14T15:51:44Z

modules/gapi/include/opencv2/gapi/ot.hpp

 *
 * Tracking status twin for vas::ot::TrackingStatus
 */
-enum class TrackingStatus: int32_t


It looked fine, why we decided to change this?

to be automatically convertable to int

TolyaTalamanov · 2023-12-14T15:53:27Z

modules/gapi/misc/python/test/test_gapi_ot.py

+        def test_skip():
+            pass
+
+    pass


Why this one is needed there?

TolyaTalamanov · 2023-12-14T15:55:42Z

modules/gapi/include/opencv2/gapi/ot.hpp

-                                                          float delta);
+GAPI_EXPORTS_W std::tuple<cv::GArray<cv::Rect>,
+                         cv::GArray<int>,
+                         cv::GArray<uint64_t>,


I actually doubt if we even need uint64_t there...

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.

TolyaTalamanov · 2023-12-14T15:57:24Z

modules/gapi/misc/python/package/gapi/__init__.py

        def __new__(self):
            return cv.GOpaqueT(cv.gapi.CV_INT)

+    class Int64():


I don't see any usage of both Int64() and UInt64() propose to remove this unless it's really needed

One of the return types of OT I guess.

UInt64 will be used if we want to create custom kernel in python and accept output from track operation.

TolyaTalamanov · 2023-12-14T15:58:09Z

modules/gapi/misc/python/package/gapi/__init__.py

    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' ,


Where do we use int64_t? I can only see uint64_t

It was exposed in the bindings already. I decided to just add it here also.

dmatveev

👍

dmatveev · 2023-12-14T16:06:34Z

modules/gapi/include/opencv2/gapi/ot.hpp

+GAPI_EXPORTS_W std::tuple<cv::GArray<cv::Rect>,
+                          cv::GArray<int>,
+                          cv::GArray<uint64_t>,
+                          cv::GArray<int>>      track(const cv::GMat& mat,


minor: please put track() on a new line after its monstrous return value type

dmatveev · 2023-12-14T16:07:09Z

modules/gapi/misc/python/package/gapi/__init__.py

        def __new__(self):
            return cv.GOpaqueT(cv.gapi.CV_INT)

+    class Int64():


One of the return types of OT I guess.

dmatveev · 2023-12-14T16:12:09Z

modules/gapi/misc/python/test/test_gapi_ot.py

+        def test_skip():
+            pass
+
+    pass


dmatveev · 2023-12-14T16:12:44Z

modules/gapi/misc/python/test/test_gapi_ot.py

+            res = comp.apply(cv.gin(in_image, in_rects, in_rects_cls),
+                             args=cv.gapi.compile_args(cv.gapi.ot.cpu.kernels()))


Should the output be tested somehow? How the LOST/NEW/TRACK status look in Python now?

AsyaPronina · 2023-12-14T17:51:40Z

Hello Dear Alexander (@asmorkalov), Dear Alexander (@opencv-alalek), Dear Maksim (@mshabunin), could you please merge this PR?

smirnov-alexey · 2023-12-15T13:25:53Z

@asmorkalov @mshabunin can we merge it today pretty please?

mshabunin

Looks good to me.

AsyaPronina requested review from TolyaTalamanov, asmorkalov, dmatveev, mshabunin and smirnov-alexey November 22, 2023 14:59

AsyaPronina assigned smirnov-alexey Nov 22, 2023

AsyaPronina force-pushed the ot_to_python branch from 651280a to ce4267f Compare November 22, 2023 15:12

AsyaPronina force-pushed the ot_to_python branch 3 times, most recently from 6e25e93 to 50a7c5e Compare December 9, 2023 00:12

AsyaPronina added category: python bindings category: g-api / gapi labels Dec 9, 2023

AsyaPronina force-pushed the ot_to_python branch from 50a7c5e to 4521894 Compare December 9, 2023 00:17

TolyaTalamanov reviewed Dec 12, 2023

View reviewed changes

TolyaTalamanov reviewed Dec 13, 2023

View reviewed changes

dmatveev reviewed Dec 13, 2023

View reviewed changes

AsyaPronina force-pushed the ot_to_python branch 9 times, most recently from 1a52653 to bf5c877 Compare December 14, 2023 01:24

AsyaPronina force-pushed the ot_to_python branch 3 times, most recently from ab17bed to 87b6096 Compare December 14, 2023 14:35

TolyaTalamanov reviewed Dec 14, 2023

View reviewed changes

dmatveev approved these changes Dec 14, 2023

View reviewed changes

dmatveev self-assigned this Dec 14, 2023

dmatveev added this to the 4.9.0 milestone Dec 14, 2023

dmatveev reviewed Dec 14, 2023

View reviewed changes

AsyaPronina force-pushed the ot_to_python branch 2 times, most recently from be67f3c to 9911ac9 Compare December 14, 2023 16:51

TolyaTalamanov approved these changes Dec 14, 2023

View reviewed changes

mshabunin approved these changes Dec 15, 2023

View reviewed changes

AsyaPronina force-pushed the ot_to_python branch from 9911ac9 to d20727a Compare December 19, 2023 17:51

Enabled VAS OT in G-API Python interface

d20727a

asmorkalov merged commit abbd878 into opencv:4.x Dec 20, 2023

asmorkalov mentioned this pull request Jan 19, 2024

5.x merge 4.x #24862

Merged


		cv.gapi.wip.GStreamerPipeline = cv.gapi_wip_gst_GStreamerPipeline

		class TrackingStatus:

		GAPI_EXPORTS IOStream& operator<< (IOStream& os, const cv::gapi::ot::TrackingStatus& status);
		GAPI_EXPORTS IIStream& operator>> (IIStream& is, cv::gapi::ot::TrackingStatus& status);

		res = comp.apply(cv.gin(in_image, in_rects, in_rects_cls),
		args=cv.gapi.compile_args(cv.gapi.ot.cpu.kernels()))

Uh oh!

Conversation

AsyaPronina commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

dmatveev commented Dec 1, 2023

Uh oh!

TolyaTalamanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TolyaTalamanov commented Dec 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TolyaTalamanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AsyaPronina commented Dec 14, 2023

Uh oh!

AsyaPronina commented Dec 14, 2023

Uh oh!

TolyaTalamanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AsyaPronina Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AsyaPronina commented Nov 22, 2023 •

edited

Loading

AsyaPronina Dec 14, 2023 •

edited

Loading