[libcamera-devel] [PATCH v3] test: gstreamer: Add a test for gstreamer multi stream
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Fri Sep 24 10:41:25 CEST 2021
On Fri, Sep 24, 2021 at 02:02:07PM +0530, Vedant Paranjape wrote:
> On Fri, Sep 24, 2021 at 12:35 PM <paul.elder at ideasonboard.com> wrote:
> >
> > On Fri, Sep 24, 2021 at 11:42:18AM +0530, Vedant Paranjape wrote:
> > > Hi Paul,
> > > Can you also tell me the expected indentation ?
> > >
> > > checkstyle suggestions didn't adhere with the 80 char width thing
> >
> > 80 char can be broken (up to 120) as long as it increases readability.
> >
> > >
> > >
> > >
> > >
> > > On Fri, 24 Sep, 2021, 11:39 , <paul.elder at ideasonboard.com> wrote:
> > >
> > > Hi Vedant,
> > >
> > > On Thu, Sep 23, 2021 at 08:26:37PM +0530, Vedant Paranjape wrote:
> > > > This patch adds a test to test if multi stream using libcamera's
> > > > gstreamer element works.
> > > >
> > > > Test will run only on devices that support multistream output, i.e.,
> > >
> > > I think s/output/capture/ might be better.
> > >
> > > Also s/i.e./e.g/
> > >
> > > > devices that use IPU3 and raspberrypi pipeline. This was tested on
> > > > a Raspberry Pi 4B+.
> > > >
> > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> > > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > > ---
> > > > .../gstreamer/gstreamer_multi_stream_test.cpp | 128 ++++++++++++++++++
> > > > test/gstreamer/meson.build | 1 +
> > > > 2 files changed, 129 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..6f63f1e748e2
> > > > --- /dev/null
> > > > +++ b/test/gstreamer/gstreamer_multi_stream_test.cpp
> > > > @@ -0,0 +1,128 @@
> > > > +/* 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>
> > >
> > > You don't use anything from this.
> > >
> > > > +
> > > > +#include <libcamera/libcamera.h>
> > > > +
> > > > +#include "libcamera/internal/source_paths.h"
> > >
> > > Nor this.
> > >
> > > (The single stream tests have these too)
> > >
> > > > +
> > > > +#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;
> > > > + }
> > > > +
> > > > + const gchar *streamDescription = "queue ! fakesink";
> > > > + g_autoptr(GError) error = NULL;
> > > > +
> > > > + stream0_ = gst_parse_bin_from_description_full
> > > (streamDescription, TRUE,
> > > > + NULL,
> > > > +
> > > GST_PARSE_FLAG_FATAL_ERRORS,
> > > > + &error);
> >
> > Oh I let this and the next one go because it goes over, but I think I
> > agree with checkstyle here:
> >
> >
> > stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE,
> > NULL,
> > GST_PARSE_FLAG_FATAL_ERRORS,
> > &error);
> >
> >
> > > > + if (!stream0_) {
> > > > + g_printerr("Stream0 could not be created (%s)\n",
> > > error->message);
> > > > + return TestFail;
> > > > + }
> > > > + g_object_ref_sink(stream0_);
> > > > +
> > > > + stream1_ = gst_parse_bin_from_description_full
> > > (streamDescription, TRUE,
> > > > + NULL,
> > > > +
> > > GST_PARSE_FLAG_FATAL_ERRORS,
> > > > + &error);
> > > > + if (!stream1_) {
> > > > + g_printerr("Stream1 could not be created (%s)\n",
> > > error->message);
> > > > + return TestFail;
> > > > + }
> > > > + g_object_ref_sink(stream1_);
> > > > +
> > > > + 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_,
> > > > + stream0_, stream1_,
> > > NULL);
> > >
> > > Indentation.
> >
> > checkstyle has this one
> >
> > >
> > > > +
> > > > + 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");
>
> This looks bad.
Get rid of the html indentation...
+ 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");
I think it's fine. If you're concerned about the line length you can
indent it.
+ 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");
Either way is fine.
>
> > > > +
> > > > + {
> > > > + g_autoptr(GstPad) queue0_sink_pad =
> > > gst_element_get_static_pad(stream0_, "sink");
> > > > + g_autoptr(GstPad) queue1_sink_pad =
> > > gst_element_get_static_pad(stream1_, "sink");
>
> even this.
Same here.
Paul
>
> > > > +
> > > > + 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) {
> > >
> > > Indentation.
> >
> >
> > 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) {
> >
> > >
> > > > + g_printerr("Pads could not be linked.\n");
> > > > + return TestFail;
> > > > + }
> > > > + }
> > > > +
> > > > + if (startPipeline() != TestPass)
> > > > + return TestFail;
> > > > +
> > > > + if (processEvent() != TestPass)
> > > > + return TestFail;
> > > > +
> > > > + return TestPass;
> > > > + }
> > > > +
> > > > + void cleanup() override
> > > > + {
> > > > + g_clear_object(&stream0_);
> > > > + g_clear_object(&stream1_);
> > > > + }
> > > > +
> > > > +private:
> > > > + std::string cameraName;
> > >
> > > s/cameraName/cameraName_/
> > >
> > > > + GstElement *stream0_;
> > > > + GstElement *stream1_;
> > > > +};
> > > > +
> > > > +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'],
> > >
> > > It would be nice if the second elements lined up.
> > >
> > >
> > > Otherwise, looks good.
> > >
> > >
> > > Can I add a R-b then?
> >
> > Not unless I give you one. I'll check in a new ersion.
> >
> >
> > Paul
> >
> > >
> > > > ]
> > > > gstreamer_dep = dependency('gstreamer-1.0', required: true)
> > > >
> > > > --
> > > > 2.25.1
> > > >
> > >
More information about the libcamera-devel
mailing list