[G-API] Added Y, UV accessors for cv::MediaFrame by AsyaPronina · Pull Request #19325 · opencv/opencv

AsyaPronina · 2021-01-15T01:19:53Z

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 other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to 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

This PR adds two operations to retrieve Y and UV GMat-s from GFrame.

force_builders=Custom,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

build_image:Custom=centos:7
buildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

build_image:Custom=ubuntu-openvino-2021.1.0:20.04
build_image:Custom Win=openvino-2021.1.0
build_image:Custom Mac=openvino-2021.1.0

test_modules:Custom=gapi
test_modules:Custom Win=gapi
test_modules:Custom Mac=gapi

buildworker:Custom=linux-1
// disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

TolyaTalamanov · 2021-01-18T13:38:02Z

Consider to use openvino:2021

TolyaTalamanov

Good, but let's discuss adapter logic locally

modules/gapi/include/opencv2/gapi/streaming/format.hpp

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

modules/gapi/src/backends/streaming/gstreamingbackend.cpp

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

modules/gapi/src/backends/streaming/gstreamingbackend.cpp

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

modules/gapi/src/backends/streaming/gstreamingbackend.cpp

modules/gapi/test/streaming/gapi_streaming_tests.cpp

TolyaTalamanov

LGTM 👍

Great, thanks!

dmatveev · 2021-03-19T17:32:09Z

modules/gapi/include/opencv2/gapi/streaming/format.hpp

 };

+G_API_OP(GY, <GMat(GFrame)>, "org.opencv.streaming.Y") {
+    static GMatDesc outMeta(const GFrameDesc frameDesc) {


dmatveev · 2021-03-19T17:32:16Z

modules/gapi/include/opencv2/gapi/streaming/format.hpp

+};
+
+G_API_OP(GUV, <GMat(GFrame)>, "org.opencv.streaming.UV") {
+    static GMatDesc outMeta(const GFrameDesc frameDesc) {


dmatveev · 2021-03-19T17:33:17Z

modules/gapi/include/opencv2/gapi/streaming/format.hpp


+/** @brief Extracts Y plane from media frame.
+
+Output image shall be 8-bit 1-channel image of @ref CV_8UC1.


is. "shall be" assumes some requirement or expectation set, but it is not the case.

dmatveev · 2021-03-19T17:33:30Z

modules/gapi/include/opencv2/gapi/streaming/format.hpp

+
+/** @brief Extracts UV plane from media frame.
+
+Output image shall be 8-bit 2-channels image of @ref CV_8UC2.


dmatveev · 2021-03-19T18:08:39Z

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

    {
-        explicit RMatMediaAdapterBGR(const cv::MediaFrame& frame) : m_frame(frame) { };
+        explicit RMatMediaFrameAdapter(const cv::MediaFrame& frame,
+            std::function<cv::Mat(const GFrameDesc&, const cv::MediaFrame::View&)> frameViewToMat) :


This looks weird, why do we need a callback like this here?

Got the idea after reviewing the rest of this code. I'd recommend to give this std::function a name (right within this class). See how our kernels are implemented in e.g. Fluid backend.

dmatveev · 2021-03-19T18:09:50Z

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

+            m_frameViewToMat(frameViewToMat)
+        {
+            const auto& d = m_frame.desc();
+            const auto& fv = m_frame.access(cv::MediaFrame::Access::R);


I'd delay this to the actual access - why do we map MediaFrame's data immediately?

Is there a performance benefit in our case?

Ah you only do it to obtain meta early -- then I'd move it to that point.

Also, if you'll do this, please take care of concurrency.

Removed this part.
Implemented retrieving of meta as discussed internally.

dmatveev · 2021-03-19T18:11:07Z

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

-            auto ptr = reinterpret_cast<uchar*>(view.ptr[0]);
-            auto stride = view.stride[0];
+            auto fv = m_frame.access(a == cv::RMat::Access::R ? cv::MediaFrame::Access::R
+                                                              : cv::MediaFrame::Access::W);


There's no guarantee in only R and W being used exclusively, there may be RW introduced one day.

This code will break then.

Added mapping lamda, but it throws exception if cv::RMat::Access is not R or W for now.

dmatveev · 2021-03-19T18:21:39Z

modules/gapi/src/backends/streaming/gstreamingbackend.cpp

+                auto view = frame.access(cv::MediaFrame::Access::R);
+                cv::Mat tmp_bgr(desc.size, CV_8UC3, view.ptr[0], view.stride[0]);
+                cv::Mat yuv;
+                cvtColor(tmp_bgr, yuv, cv::COLOR_BGR2YUV_I420);


In case of BGR() accessor called on an NV12 MediaFrame, we simply throw an exception (except some target platform where that conversion is implemented in firmware).

Since in case of Y() accessor we actually do the conversion (on CPU - what is debatable for high resolutions we target to), I'd align the behavior between the accessors.

Also, since the on-the-fly conversion is costly, I'd put a warning but only once per actor - not for every frame we process. You might want to look at std::call_once for that.

We don't throw exception in the OCV BGR kernel also..

Added warning for every accessor in case of non-consistent input format.
Warning is thrown only once per frame.

dmatveev · 2021-03-24T04:45:02Z

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

+        using MapDescF = std::function<cv::GMatDesc(const GFrameDesc&)>;
+        using MapDataF = std::function<cv::Mat(const GFrameDesc&, const cv::MediaFrame::View&)>;
+
+        explicit RMatMediaFrameAdapter(const cv::MediaFrame& frame,


There is no need in explicit anymore as the constructor takes >1 parameter

dmatveev · 2021-03-24T04:47:56Z

modules/gapi/src/backends/streaming/gstreamingbackend.cpp

+        {
+            std::call_once(m_warnFlag,
+                [](){
+                    GAPI_LOG_WARNING(NULL, "On-fly conversion from NV12 to BGR will be happened.");


"On-the-fly", "will happen". I'd also put a recommendation here so user could figure out how to change his code to avoid that (and briefly explain why it is a problem)

dmatveev

Please merge this now and fix the text later.

dmatveev · 2021-03-24T12:24:28Z

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

+                        cv::util::throw_error(std::logic_error("cv::RMat::Access::R or "
+                            "cv::RMat::Access::W can only be mapped to cv::MediaFrame::Access!"));


I believe this text is misleading and it'd be enough to only put an assert here, saying that only R/W are accepted.

Please do it later (not in this PR).

dmatveev · 2021-03-24T12:26:19Z

modules/gapi/src/backends/streaming/gstreamingbackend.cpp

+            std::call_once(m_warnFlag,
+                [](){
+                    GAPI_LOG_WARNING(NULL, "\nOn-the-fly conversion from NV12 to BGR will happen.\n"
+                        "Conversion may cost a lot for images with high resolution.\n"


cost a lot -> may be costly.

Again, please fix it later.

dmatveev · 2021-03-24T12:27:07Z

modules/gapi/src/backends/streaming/gstreamingbackend.cpp

+                    GAPI_LOG_WARNING(NULL, "\nOn-the-fly conversion from NV12 to BGR will happen.\n"
+                        "Conversion may cost a lot for images with high resolution.\n"
+                        "To retrieve cv::Mat-s from NV12 cv::MediaFrame for free, you may use "
+                        "cv::gapi::streaming::Y and cv::gapi::streaming::UV accessors.\n");


This may be misleading too since Y and UV are working at cv::GMat level (I mean, for user), there's no Mats etc.

dmatveev · 2021-03-24T12:27:35Z

modules/gapi/src/backends/streaming/gstreamingbackend.cpp

+                        "Conversion may cost a lot for images with high resolution.\n"
+                        "To retrieve cv::Mat from BGR cv::MediaFrame for free, you may use "
+                        "cv::gapi::streaming::BGR accessor.\n");


Same comment here

dmatveev · 2021-03-24T12:27:48Z

modules/gapi/src/backends/streaming/gstreamingbackend.cpp

+                    GAPI_LOG_WARNING(NULL, "\nOn-the-fly conversion from BGR to NV12 UV plane will "
+                        "happen.\n"
+                        "Conversion may cost a lot for images with high resolution.\n"
+                        "To retrieve cv::Mat from BGR cv::MediaFrame for free, you may use "


AsyaPronina · 2021-03-24T13:14:34Z

Please merge this now and fix the text later.

Thanks a lot!
@alalek, could you please merge this?

AsyaPronina requested review from TolyaTalamanov and rgarnov January 15, 2021 01:19

AsyaPronina force-pushed the asyadev/extract_y branch 3 times, most recently from 37060c6 to 2387594 Compare January 15, 2021 01:35

AsyaPronina requested a review from dmatveev January 15, 2021 01:36

AsyaPronina force-pushed the asyadev/extract_y branch 2 times, most recently from ef03dd7 to 033ac49 Compare January 15, 2021 18:06

TolyaTalamanov reviewed Jan 18, 2021

View reviewed changes

asmorkalov added the category: g-api / gapi label Jan 20, 2021

AsyaPronina commented Mar 2, 2021

View reviewed changes

modules/gapi/src/backends/streaming/gstreamingbackend.cpp Outdated Show resolved Hide resolved

AsyaPronina commented Mar 2, 2021

View reviewed changes

modules/gapi/src/backends/streaming/gstreamingbackend.cpp Outdated Show resolved Hide resolved

AsyaPronina commented Mar 2, 2021

View reviewed changes

modules/gapi/src/backends/streaming/gstreamingbackend.cpp Outdated Show resolved Hide resolved

AsyaPronina commented Mar 2, 2021

View reviewed changes

modules/gapi/src/backends/streaming/gstreamingbackend.cpp Outdated Show resolved Hide resolved

TolyaTalamanov reviewed Mar 4, 2021

View reviewed changes

AsyaPronina force-pushed the asyadev/extract_y branch 9 times, most recently from ae01eb2 to 4102140 Compare March 18, 2021 08:04

TolyaTalamanov reviewed Mar 18, 2021

View reviewed changes

AsyaPronina force-pushed the asyadev/extract_y branch from 4102140 to f346d64 Compare March 19, 2021 08:48

TolyaTalamanov self-requested a review March 19, 2021 12:03

TolyaTalamanov approved these changes Mar 19, 2021

View reviewed changes

AsyaPronina force-pushed the asyadev/extract_y branch 2 times, most recently from b44774c to d222419 Compare March 19, 2021 14:53

dmatveev reviewed Mar 19, 2021

View reviewed changes

AsyaPronina force-pushed the asyadev/extract_y branch 3 times, most recently from 0ab881f to 71fab18 Compare March 23, 2021 21:08

dmatveev reviewed Mar 24, 2021

View reviewed changes

Added Y, UV accessors to cv::MediaFrame

b0d66e0

AsyaPronina force-pushed the asyadev/extract_y branch from 71fab18 to b0d66e0 Compare March 24, 2021 09:35

dmatveev approved these changes Mar 24, 2021

View reviewed changes

alalek assigned dmatveev Mar 24, 2021

alalek merged commit 7949983 into opencv:master Mar 24, 2021

alalek mentioned this pull request Apr 9, 2021

(5.x) Merge 4.x #19885

Merged


		/** @brief Extracts Y plane from media frame.

		Output image shall be 8-bit 1-channel image of @ref CV_8UC1.


		/** @brief Extracts UV plane from media frame.

		Output image shall be 8-bit 2-channels image of @ref CV_8UC2.

		cv::util::throw_error(std::logic_error("cv::RMat::Access::R or "
		"cv::RMat::Access::W can only be mapped to cv::MediaFrame::Access!"));

Uh oh!

Conversation

AsyaPronina commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

This PR adds two operations to retrieve Y and UV GMat-s from GFrame.

Uh oh!

TolyaTalamanov commented Jan 18, 2021

Uh oh!

TolyaTalamanov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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

AsyaPronina commented Jan 15, 2021 •

edited

Loading