| Message ID | 20190828011710.32128-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > 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? 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
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; }; + 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; + } + cameraData_[camera.get()] = std::move(data); cameras_.push_back(camera); manager_->addCamera(std::move(camera));
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@ragnatech.se> --- src/libcamera/include/pipeline_handler.h | 2 ++ src/libcamera/pipeline_handler.cpp | 12 ++++++++++++ 2 files changed, 14 insertions(+)