Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add stateful kernel to python G-API #22037

Merged
merged 3 commits into from Sep 9, 2022
Merged

Add stateful kernel to python G-API #22037

merged 3 commits into from Sep 9, 2022

Conversation

xiong-jie-y
Copy link
Contributor

Please review both interface design and code. 馃檪

Motivation

Stateful kernel in C++ is really useful to many usecases like tracking.
This PR will enable python G-API users to use stateful kernel and expand the usecases in python G-API.

Feature Design

Here is the example video and the example code of this feature.
The example kernel is like this.

class State:
    def __init__(self, tracker, initial_box):
        self.tracker = tracker
        self.initial_box = initial_box

@cv.gapi.kernel(Tracker)
class TrackerImpl:
    @staticmethod
    def setup(input_image_desc, box):
        tracker = cv.TrackerKCF_create()
        return State(tracker, None)

    @staticmethod
    def run(image, box, state):
        if state.initial_box is None:
            state.tracker.init(image, box)
            state.initial_box = box
            return box
        else:
            ok, bbox = state.tracker.update(image)
            return bbox

Basically it create internal state in the setup method and pass it to the G-API framework by returning it.
run method recieves additional state argument and user can handle state with this argument.
It is quite similar to the C++ stateful kernel API GAPI_OCV_KERNEL_ST and difference is setup method return state to the framework. This is the interface that I'd like to ask you to review 馃檪

How to run the demo code?

  1. Build OpenCV as usual.
  2. Run pip install numpy click
  3. Run demo program by the following command.

For video input.

python track_video.py --input ${DEVICE_ID}

For video file

python track_video.py --input ${VIDEO_FILE_PATH}

TODO

  • Fix interface design
  • Add example PR following this PR.
  • Add unit test for this feature.

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 https://github.com/opencv/opencv/pull/17350/files
  • [] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
    -> No need for performance
  • The feature is well documented and sample code can be built with the project CMake
  • I will work on example code later.

@TolyaTalamanov
Copy link
Contributor

Hi @xiong-jie-y the proposal & design looks great the only question is that really should be exposed to python?
Could you have a look on this example of stateful kernel: https://github.com/opencv/opencv/blob/4.x/modules/gapi/misc/python/test/test_gapi_sample_pipelines.py#L435-L445

I think it should cover your needs.

@TolyaTalamanov TolyaTalamanov self-requested a review August 18, 2022 08:43
@dmatveev dmatveev assigned dmatveev and unassigned dmatveev Aug 25, 2022
@dmatveev
Copy link
Contributor

Hi @xiong-jie-y the proposal & design looks great the only question is that really should be exposed to python? Could you have a look on this example of stateful kernel: https://github.com/opencv/opencv/blob/4.x/modules/gapi/misc/python/test/test_gapi_sample_pipelines.py#L435-L445

I think it should cover your needs.

@TolyaTalamanov in your example, is the state shared across all instances of your kernel?

We do have stateful kernels as special objects in G-API since the state is managed explicitly and its lifetime is well-defined.

@TolyaTalamanov
Copy link
Contributor

Hi @xiong-jie-y the proposal & design looks great the only question is that really should be exposed to python? Could you have a look on this example of stateful kernel: https://github.com/opencv/opencv/blob/4.x/modules/gapi/misc/python/test/test_gapi_sample_pipelines.py#L435-L445
I think it should cover your needs.

@TolyaTalamanov in your example, is the state shared across all instances of your kernel?

We do have stateful kernels as special objects in G-API since the state is managed explicitly and its lifetime is well-defined.

Yes, it's shared across all instances

@dmatveev
Copy link
Contributor

Yes, it's shared across all instances

Thats not a way to go I believe :) Imagine running multiple trackers on multiple streams in the same app (and it is a pretty valid scenario).

We should prioritize merging the @xiong-jie-y 's feature than


cv::GRunArgs operator()(const GPythonContext& ctx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's redundant to wrap every stored std::function to method

@TolyaTalamanov
Copy link
Contributor

@dmatveev @xiong-jie-y @asmorkalov @alalek Folks, do you have any comments? Can it be merged?

@dmatveev dmatveev added this to the 4.7.0 milestone Sep 9, 2022
Copy link
Contributor

@dmatveev dmatveev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤 thanks Anatoliy for completing this

@asmorkalov asmorkalov merged commit 8661914 into opencv:4.x Sep 9, 2022
@TolyaTalamanov
Copy link
Contributor

馃挘

@alalek alalek mentioned this pull request Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants