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

[Umang Jain]

Hi,

On 7/21/22 19:53, Jacopo Mondi wrote:
> Hi Umang,
>
> 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 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

>
> Thanks
>     j
>> +{
>> +	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);
>>   };
>> --
>> 2.31.1
>>