[libcamera-devel] [PATCH v4 01/11] libcamera: pipeline: Move IPA from pipeline to camera data

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, Oct 03, 2019 at 07:49:31PM +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.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/include/pipeline_handler.h |  2 ++
>  src/libcamera/pipeline/vimc.cpp          | 12 +++++-------
>  src/libcamera/pipeline_handler.cpp       |  8 ++++++++
>  3 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index 1fdef9cea40f1f0a..42b90a4bf1a6e414 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>
>  
> @@ -43,6 +44,7 @@ public:
>  	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 26477dccbb8fcd5b..f1cfd0ed35cfd9cd 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -101,8 +101,6 @@ private:
>  		return static_cast<VimcCameraData *>(
>  			PipelineHandler::cameraData(camera));
>  	}
> -
> -	std::unique_ptr<IPAInterface> ipa_;
>  };
>  
>  VimcCameraConfiguration::VimcCameraConfiguration()
> @@ -353,13 +351,13 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>  	if (!media)
>  		return false;
>  
> -	ipa_ = IPAManager::instance()->createIPA(this, 0, 0);
> -	if (ipa_ == nullptr)
> +	std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this);
> +
> +	data->ipa_ = IPAManager::instance()->createIPA(this, 0, 0);
> +	if (data->ipa_ == nullptr)
>  		LOG(VIMC, Warning) << "no matching IPA found";
>  	else
> -		ipa_->init();
> -
> -	std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this);
> +		data->ipa_->init();
>  
>  	/* Locate and open the capture video node. */
>  	if (data->init(media))
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 3e54aa23d92b9a36..2d69dea843dd0980 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -96,6 +96,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.

How about "If no IPA exists for the camera, this field is set to
nullptr." or something similar ? This is a requirement, and "should"
isn't strong enough.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> + */
> +
>  /**
>   * \class PipelineHandler
>   * \brief Create and manage cameras based on a set of media devices

-- 
Regards,

Laurent Pinchart