[libcamera-devel,v2] test: gstreamer: Add a test for gstreamer multi stream
diff mbox series

Message ID 20210910181152.878132-1-vedantparanjape160201@gmail.com
State Accepted
Headers show
Series
  • [libcamera-devel,v2] test: gstreamer: Add a test for gstreamer multi stream
Related show

Commit Message

Vedant Paranjape Sept. 10, 2021, 6:11 p.m. UTC 
This patch adds a test to test if multi stream using libcamera's
gstreamer element works.

Signed-off-by: Vedant Paranjape <vedantparanjape160201@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

Comments

Paul Elder Sept. 14, 2021, 1:33 a.m. UTC | #1 
Hi Vedant,

On Fri, Sep 10, 2021 at 11:41:52PM +0530, Vedant Paranjape wrote:
> This patch adds a test to test if multi stream using libcamera's
> gstreamer element works.
> 
> Signed-off-by: Vedant Paranjape <vedantparanjape160201@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()) {

This isn't an index, it's a Camera object, so I'd s/i/camera/

> +			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);
> +
> +		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;
> +
> +		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);

Indentation.

> +		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");
> +
> +		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) {

checkstyle says to put this on one line but you can do:

+		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) {

I think that looks nicer.

> +			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);

checkstyle ?

> +
> +		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'],

checkstyle.


With those fixed, looks good.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

It runs fine too (on uvc skips properly, and it passes on raspberrypi).

Tested-by: Paul Elder <paul.elder@ideasonboard.com>

>  ]
>  gstreamer_dep = dependency('gstreamer-1.0', required: true)
>  
> -- 
> 2.25.1
>
Vedant Paranjape Sept. 14, 2021, 5:34 a.m. UTC | #2 
Hi Paul,
Thanks for the review.

The checkstyle was asking to out code in long lines which cross 80 char
limit, should I still go for it?

On Tue, 14 Sep, 2021, 07:03 , <paul.elder@ideasonboard.com> wrote:

> Hi Vedant,
>
> On Fri, Sep 10, 2021 at 11:41:52PM +0530, Vedant Paranjape wrote:
> > This patch adds a test to test if multi stream using libcamera's
> > gstreamer element works.
> >
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201@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()) {
>
> This isn't an index, it's a Camera object, so I'd s/i/camera/
>
> > +                     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);
> > +
> > +             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;
> > +
> > +             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);
>
> Indentation.
>
> > +             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");
> > +
> > +             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) {
>
> checkstyle says to put this on one line but you can do:
>
> +               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) {
>
> I think that looks nicer.
>
> > +                     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);
>
> checkstyle ?
>
> > +
> > +             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'],
>
> checkstyle.
>
>
> With those fixed, looks good.
>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>
> It runs fine too (on uvc skips properly, and it passes on raspberrypi).
>
> Tested-by: Paul Elder <paul.elder@ideasonboard.com>
>
> >  ]
> >  gstreamer_dep = dependency('gstreamer-1.0', required: true)
> >
> > --
> > 2.25.1
> >
>
Nicolas Dufresne Sept. 14, 2021, 5:38 p.m. UTC | #3 
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@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)
>
Vedant Paranjape Sept. 14, 2021, 5:46 p.m. UTC | #4 
Hi Nicolas,

On Tue, 14 Sep, 2021, 23:08 Nicolas Dufresne, <nicolas@ndufresne.ca> wrote:

> 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@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.
>

Okay, will change.


> > +
> > +             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.
>

Okay, makes sense.

> +
> > +             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.
>

I agree with this, I'll take a look at the function.

> +
> > +             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 ?
>

How ? I have added all to pipeline, it will take care of their lifetime.

> +};
> > +
> > +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)
> >
>
>
>
Nicolas Dufresne Sept. 14, 2021, 5:49 p.m. UTC | #5 
Le mardi 14 septembre 2021 à 23:16 +0530, Vedant Paranjape a écrit :
> Hi Nicolas,
> 
> On Tue, 14 Sep, 2021, 23:08 Nicolas Dufresne, <nicolas@ndufresne.ca> wrote:
> > 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@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.
> 
> Okay, will change.
> 
> > 
> > > +
> > > +             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.
> 
> Okay, makes sense.
> 
> > > +
> > > +             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.
> 
> I agree with this, I'll take a look at the function.
> 
> > > +
> > > +             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 ?
> 
> How ? I have added all to pipeline, it will take care of their lifetime.

Well, if you remove the floating ref, ownership is not transferred to the
pipeline and you have to unref when you are done. That is the core principal of
floating references.

> 
> > > +};
> > > +
> > > +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)
> > >  
> > 
> >

Patch
diff mbox series 

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);
+
+		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;
+
+		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");
+
+		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)