[libcamera-devel] [PATCH 04/13] libcamera: pipeline: Add initialization hook for CameraData
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Aug 29 21:11:56 CEST 2019
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
More information about the libcamera-devel
mailing list