[libcamera-devel] [PATCH v3] test: gstreamer: Factor out code into a base class
Vedant Paranjape
vedantparanjape160201 at gmail.com
Wed Sep 8 06:14:58 CEST 2021
Hi Paul,
Thanks for the review.
On Wed, Sep 8, 2021 at 9:30 AM <paul.elder at ideasonboard.com> wrote:
> Hi Vedant,
>
> On Wed, Sep 01, 2021 at 01:32:17AM +0530, Vedant Paranjape wrote:
> > Lot of code used in single stream test is boiler plate and common across
> > every gstreamer test. Factored out this code into a base class called
> > GstreamerTest.
> >
> > Also updated the gstreamer_single_stream_test to use the GstreamerTest
> > base class
> >
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> > ---
> > .../gstreamer_single_stream_test.cpp | 145 +++-------------
> > test/gstreamer/gstreamer_test.cpp | 157 ++++++++++++++++++
> > test/gstreamer/gstreamer_test.h | 39 +++++
> > test/gstreamer/meson.build | 2 +-
> > 4 files changed, 221 insertions(+), 122 deletions(-)
> > create mode 100644 test/gstreamer/gstreamer_test.cpp
> > create mode 100644 test/gstreamer/gstreamer_test.h
> >
> > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp
> b/test/gstreamer/gstreamer_single_stream_test.cpp
> > index 4c8d4804..c27e4d36 100644
> > --- a/test/gstreamer/gstreamer_single_stream_test.cpp
> > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp
> > @@ -14,104 +14,48 @@
> >
> > #include <gst/gst.h>
> >
> > +#include "gstreamer_test.h"
> > #include "test.h"
> >
> > using namespace std;
> >
> > -extern "C" {
> > -const char *__asan_default_options()
> > -{
> > - /*
> > - * Disable leak detection due to a known global variable
> initialization
> > - * leak in glib's g_quark_init(). This should ideally be handled by
> > - * using a suppression file instead of disabling leak detection.
> > - */
> > - return "detect_leaks=false";
> > -}
> > -}
> > -
> > -class GstreamerSingleStreamTest : public Test
> > +class GstreamerSingleStreamTest : public GstreamerTest, public Test
> > {
> > +public:
> > + GstreamerSingleStreamTest()
> > + : GstreamerTest()
> > + {
> > + }
> > +
> > protected:
> > int init() override
> > {
> > - /*
> > - * GStreamer by default spawns a process to run the
> > - * gst-plugin-scanner helper. If libcamera is compiled
> with ASan
> > - * enabled, and as GStreamer is most likely not, this
> causes the
> > - * ASan link order check to fail when gst-plugin-scanner
> > - * dlopen()s the plugin as many libraries will have
> already been
> > - * loaded by then. Fix this issue by disabling spawning of
> a
> > - * child helper process when scanning the build directory
> for
> > - * plugins.
> > - */
> > - gst_registry_fork_set_enabled(false);
> > -
> > - /* Initialize GStreamer */
> > - g_autoptr(GError) errInit = NULL;
> > - if (!gst_init_check(nullptr, nullptr, &errInit)) {
> > - g_printerr("Could not initialize GStreamer: %s\n",
> > - errInit ? errInit->message : "unknown
> error");
> > + if (status_ != TestPass)
> > + return status_;
> >
> > - return TestFail;
> > - }
> > + g_autoptr(GstElement) convert0__ =
> gst_element_factory_make("videoconvert", "convert0");
>
> For local variables, you just remove the ending underscore altogether
> (also the other instances where you do so).
>
Okay, done !
> + g_autoptr(GstElement) sink0__ =
> gst_element_factory_make("fakesink", "sink0");
> > + g_object_ref_sink(convert0__);
> > + g_object_ref_sink(sink0__);
> >
> > - /*
> > - * Remove the system libcamera plugin, if any, and add the
> > - * plugin from the build directory.
> > - */
> > - GstRegistry *registry = gst_registry_get();
> > - GstPlugin *plugin = gst_registry_lookup(registry,
> "libgstlibcamera.so");
> > - if (plugin) {
> > - gst_registry_remove_plugin(registry, plugin);
> > - gst_object_unref(plugin);
> > - }
> > + if (!convert0__ || !sink0__) {
> > + g_printerr("Not all elements could be created.
> %p.%p\n",
> > + convert0__, sink0__);
> >
> > - std::string path = libcamera::utils::libcameraBuildPath()
> > - + "src/gstreamer";
> > - if (!gst_registry_scan_path(registry, path.c_str())) {
> > - g_printerr("Failed to add plugin to registry\n");
> > - gst_deinit();
> > return TestFail;
> > }
> >
> > - /* Create the elements */
> > - libcameraSrc_ = gst_element_factory_make("libcamerasrc",
> "libcamera");
> > - convert0_ = gst_element_factory_make("videoconvert",
> "convert0");
> > - sink0_ = gst_element_factory_make("fakesink", "sink0");
> > -
> > - /* Create the empty pipeline_ */
> > - pipeline_ = gst_pipeline_new("test-pipeline");
> > -
> > - if (!pipeline_ || !convert0_ || !sink0_ || !libcameraSrc_)
> {
> > - g_printerr("Not all elements could be created.
> %p.%p.%p.%p\n",
> > - pipeline_, convert0_, sink0_,
> libcameraSrc_);
> > - if (pipeline_)
> > - gst_object_unref(pipeline_);
> > - if (convert0_)
> > - gst_object_unref(convert0_);
> > - if (sink0_)
> > - gst_object_unref(sink0_);
> > - if (libcameraSrc_)
> > - gst_object_unref(libcameraSrc_);
> > - gst_deinit();
> > + convert0_ = reinterpret_cast<GstElement
> *>(g_steal_pointer(&convert0__));
> > + sink0_ = reinterpret_cast<GstElement
> *>(g_steal_pointer(&sink0__));
> >
> > + if (gstreamer_create_pipeline() != TestPass)
> > return TestFail;
> > - }
> >
> > return TestPass;
> > }
> >
> > - void cleanup() override
> > - {
> > - gst_object_unref(pipeline_);
> > - gst_deinit();
> > - }
> > -
> > int run() override
> > {
> > - GstStateChangeReturn ret;
> > -
> > /* Build the pipeline */
> > gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_,
> convert0_, sink0_, NULL);
> > if (gst_element_link_many(libcameraSrc_, convert0_,
> sink0_, NULL) != TRUE) {
> > @@ -119,57 +63,16 @@ protected:
> > return TestFail;
> > }
> >
> > - /* Start playing */
> > - ret = gst_element_set_state(pipeline_, GST_STATE_PLAYING);
> > - if (ret == GST_STATE_CHANGE_FAILURE) {
> > - g_printerr("Unable to set the pipeline to the
> playing state.\n");
> > + if (gstreamer_start_pipeline() != TestPass)
> > return TestFail;
> > - }
> >
> > - /* Wait until error or EOS or timeout after 2 seconds */
> > - constexpr GstMessageType msgType =
> > - static_cast<GstMessageType>(GST_MESSAGE_ERROR |
> GST_MESSAGE_EOS);
> > - constexpr GstClockTime timeout = 2 * GST_SECOND;
> > -
> > - g_autoptr(GstBus) bus = gst_element_get_bus(pipeline_);
> > - g_autoptr(GstMessage) msg =
> gst_bus_timed_pop_filtered(bus, timeout, msgType);
> > -
> > - gst_element_set_state(pipeline_, GST_STATE_NULL);
> > -
> > - /* Parse error message */
> > - if (msg == NULL)
> > - return TestPass;
> > -
> > - switch (GST_MESSAGE_TYPE(msg)) {
> > - case GST_MESSAGE_ERROR:
> > - gstreamer_print_error(msg);
> > - break;
> > - case GST_MESSAGE_EOS:
> > - g_print("End-Of-Stream reached.\n");
> > - break;
> > - default:
> > - g_printerr("Unexpected message received.\n");
> > - break;
> > - }
> > + if (gstreamer_process_event() != TestPass)
> > + return TestFail;
> >
> > - return TestFail;
> > + return TestPass;
> > }
> >
> > private:
> > - void gstreamer_print_error(GstMessage *msg)
> > - {
> > - g_autoptr(GError) err = NULL;
> > - g_autofree gchar *debug_info = NULL;
> > -
> > - gst_message_parse_error(msg, &err, &debug_info);
> > - g_printerr("Error received from element %s: %s\n",
> > - GST_OBJECT_NAME(msg->src), err->message);
> > - g_printerr("Debugging information: %s\n",
> > - debug_info ? debug_info : "none");
> > - }
> > -
> > - GstElement *pipeline_;
> > - GstElement *libcameraSrc_;
> > GstElement *convert0_;
> > GstElement *sink0_;
> > };
> > diff --git a/test/gstreamer/gstreamer_test.cpp
> b/test/gstreamer/gstreamer_test.cpp
> > new file mode 100644
> > index 00000000..22128c4c
> > --- /dev/null
> > +++ b/test/gstreamer/gstreamer_test.cpp
> > @@ -0,0 +1,157 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2021, Vedant Paranjape
> > + *
> > + * libcamera Gstreamer element API tests
> > + */
> > +
> > +#include "gstreamer_test.h"
> > +
> > +#include "test.h"
> > +
> > +using namespace std;
> > +
> > +extern "C" {
> > +const char *__asan_default_options()
> > +{
> > + /*
> > + * Disable leak detection due to a known global variable
> initialization
> > + * leak in glib's g_quark_init(). This should ideally be handled by
> > + * using a suppression file instead of disabling leak detection.
> > + */
> > + return "detect_leaks=false";
> > +}
> > +}
> > +
> > +GstreamerTest::GstreamerTest()
> > +{
> > + /*
> > + * GStreamer by default spawns a process to run the
> > + * gst-plugin-scanner helper. If libcamera is compiled with ASan
> > + * enabled, and as GStreamer is most likely not, this causes the
> > + * ASan link order check to fail when gst-plugin-scanner
> > + * dlopen()s the plugin as many libraries will have already been
> > + * loaded by then. Fix this issue by disabling spawning of a
> > + * child helper process when scanning the build directory for
> > + * plugins.
> > + */
> > + gst_registry_fork_set_enabled(false);
> > +
> > + /* Initialize GStreamer */
> > + g_autoptr(GError) errInit = NULL;
> > + if (!gst_init_check(nullptr, nullptr, &errInit)) {
> > + g_printerr("Could not initialize GStreamer: %s\n",
> > + errInit ? errInit->message : "unknown error");
> > +
> > + status_ = TestFail;
> > + return;
> > + }
> > +
> > + /*
> > + * Remove the system libcamera plugin, if any, and add the
> > + * plugin from the build directory.
> > + */
> > + GstRegistry *registry = gst_registry_get();
> > + g_autoptr(GstPlugin) plugin = gst_registry_lookup(registry,
> "libgstlibcamera.so");
> > + if (plugin) {
> > + gst_registry_remove_plugin(registry, plugin);
> > + }
>
> You don't need braces here.
>
> > +
> > + std::string path = libcamera::utils::libcameraBuildPath() +
> "src/gstreamer";
> > + if (!gst_registry_scan_path(registry, path.c_str())) {
> > + g_printerr("Failed to add plugin to registry\n");
> > +
> > + status_ = TestFail;
> > + return;
> > + }
> > +
> > + status_ = TestPass;
> > +}
> > +
> > +GstreamerTest::~GstreamerTest()
> > +{
> > + if (pipeline_)
> > + gst_object_unref(pipeline_);
> > + if (status_ == TestFail) {
> > + gst_object_unref(libcameraSrc_);
> > + }
>
> You don't need braces here.
>
> How come you unref libcameraSrc_ but not the others upon failure?
> What's the difference between libcameraSrc_ and the other pipeline
> components? Is TestFail both sufficient and necessary to need to free
> libcameraSrc_?
>
This is because the other elements are added to the pipeline, once that is
done successfully the pipeline owns them and takes care of their existence.
So, there's going to be a minor change here though. But, in this case,
since create pipeline is called before the elements are added to the
pipeline, I need
to take care of libcameraSrc.
> +
> > + gst_deinit();
> > +}
> > +
> > +int GstreamerTest::gstreamer_create_pipeline()
>
> All of these functions should be camelCase. Also since they're all
> members of GstreamerTest, I think you can drop the gstreamer prefix too.
>
Okay
> +{
> > + g_autoptr(GstElement) libcameraSrc__ =
> gst_element_factory_make("libcamerasrc", "libcamera");
> > + pipeline_ = gst_pipeline_new("test-pipeline");
> > + g_object_ref_sink(libcameraSrc__);
>
> You can just drop the underscores completely.
>
>
> Hm other than those, I think it's ready.
>
>
> Paul
>
> > +
> > + if (!libcameraSrc__ || !pipeline_) {
> > + g_printerr("Unable to create create pipeline %p.%p\n",
> > + libcameraSrc__, pipeline_);
> > +
> > + return TestFail;
> > + }
> > +
> > + libcameraSrc_ = reinterpret_cast<GstElement
> *>(g_steal_pointer(&libcameraSrc__));
> > +
> > + return TestPass;
> > +}
> > +
> > +int GstreamerTest::gstreamer_start_pipeline()
> > +{
> > + GstStateChangeReturn ret;
> > +
> > + /* Start playing */
> > + ret = gst_element_set_state(pipeline_, GST_STATE_PLAYING);
> > + if (ret == GST_STATE_CHANGE_FAILURE) {
> > + g_printerr("Unable to set the pipeline to the playing
> state.\n");
> > + return TestFail;
> > + }
> > +
> > + return TestPass;
> > +}
> > +
> > +int GstreamerTest::gstreamer_process_event()
> > +{
> > + /* Wait until error or EOS or timeout after 2 seconds */
> > + constexpr GstMessageType msgType =
> > + static_cast<GstMessageType>(GST_MESSAGE_ERROR |
> GST_MESSAGE_EOS);
> > + constexpr GstClockTime timeout = 2 * GST_SECOND;
> > +
> > + g_autoptr(GstBus) bus = gst_element_get_bus(pipeline_);
> > + g_autoptr(GstMessage) msg = gst_bus_timed_pop_filtered(bus,
> timeout, msgType);
> > +
> > + gst_element_set_state(pipeline_, GST_STATE_NULL);
> > +
> > + /* Parse error message */
> > + if (msg == NULL) {
> > + return TestPass;
> > + }
> > +
> > + switch (GST_MESSAGE_TYPE(msg)) {
> > + case GST_MESSAGE_ERROR:
> > + gstreamer_print_error(msg);
> > + break;
> > + case GST_MESSAGE_EOS:
> > + g_print("End-Of-Stream reached.\n");
> > + break;
> > + default:
> > + g_printerr("Unexpected message received.\n");
> > + break;
> > + }
> > +
> > + return TestFail;
> > +}
> > +
> > +void GstreamerTest::gstreamer_print_error(GstMessage *msg)
> > +{
> > + g_autoptr(GError) err = NULL;
> > + g_autofree gchar *debug_info = NULL;
> > +
> > + gst_message_parse_error(msg, &err, &debug_info);
> > + g_printerr("Error received from element %s: %s\n",
> > + GST_OBJECT_NAME(msg->src), err->message);
> > + g_printerr("Debugging information: %s\n",
> > + debug_info ? debug_info : "none");
> > +}
> > +
> > diff --git a/test/gstreamer/gstreamer_test.h
> b/test/gstreamer/gstreamer_test.h
> > new file mode 100644
> > index 00000000..cdffdea9
> > --- /dev/null
> > +++ b/test/gstreamer/gstreamer_test.h
> > @@ -0,0 +1,39 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2021, Vedant Paranjape
> > + *
> > + * gstreamer_test.cpp - GStreamer test base class
> > + */
> > +
> > +#ifndef __LIBCAMERA_GSTREAMER_TEST_H__
> > +#define __LIBCAMERA_GSTREAMER_TEST_H__
> > +
> > +#include <iostream>
> > +#include <unistd.h>
> > +
> > +#include <libcamera/base/utils.h>
> > +
> > +#include "libcamera/internal/source_paths.h"
> > +
> > +#include <gst/gst.h>
> > +
> > +using namespace std;
> > +
> > +class GstreamerTest
> > +{
> > +public:
> > + GstreamerTest();
> > + virtual ~GstreamerTest();
> > +
> > +protected:
> > + virtual int gstreamer_create_pipeline();
> > + int gstreamer_start_pipeline();
> > + int gstreamer_process_event();
> > + void gstreamer_print_error(GstMessage *msg);
> > +
> > + GstElement *pipeline_;
> > + GstElement *libcameraSrc_;
> > + int status_;
> > +};
> > +
> > +#endif /* __LIBCAMERA_GSTREAMER_TEST_H__ */
> > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build
> > index b99aa0da..aca53b92 100644
> > --- a/test/gstreamer/meson.build
> > +++ b/test/gstreamer/meson.build
> > @@ -10,7 +10,7 @@ gstreamer_tests = [
> > gstreamer_dep = dependency('gstreamer-1.0', required: true)
> >
> > foreach t : gstreamer_tests
> > - exe = executable(t[0], t[1],
> > + exe = executable(t[0], t[1], 'gstreamer_test.cpp',
> > dependencies : [libcamera_private, gstreamer_dep],
> > link_with : test_libraries,
> > include_directories : test_includes_internal)
> > --
> > 2.25.1
> >
>
Regards,
*Vedant Paranjape*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210908/ea9c776f/attachment-0001.htm>
More information about the libcamera-devel
mailing list