[libcamera-devel] [PATCH v3] test: gstreamer: Add a test for gstreamer multi stream
Umang Jain
umang.jain at ideasonboard.com
Tue Sep 14 09:23:45 CEST 2021
Hi Vedant,
On 9/14/21 12:04 PM, Vedant Paranjape wrote:
> This patch adds a test to test if multi stream using libcamera's
> gstreamer element works.
Can you please document how and when this test is run?
Currently, my system (with one UVC and vimc) has been able to run
single_stream_test but multi_stream_test is run:
11/65 libcamera:gstreamer / single_stream_test OK 2.53s
12/65 libcamera:gstreamer / multi_stream_test SKIP 0.07s
What's your test setup where this tests passes. I assume I have to
satisfy the following condition from the test, in order to run:
if (camera->streams().size() > 1) {
cameraFound = true;
....
}
But I don't know how, looking at the patch. So can you explain (not just
here but in the patch / commit message as well) ? It shall help others
as well.
>
> Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> Tested-by: Paul Elder <paul.elder at ideasonboard.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
Right off the bat, there is a checkstyle failure: Can you look into it,
please?
[uajain at fedora libcamera]$ ./utils/checkstyle.py HEAD
-----------------------------------------------------------------------------------------------
9d0efed89b03d57790db4eebefa909bf169ce789 test: gstreamer: Add a test for
gstreamer multi stream
-----------------------------------------------------------------------------------------------
--- test/gstreamer/gstreamer_multi_stream_test.cpp
+++ test/gstreamer/gstreamer_multi_stream_test.cpp
@@ -104,8 +104,7 @@
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 (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)
---
1 potential issue detected, please review
>
> diff --git a/test/gstreamer/gstreamer_multi_stream_test.cpp b/test/gstreamer/gstreamer_multi_stream_test.cpp
> new file mode 100644
> index 000000000000..aba86a3f8e27
> --- /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 &camera : cm.cameras()) {
> + if (camera->streams().size() > 1) {
> + cameraName = camera->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);
What's the idea here of first taking ownership by locally scope
g_autoptr() pointers and then setting to the private members below? Is
it to validate/null-check them? I would directly set to private members
and null-check them directly instead, and if it fails, so be it and
return TestFail. Would you be tempted to take this route?
> +
> + 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));
> +
> + if (createPipeline() != TestPass)
> + return TestFail;
Should this `if` block be present before you set the elements above?
> +
> + 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;
> + }
> +
> + 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");
Why can't you use g_autoptr() for queue0_sink_pad and queue1_sink_pad
too? gst_element_get_static_pad() will return a [Full] transfer-able
value, so it should be un-reffed after usage (like you do below). If you
use g_autoptr() the manual _unref shall be taken care of by itself no?
Am I missing something? It'll help cleanup error paths.
> +
> + 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_;
> +};
> +
> +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)
>
More information about the libcamera-devel
mailing list