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

[Umang Jain]

Hi Jacopo,

On 7/21/22 18:02, Jacopo Mondi wrote:
> 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;
> }


I expect someone saying this is like moving away from the "base" class. 
As we introduced some specifies here itself.

I guess it should be fine given the test cases we have right now(single 
and multistream). Let me cook this up. Thanks for the suggestion.

>
>> [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
>>>>>>