[libcamera-devel] [PATCH 02/13] libcamera: pipeline: Move IPA from pipeline to camera data

[Niklas Söderlund]

Hi Kieran,

Thanks for your feedback.

On 2019-08-29 14:52:28 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 28/08/2019 02:16, Niklas Söderlund wrote:
> > The IPA acts on a camera and not on a pipeline which can expose more
> > then once camera. Move the IPA reference to the CameraData and move the
> 
> s/once/one/
> 
> 
> > loading of an IPA from the specific pipeline handler implementation to
> > base PipelineHandler.
> 
> This sounds like a good thing, and removes the code duplication for
> loading IPAs.
> 
> > 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 at ragnatech.se>
> > ---
> >  src/libcamera/include/pipeline_handler.h | 12 ++++++--
> >  src/libcamera/pipeline/vimc.cpp          | 10 +-----
> >  src/libcamera/pipeline_handler.cpp       | 39 +++++++++++++++++++++++-
> >  3 files changed, 49 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index 1fdef9cea40f1f0a..ffc7adb802215313 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -15,6 +15,7 @@
> >  #include <vector>
> >  
> >  #include <libcamera/controls.h>
> > +#include <libcamera/ipa/ipa_interface.h>
> >  #include <libcamera/stream.h>
> >  
> >  namespace libcamera {
> > @@ -33,8 +34,11 @@ class Request;
> >  class CameraData
> >  {
> >  public:
> > -	explicit CameraData(PipelineHandler *pipe)
> > -		: pipe_(pipe)
> > +	explicit CameraData(PipelineHandler *pipe,
> > +			    uint32_t minIPAVersion = 0,
> > +			    uint32_t maxIPAVersion = 0)
> > +		: pipe_(pipe), ipa_(nullptr), minIPAVersion_(minIPAVersion),
> > +		  maxIPAVersion_(maxIPAVersion)
> >  	{
> >  	}
> >  	virtual ~CameraData() {}
> > @@ -44,6 +48,10 @@ public:
> >  	std::list<Request *> queuedRequests_;
> >  	ControlInfoMap controlInfo_;
> >  
> > +	std::unique_ptr<IPAInterface> ipa_;
> 
> Is this ok to be public? Or should it be more restricted?
> 
> > +	const uint32_t minIPAVersion_;
> > +	const uint32_t maxIPAVersion_;
> 
> And these?
> 
> Hrm ... looking at CameraData - we treat it as an internal 'public'
> object anyway, so I guess it's matching the existing precedence on that
> class.

Yes I think in the future we might strict up the access around 
CameraData, but for now I think this is sane. It follows what we already 
do and it's not visible to the application.

> 
> 
> > +
> >  private:
> >  	CameraData(const CameraData &) = delete;
> >  	CameraData &operator=(const CameraData &) = delete;
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index e5c4890501db71c8..be6507cd4bc0d1b9 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -38,7 +38,7 @@ class VimcCameraData : public CameraData
> >  {
> >  public:
> >  	VimcCameraData(PipelineHandler *pipe)
> > -		: CameraData(pipe), sensor_(nullptr), debayer_(nullptr),
> > +		: CameraData(pipe, 1, 1), sensor_(nullptr), debayer_(nullptr),
> >  		  scaler_(nullptr), video_(nullptr), raw_(nullptr)
> >  	{
> >  	}
> > @@ -100,8 +100,6 @@ private:
> >  		return static_cast<VimcCameraData *>(
> >  			PipelineHandler::cameraData(camera));
> >  	}
> > -
> > -	std::unique_ptr<IPAInterface> ipa_;
> >  };
> >  
> >  VimcCameraConfiguration::VimcCameraConfiguration()
> > @@ -361,12 +359,6 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> >  	if (!media)
> >  		return false;
> >  
> > -	ipa_ = IPAManager::instance()->createIPA(this, 1, 1);
> > -	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..89b67806597728f9 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"
> > @@ -49,13 +50,20 @@ LOG_DEFINE_CATEGORY(Pipeline)
> >   */
> >  
> >  /**
> > - * \fn CameraData::CameraData(PipelineHandler *pipe)
> > + * \fn CameraData::CameraData(PipelineHandler *pipe, uint32_t minIPAVersion,
> > + * uint32_t maxIPAVersion)
> >   * \brief Construct a CameraData instance for the given pipeline handler
> >   * \param[in] pipe The pipeline handler
> > + * \param[in] minIPAVersion Minimum acceptable version of IPA module
> > + * \param[in] maxIPAVersion Maximum acceptable version of IPA module
> >   *
> >   * The reference to the pipeline handler is stored internally, the caller shall
> >   * guarantee that the pointer remains valid as long as the CameraData instance
> >   * exists.
> > + *
> > + * The IPA maximum and minimum version numbers are used to match with an IPA
> > + * interface that would be compatible with the Camera. If no IPA interface
> > + * is needed for the camera both parameters should be set to 0.
> >   */
> >  
> >  /**
> > @@ -96,6 +104,24 @@ 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 IPA are in operation this should be set to nullptr.
> 
> I'm not sure how to handle the pluralism there. I interpret "IPA" as
> singular, which would mean this should read "If no IPA is in operation..."
> 
> But you do define IPA as "...Algorithms" ... but I'm not sure the
> acronym can convey the pluralism, so it would have to be "If no IPAs are
> in operation..."
> 
> A bit of googling makes me believe we should pluralise the acronym,
> making it the "If no IPAs are in operation..." option.

Good point.

Do you still think the definition, 'Image Processing Algorithms (IPA)' 
is OK?

> 
> > + */
> > +
> > +/**
> > + * \var CameraData::minIPAVersion_
> > + * \brief Minimum acceptable version of IPA module
> > + */
> > +
> > +/**
> > + * \var CameraData::maxIPAVersion_
> > + * \brief Maximum acceptable version of IPA module
> > + */
> > +
> >  /**
> >   * \class PipelineHandler
> >   * \brief Create and manage cameras based on a set of media devices
> > @@ -424,6 +450,17 @@ void PipelineHandler::completeRequest(Camera *camera, Request *request)
> >  void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
> >  				     std::unique_ptr<CameraData> data)
> >  {
> > +	if (data->minIPAVersion_ || data->maxIPAVersion_) {
> > +		data->ipa_ = IPAManager::instance()->createIPA(this,
> > +							       data->minIPAVersion_,
> > +							       data->maxIPAVersion_);
> > +		if (!data->ipa_) {
> > +			LOG(Pipeline, Warning) << "Skipping " << camera->name()
> > +					       << " no IPA found";
> > +			return;
> > +		}
> > +	}
> > +
> 
> The code removed from VIMC calls:
> 
> > -		ipa_->init();
> 
> Shouldn't that be added here somewhere?
> Or is that going to happen somewhere else?
> 
> 
> Do we need to manually init() an ipa? Can't that happen as part of
> createIPA() ?
> 
> 
> With the correct resolution for the missing data->ipa_->init() call,
> which perhaps is handled elsewhere, or can be added if required:

The IPAInterface::init() function is removed in patch 10/13. As it never 
preformed anything useful (other then printing hello world) it was not 
noticed when testing. I will create a separate patch where it's removed 
before this patch in the series to make things clear, thanks for 
spotting this!

> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> 
> >  	data->camera_ = camera.get();
> >  	cameraData_[camera.get()] = std::move(data);
> >  	cameras_.push_back(camera);
> > 
> 
> -- 
> Regards
> --
> Kieran

-- 
Regards,
Niklas Söderlund