[libcamera-devel,3/5] libcamera: pipeline: simple: Factor out format test to separate function

15776 diff mbox series

To prepare for the implementation of a more complex format discovery
method, factor out code that tries a pipeline configuration to a
separate function. No functional change intended.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 110 ++++++++++++-----------
 1 file changed, 59 insertions(+), 51 deletions(-)

On Wed,  4 May 2022 16:21:52 +0300
laurent.pinchart at ideasonboard.com (Laurent Pinchart) wrote:

> To prepare for the implementation of a more complex format discovery
> method, factor out code that tries a pipeline configuration to a
> separate function. No functional change intended.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 110 ++++++++++++-----------
>  1 file changed, 59 insertions(+), 51 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 4f963df16315..19deb9447c96 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -250,6 +250,9 @@ public:
>  	std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_;
>  
>  private:
> +	void tryPipeline(unsigned int code, const Size &size);
> +	static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink);
This doesn't seem to be used in this patch.

> +
>  	void converterInputDone(FrameBuffer *buffer);
>  	void converterOutputDone(FrameBuffer *buffer);
>  };
> @@ -466,58 +469,11 @@ int SimpleCameraData::init()
>  		return ret;
>  
>  	/*
> -	 * Enumerate the possible pipeline configurations. For each media bus
> -	 * format supported by the sensor, propagate the formats through the
> -	 * pipeline, and enumerate the corresponding possible V4L2 pixel
> -	 * formats on the video node.
> +	 * Generate the list of possible pipeline configurations by trying each
> +	 * media bus format supported by the sensor.
>  	 */
> -	for (unsigned int code : sensor_->mbusCodes()) {
> -		V4L2SubdeviceFormat format{};
> -		format.mbus_code = code;
> -		format.size = sensor_->resolution();
> -
> -		ret = setupFormats(&format, V4L2Subdevice::TryFormat);
> -		if (ret < 0) {
> -			LOG(SimplePipeline, Debug)
> -				<< "Media bus code " << utils::hex(code, 4)
> -				<< " not supported for this pipeline";
> -			/* Try next mbus_code supported by the sensor */
> -			continue;
> -		}
> -
> -		V4L2VideoDevice::Formats videoFormats =
> -			video_->formats(format.mbus_code);
> -
> -		LOG(SimplePipeline, Debug)
> -			<< "Adding configuration for " << format.size
> -			<< " in pixel formats [ "
> -			<< utils::join(videoFormats, ", ",
> -				       [](const auto &f) {
> -					       return f.first.toString();
> -				       })
> -			<< " ]";
> -
> -		for (const auto &videoFormat : videoFormats) {
> -			PixelFormat pixelFormat = videoFormat.first.toPixelFormat();
> -			if (!pixelFormat)
> -				continue;
> -
> -			Configuration config;
> -			config.code = code;
> -			config.captureFormat = pixelFormat;
> -			config.captureSize = format.size;
> -
> -			if (!converter_) {
> -				config.outputFormats = { pixelFormat };
> -				config.outputSizes = config.captureSize;
> -			} else {
> -				config.outputFormats = converter_->formats(pixelFormat);
> -				config.outputSizes = converter_->sizes(format.size);
> -			}
> -
> -			configs_.push_back(config);
> -		}
> -	}
> +	for (unsigned int code : sensor_->mbusCodes())
> +		tryPipeline(code, sensor_->resolution());
>  
>  	if (configs_.empty()) {
>  		LOG(SimplePipeline, Error) << "No valid configuration found";
> @@ -541,6 +497,58 @@ int SimpleCameraData::init()
>  	return 0;
>  }
>  
> +void SimpleCameraData::tryPipeline(unsigned int code, const Size &size)
> +{
> +	/*
> +	 * Propagate the format through the pipeline, and enumerate the
> +	 * corresponding possible V4L2 pixel formats on the video node.
> +	 */
What are the output values/effects of this call? I can't easily track them, despite having rewritten this method twice in the past.

--Dorota
> +	V4L2SubdeviceFormat format{};
> +	format.mbus_code = code;
> +	format.size = size;
> +
> +	int ret = setupFormats(&format, V4L2Subdevice::TryFormat);
> +	if (ret < 0) {
> +		/* Pipeline configuration failed, skip this configuration. */
> +		LOG(SimplePipeline, Debug)
> +			<< "Media bus code " << utils::hex(code, 4)
> +			<< " not supported for this pipeline";
> +		return;
> +	}
> +
> +	V4L2VideoDevice::Formats videoFormats = video_->formats(format.mbus_code);
> +
> +	LOG(SimplePipeline, Debug)
> +		<< "Adding configuration for " << format.size
> +		<< " in pixel formats [ "
> +		<< utils::join(videoFormats, ", ",
> +			       [](const auto &f) {
> +				       return f.first.toString();
> +			       })
> +		<< " ]";
> +
> +	for (const auto &videoFormat : videoFormats) {
> +		PixelFormat pixelFormat = videoFormat.first.toPixelFormat();
> +		if (!pixelFormat)
> +			continue;
> +
> +		Configuration config;
> +		config.code = code;
> +		config.captureFormat = pixelFormat;
> +		config.captureSize = format.size;
> +
> +		if (!converter_) {
> +			config.outputFormats = { pixelFormat };
> +			config.outputSizes = config.captureSize;
> +		} else {
> +			config.outputFormats = converter_->formats(pixelFormat);
> +			config.outputSizes = converter_->sizes(format.size);
> +		}
> +
> +		configs_.push_back(config);
> +	}
> +}
> +
>  int SimpleCameraData::setupLinks()
>  {
>  	int ret;

Hi Dorota,

On Wed, Jun 08, 2022 at 03:49:22PM +0200, Dorota Czaplejewicz via libcamera-devel wrote:
> On Wed,  4 May 2022 16:21:52 +0300 Laurent Pinchart wrote:
> > To prepare for the implementation of a more complex format discovery
> > method, factor out code that tries a pipeline configuration to a
> > separate function. No functional change intended.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 110 ++++++++++++-----------
> >  1 file changed, 59 insertions(+), 51 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 4f963df16315..19deb9447c96 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -250,6 +250,9 @@ public:
> >  	std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_;
> >  
> >  private:
> > +	void tryPipeline(unsigned int code, const Size &size);
> > +	static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink);
> 
> This doesn't seem to be used in this patch.

Indeed, that belongs to a subsequent patch. I'll fix it.

> > +
> >  	void converterInputDone(FrameBuffer *buffer);
> >  	void converterOutputDone(FrameBuffer *buffer);
> >  };
> > @@ -466,58 +469,11 @@ int SimpleCameraData::init()
> >  		return ret;
> >  
> >  	/*
> > -	 * Enumerate the possible pipeline configurations. For each media bus
> > -	 * format supported by the sensor, propagate the formats through the
> > -	 * pipeline, and enumerate the corresponding possible V4L2 pixel
> > -	 * formats on the video node.
> > +	 * Generate the list of possible pipeline configurations by trying each
> > +	 * media bus format supported by the sensor.
> >  	 */
> > -	for (unsigned int code : sensor_->mbusCodes()) {
> > -		V4L2SubdeviceFormat format{};
> > -		format.mbus_code = code;
> > -		format.size = sensor_->resolution();
> > -
> > -		ret = setupFormats(&format, V4L2Subdevice::TryFormat);
> > -		if (ret < 0) {
> > -			LOG(SimplePipeline, Debug)
> > -				<< "Media bus code " << utils::hex(code, 4)
> > -				<< " not supported for this pipeline";
> > -			/* Try next mbus_code supported by the sensor */
> > -			continue;
> > -		}
> > -
> > -		V4L2VideoDevice::Formats videoFormats =
> > -			video_->formats(format.mbus_code);
> > -
> > -		LOG(SimplePipeline, Debug)
> > -			<< "Adding configuration for " << format.size
> > -			<< " in pixel formats [ "
> > -			<< utils::join(videoFormats, ", ",
> > -				       [](const auto &f) {
> > -					       return f.first.toString();
> > -				       })
> > -			<< " ]";
> > -
> > -		for (const auto &videoFormat : videoFormats) {
> > -			PixelFormat pixelFormat = videoFormat.first.toPixelFormat();
> > -			if (!pixelFormat)
> > -				continue;
> > -
> > -			Configuration config;
> > -			config.code = code;
> > -			config.captureFormat = pixelFormat;
> > -			config.captureSize = format.size;
> > -
> > -			if (!converter_) {
> > -				config.outputFormats = { pixelFormat };
> > -				config.outputSizes = config.captureSize;
> > -			} else {
> > -				config.outputFormats = converter_->formats(pixelFormat);
> > -				config.outputSizes = converter_->sizes(format.size);
> > -			}
> > -
> > -			configs_.push_back(config);
> > -		}
> > -	}
> > +	for (unsigned int code : sensor_->mbusCodes())
> > +		tryPipeline(code, sensor_->resolution());
> >  
> >  	if (configs_.empty()) {
> >  		LOG(SimplePipeline, Error) << "No valid configuration found";
> > @@ -541,6 +497,58 @@ int SimpleCameraData::init()
> >  	return 0;
> >  }
> >  
> > +void SimpleCameraData::tryPipeline(unsigned int code, const Size &size)
> > +{
> > +	/*
> > +	 * Propagate the format through the pipeline, and enumerate the
> > +	 * corresponding possible V4L2 pixel formats on the video node.
> > +	 */
> 
> What are the output values/effects of this call? I can't easily track
> them, despite having rewritten this method twice in the past.

I'll add a comment block above the function to explain this.

> > +	V4L2SubdeviceFormat format{};
> > +	format.mbus_code = code;
> > +	format.size = size;
> > +
> > +	int ret = setupFormats(&format, V4L2Subdevice::TryFormat);
> > +	if (ret < 0) {
> > +		/* Pipeline configuration failed, skip this configuration. */
> > +		LOG(SimplePipeline, Debug)
> > +			<< "Media bus code " << utils::hex(code, 4)
> > +			<< " not supported for this pipeline";
> > +		return;
> > +	}
> > +
> > +	V4L2VideoDevice::Formats videoFormats = video_->formats(format.mbus_code);
> > +
> > +	LOG(SimplePipeline, Debug)
> > +		<< "Adding configuration for " << format.size
> > +		<< " in pixel formats [ "
> > +		<< utils::join(videoFormats, ", ",
> > +			       [](const auto &f) {
> > +				       return f.first.toString();
> > +			       })
> > +		<< " ]";
> > +
> > +	for (const auto &videoFormat : videoFormats) {
> > +		PixelFormat pixelFormat = videoFormat.first.toPixelFormat();
> > +		if (!pixelFormat)
> > +			continue;
> > +
> > +		Configuration config;
> > +		config.code = code;
> > +		config.captureFormat = pixelFormat;
> > +		config.captureSize = format.size;
> > +
> > +		if (!converter_) {
> > +			config.outputFormats = { pixelFormat };
> > +			config.outputSizes = config.captureSize;
> > +		} else {
> > +			config.outputFormats = converter_->formats(pixelFormat);
> > +			config.outputSizes = converter_->sizes(format.size);
> > +		}
> > +
> > +		configs_.push_back(config);
> > +	}
> > +}
> > +
> >  int SimpleCameraData::setupLinks()
> >  {
> >  	int ret;