<div dir="ltr"><div dir="ltr"><div>Hi Umang,</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 14, 2021 at 3:08 PM Umang Jain <<a href="mailto:umang.jain@ideasonboard.com">umang.jain@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Vedant<br>
<br>
On 9/14/21 2:12 PM, Vedant Paranjape wrote:<br>
> Hi Umang,<br>
><br>
><br>
> On Tue, Sep 14, 2021 at 12:53 PM Umang Jain <<a href="mailto:umang.jain@ideasonboard.com" target="_blank">umang.jain@ideasonboard.com</a>><br>
> wrote:<br>
><br>
>> Hi Vedant,<br>
>><br>
>> On 9/14/21 12:04 PM, Vedant Paranjape wrote:<br>
>>> This patch adds a test to test if multi stream using libcamera's<br>
>>> gstreamer element works.<br>
>><br>
>> Can you please document how and when this test is run?<br>
>><br>
>> Currently, my system (with one UVC and vimc) has been able to run<br>
>> single_stream_test but multi_stream_test is run:<br>
>><br>
>><br>
>> 11/65 libcamera:gstreamer / single_stream_test OK 2.53s<br>
>> 12/65 libcamera:gstreamer / multi_stream_test SKIP 0.07s<br>
>><br>
>> What's your test setup where this tests passes. I assume I have to<br>
>> satisfy the following condition from the test, in order to run:<br>
>><br>
>> if (camera->streams().size() > 1) {<br>
>><br>
>> cameraFound = true;<br>
>><br>
>> ....<br>
>><br>
>> }<br>
>><br>
>> But I don't know how, looking at the patch. So can you explain (not just<br>
>> here but in the patch / commit message as well) ? It shall help others<br>
>> as well.<br>
>><br>
> Multistream will run on rpi and IPU pipelines only. Will add it to commit<br>
> message.<br>
<br>
<br>
OK, I'll see if I can carve some timeout to test on IPU3 maybe? You are <br>
testing RPi I assume?<br></blockquote><div><br></div><div>Yes</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
>>> Signed-off-by: Vedant Paranjape <<a href="mailto:vedantparanjape160201@gmail.com" target="_blank">vedantparanjape160201@gmail.com</a>><br>
>>> Reviewed-by: Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>><br>
>>> Tested-by: Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>><br>
>>> ---<br>
>>> .../gstreamer/gstreamer_multi_stream_test.cpp | 138 ++++++++++++++++++<br>
>>> test/gstreamer/meson.build | 1 +<br>
>>> 2 files changed, 139 insertions(+)<br>
>>> create mode 100644 test/gstreamer/gstreamer_multi_stream_test.cpp<br>
>><br>
>> Right off the bat, there is a checkstyle failure: Can you look into it,<br>
>> please?<br>
>><br>
>> [uajain@fedora libcamera]$ ./utils/checkstyle.py HEAD<br>
>><br>
>> -----------------------------------------------------------------------------------------------<br>
>> 9d0efed89b03d57790db4eebefa909bf169ce789 test: gstreamer: Add a test for<br>
>> gstreamer multi stream<br>
>><br>
>> -----------------------------------------------------------------------------------------------<br>
>> --- test/gstreamer/gstreamer_multi_stream_test.cpp<br>
>> +++ test/gstreamer/gstreamer_multi_stream_test.cpp<br>
>> @@ -104,8 +104,7 @@<br>
>> GstPad *queue0_sink_pad = gst_element_get_static_pad(queue0_,<br>
>> "sink");<br>
>> GstPad *queue1_sink_pad = gst_element_get_static_pad(queue1_,<br>
>> "sink");<br>
>><br>
>> - if (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK<br>
>> - || gst_pad_link(request_pad, queue1_sink_pad) !=<br>
>> GST_PAD_LINK_OK) {<br>
>> + if (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK<br>
>> || gst_pad_link(request_pad, queue1_sink_pad) != GST_PAD_LINK_OK) {<br>
>> if (queue0_sink_pad)<br>
>> gst_object_unref(queue0_sink_pad);<br>
>> if (queue1_sink_pad)<br>
>> ---<br>
>> 1 potential issue detected, please review<br>
>><br>
> Please see Paul's review !<br>
<br>
<br>
Where? Now that you mention it, I see tags from Paul but the v1 had no <br>
comments and v2 has couple of Laurent's comments. Are you sure you only <br>
have one of v1 and one of v2 series versioning?<br></blockquote><div><br></div><div>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</div><div>Paul's review: <a href="https://patchwork.libcamera.org/patch/13813/">https://patchwork.libcamera.org/patch/13813/</a></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
>>> diff --git a/test/gstreamer/gstreamer_multi_stream_test.cpp<br>
>> b/test/gstreamer/gstreamer_multi_stream_test.cpp<br>
>>> new file mode 100644<br>
>>> index 000000000000..aba86a3f8e27<br>
>>> --- /dev/null<br>
>>> +++ b/test/gstreamer/gstreamer_multi_stream_test.cpp<br>
>>> @@ -0,0 +1,138 @@<br>
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */<br>
>>> +/*<br>
>>> + * Copyright (C) 2021, Vedant Paranjape<br>
>>> + *<br>
>>> + * gstreamer_multi_stream_test.cpp - GStreamer multi stream capture test<br>
>>> + */<br>
>>> +<br>
>>> +#include <iostream><br>
>>> +#include <unistd.h><br>
>>> +<br>
>>> +#include <libcamera/base/utils.h><br>
>>> +<br>
>>> +#include <libcamera/libcamera.h><br>
>>> +<br>
>>> +#include "libcamera/internal/source_paths.h"<br>
>>> +<br>
>>> +#include <gst/gst.h><br>
>>> +<br>
>>> +#include "gstreamer_test.h"<br>
>>> +#include "test.h"<br>
>>> +<br>
>>> +using namespace std;<br>
>>> +<br>
>>> +class GstreamerMultiStreamTest : public GstreamerTest, public Test<br>
>>> +{<br>
>>> +public:<br>
>>> + GstreamerMultiStreamTest()<br>
>>> + : GstreamerTest()<br>
>>> + {<br>
>>> + }<br>
>>> +<br>
>>> +protected:<br>
>>> + int init() override<br>
>>> + {<br>
>>> + if (status_ != TestPass)<br>
>>> + return status_;<br>
>>> +<br>
>>> + /* Check if platform support multistream output */<br>
>>> + libcamera::CameraManager cm;<br>
>>> + cm.start();<br>
>>> + bool cameraFound = false;<br>
>>> + for (auto &camera : cm.cameras()) {<br>
>>> + if (camera->streams().size() > 1) {<br>
>>> + cameraName = camera->id();<br>
>>> + cameraFound = true;<br>
>>> + cm.stop();<br>
>>> + break;<br>
>>> + }<br>
>>> + }<br>
>>> +<br>
>>> + if (!cameraFound) {<br>
>>> + cm.stop();<br>
>>> + return TestSkip;<br>
>>> + }<br>
>>> +<br>
>>> + g_autoptr(GstElement) convert0 =<br>
>> gst_element_factory_make("videoconvert", "convert0");<br>
>>> + g_autoptr(GstElement) convert1 =<br>
>> gst_element_factory_make("videoconvert", "convert1");<br>
>>> + g_autoptr(GstElement) sink0 =<br>
>> gst_element_factory_make("fakesink", "sink0");<br>
>>> + g_autoptr(GstElement) sink1 =<br>
>> gst_element_factory_make("fakesink", "sink1");<br>
>>> + g_autoptr(GstElement) queue0 =<br>
>> gst_element_factory_make("queue", "queue0");<br>
>>> + g_autoptr(GstElement) queue1 =<br>
>> gst_element_factory_make("queue", "queue1");<br>
>>> + g_object_ref_sink(convert0);<br>
>>> + g_object_ref_sink(convert1);<br>
>>> + g_object_ref_sink(sink0);<br>
>>> + g_object_ref_sink(sink1);<br>
>>> + g_object_ref_sink(queue0);<br>
>>> + g_object_ref_sink(queue1);<br>
>><br>
>> What's the idea here of first taking ownership by locally scope<br>
>> g_autoptr() pointers and then setting to the private members below? Is<br>
>> it to validate/null-check them? I would directly set to private members<br>
>> and null-check them directly instead, and if it fails, so be it and<br>
>> return TestFail. Would you be tempted to take this route?<br>
>><br>
> I was doing it before, and it let to a lot of repetitive code and buggy<br>
> error handling. Also, Laurent and Nicolas were kinda reserved against it.<br>
<br>
<br>
If it was repetitive, I guess you can encapsulate all that in a helper <br>
function, to be called on error path to cleanup and return TestFail.<br>
</blockquote></div><div class="gmail_quote"> </div><div class="gmail_quote">I prefer using g_autoptr based approach, I'll ping <a class="gmail_plusreply" id="plusReplyChip-1" href="mailto:nicolas@ndufresne.ca" tabindex="-1">@Nicolas Dufresne</a> for his views, ~he suggested me to do it this way !</div><div class="gmail_quote"> <br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
>> +<br>
>>> + if (!convert0 || !convert1 || !sink0 || !sink1 || !queue0<br>
>> || !queue1) {<br>
>>> + g_printerr("Not all elements could be created.<br>
>> %p.%p.%p.%p.%p.%p\n",<br>
>>> + convert0, convert1, sink0, sink1,<br>
>> queue0, queue1);<br>
>>> +<br>
>>> + return TestFail;<br>
>>> + }<br>
>>> +<br>
>>> + convert0_ = reinterpret_cast<GstElement<br>
>> *>(g_steal_pointer(&convert0));<br>
>>> + convert1_ = reinterpret_cast<GstElement<br>
>> *>(g_steal_pointer(&convert1));<br>
>>> + sink0_ = reinterpret_cast<GstElement<br>
>> *>(g_steal_pointer(&sink0));<br>
>>> + sink1_ = reinterpret_cast<GstElement<br>
>> *>(g_steal_pointer(&sink1));<br>
>>> + queue0_ = reinterpret_cast<GstElement<br>
>> *>(g_steal_pointer(&queue0));<br>
>>> + queue1_ = reinterpret_cast<GstElement<br>
>> *>(g_steal_pointer(&queue1));<br>
>>> +<br>
>>> + if (createPipeline() != TestPass)<br>
>>> + return TestFail;<br>
>><br>
>> Should this `if` block be present before you set the elements above?<br>
>><br>
> This was a good catch, all those elements won't be unrefed if<br>
> createPipeline fails. I will make the fix !<br>
><br>
>> +<br>
>>> + return TestPass;<br>
>>> + }<br>
>>> +<br>
>>> + int run() override<br>
>>> + {<br>
>>> + g_object_set(libcameraSrc_, "camera-name",<br>
>> cameraName.c_str(), NULL);<br>
>>> +<br>
>>> + /* Build the pipeline */<br>
>>> + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_,<br>
>> queue0_, queue1_,<br>
>>> + convert0_, convert1_, sink0_, sink1_,<br>
>> NULL);<br>
>>> + if (gst_element_link_many(queue0_, convert0_, sink0_,<br>
>> NULL) != TRUE ||<br>
>>> + gst_element_link_many(queue1_, convert1_, sink1_,<br>
>> NULL) != TRUE) {<br>
>>> + g_printerr("Elements could not be linked.\n");<br>
>>> + return TestFail;<br>
>>> + }<br>
>>> +<br>
>>> + g_autoptr(GstPad) src_pad =<br>
>> gst_element_get_static_pad(libcameraSrc_, "src");<br>
>>> + g_autoptr(GstPad) request_pad =<br>
>> gst_element_get_request_pad(libcameraSrc_, "src_%u");<br>
>>> + GstPad *queue0_sink_pad =<br>
>> gst_element_get_static_pad(queue0_, "sink");<br>
>>> + GstPad *queue1_sink_pad =<br>
>> gst_element_get_static_pad(queue1_, "sink");<br>
>><br>
>><br>
>> Why can't you use g_autoptr() for queue0_sink_pad and queue1_sink_pad<br>
>> too? gst_element_get_static_pad() will return a [Full] transfer-able<br>
>> value, so it should be un-reffed after usage (like you do below). If you<br>
>> use g_autoptr() the manual _unref shall be taken care of by itself no?<br>
>> Am I missing something? It'll help cleanup error paths.<br>
>><br>
> The pad must be unrefed immediately after use, that is after the pads are<br>
<br>
<br>
I referred some documentation and I didn't see the word "immediately" <br>
anywhere.<br>
<br>
The pads needs to be un-reffed after use, but I don't think "immediate <br>
free" is a requirement. It's bad design upfront and I think gstreamer <br>
folks would have known it. Can you provide link to documentation which <br>
made you think it should be "immediately" un-reffed and not when <br>
function returns (which happens in case g_autoptr is used)?<br></blockquote><div><br></div><div>I'm not a gstreamer expert, so I just followed the gstreamer example code, <a href="https://gstreamer.freedesktop.org/documentation/tutorials/basic/multithreading-and-pad-availability.html?gi-language=c">https://gstreamer.freedesktop.org/documentation/tutorials/basic/multithreading-and-pad-availability.html?gi-language=c</a><br></div><div>It unrefs immediately. <a class="gmail_plusreply" id="plusReplyChip-4" href="mailto:nicolas@ndufresne.ca" tabindex="-1">@Nicolas Dufresne</a> can you confirm this ?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> linked, and if I use an autoptr it won't unref right after the if.<br>
> I had thought to put the code where pads are created and linked into an<br>
> inner scope like this, but then it will be over kill.<br>
><br>
> func<br>
> {<br>
> { // code // }<br>
> }<br>
<br>
<br>
I have seen this design as well for GLib projects as well, mainly for <br>
g_autoptr and mutexes combine and don't think it's an overkill. <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> I assume this is a nitpick and not a blocker. I don't see much improvement<br>
> by doing so, It'd be a different scenario if I had, say 10 pads.<br>
<br>
<br>
Here as well, I would suggest to get a helper function if you are <br>
managing so many pads at once.<br></blockquote><div><br></div><div>I just stated if there would be 10 pads, I'd think about that, but this test will only ever have two pads.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I'll check if I am able to run the test on platforms available to me and <br>
incorporate the suggestions. This is just a reading and understanding of <br>
the code.<br></blockquote><div><br></div><div>Thanks. Please, let me know if it works !</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
>> +<br>
>>> + if (gst_pad_link(src_pad, queue0_sink_pad) !=<br>
>> GST_PAD_LINK_OK<br>
>>> + || gst_pad_link(request_pad, queue1_sink_pad) !=<br>
>> GST_PAD_LINK_OK) {<br>
>>> + if (queue0_sink_pad)<br>
>>> + gst_object_unref(queue0_sink_pad);<br>
>>> + if (queue1_sink_pad)<br>
>>> + gst_object_unref(queue1_sink_pad);<br>
>>> + g_printerr("Pads could not be linked.\n");<br>
>>> + return TestFail;<br>
>>> + }<br>
>>> + gst_object_unref(queue0_sink_pad);<br>
>>> + gst_object_unref(queue1_sink_pad);<br>
>>> +<br>
>>> + if (startPipeline() != TestPass)<br>
>>> + return TestFail;<br>
>>> +<br>
>>> + if (processEvent() != TestPass)<br>
>>> + return TestFail;<br>
>>> +<br>
>>> + return TestPass;<br>
>>> + }<br>
>>> +<br>
>>> +private:<br>
>>> + std::string cameraName;<br>
>>> + GstElement *convert0_;<br>
>>> + GstElement *convert1_;<br>
>>> + GstElement *sink0_;<br>
>>> + GstElement *sink1_;<br>
>>> + GstElement *queue0_;<br>
>>> + GstElement *queue1_;<br>
>>> +};<br>
>>> +<br>
>>> +TEST_REGISTER(GstreamerMultiStreamTest)<br>
>>> diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build<br>
>>> index aca53b920365..13652e87d05c 100644<br>
>>> --- a/test/gstreamer/meson.build<br>
>>> +++ b/test/gstreamer/meson.build<br>
>>> @@ -6,6 +6,7 @@ endif<br>
>>><br>
>>> gstreamer_tests = [<br>
>>> ['single_stream_test', 'gstreamer_single_stream_test.cpp'],<br>
>>> + ['multi_stream_test', 'gstreamer_multi_stream_test.cpp'],<br>
>>> ]<br>
>>> gstreamer_dep = dependency('gstreamer-1.0', required: true)<br>
>>><br>
> Regards,<br>
> Vedant Paranjape<br>
><br></blockquote><div> </div><div> Regards,<br>Vedant Paranjape<br></div></div></div>