[libcamera-devel] [PATCH 04/13] libcamera: pipeline: Add initialization hook for CameraData

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2019-08-28 18:07:59 +0200, Jacopo Mondi wrote:
> Hi Niklas
> 
> On Wed, Aug 28, 2019 at 03:17:01AM +0200, Niklas Söderlund wrote:
> > Add a hook so CameraData can be initialized before a camera is exposed
> > to an application but after all resources and possibly an IPA have been
> > found.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/include/pipeline_handler.h |  2 ++
> >  src/libcamera/pipeline_handler.cpp       | 12 ++++++++++++
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index 91d40ef40a465c4e..45f9457879c64363 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -43,6 +43,8 @@ public:
> >  	}
> >  	virtual ~CameraData() {}
> >
> > +	virtual int initCameraData() { return 0; };
> > +
> 
> As this is CameraData::initCameraData() I think it could be just named
> init().
> 
> However, as I see it in use, and as you state in the commit message,
> this seems to be related to later initialization, specifically when
> for the IPA initialization. Do you see other uses for this? Or should
> this be made an initIPA() operation ?

I see no other use for it and the only call site for this should be in 
pipeline_handler.cpp as the intention is to initialize the camera data 
after all resources (including IPA) have been found.

I do agree with you that initCameraData() was a bad name and will rename 
it to init().

> 
> 
> >  	Camera *camera_;
> >  	PipelineHandler *pipe_;
> >  	std::list<Request *> queuedRequests_;
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 766fd496306ece9c..6b3ee0ed8f39676a 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -66,6 +66,12 @@ LOG_DEFINE_CATEGORY(Pipeline)
> >   * is needed for the camera both parameters should be set to 0.
> >   */
> >
> > +/**
> > + * \fn CameraData::initCameraData()
> > + * \brief Hook to initialize the camera data
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +
> >  /**
> >   * \var CameraData::camera_
> >   * \brief The camera related to this CameraData instance
> > @@ -462,6 +468,12 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
> >  	}
> >
> >  	data->camera_ = camera.get();
> > +	if (data->initCameraData()) {
> > +		LOG(Pipeline, Warning) << "Skipping " << camera->name()
> > +				       << " initialization camera data failed";
> > +		return;
> > +	}
> > +
> 
> Shouldn't the error be reported up and fail the whole cameras
> registration?

THe camera is not registered by the system if this fails, as 
CameraManager::addCamera() is never called.

We could propagate the error to an upper layer (pipeline handler 
match()), but what would it do with it? Fail the whole pipeline or print 
an error message and try to work with the (potential) other cameras 
handled by the same pipeline?

I think the second option is the most sane as it will make the error 
known while still providing as much functionality as possible for the 
user. I'm open to discuss this tho if we think the whole pipeline should 
fail instead.

> 
> Thanks
>   j
> >  	cameraData_[camera.get()] = std::move(data);
> >  	cameras_.push_back(camera);
> >  	manager_->addCamera(std::move(camera));
> > --
> > 2.22.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel



-- 
Regards,
Niklas Söderlund