G-API: Add ROI processing in VPP preproc #21658

sivanov-work · 2022-02-24T13:49:43Z

Add ROI processing for core part of VPP preproc which is essential from #21554

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

Build Configuration

force_builders=XCustom,Custom Win,Custom Mac
build_gapi_standalone:Linux x64=ade-0.1.1f
build_gapi_standalone:Win64=ade-0.1.1f
build_gapi_standalone:Mac=ade-0.1.1f
build_gapi_standalone:Linux x64 Debug=ade-0.1.1f
Xbuild_image:Custom=centos:7
Xbuildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f
build_image:Custom=ubuntu-openvino-2021.3.0:20.04
Xbuild_image:Custom Win=openvino-2021.2.0
build_image:Custom Mac=openvino-2021.2.0
test_modules:Custom=gapi,python2,python3,java
test_modules:Custom Win=gapi,python2,python3,java
test_modules:Custom Mac=gapi,python2,python3,java
buildworker:Custom=linux-1
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*
build_image:Custom Win=gapi-onevpl-2021.6.0
buildworker:Custom Win=windows-3
build_contrib:Custom Win=OFF

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_engine.cpp

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_session.cpp

modules/gapi/test/streaming/gapi_streaming_vpp_preproc_test.cpp

dmatveev · 2022-02-25T07:34:57Z

modules/gapi/test/streaming/gapi_streaming_vpp_preproc_test.cpp

+            decoded_frame = cv::MediaFrame();
+            preproc_number++;
+        }
+    } catch (...) {}


what's the reason to have catch here? Normally if ASSERT_TRUE fails you may catch it (I believe those macros throw exceptions in gtest as the overall test suite execution continues after that)

i don't exactly remember reasons why it was done as throwing exception (this code copy-pasted from older and older PR):
Maybe, I don't want to wrap each function in NO_THROW macro (3 at least), because as far i remember, some pp_* have no default ctors. Also in any tests throwing exception and breaking while-loop is expected behavior

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_engine.cpp

OrestChura

some cosmetic-rearranging comments

OrestChura · 2022-03-01T11:51:09Z

modules/gapi/test/streaming/gapi_streaming_vpp_preproc_test.cpp

-                        testing::ValuesIn(files));
-
-using roi_t = cv::Rect;
+using roi_t = cv::util::optional<cv::Rect>;


this using can be declared at :183 and be used instead of all cv::util::optional<cv::Rect> usages

OrestChura · 2022-03-01T12:07:06Z

modules/gapi/test/streaming/gapi_streaming_vpp_preproc_test.cpp

    preproc_roi_args_t {"highgui/video/big_buck_bunny.h265",
                    MFX_CODEC_HEVC,     MFX_ACCEL_MODE_VIA_D3D11,
                    out_frame_info_t{cv::GFrameDesc {cv::MediaFormat::NV12, {1920, 1280}}},
-                    roi_t{0,0,100,100}},
+                    roi_t{}},


you can use empty_roi variable here? (and on :455)

modules/gapi/test/streaming/gapi_streaming_vpp_preproc_test.cpp

rgarnov

Looks good

TolyaTalamanov · 2022-03-04T09:30:40Z

modules/gapi/test/streaming/gapi_streaming_vpp_preproc_test.cpp

@@ -181,6 +181,8 @@ using acceleration_t    = int;
 using out_frame_info_t  = cv::GFrameDesc;
 using preproc_args_t    = std::tuple<source_t, decoder_t, acceleration_t, out_frame_info_t>;

+static cv::util::optional<cv::Rect> empty_roi;


Why it's needed?
Does it make sense to just pass cv::optional<cv::Rect>{} ?

TolyaTalamanov · 2022-03-04T09:32:32Z

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

    TEST_CYCLE()
    {
        source_ptr->pull(out);
        cv::MediaFrame frame = cv::util::get<cv::MediaFrame>(out);
        cv::util::optional<pp_params> param = preproc_engine.is_applicable(frame);
        pp_session sess = preproc_engine.initialize_preproc(param.value(),
                                                            required_frame_param);
-        (void)preproc_engine.run_sync(sess, frame);
+        (void)preproc_engine.run_sync(sess, frame, empty_roi);


Honestly, I didn't understand how it works
Does it do crop if roi is passed and doesn't do crop otherwise.

optinal cv::Rect passed through engine and falls in pipeline execution, please check apply_roi:
https://github.com/opencv/opencv/pull/21658/files#diff-d281efabce2ff069582bd99d9c40e910c4386b65d60010ed9e4b106f4f2b237bR39

If no roi stored in optional then no crop operation would happen

dmatveev

👍 thanks

dmatveev · 2022-03-04T23:41:31Z

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_session.cpp

+    memcpy(&(original_surface_ptr->Info),
+           &original_frame_info, sizeof(Surface::info_t));


This may be way to C-ish, and unsafe for C++ data types if any. Would operator= work better where? Just wondering

It is packed C-structure in VPL/MFX without operator=. I didn't implement our custom operator= because it doesn't protect us from VPL API changes ( when new fields will added in such c-structure from vpl side)

sivanov-work · 2022-03-05T05:46:11Z

@alalek Could you merge it please? Minor comments elapsed which may be implemented in the next PRs

G-API: Add ROI processing in VPP preproc * Add ROI in VPP prepro * Apply comments

Add ROI in VPP prepro

401a699

sivanov-work requested review from dmatveev, rgarnov, OrestChura and TolyaTalamanov February 24, 2022 13:49

sivanov-work added the category: g-api / gapi label Feb 24, 2022

sivanov-work mentioned this pull request Feb 24, 2022

G-API: oneVPL Add ROI support in VPP IE preproc #21554

Closed

6 tasks

OrestChura mentioned this pull request Feb 24, 2022

G-API: Add VPL/VPP preproc core module #21618

Merged

6 tasks

OrestChura reviewed Feb 25, 2022

View reviewed changes

dmatveev reviewed Feb 25, 2022

View reviewed changes

Apply comments

e692ddf

OrestChura approved these changes Mar 1, 2022

View reviewed changes

dmatveev self-assigned this Mar 1, 2022

dmatveev added this to the 4.6.0 milestone Mar 1, 2022

rgarnov reviewed Mar 1, 2022

View reviewed changes

modules/gapi/test/streaming/gapi_streaming_vpp_preproc_test.cpp Show resolved Hide resolved

rgarnov approved these changes Mar 3, 2022

View reviewed changes

TolyaTalamanov reviewed Mar 4, 2022

View reviewed changes

dmatveev approved these changes Mar 4, 2022

View reviewed changes

alalek merged commit 44c2c77 into opencv:4.x Mar 5, 2022

opencv-pushbot mentioned this pull request Apr 23, 2022

(5.x) Merge 4.x #21903

Merged

a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023

Merge pull request opencv#21658 from sivanov-work:vpp_core_add_roi

f4f74e6

G-API: Add ROI processing in VPP preproc * Add ROI in VPP prepro * Apply comments

		memcpy(&(original_surface_ptr->Info),
		&original_frame_info, sizeof(Surface::info_t));

Uh oh!

G-API: Add ROI processing in VPP preproc #21658

G-API: Add ROI processing in VPP preproc #21658

Uh oh!

Conversation

sivanov-work commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Build Configuration

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

OrestChura 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!

Uh oh!

rgarnov 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!

dmatveev 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!

sivanov-work commented Mar 5, 2022

Uh oh!

Uh oh!

sivanov-work commented Feb 24, 2022 •

edited

Loading