[libcamera-devel,v3,01/13] libcamera: pipeline: Move IPA from pipeline to camera data

2025 diff mbox series

The IPA acts on a camera and not on a pipeline which can expose more
then one camera. Move the IPA reference to the CameraData and move the
loading of an IPA from the specific pipeline handler implementation to
base PipelineHandler.

It's still possible to expose a camera without an IPA but if an IPA is
request the camera is not valid and will not be registered in the system
if a suiting IPA module can't be found.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/include/pipeline_handler.h |  4 +++
 src/libcamera/pipeline/vimc.cpp          | 17 ++++++------
 src/libcamera/pipeline_handler.cpp       | 33 ++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 8 deletions(-)

Hi Niklas,

Thank you for the patch.

On Fri, Sep 27, 2019 at 04:44:05AM +0200, Niklas Söderlund wrote:
> The IPA acts on a camera and not on a pipeline which can expose more
> then one camera. Move the IPA reference to the CameraData and move the
> loading of an IPA from the specific pipeline handler implementation to
> base PipelineHandler.
> 
> It's still possible to expose a camera without an IPA but if an IPA is
> request the camera is not valid and will not be registered in the system
> if a suiting IPA module can't be found.

s/suiting/suitable/

Please take into consideration the development use cases, where we often
run the cam and qcam utilities directly from the build directory,
without installing IPAs first. With this patch the vimc camera isn't
created anymore.

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/include/pipeline_handler.h |  4 +++
>  src/libcamera/pipeline/vimc.cpp          | 17 ++++++------
>  src/libcamera/pipeline_handler.cpp       | 33 ++++++++++++++++++++++++
>  3 files changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index 1fdef9cea40f1f0a..4400c7ed835f551d 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -14,6 +14,7 @@
>  #include <string>
>  #include <vector>
>  
> +#include <ipa/ipa_interface.h>
>  #include <libcamera/controls.h>
>  #include <libcamera/stream.h>
>  
> @@ -39,10 +40,13 @@ public:
>  	}
>  	virtual ~CameraData() {}
>  
> +	virtual int loadIPA() { return 0; };
> +
>  	Camera *camera_;
>  	PipelineHandler *pipe_;
>  	std::list<Request *> queuedRequests_;
>  	ControlInfoMap controlInfo_;
> +	std::unique_ptr<IPAInterface> ipa_;
>  
>  private:
>  	CameraData(const CameraData &) = delete;
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index f26a91f86ec1794c..2fa48586fea0b80e 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -52,6 +52,15 @@ public:
>  		delete raw_;
>  	}
>  
> +	int loadIPA() override
> +	{
> +		ipa_ = IPAManager::instance()->createIPA(pipe_, 0, 0);
> +		if (!ipa_)
> +			return -ENOENT;
> +
> +		return 0;
> +	}
> +
>  	int init(MediaDevice *media);
>  	void bufferReady(Buffer *buffer);
>  
> @@ -100,8 +109,6 @@ private:
>  		return static_cast<VimcCameraData *>(
>  			PipelineHandler::cameraData(camera));
>  	}
> -
> -	std::unique_ptr<IPAInterface> ipa_;
>  };
>  
>  VimcCameraConfiguration::VimcCameraConfiguration()
> @@ -361,12 +368,6 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>  	if (!media)
>  		return false;
>  
> -	ipa_ = IPAManager::instance()->createIPA(this, 0, 0);
> -	if (ipa_ == nullptr)
> -		LOG(VIMC, Warning) << "no matching IPA found";
> -	else
> -		ipa_->init();
> -
>  	std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this);
>  
>  	/* Locate and open the capture video node. */
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 3e54aa23d92b9a36..b8a3787e10a587b5 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -12,6 +12,7 @@
>  #include <libcamera/camera_manager.h>
>  
>  #include "device_enumerator.h"
> +#include "ipa_manager.h"
>  #include "log.h"
>  #include "media_device.h"
>  #include "utils.h"
> @@ -58,6 +59,20 @@ LOG_DEFINE_CATEGORY(Pipeline)
>   * exists.
>   */
>  
> +/**
> + * \fn CameraData::loadIPA()
> + * \brief Load an IPA for the camera
> + *
> + * This function shall be implemented by pipeline handlers that wish to have an
> + * IPA. The function must locate and load an IPA, storing a pointer to it in
> + * the \a ipa_ or fail.
> + *
> + * If the pipeline handler do not wish to use an IPA this function shall not
> + * be implemented.
> + *
> + * \return True if a IPA could be loaded, false otherwise
> + */
> +
>  /**
>   * \var CameraData::camera_
>   * \brief The camera related to this CameraData instance
> @@ -96,6 +111,14 @@ LOG_DEFINE_CATEGORY(Pipeline)
>   * creating the camera, and shall not be modified afterwards.
>   */
>  
> +/**
> + * \var CameraData::ipa_
> + * \brief The IPA module used by the camera
> + *
> + * Reference to the Image Processing Algorithms (IPA) operating on the camera's
> + * stream(s). If no IPAs are in operation this should be set to nullptr.
> + */
> +
>  /**
>   * \class PipelineHandler
>   * \brief Create and manage cameras based on a set of media devices
> @@ -425,6 +448,16 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
>  				     std::unique_ptr<CameraData> data)
>  {
>  	data->camera_ = camera.get();
> +
> +	if (data->loadIPA()) {
> +		LOG(Pipeline, Warning) << "Skipping " << camera->name()
> +				       << " no IPA found";
> +		return;

Do we need to propagate this error up ? With this patch the pipeline
handler will believe the camera is correctly registered, couldn't it
cause issues ? What if the pipeline handler provides multiple cameras
and IPA load fails for some of them only ? I think we need to take the
different scenarios into account and handle them properly.

> +	}

I wonder if all this is really worth it, or if it's a bit
over-engineered. I agree with the move the of ipa_ pointer to the
CameraData class, but calling loadIPA() here with the method being
implemented by pipeline handlers seems a bit overkill.

How about splitting this in two, with one patch that moves the ipa
pointer to the CameraData class, and another patch that automates the
IPA loading ? The former shouldn't be controversial and could be merged
quickly, while the latter could then been discussed.

I think that at the end of the day it really depends on how far you want
to go with automated IPA loading.

> +
> +	if (data->ipa_)
> +		data->ipa_->init();
> +
>  	cameraData_[camera.get()] = std::move(data);
>  	cameras_.push_back(camera);
>  	manager_->addCamera(std::move(camera));