Ported GStreamerSource to OpenCV by AsyaPronina · Pull Request #20709 · opencv/opencv

AsyaPronina · 2021-09-16T22:36:52Z

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

Build configuration

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

Xbuild_image:Custom=centos:7
Xbuildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

build_image:Custom=ubuntu-openvino-2021.4.1:20.04
build_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
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

This PR is about porting of GStreamer streaming source for G-API pipeline functionality to public OpenCV.

sivanov-work

please pay attention about GStreamerPtr assigning pointer ourselves problem at least. Moreover, from my perspective, whole class implementation is danger

sivanov-work · 2021-09-20T07:10:26Z

modules/gapi/src/streaming/gstreamer/gstreamer_buffer_utils.cpp

+
+void mapBufferToFrame(GstBuffer* buffer, GstVideoInfo* info, GstVideoFrame* frame,
+                      GstMapFlags mapFlags) {
+    GAPI_Assert(info && "GstVideoInfo is absent during GstBuffer mapping");


You can get rid of validity checking by passing references as argument (for buffer and frame also)

Fixed, thanks!

modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamersource.hpp

sivanov-work · 2021-09-20T07:28:38Z

modules/gapi/src/streaming/gstreamer/gstreamer_media_adapter.cpp

+                                                  GST_MAP_WRITE);
+                m_mappedForWrite.store(true, std::memory_order_release);
+            } else {
+                gstreamer_utils::mapBufferToFrame(m_buffer, m_videoInfo, &m_videoFrame,


thread 1, R: goes here and make mapBufferToFrame
thread 2, W goes line 67-68 and make gst_video_frame_unmap AND put false in m_isMapped <context switch)
thread 1, R awakes and set m_isMapped to true at line 95.

Does this situation lead to broken invariants?

This situation should never happen according to framework design

modules/gapi/src/streaming/gstreamer/gstreamer_media_adapter.cpp

sivanov-work · 2021-09-20T08:40:29Z

modules/gapi/src/streaming/gstreamer/gstreamerptr.hpp

+    GStreamerPtr& operator=(GStreamerPtr&&) = delete;
+
+private:
+    T* ptr;


How's about to do not use home-grown implementation but default and highly-tested std::unique_ptr with custom dtor :)

tempate<...> using GStreamerPtr = std::unique_ptr<Type, decltype(GStreamerPtrRelease<Type>)> make_ptr(Type* ptr) { return GStreamerPtr (ptr, &GStreamerPtrRelease<Type>); }

}

It is very good suggestion

Fixed! Thanks!

sivanov-work · 2021-09-20T08:41:32Z

modules/gapi/src/streaming/gstreamer/gstreamersource.cpp

+    m_outputType(outputType)
+{
+    auto appsinks = m_pipeline->getElementsByFactoryName("appsink");
+    GAPI_Assert(1ul == appsinks.size());


What do you think to add warning log message additionally?

I think that this is very good idea

Fixed, thanks!

sivanov-work · 2021-09-20T08:42:22Z

modules/gapi/src/streaming/gstreamer/gstreamersource.cpp

+            }
+            break;
+        }
+        default: {


from my point of view we should handle this situation at construction time

I don't understand, what do you mean here? Could you please add some more details?

sure, I mean do not allow to create Source with invalid m_output here
https://github.com/opencv/opencv/pull/20709/files#diff-1387240a1379983779d72695d2b19e9c70195f80ab2784ef8dd3f4885cf78e3fR64

and to remove this switch-case like check from other part of a Source code

Thanks! Got the idea

Fixed, thanks!

sivanov-work · 2021-09-20T08:43:05Z

modules/gapi/src/streaming/gstreamer/gstreamersource.cpp

+    gst_app_sink_set_emit_signals(GST_APP_SINK(m_appsink.get()), FALSE);
+
+    GStreamerPtr<GstCaps> nv12Caps(gst_caps_from_string(
+        "video/x-raw,format=NV12;video/x-raw(memory:DMABuf),format=NV12")); /* transfer full */


suggest to make global constant

Fixed, thanks!

modules/gapi/src/streaming/gstreamer/gstreamersource.cpp

alalek · 2021-10-04T02:57:50Z

modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamerpipeline.hpp

+//
+// Copyright 2021 Intel Corporation.
+//
+// This software and the related documents are Intel copyrighted materials,


Use the right license header.

Thanks for the catch!

Fixed, thanks!

TolyaTalamanov · 2021-10-07T08:58:31Z

modules/gapi/CMakeLists.txt

+    src/streaming/gstreamer/gstreamersource.cpp
+    src/streaming/gstreamer/gstreamer_buffer_utils.cpp
+    src/streaming/gstreamer/gstreamer_media_adapter.cpp
+    src/streaming/gstreamer/gstreamerinit.cpp


Don't forget to add:
"${CMAKE_CURRENT_LIST_DIR}/include/opencv2/${name}/streaming/gstreamer/*.hpp" to gapi_ext_hdrs.
Could I also ask you to add:
"${CMAKE_CURRENT_LIST_DIR}/include/opencv2/${name}/streaming/onevpl/*.hpp" to there ?

Thanks! Sure!

Done, please check!

TolyaTalamanov · 2021-10-07T09:00:30Z

modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamersource.hpp

+    GStreamerSource(const std::string& pipeline,
+                    const GStreamerSource::OutputType outputType =
+                        GStreamerSource::OutputType::MAT);
+    GStreamerSource(std::shared_ptr<GStreamerPipelineFacade> pipeline,


Is the ctor really needed in public section ?
Since there is only forward declaration for GStreamerPipelineFacade it's supposed to be in private section

No, no, this one is used in GStreamerPipeline class, and these classes are not friends.

TolyaTalamanov · 2021-10-07T09:18:24Z

modules/gapi/test/streaming/gapi_gstreamersource_tests.cpp

+    cv::GComputation c(cv::GIn(in), cv::GOut(out));
+
+    // Graph compilation for streaming mode:
+    auto ccomp = c.compileStreaming(std::move(cv::compile_args(cv::gapi::core::cpu::kernels())));


U don't need to pass this package explicitly, do you ?

Yes, I don't, thanks!

Fixed, thanks!

TolyaTalamanov · 2021-10-07T09:49:30Z

I've opened this one: #20832
based on your branch

TolyaTalamanov · 2021-10-07T09:51:06Z

Whitespaces:

modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamersource.hpp:33: trailing whitespace.
+ * 
modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamersource.hpp:42: trailing whitespace.
+ *        
modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamersource.hpp:44: trailing whitespace.
+ * 
modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamersource.hpp:51: trailing whitespace.
+ * 
modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamersource.hpp:54: trailing whitespace.
+ * 
modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamersource.hpp:70: trailing whitespace.
+ 
modules/gapi/src/streaming/gstreamer/gstreamer_pipeline_facade.cpp:62: trailing whitespace.
+// The destructors are noexcept by default (since C++11). 
modules/gapi/src/streaming/gstreamer/gstreamer_pipeline_facade.hpp:69: trailing whitespace.
+    std::string m_pipelineDesc; 
modules/gapi/src/streaming/gstreamer/gstreamerinit.hpp:26: trailing whitespace.
+ * 
modules/gapi/src/streaming/gstreamer/gstreamerinit.hpp:27: trailing whitespace.
+ * 
modules/gapi/src/streaming/gstreamer/gstreamerptr.hpp:69: trailing whitespace.
+{ 
modules/gapi/src/streaming/gstreamer/gstreamersource_priv.hpp:60: trailing whitespace.
+    GstBuffer*                             m_buffer = nullptr; // Actual frame memory holder 
modules/gapi/test/streaming/gapi_gstreamer_pipeline_facade_int_tests.cpp:125: trailing whitespace.
+    GstStateChangeReturn status = 
modules/gapi/test/streaming/gapi_gstreamersource_tests.cpp:66: trailing whitespace.
+    // Streaming - pulling of frames until the end:  
modules/gapi/test/streaming/gapi_gstreamersource_tests.cpp:333: trailing whitespace.
+ 
modules/gapi/test/streaming/gapi_gstreamersource_tests.cpp:339: trailing whitespace.
+    cv::gapi::wip::Data leftImData, rightImData; 
modules/gapi/test/streaming/gapi_gstreamersource_tests.cpp:347: trailing whitespace.
+    std::tuple<cv::GComputation, cv::gapi::wip::GStreamerSource::OutputType> params = GetParam();

sivanov-work

LGTM

modules/gapi/src/streaming/gstreamer/gstreamer_media_adapter.cpp

sivanov-work · 2021-10-08T05:39:17Z

modules/gapi/src/streaming/gstreamer/gstreamer_pipeline_facade.cpp

+                "GStreamer pipeline has not been created!");
+
+    PipelineState state;
+    GstClockTime timeout = 5 * GST_SECOND;


Then what happened if query state would reach timeout?

modules/gapi/src/streaming/gstreamer/gstreamer_pipeline_facade.cpp

sivanov-work · 2021-10-08T05:47:18Z

modules/gapi/src/streaming/gstreamer/gstreamerptr.hpp

+    }
+}
+
+template<typename T> static inline void GStreamerPtrRelease(T** pPtr);


No, i mean - use determined specializations for several types of T, but when user or-someone-else just call this function with something not related to correct GStreamer resource, for Foo class for example. Then correct specialization would not be deducted and compiler generated declaration for Foo, which has no body definition and user would face with LINK error, at least

The good approach is do not wait for LINK error but break compilation with some message using static_assert

Something like that:

template<T> GStreamerRelease( T***) { static_assert(std::is_same<T, ...>::value, "Unsupported type in GStreamer Release") }

The part with std::is_same can be more intellectual with variadic templates

Feel free to ignore this comment

sivanov-work · 2021-10-08T05:53:58Z

modules/gapi/src/streaming/gstreamer/gstreamersource.cpp

+            }
+            break;
+        }
+        default: {


sure, I mean do not allow to create Source with invalid m_output here
https://github.com/opencv/opencv/pull/20709/files#diff-1387240a1379983779d72695d2b19e9c70195f80ab2784ef8dd3f4885cf78e3fR64

and to remove this switch-case like check from other part of a Source code

modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamer_pipeline.hpp

OrestChura · 2021-09-22T00:27:12Z

modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamersource.hpp

+ * To create GStreamerSource instance you need to pass 'pipeline' and, optionally, 'outputType'
+ * arguments into constructor.
+ * 'pipeline' should represent GStreamer pipeline in form of textual description.
+ * Almost any custom pipeline is supported which can be successfully ran via gst-launch.


Suggested change

* To create GStreamerSource instance you need to pass 'pipeline' and, optionally, 'outputType'

* arguments into constructor.

* 'pipeline' should represent GStreamer pipeline in form of textual description.

* Almost any custom pipeline is supported which can be successfully ran via gst-launch.

* To create a GStreamerSource instance you need to pass 'pipeline' and, optionally, 'outputType'

* arguments into constructor.

* 'pipeline' should represent a GStreamer pipeline in form of textual description.

* Almost any custom pipeline is supported which can be successfully run via gst-launch.

It is irregular verb, this is why "ran" is used here

Yes, but here is 3rd form needed, which is "run"

please consider to correct

modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamersource.hpp

modules/gapi/src/streaming/gstreamer/gstreamer_media_adapter.hpp

modules/gapi/src/streaming/gstreamer/gstreamer_buffer_utils.hpp

modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamersource.hpp

modules/gapi/src/streaming/gstreamer/gstreamer_pipeline_facade.hpp

modules/gapi/src/streaming/gstreamer/gstreamerpipeline_priv.hpp

AsyaPronina · 2021-10-15T17:53:30Z

GStreamer is built here; https://pullrequest.opencv.org/buildbot/builders/precommit_custom_linux/builds/6580/steps/cmake/logs/stdio

AsyaPronina · 2021-10-15T17:55:12Z

GStreamer tests: https://pullrequest.opencv.org/buildbot/builders/precommit_custom_linux/builds/6580/steps/test_gapi/logs/stdio

AsyaPronina · 2021-10-18T15:55:08Z

modules/gapi/src/streaming/gstreamer/gstreamerenv.hpp

+
+#ifndef OPENCV_GAPI_STREAMING_GSTREAMER_GStreamerEnv_HPP
+#define OPENCV_GAPI_STREAMING_GSTREAMER_GStreamerEnv_HPP
+


#ifdef HAVE_GSTREAMER?

OrestChura

Great!

OrestChura · 2021-10-18T16:41:37Z

modules/gapi/include/opencv2/gapi/streaming/gstreamer/gstreamersource.hpp

+ * To create GStreamerSource instance you need to pass 'pipeline' and, optionally, 'outputType'
+ * arguments into constructor.
+ * 'pipeline' should represent GStreamer pipeline in form of textual description.
+ * Almost any custom pipeline is supported which can be successfully ran via gst-launch.


please consider to correct

modules/gapi/src/streaming/gstreamer/gstreamer_pipeline_facade.cpp

…v/integrate_gstreamer_source

mpashchenkov · 2021-11-29T12:37:10Z

modules/gapi/test/streaming/gapi_gstreamer_pipeline_facade_int_tests.cpp

+    pipelineFacade.completePreroll();
+
+    cv::gapi::wip::gst::GStreamerPtr<GstSample> prerollSample(
+    //        gst_app_sink_try_pull_preroll(GST_APP_SINK(appsink), 5 * GST_SECOND));


Debug code?

Thanks for a catch!! This code should be version-dependent!

OrestChura · 2021-12-01T09:14:39Z

👍🏻

OrestChura · 2021-12-01T11:57:45Z

modules/gapi/test/streaming/gapi_gstreamersource_tests.cpp

+    std::tuple<cv::GComputation, cv::gapi::wip::GStreamerSource::OutputType> params = GetParam();
+    cv::GComputation extractImage = std::move(std::get<0>(params));
+    cv::gapi::wip::GStreamerSource::OutputType outputType = std::get<1>(params);


can std::tie() be used here instead?

Suggested change

std::tuple<cv::GComputation, cv::gapi::wip::GStreamerSource::OutputType> params = GetParam();

cv::GComputation extractImage = std::move(std::get<0>(params));

cv::gapi::wip::GStreamerSource::OutputType outputType = std::get<1>(params);

cv::GComputation extractImage;

cv::gapi::wip::GStreamerSource::OutputType outputType = /*default value*/ 0;

std::tie(extractImage, outputType ) = GetParam();

Is it because GComputation can't be declared like that?

dmatveev

Merge this please

alalek · 2021-12-06T11:40:35Z

PR should not be merged in current form due to broken compilation in Custom builder with GStreamer:

/build/precommit_custom_linux/4.x/opencv/modules/gapi/test/streaming/gapi_gstreamer_pipeline_facade_int_tests.cpp: In member function 'virtual void opencv_test::GStreamerPipelineFacadeTest_CompletePrerollUnitTest_Test::Body()':
/build/precommit_custom_linux/4.x/opencv/modules/gapi/test/streaming/gapi_gstreamer_pipeline_facade_int_tests.cpp:95:13: error: 'gst_app_sink_try_pull_prerollGST_APP_SINK' was not declared in this scope; did you mean 'gst_app_sink_try_pull_preroll'?
   95 |             gst_app_sink_try_pull_prerollGST_APP_SINK(appsink), 5 * GST_SECOND));
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |             gst_app_sink_try_pull_preroll
/build/precommit_custom_linux/4.x/opencv/modules/gapi/test/streaming/gapi_gstreamer_pipeline_facade_int_tests.cpp:95:80: error: expected ',' or ';' before ')' token
   95 |             gst_app_sink_try_pull_prerollGST_APP_SINK(appsink), 5 * GST_SECOND));
      |                                                                                ^
make[2]: *** [modules/gapi/CMakeFiles/opencv_test_gapi.dir/build.make:1025: modules/gapi/CMakeFiles/opencv_test_gapi.dir/test/streaming/gapi_gstreamer_pipeline_facade_int_tests.cpp.o] Error 1

…treamer_source Ported GStreamerSource to OpenCV * Ported GStreamerSource to OpenCV * Fixed CI failures * Whitespaces * Whitespaces + removed exception from destructors C4722 * Removed assert for Priv's getSS and descr_of * Removed assert for pull * Fixed last review comment Co-authored-by: Pashchenkov Maxim <maxim.pashchenkov@intel.com>

AsyaPronina requested review from TolyaTalamanov, dmatveev, rgarnov, sivanov-work and smirnov-alexey September 16, 2021 22:36

AsyaPronina force-pushed the asyadev/integrate_gstreamer_source branch 2 times, most recently from 891445e to 5a16a93 Compare September 16, 2021 23:55

asmorkalov added the category: g-api / gapi label Sep 17, 2021

sivanov-work reviewed Sep 20, 2021

View reviewed changes

AsyaPronina force-pushed the asyadev/integrate_gstreamer_source branch 4 times, most recently from 0557257 to 98ccf8f Compare October 1, 2021 22:29

alalek reviewed Oct 4, 2021

View reviewed changes

TolyaTalamanov reviewed Oct 7, 2021

View reviewed changes

AsyaPronina force-pushed the asyadev/integrate_gstreamer_source branch from 98ccf8f to 37339d2 Compare October 7, 2021 23:30

sivanov-work approved these changes Oct 8, 2021

View reviewed changes

OrestChura reviewed Oct 8, 2021

View reviewed changes

AsyaPronina force-pushed the asyadev/integrate_gstreamer_source branch 5 times, most recently from ff807b5 to 7c22705 Compare October 11, 2021 09:20

AsyaPronina commented Oct 18, 2021

View reviewed changes

OrestChura approved these changes Oct 18, 2021

View reviewed changes

AsyaPronina force-pushed the asyadev/integrate_gstreamer_source branch 5 times, most recently from 6cf3d58 to 0dd69f1 Compare November 14, 2021 21:58

AsyaPronina added 2 commits November 22, 2021 15:02

Ported GStreamerSource to OpenCV

80de5d2

Merge branch 'master' of https://github.com/opencv/opencv into asyade…

7a38fb0

…v/integrate_gstreamer_source

AsyaPronina force-pushed the asyadev/integrate_gstreamer_source branch from 0dd69f1 to 7a38fb0 Compare November 22, 2021 13:23

Fixed CI failures

94b0837

dmatveev added this to the 4.5.5 milestone Nov 26, 2021

mpashchenkov reviewed Nov 29, 2021

View reviewed changes

Pashchenkov Maxim added 4 commits November 29, 2021 15:40

Whitespaces

52a8992

Whitespaces + removed exception from destructors C4722

2b728e3

Removed assert for Priv's getSS and descr_of

4ac6ad2

Removed assert for pull

eec8141

OrestChura reviewed Dec 1, 2021

View reviewed changes

dmatveev self-assigned this Dec 6, 2021

dmatveev approved these changes Dec 6, 2021

View reviewed changes

Fixed last review comment

97fadfc

AsyaPronina force-pushed the asyadev/integrate_gstreamer_source branch from 5309176 to 97fadfc Compare December 6, 2021 14:42

alalek merged commit 8dd6882 into opencv:4.x Dec 6, 2021

alalek mentioned this pull request Dec 30, 2021

(5.x) Merge 4.x #21371

Merged

alalek mentioned this pull request Feb 22, 2022

(5.x) Merge 4.x #21651

Merged


		#ifndef OPENCV_GAPI_STREAMING_GSTREAMER_GStreamerEnv_HPP
		#define OPENCV_GAPI_STREAMING_GSTREAMER_GStreamerEnv_HPP

Uh oh!

Conversation

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

Uh oh!

Pull Request Readiness Checklist

Build configuration

Uh oh!

sivanov-work left a comment • 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

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!

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!

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!

TolyaTalamanov commented Oct 7, 2021

Uh oh!

TolyaTalamanov commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sivanov-work left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AsyaPronina commented Sep 16, 2021 •

edited

Loading

sivanov-work left a comment •

edited

Loading

TolyaTalamanov commented Oct 7, 2021 •

edited

Loading