[libcamera-devel] [PATCH v3] test: gstreamer: Add a test for gstreamer multi stream
Vedant Paranjape
vedantparanjape160201 at gmail.com
Tue Sep 14 12:28:45 CEST 2021
Hi Umang,
On Tue, Sep 14, 2021 at 3:08 PM Umang Jain <umang.jain at ideasonboard.com>
wrote:
> Hi Vedant
>
> On 9/14/21 2:12 PM, Vedant Paranjape wrote:
> > Hi Umang,
> >
> >
> > On Tue, Sep 14, 2021 at 12:53 PM Umang Jain <umang.jain at ideasonboard.com
> >
> > wrote:
> >
> >> 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.
> >>
> > Multistream will run on rpi and IPU pipelines only. Will add it to commit
> > message.
>
>
> OK, I'll see if I can carve some timeout to test on IPU3 maybe? You are
> testing RPi I assume?
>
Yes
>
> >>> 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
> >>
> > Please see Paul's review !
>
>
> Where? Now that you mention it, I see tags from Paul but the v1 had no
> comments and v2 has couple of Laurent's comments. Are you sure you only
> have one of v1 and one of v2 series versioning?
>
No, Paul was the one who commented on v2. One with Laurent's comments is
pretty old and outdated, as I now have a base class for gstreamer test,
earlier one didn't
Paul's review: https://patchwork.libcamera.org/patch/13813/
>
> >>> 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?
> >>
> > I was doing it before, and it let to a lot of repetitive code and buggy
> > error handling. Also, Laurent and Nicolas were kinda reserved against it.
>
>
> If it was repetitive, I guess you can encapsulate all that in a helper
> function, to be called on error path to cleanup and return TestFail.
>
I prefer using g_autoptr based approach, I'll ping @Nicolas Dufresne
<nicolas at ndufresne.ca> for his views, ~he suggested me to do it this way !
> >
> >> +
> >>> + 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?
> >>
> > This was a good catch, all those elements won't be unrefed if
> > createPipeline fails. I will make the fix !
> >
> >> +
> >>> + 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.
> >>
> > The pad must be unrefed immediately after use, that is after the pads are
>
>
> I referred some documentation and I didn't see the word "immediately"
> anywhere.
>
> The pads needs to be un-reffed after use, but I don't think "immediate
> free" is a requirement. It's bad design upfront and I think gstreamer
> folks would have known it. Can you provide link to documentation which
> made you think it should be "immediately" un-reffed and not when
> function returns (which happens in case g_autoptr is used)?
>
I'm not a gstreamer expert, so I just followed the gstreamer example code,
https://gstreamer.freedesktop.org/documentation/tutorials/basic/multithreading-and-pad-availability.html?gi-language=c
It unrefs immediately. @Nicolas Dufresne <nicolas at ndufresne.ca> can you
confirm this ?
> linked, and if I use an autoptr it won't unref right after the if.
> > I had thought to put the code where pads are created and linked into an
> > inner scope like this, but then it will be over kill.
> >
> > func
> > {
> > { // code // }
> > }
>
>
> I have seen this design as well for GLib projects as well, mainly for
> g_autoptr and mutexes combine and don't think it's an overkill.
>
>
> > I assume this is a nitpick and not a blocker. I don't see much
> improvement
> > by doing so, It'd be a different scenario if I had, say 10 pads.
>
>
> Here as well, I would suggest to get a helper function if you are
> managing so many pads at once.
>
I just stated if there would be 10 pads, I'd think about that, but this
test will only ever have two pads.
I'll check if I am able to run the test on platforms available to me and
> incorporate the suggestions. This is just a reading and understanding of
> the code.
>
Thanks. Please, let me know if it works !
>
> >> +
> >>> + 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)
> >>>
> > Regards,
> > Vedant Paranjape
> >
>
Regards,
Vedant Paranjape
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210914/56096248/attachment-0001.htm>
More information about the libcamera-devel
mailing list