Message ID | 20210914121700.122591-2-umang.jain@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Le mardi 14 septembre 2021 à 17:46 +0530, Umang Jain a écrit : > In gstreamer, when elements are created, usually a floating [1] > reference is returned which simply means, there is no ownership > transfer (yet). Once can simply check for NULL and return through > an error path, without bothering to clean up. Hence, g_autoptr is > not much of help here. > > If the NULL checks have been passed successfully, elements are ready > to use. However, we must claim ownership/reference it before using > them via g_object_ref_sink(). > > This patch build upon this principle and removes the g_autoptr > from gstreamer tests whereever necessary to tide up the code. > > [1] https://gstreamer.freedesktop.org/documentation/additional/design/MT-refcounting.html?gi-language=c#refcounting1 > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> Note, I still made a comment below .... > --- > test/gstreamer/gstreamer_single_stream_test.cpp | 14 ++++++-------- > test/gstreamer/gstreamer_test.cpp | 9 ++++----- > 2 files changed, 10 insertions(+), 13 deletions(-) > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > index 7292f328..d992455c 100644 > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > @@ -33,20 +33,18 @@ protected: > if (status_ != TestPass) > return status_; > > - g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0"); > - g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0"); > - g_object_ref_sink(convert0); > - g_object_ref_sink(sink0); > + convert0_ = gst_element_factory_make("videoconvert", "convert0"); > + sink0_ = gst_element_factory_make("fakesink", "sink0"); > > - if (!convert0 || !sink0) { > + if (!convert0_ || !sink0_) { > g_printerr("Not all elements could be created. %p.%p\n", > - convert0, sink0); > + convert0_, sink0_); > > return TestFail; > } > > - convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0)); > - sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0)); > + g_object_ref_sink(convert0_); > + g_object_ref_sink(sink0_); I prefer when ref_sink are inline or close to their allocation. > > if (createPipeline() != TestPass) > return TestFail; > diff --git a/test/gstreamer/gstreamer_test.cpp b/test/gstreamer/gstreamer_test.cpp > index 41712505..227a5c37 100644 > --- a/test/gstreamer/gstreamer_test.cpp > +++ b/test/gstreamer/gstreamer_test.cpp > @@ -78,18 +78,17 @@ GstreamerTest::~GstreamerTest() > > int GstreamerTest::createPipeline() > { > - g_autoptr(GstElement) libcameraSrc = gst_element_factory_make("libcamerasrc", "libcamera"); > + libcameraSrc_ = gst_element_factory_make("libcamerasrc", "libcamera"); > pipeline_ = gst_pipeline_new("test-pipeline"); > - g_object_ref_sink(libcameraSrc); > > - if (!libcameraSrc || !pipeline_) { > + if (!libcameraSrc_ || !pipeline_) { > g_printerr("Unable to create create pipeline %p.%p\n", > - libcameraSrc, pipeline_); > + libcameraSrc_, pipeline_); > > return TestFail; > } > > - libcameraSrc_ = reinterpret_cast<GstElement *>(g_steal_pointer(&libcameraSrc)); > + g_object_ref_sink(libcameraSrc_); > > return TestPass; > }
diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp index 7292f328..d992455c 100644 --- a/test/gstreamer/gstreamer_single_stream_test.cpp +++ b/test/gstreamer/gstreamer_single_stream_test.cpp @@ -33,20 +33,18 @@ protected: if (status_ != TestPass) return status_; - g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0"); - g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0"); - g_object_ref_sink(convert0); - g_object_ref_sink(sink0); + convert0_ = gst_element_factory_make("videoconvert", "convert0"); + sink0_ = gst_element_factory_make("fakesink", "sink0"); - if (!convert0 || !sink0) { + if (!convert0_ || !sink0_) { g_printerr("Not all elements could be created. %p.%p\n", - convert0, sink0); + convert0_, sink0_); return TestFail; } - convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0)); - sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0)); + g_object_ref_sink(convert0_); + g_object_ref_sink(sink0_); if (createPipeline() != TestPass) return TestFail; diff --git a/test/gstreamer/gstreamer_test.cpp b/test/gstreamer/gstreamer_test.cpp index 41712505..227a5c37 100644 --- a/test/gstreamer/gstreamer_test.cpp +++ b/test/gstreamer/gstreamer_test.cpp @@ -78,18 +78,17 @@ GstreamerTest::~GstreamerTest() int GstreamerTest::createPipeline() { - g_autoptr(GstElement) libcameraSrc = gst_element_factory_make("libcamerasrc", "libcamera"); + libcameraSrc_ = gst_element_factory_make("libcamerasrc", "libcamera"); pipeline_ = gst_pipeline_new("test-pipeline"); - g_object_ref_sink(libcameraSrc); - if (!libcameraSrc || !pipeline_) { + if (!libcameraSrc_ || !pipeline_) { g_printerr("Unable to create create pipeline %p.%p\n", - libcameraSrc, pipeline_); + libcameraSrc_, pipeline_); return TestFail; } - libcameraSrc_ = reinterpret_cast<GstElement *>(g_steal_pointer(&libcameraSrc)); + g_object_ref_sink(libcameraSrc_); return TestPass; }
In gstreamer, when elements are created, usually a floating [1] reference is returned which simply means, there is no ownership transfer (yet). Once can simply check for NULL and return through an error path, without bothering to clean up. Hence, g_autoptr is not much of help here. If the NULL checks have been passed successfully, elements are ready to use. However, we must claim ownership/reference it before using them via g_object_ref_sink(). This patch build upon this principle and removes the g_autoptr from gstreamer tests whereever necessary to tide up the code. [1] https://gstreamer.freedesktop.org/documentation/additional/design/MT-refcounting.html?gi-language=c#refcounting1 Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- test/gstreamer/gstreamer_single_stream_test.cpp | 14 ++++++-------- test/gstreamer/gstreamer_test.cpp | 9 ++++----- 2 files changed, 10 insertions(+), 13 deletions(-)