[libcamera-devel] [PATCH] test: gstreamer_single_stream: Check for cameras before running

[Jacopo Mondi]

On Thu, Jul 21, 2022 at 05:43:31PM +0530, Umang Jain wrote:
>
> On 7/21/22 17:09, Jacopo Mondi wrote:
> > Hi Umang
> >
> > On Thu, Jul 21, 2022 at 05:04:24PM +0530, Umang Jain wrote:
> > > Hi Jacopo,
> > >
> > > On 7/21/22 16:16, Jacopo Mondi wrote:
> > > > Hi Umang,
> > > >
> > > > On Thu, Jul 21, 2022 at 04:00:20PM +0530, Umang Jain via libcamera-devel wrote:
> > > > > Before running or setting up the pipeline, check for cameras availablity
> > > > > first. If no cameras are available, skip the test.
> > > > >
> > > > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > > Is this to fix the recurring failing test on rpi ?
> > >
> > > Yes,  I tested on Rpi that no cameras will fail the test in the same way as
> > > https://buildbot.libcamera.org/#/builders/16/builds/168
> > >
> > > Having a camera passes the test, so the test assumes that a camera is
> > > present on the system.
> > >
> > > > Nit: could you add a Fixes tag ? I also used Reported-by: with a link
> > > > to a failing build from buildbot. Not that we use it now, but it's
> > > > nice for statistics ?
> > > >
> > > > > ---
> > > > >    test/gstreamer/gstreamer_single_stream_test.cpp | 13 +++++++++++++
> > > > >    1 file changed, 13 insertions(+)
> > > > >
> > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp
> > > > > index a0dd12cf..1e0801e8 100644
> > > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp
> > > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp
> > > > > @@ -8,6 +8,8 @@
> > > > >    #include <iostream>
> > > > >    #include <unistd.h>
> > > > >
> > > > > +#include <libcamera/libcamera.h>
> > > > > +
> > > > >    #include <gst/gst.h>
> > > > >
> > > > >    #include "gstreamer_test.h"
> > > > > @@ -29,6 +31,17 @@ protected:
> > > > >    		if (status_ != TestPass)
> > > > >    			return status_;
> > > > >
> > > > > +		libcamera::CameraManager cm;
> > > > > +		cm.start();
> > > > > +
> > > > > +		bool cameraFound = cm.cameras().size() > 1 ? true : false;
> > > > > +		if (!cameraFound) {
> > > > > +			cm.stop();
> > > > > +			return TestSkip;
> > > > > +		}
> > > > > +
> > > > > +		cm.stop();
> > > > > +
> > > > The multistream test has a very similar:
> > > >
> > > > 		/* Check if platform supports multistream capture */
> > > > 		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;
> > > > 		}
> > > >
> > > > Is this something that should go in the base class ?
> > >
> > > I did contemplate putting in the base class itself. That approach might end
> > > up with multiple cameraManager's start()/stop() cycles (which  I think is
> > > Okay?).
> > >
> > > For e.g. in the multistream test, the base class would start() -> find
> > > cameras ->stop() and the multistream class would start() -> finding cameras
> > > with multistream support -> stop(). Then the actual test starts with
> > > libcamerasrc element. So there's additional one cycle in this case.
> > >
> > > whereas single stream test wouldn't need to have any start() -> stop()
> > > sequence as base class would handle it.
> > >
> > Isn't it just a numStream parameter to the base class init() function ?
>
>
> Not sure what you mean, grepping numStreams denotes:

I mean that the desired number of streams to filter the cameras whith can
be a paramter to a base class function.

bool GstreamerTest::cameraInit(unsigned int numStreams)
{
        /* Check if platform supports multistream capture */
        libcamera::CameraManager cm;
        bool cameraFound = false;

        cm.start();

        for (auto &camera : cm.cameras()) {
                if (camera->streams().size() < numStreams)
                        continue;

                cameraFound = true;
                break;
        }

        cm.stop();

        return cameraFound;
}

>
> [uajain at fedora libcamera]$ git grep -ni "numStream"
> src/libcamera/pipeline/simple/simple.cpp:198: unsigned int numStreams,
> src/libcamera/pipeline/simple/simple.cpp:350: unsigned int numStreams,
> src/libcamera/pipeline/simple/simple.cpp:352:   : Camera::Private(pipe),
> streams_(numStreams)
> src/libcamera/pipeline/simple/simple.cpp:1266:  unsigned int numStreams = 1;
> src/libcamera/pipeline/simple/simple.cpp:1284: numStreams = streams;
> src/libcamera/pipeline/simple/simple.cpp:1307:
> std::make_unique<SimpleCameraData>(this, numStreams, sensor);
>
> >
> > > Probably it's worth to do it in the base class ... Let me see and get back
> > > to you
> > >
> > > > >    		const gchar *streamDescription = "videoconvert ! fakesink";
> > > > >    		g_autoptr(GError) error0 = NULL;
> > > > >    		stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE,
> > > > > --
> > > > > 2.31.1
> > > > >