[libcamera-devel] [PATCH v1] test: gstreamer: Simplify single stream test using functions from GstUtils

[Nicolas Dufresne]

Le mercredi 22 septembre 2021 à 11:41 +0300, Laurent Pinchart a écrit :
> Hi Vedant,
> 
> On Tue, Sep 21, 2021 at 10:55:11PM +0530, Vedant Paranjape wrote:
> > On Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart wrote:
> > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:
> > > > Simplify memory handling and complexity of the test by using
> > > > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported
> > > > by Umang Jain, described in his fix [2].
> > > > 
> > > > [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full
> > > > [2] https://patchwork.libcamera.org/patch/13845/
> > > > 
> > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> > > > ---
> > > >  .../gstreamer_single_stream_test.cpp          | 29 +++++++++----------
> > > >  1 file changed, 14 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp
> > > > index 7292f3280617..a20d79109885 100644
> > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp
> > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp
> > > > @@ -33,20 +33,15 @@ protected:
> > > >               if (status_ != TestPass)
> > > >                       return status_;
> > > > 
> > > > -             g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0");
> > > > -             g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0");
> > > > -             g_object_ref_sink(convert0);
> > > > -             g_object_ref_sink(sink0);
> > > > -
> > > > -             if (!convert0 || !sink0) {
> > > > -                     g_printerr("Not all elements could be created. %p.%p\n",
> > > > -                                convert0, sink0);
> > > > +             const gchar* streamDescription = "queue ! videoconvert ! fakesink";
> > > 
> > > There's a queue introduced here that isn't mentioned in the commit
> > > message. Why is it needed ?
> > 
> > WARNING: from element
> > /GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage:
> > Pipeline construction is invalid, please add queues.
> > 
> > Because Gstreamer says so, it won't be needed with fakesink I think, I'll
> > fix it :)
> 
> I'm not disputing that, but it wasn't the point. Before this patch, we
> don't have a queue element inserted in the pipeline as far as I can
> tell. It's added by this patch, for a reason that isn't explained in the
> commit message. So, if the queue is needed, it should say, but the
> reason needs to be explained (including why the problem doesn't occur
> today).

The appropriate reason GStreamer core request queues is that the reported min-
latency (sum of all reported latency in the chain) have gone larger then the
reported max-latency (the about of data the pipeline can hold because buffers
are dropped). This is a bit of a side effect of the introduction of the
processing-deadline in GstVideoSink base class, which now declare some latency
to account for the processing time, but does not have any queue to hold on the
buffers if they arrive without that introduced processing delay.

In libcamerasrc, we currently declare min-latency == max-latency, so basically
we are saying that we can only hold as much latency as what we contribute. 
Normally the max-latency would be correlated to the number of allocated buffers
assuming the capture thread can't starve. This way, the capture can keep going
without loosing frames even if the pipeline is pushing back due to some added
latency.

Perhaps we want to fix that now ? We need an approximation of the frame duration
though.

  - min-latency: This remains the observed latency for the first frame
  - max-latency: This is the min-latency + (num_buffers - 1) * estimated_duration

> 
> > > > +             g_autoptr(GError) error0 = NULL;
> > > > +             stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0);
> > > 
> > > That's a very long line. Wrapping at 80 columns wouldn't be very
> > > practical here, but
> > > 
> > >                 stream0_ =
> > > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,
> > > 
> > >  GST_PARSE_FLAG_FATAL_ERRORS,
> > >                                                                &error0);
> > > would already be more readable I think.
> > > 
> > > > 
> > > > +             if (!stream0_) {
> > > > +                     g_printerr("Not all bins could be created. %p\n",
> > > stream0_);
> > > 
> > > Given that stream0_ is null here, why do you print it ? Can error0 be
> > > printed instead somehow (not as a %p I suppose) ?
> > 
> > Nice catch. I'll make the changes.
> > 
> > >                       return TestFail;
> > > >               }
> > > > -
> > > > -             convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0));
> > > > -             sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0));
> > > > +             g_object_ref_sink(stream0_);
> > > > 
> > > >               if (createPipeline() != TestPass)
> > > >                       return TestFail;
> > > > @@ -57,8 +52,8 @@ protected:
> > > >       int run() override
> > > >       {
> > > >               /* Build the pipeline */
> > > > -             gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL);
> > > > -             if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) {
> > > > +             gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL);
> > > > +             if (gst_element_link(libcameraSrc_, stream0_) != TRUE) {
> > > >                       g_printerr("Elements could not be linked.\n");
> > > >                       return TestFail;
> > > >               }
> > > > @@ -72,9 +67,13 @@ protected:
> > > >               return TestPass;
> > > >       }
> > > > 
> > > > +     void cleanup() override
> > > > +     {
> > > > +             g_clear_object(&stream0_);
> > > > +     }
> > > > +
> > > >  private:
> > > > -     GstElement *convert0_;
> > > > -     GstElement *sink0_;
> > > > +     GstElement *stream0_;
> > > >  };
> > > > 
> > > >  TEST_REGISTER(GstreamerSingleStreamTest)
>