[PATCH v2 2/3] test: gstreamer: Simplify single stream test

[Laurent Pinchart]

On Thu, May 09, 2024 at 04:57:29PM +0100, Kieran Bingham wrote:
> Quoting Nicolas Dufresne (2024-05-09 16:12:18)
> > Hi,
> > 
> > Le jeudi 09 mai 2024 à 15:50 +0100, Kieran Bingham a écrit :
> > > Quoting Nicolas Dufresne (2024-03-05 15:30:57)
> > > > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > > 
> > > 
> > > Why? Is it just better?
> > 
> > I commit and sign with my Collabora address for all the work I do on my employer
> > time. But I'm using my personal address for mailing lists. I did no action here,
> > "git send-email" noticed and added the from. If you use "git am". it should
> > transparently reflect my choice here.
> 
> Sorry - I meant - the commit message was empty. I don't mind (or care)
> which the From is from ;-)
> 
> I meant "Simplify single stream test" - Why - is it better simplified?
> The empty commit message didn't tell us anything!
> 
> > > I'll try to interpret as I read the patch. Please correct me whereever
> > > I'm wrong.
> > > 
> > > """
> > > The single stream test for the gstreamer component has a simple pipeline
> > > construction using only a fakesink.
> > > 
> > > The impelementation currently supports connecting to a more complex

s/impelementation/implementation/

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > > stream construction defined by the streamDescription, but this is over
> > > engineered for the simple need to start a stream to capture and discard
> > > the frames.
> > > 
> > > Convert the use of gst_parse_bin_from_description_full() which uses only
> > > a single element 'fakesink' to construct the fakesink directly and link
> > > it to the libcamerasrc.
> > > """
> > 
> > That is very good, I should have wrote something, but if the target was the
> > GStreamer project, it would have been considered overkill due to the trivality
> > of the changes (from a GStreamer dev point of view of course). I'll keep in mind
> > for next time.
> 
> The difficulty for me is I'm not a gstreamer dev, so I find the
> gstreamer element difficult to read. An introduction is very helpful to
> go into the patch with something in mind.
> 
> If you're happy with the commit message, it can be added at applying or
> in a v2 - I don't know if a v2 is needed yet.

I'll merge this patch already, independently from the rest of the
series.

> > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > 
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > 
> > > > ---
> > > >  .../gstreamer_single_stream_test.cpp          | 27 +++++++------------
> > > >  1 file changed, 9 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp
> > > > index 301e4a93..815c6609 100644
> > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp
> > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp
> > > > @@ -29,30 +29,21 @@ protected:
> > > >                 if (status_ != TestPass)
> > > >                         return status_;
> > > >  
> > > > -               const gchar *streamDescription = "fakesink";
> > > > -               g_autoptr(GError) error0 = NULL;
> > > > -               stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE,
> > > > -                                               NULL,
> > > > -                                               GST_PARSE_FLAG_FATAL_ERRORS,
> > > > -                                               &error0);
> > > > -
> > > > -               if (!stream0_) {
> > > > -                       g_printerr("Bin could not be created (%s)\n", error0->message);
> > > > +               fakesink_ = gst_element_factory_make("fakesink", nullptr);
> > > > +               if (!fakesink_) {
> > > > +                       g_printerr("Your installation is missing 'fakesink'\n");
> > > >                         return TestFail;
> > > >                 }
> > > > -               g_object_ref_sink(stream0_);
> > > > -
> > > > -               if (createPipeline() != TestPass)
> > > > -                       return TestFail;
> > > > +               g_object_ref_sink(fakesink_);
> > > >  
> > > > -               return TestPass;
> > > > +               return createPipeline();
> > > >         }
> > > >  
> > > >         int run() override
> > > >         {
> > > >                 /* Build the pipeline */
> > > > -               gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL);
> > > > -               if (gst_element_link(libcameraSrc_, stream0_) != TRUE) {
> > > > +               gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, fakesink_, nullptr);
> > > > +               if (!gst_element_link(libcameraSrc_, fakesink_)) {
> > > >                         g_printerr("Elements could not be linked.\n");
> > > >                         return TestFail;
> > > >                 }
> > > > @@ -68,11 +59,11 @@ protected:
> > > >  
> > > >         void cleanup() override
> > > >         {
> > > > -               g_clear_object(&stream0_);
> > > > +               g_clear_object(&fakesink_);
> > > >         }
> > > >  
> > > >  private:
> > > > -       GstElement *stream0_;
> > > > +       GstElement *fakesink_;
> > > >  };
> > > >  
> > > >  TEST_REGISTER(GstreamerSingleStreamTest)

-- 
Regards,

Laurent Pinchart