[PATCH 2/2] pipeline: simple: Fix matching with empty media graphs

[Laurent Pinchart]

Hi Kieran and Paul,

Thank you for the patch.

On Thu, Aug 08, 2024 at 06:39:21PM +0100, Kieran Bingham wrote:
> From: Paul Elder <paul.elder at ideasonboard.com>
> 
> The match() function currently reports that it is not possible to create
> any cameras if it encounters an empty media graph.
> 
> Fix this by looping over all media graphs and only returning false when
> all of them fail to create a camera.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 50 ++++++++++++++++--------
>  1 file changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 222052ce02f8..115f8528fc71 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -363,6 +363,9 @@ private:
>  		return static_cast<SimpleCameraData *>(camera->_d());
>  	}
>  
> +	bool matchDevice(MediaDevice *media, const SimplePipelineInfo &info,
> +			 DeviceEnumerator *enumerator);
> +
>  	std::vector<MediaEntity *> locateSensors(MediaDevice *media);
>  	static int resetRoutingTable(V4L2Subdevice *subdev);
>  
> @@ -1516,25 +1519,13 @@ int SimplePipelineHandler::resetRoutingTable(V4L2Subdevice *subdev)
>  	return 0;
>  }
>  
> -bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> +bool SimplePipelineHandler::matchDevice(MediaDevice *media,
> +					const SimplePipelineInfo &info,
> +					DeviceEnumerator *enumerator)
>  {
> -	const SimplePipelineInfo *info = nullptr;
>  	unsigned int numStreams = 1;
> -	MediaDevice *media;
>  
> -	for (const SimplePipelineInfo &inf : supportedDevices) {
> -		DeviceMatch dm(inf.driver);
> -		media = acquireMediaDevice(enumerator, dm);
> -		if (media) {
> -			info = &inf;
> -			break;
> -		}
> -	}
> -
> -	if (!media)
> -		return false;
> -
> -	for (const auto &[name, streams] : info->converters) {
> +	for (const auto &[name, streams] : info.converters) {
>  		DeviceMatch converterMatch(name);
>  		converter_ = acquireMediaDevice(enumerator, converterMatch);
>  		if (converter_) {
> @@ -1543,7 +1534,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>  		}
>  	}
>  
> -	swIspEnabled_ = info->swIspEnabled;
> +	swIspEnabled_ = info.swIspEnabled;
>  
>  	/* Locate the sensors. */
>  	std::vector<MediaEntity *> sensors = locateSensors(media);
> @@ -1662,6 +1653,31 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>  	return registered;
>  }
>  
> +bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> +{
> +	MediaDevice *media;
> +
> +	for (const SimplePipelineInfo &inf : supportedDevices) {

s/inf/info/

> +		DeviceMatch dm(inf.driver);
> +		while ((media = acquireMediaDevice(enumerator, dm))) {
> +			/*
> +			 * If match succeeds, return true to let match() be
> +			 * called again on a new instance of the pipeline
> +			 * handler. Otherwise keep looping until we do
> +			 * successfully match one (or run out).
> +			 */
> +			if (matchDevice(media, inf, enumerator)) {
> +				LOG(SimplePipeline, Debug)
> +					<< "Matched on device: "
> +					<< media->deviceNode();
> +				return true;
> +			}
> +		}

If some media devices match, they will get added to the mediaDevices_
list by acquireMediaDevice(), regardless of whether or not they result
in a valid pipeline. This means that invalid media devices followed by a
valid media device will be stored in the mediaDevices_ list for the
pipeline handler instance corresponding to the valid media device. The
cameras create by that pipeline handler instance will end up reporting
the invalid media devices in the SystemDevices property. Is that
something you're aware of, and is that acceptable ?

> +	}
> +
> +	return false;
> +}
> +
>  V4L2VideoDevice *SimplePipelineHandler::video(const MediaEntity *entity)
>  {
>  	auto iter = entities_.find(entity);

-- 
Regards,

Laurent Pinchart