[libcamera-devel] [PATCH v3] test: gstreamer: Add a test for gstreamer multi stream
Umang Jain
umang.jain at ideasonboard.com
Tue Sep 14 11:38:15 CEST 2021
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?
>
>>> 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?
>
>>> 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.
>
>> +
>>> + 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)?
> 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'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.
>
>> +
>>> + 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
>
More information about the libcamera-devel
mailing list