[libcamera-devel] [PATCH v2] test: gstreamer: Add a test for gstreamer multi stream

[Nicolas Dufresne]

Le vendredi 10 septembre 2021 à 23:41 +0530, Vedant Paranjape a écrit :
> This patch adds a test to test if multi stream using libcamera's
> gstreamer element works.
> 
> Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> ---
>  .../gstreamer/gstreamer_multi_stream_test.cpp | 138 ++++++++++++++++++
>  test/gstreamer/meson.build                    |   1 +
>  2 files changed, 139 insertions(+)
>  create mode 100644 test/gstreamer/gstreamer_multi_stream_test.cpp
> 
> diff --git a/test/gstreamer/gstreamer_multi_stream_test.cpp b/test/gstreamer/gstreamer_multi_stream_test.cpp
> new file mode 100644
> index 000000000000..e5c909c85da2
> --- /dev/null
> +++ b/test/gstreamer/gstreamer_multi_stream_test.cpp
> @@ -0,0 +1,138 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2021, Vedant Paranjape
> + *
> + * gstreamer_multi_stream_test.cpp - GStreamer multi stream capture test
> + */
> +
> +#include <iostream>
> +#include <unistd.h>
> +
> +#include <libcamera/base/utils.h>
> +
> +#include <libcamera/libcamera.h>
> +
> +#include "libcamera/internal/source_paths.h"
> +
> +#include <gst/gst.h>
> +
> +#include "gstreamer_test.h"
> +#include "test.h"
> +
> +using namespace std;
> +
> +class GstreamerMultiStreamTest : public GstreamerTest, public Test
> +{
> +public:
> +	GstreamerMultiStreamTest()
> +		: GstreamerTest()
> +	{
> +	}
> +
> +protected:
> +	int init() override
> +	{
> +		if (status_ != TestPass)
> +			return status_;
> +
> +		/* Check if platform support multistream output */
> +		libcamera::CameraManager cm;
> +		cm.start();
> +		bool cameraFound = false;
> +		for (auto &i : cm.cameras()) {
> +			if (i->streams().size() > 1) {
> +				cameraName = i->id();
> +				cameraFound = true;
> +				cm.stop();
> +				break;
> +			}
> +		}
> +
> +		if (!cameraFound) {
> +			cm.stop();
> +			return TestSkip;
> +		}
> +
> +		g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0");
> +		g_autoptr(GstElement) convert1 = gst_element_factory_make("videoconvert", "convert1");
> +		g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0");
> +		g_autoptr(GstElement) sink1 = gst_element_factory_make("fakesink", "sink1");
> +		g_autoptr(GstElement) queue0 = gst_element_factory_make("queue", "queue0");
> +		g_autoptr(GstElement) queue1 = gst_element_factory_make("queue", "queue1");
> +		g_object_ref_sink(convert0);
> +		g_object_ref_sink(convert1);
> +		g_object_ref_sink(sink0);
> +		g_object_ref_sink(sink1);
> +		g_object_ref_sink(queue0);
> +		g_object_ref_sink(queue1);

ref_sink should be inline or next to their allocation.

> +
> +		if (!convert0 || !convert1 || !sink0 || !sink1 || !queue0 || !queue1) {
> +			g_printerr("Not all elements could be created. %p.%p.%p.%p.%p.%p\n",
> +				   convert0, convert1, sink0, sink1, queue0, queue1);
> +
> +			return TestFail;
> +		}
> +
> +		convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0));
> +		convert1_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert1));
> +		sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0));
> +		sink1_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink1));
> +		queue0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&queue0));
> +		queue1_ = reinterpret_cast<GstElement *>(g_steal_pointer(&queue1));

What is the point of using autoptr here, you will clean them up in your
destructure, just store them directly in the class. In general, if you feel the
code is ugly, that's likely before you are doing something that is not needed.

> +
> +		if (createPipeline() != TestPass)
> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +
> +	int run() override
> +	{
> +		g_object_set(libcameraSrc_, "camera-name", cameraName.c_str(), NULL);
> +
> +		/* Build the pipeline */
> +		gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, queue0_, queue1_,
> +									convert0_, convert1_, sink0_, sink1_, NULL);
> +		if (gst_element_link_many(queue0_, convert0_, sink0_, NULL) != TRUE
> +			|| gst_element_link_many(queue1_, convert1_, sink1_, NULL) != TRUE) {
> +			g_printerr("Elements could not be linked.\n");
> +			return TestFail;
> +		}

I think we'd gain in readability of using gst_parse_launch_full(). You can even
link requested pad with that. It's also very ackward here, since you have a
run() function which cannot be run twice.

> +
> +		g_autoptr(GstPad) src_pad = gst_element_get_static_pad(libcameraSrc_, "src");
> +		g_autoptr(GstPad) request_pad = gst_element_get_request_pad(libcameraSrc_, "src_%u");
> +		GstPad *queue0_sink_pad = gst_element_get_static_pad(queue0_, "sink");
> +		GstPad *queue1_sink_pad = gst_element_get_static_pad(queue1_, "sink");
> +
> +		if (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK
> +			|| gst_pad_link(request_pad, queue1_sink_pad) != GST_PAD_LINK_OK) {
> +			if (queue0_sink_pad)
> +				gst_object_unref(queue0_sink_pad);
> +			if (queue1_sink_pad)
> +				gst_object_unref(queue1_sink_pad);
> +			g_printerr("Pads could not be linked.\n");
> +			return TestFail;
> +		}
> +        gst_object_unref(queue0_sink_pad);
> +        gst_object_unref(queue1_sink_pad);
> +
> +		if (startPipeline() != TestPass)
> +			return TestFail;
> +
> +		if (processEvent() != TestPass)
> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +
> +private:
> +	std::string cameraName;
> +	GstElement *convert0_;
> +	GstElement *convert1_;
> +	GstElement *sink0_;
> +	GstElement *sink1_;
> +	GstElement *queue0_;
> +	GstElement *queue1_;

No destructor ? All leaked ?

> +};
> +
> +TEST_REGISTER(GstreamerMultiStreamTest)
> diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build
> index aca53b920365..13652e87d05c 100644
> --- a/test/gstreamer/meson.build
> +++ b/test/gstreamer/meson.build
> @@ -6,6 +6,7 @@ endif
>  
>  gstreamer_tests = [
>      ['single_stream_test',   'gstreamer_single_stream_test.cpp'],
> +    ['multi_stream_test',   'gstreamer_multi_stream_test.cpp'],
>  ]
>  gstreamer_dep = dependency('gstreamer-1.0', required: true)
>