[libcamera-devel] [PATCH 2/2] test: gstreamer: Check availability of cameras before running

[Laurent Pinchart]

Hi Umang,

On Fri, Jul 22, 2022 at 06:34:35PM +0530, Umang Jain via libcamera-devel wrote:
> On 7/21/22 19:53, Jacopo Mondi wrote:
> > On Thu, Jul 21, 2022 at 07:15:31PM +0530, Umang Jain via libcamera-devel wrote:
> >> Move the logic for checking the availability of cameras from
> >> multi_stream_test to gstreamer test base class. Since
> >> single_stream_class always assumes that a camera is available on the
> >> system (which is not always the case for e.g. RPi in CI/CD environments)
> >> it makes sense to have the availability check in the base class.
> >> If no cameras are available, the behaviour should be to skip instead
> >> of a failure.
> >>
> >> We currently have 2 tests for gstreamer differing based on number
> >> of streams supported by the camera. Hence, the camera availability
> >> is checked in conjunction with the number of the streams required by
> >> the derived class.
> >>
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >>   .../gstreamer/gstreamer_multi_stream_test.cpp | 20 +----------
> >>   test/gstreamer/gstreamer_test.cpp             | 33 ++++++++++++++++++-
> >>   test/gstreamer/gstreamer_test.h               |  4 ++-
> >>   3 files changed, 36 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/test/gstreamer/gstreamer_multi_stream_test.cpp b/test/gstreamer/gstreamer_multi_stream_test.cpp
> >> index 112f1dee..b8387c10 100644
> >> --- a/test/gstreamer/gstreamer_multi_stream_test.cpp
> >> +++ b/test/gstreamer/gstreamer_multi_stream_test.cpp
> >> @@ -29,7 +29,7 @@ class GstreamerMultiStreamTest : public GstreamerTest, public Test
> >>   {
> >>   public:
> >>   	GstreamerMultiStreamTest()
> >> -		: GstreamerTest()
> >> +		: GstreamerTest(2)
> >
> > As this is a test, I think it's ok even if it's a tad rough
> >
> >>   	{
> >>   	}
> >>
> >> @@ -39,24 +39,6 @@ protected:
> >>   		if (status_ != TestPass)
> >>   			return status_;
> >>
> >> -		/* 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;
> >> -		}
> >> -
> >>   		const gchar *streamDescription = "queue ! fakesink";
> >>   		g_autoptr(GError) error = NULL;
> >>
> >> diff --git a/test/gstreamer/gstreamer_test.cpp b/test/gstreamer/gstreamer_test.cpp
> >> index cfb8afc6..4668aa00 100644
> >> --- a/test/gstreamer/gstreamer_test.cpp
> >> +++ b/test/gstreamer/gstreamer_test.cpp
> >> @@ -5,6 +5,7 @@
> >>    * libcamera Gstreamer element API tests
> >>    */
> >>
> >> +#include <libcamera/libcamera.h>
> >
> > Empty line ?
> >
> >>   #include <libcamera/base/utils.h>
> >>
> >>   #include "gstreamer_test.h"
> >> @@ -25,9 +26,10 @@ const char *__asan_default_options()
> >>   }
> >>   }
> >>
> >> -GstreamerTest::GstreamerTest()
> >> +GstreamerTest::GstreamerTest(unsigned int numStreams)
> >>   	: pipeline_(nullptr), libcameraSrc_(nullptr)
> >>   {
> >> +
> >
> > No empty line :)
> >
> >>   	/*
> >>   	* GStreamer by default spawns a process to run the
> >>   	* gst-plugin-scanner helper. If libcamera is compiled with ASan
> >> @@ -67,9 +69,38 @@ GstreamerTest::GstreamerTest()
> >>   		return;
> >>   	}
> >>
> >> +	/*
> >> +	 * Atleast one camera should be available with numStreams streams
> >> +	 * otherwise, skip the test entirely.
> >> +	 */
> >> +	if (!satisfyCameraStreams(numStreams)) {
> >> +		status_ = TestSkip;
> >> +		return;
> >> +	}
> >> +
> >>   	status_ = TestPass;
> >>   }
> >>
> >> +bool GstreamerTest::satisfyCameraStreams(unsigned int numStreams)
> > 
> > Bikeshedding on the function name apart...
> > And with Nicolas comment clarified
> 
> For naming, it's tricky as this function checks for 2 things:
> 1) Cameras availability
> 2) Numbers of required streams per camera
> 
> It's tricky to combine 1) and 2) in a naming scheme.
> 
> checkForStreams() or simple perCameraStreams() or checkMinCameraStreams() ?
> 
> I think I'll go with the last one.

I don't mind much, I'm fine with the existing name too.

> I think Nicolas comment doesn't not apply here since, cameraManager 
> instance is local to a function.
> 
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> Thanks
> 
> >> +{
> >> +	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;
> >> +}
> >> +
> >>   GstreamerTest::~GstreamerTest()
> >>   {
> >>   	g_clear_object(&pipeline_);
> >> diff --git a/test/gstreamer/gstreamer_test.h b/test/gstreamer/gstreamer_test.h
> >> index 35adab0e..fa41721f 100644
> >> --- a/test/gstreamer/gstreamer_test.h
> >> +++ b/test/gstreamer/gstreamer_test.h
> >> @@ -15,7 +15,7 @@
> >>   class GstreamerTest
> >>   {
> >>   public:
> >> -	GstreamerTest();
> >> +	GstreamerTest(unsigned int numStreams = 1);
> >>   	virtual ~GstreamerTest();
> >>
> >>   protected:
> >> @@ -27,4 +27,6 @@ protected:
> >>   	GstElement *pipeline_;
> >>   	GstElement *libcameraSrc_;
> >>   	int status_;
> >> +private:
> >> +	bool satisfyCameraStreams(unsigned int numStreams);
> >>   };

-- 
Regards,

Laurent Pinchart