<div dir="auto"><span style="font-family:sans-serif">Hi Um</span><span style="font-family:sans-serif">ang,</span><div dir="auto" style="font-family:sans-serif">Thanks for the reply. I'll keep this in mind, next time I send patches.</div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">But can you test it and give a tested by tag?</div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">Also, I can help you understand parts that you are not familiar with.</div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">Regards,</div><div dir="auto" style="font-family:sans-serif"><b><i>Vedant Paranjape</i></b></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 18 Sep, 2021, 23:54 Umang Jain, <<a href="mailto:umang.jain@ideasonboard.com">umang.jain@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Vedant,<br>
<br>
Thank you for the patch.<br>
<br>
On 9/15/21 8:39 PM, Vedant Paranjape wrote:<br>
> Simplify memory handling and complexity of the test by using<br>
> gst_parse_bin_from_description_full [1]. It also fixed memory leak reported<br>
> by Umang Jain, described in his fix [2].<br>
><br>
> [1] <a href="https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full" rel="noreferrer noreferrer" target="_blank">https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full</a><br>
> [2] <a href="https://patchwork.libcamera.org/patch/13845/" rel="noreferrer noreferrer" target="_blank">https://patchwork.libcamera.org/patch/13845/</a><br>
<br>
<br>
To be honest, this patch in general introduces too many changes at once. <br>
The trick to get good and fast reviews is generally to break it down to <br>
focus one task per patch in most cases. This granularity helps the <br>
reviewer to be confident looking at the changes in front of them and <br>
give their R-b tags on those specific patches. It might happen that for <br>
e.g. I am not well-versed with the API you introduced, but rather I can <br>
see the other fixes good to go - but overall I will still be holding off <br>
my R-b because there is something I don't understand fully. This is just <br>
an example but you get the idea.<br>
<br>
If you could have broken this down to let's say 3 patches (piggybacking <br>
on my 2 patches earlier and introduce a 3rd patch by yourself about the <br>
new API) - it helps to establish a flow. The two patches had one R-b <br>
tags earlier so you are already closer to get them merged (need 2 R-b <br>
tags for merge-eligibility per patch) and the 3rd can be discussed <br>
meanwhile. It helps to move and unblock patches / fixes as part of <br>
development process.<br>
<br>
<br>
><br>
> Signed-off-by: Vedant Paranjape <<a href="mailto:vedantparanjape160201@gmail.com" target="_blank" rel="noreferrer">vedantparanjape160201@gmail.com</a>><br>
> ---<br>
> .../gstreamer_single_stream_test.cpp | 29 +++++++++----------<br>
> 1 file changed, 14 insertions(+), 15 deletions(-)<br>
><br>
> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp<br>
> index 7292f3280617..a20d79109885 100644<br>
> --- a/test/gstreamer/gstreamer_single_stream_test.cpp<br>
> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp<br>
> @@ -33,20 +33,15 @@ protected:<br>
> if (status_ != TestPass)<br>
> return status_;<br>
> <br>
> - g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0");<br>
> - g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0");<br>
> - g_object_ref_sink(convert0);<br>
> - g_object_ref_sink(sink0);<br>
> -<br>
> - if (!convert0 || !sink0) {<br>
> - g_printerr("Not all elements could be created. %p.%p\n",<br>
> - convert0, sink0);<br>
> + const gchar* streamDescription = "queue ! videoconvert ! fakesink";<br>
> + g_autoptr(GError) error0 = NULL;<br>
> + stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0);<br>
> <br>
> + if (!stream0_) {<br>
> + g_printerr("Not all bins could be created. %p\n", stream0_);<br>
> return TestFail;<br>
> }<br>
> -<br>
> - convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0));<br>
> - sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0));<br>
> + g_object_ref_sink(stream0_);<br>
> <br>
> if (createPipeline() != TestPass)<br>
> return TestFail;<br>
> @@ -57,8 +52,8 @@ protected:<br>
> int run() override<br>
> {<br>
> /* Build the pipeline */<br>
> - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL);<br>
> - if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) {<br>
> + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL);<br>
> + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) {<br>
> g_printerr("Elements could not be linked.\n");<br>
> return TestFail;<br>
> }<br>
> @@ -72,9 +67,13 @@ protected:<br>
> return TestPass;<br>
> }<br>
> <br>
> + void cleanup() override<br>
> + {<br>
> + g_clear_object(&stream0_);<br>
> + }<br>
> +<br>
> private:<br>
> - GstElement *convert0_;<br>
> - GstElement *sink0_;<br>
> + GstElement *stream0_;<br>
> };<br>
> <br>
> TEST_REGISTER(GstreamerSingleStreamTest)<br>
</blockquote></div>