| Message ID | 20190122181225.12922-2-jacopo@jmondi.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Tue, Jan 22, 2019 at 07:12:24PM +0100, Jacopo Mondi wrote: > Add abstract base class CameraData for pipeline handlers to store > pipeline specific data in Camera class instances. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/camera.h | 13 ++++++++++ > src/libcamera/camera.cpp | 50 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 63 insertions(+) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 2ea1a68..50041f1 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -12,6 +12,15 @@ > > namespace libcamera { > > +class CameraData > +{ > +public: > + virtual ~CameraData() { } Missing blank line. > +protected: > + CameraData() { } > + CameraData(CameraData &) = delete; Note that the copy constructor is CameraData(const CameraData &). I think you also want to delete the operator=(const CameraData &). > +}; > + > class Camera final > { > public: > @@ -22,11 +31,15 @@ public: > > const std::string &name() const; > > + CameraData *cameraData() const { return data_.get(); } > + void setCameraData(CameraData *data); You should define this as void setCameraData(std::unique_ptr<CameraData> data); to show that the function takes ownership of the object. Bikeshedding, this is pipeline-specific data tied to a camera instance, should the function be called setCameraData or setPipelineData ? I think the "camera" part of the name is redundant as the method is in the Camera class, so I'd go for setPipelineData(), or even setData(). The next bikeshedding question is whether the class should be called CameraData or PipelineData, and I'm not sure about which option is best. Maybe CameraPipelineData to make sure everybody is happy ? :-) We should make those two functions available to pipeline handlers only. One option is to make them private and add the PipelineHandler class as a friend. Classes deriving from PipelineHandler won't be able to access them directly though, so you will need a method in PipelineHandler to do so: static CameraData *cameraData(Camera *camera) { return camera->cameraData(); } static void setCameraData(Camera *camera, CameraData *data) { camera->setCameraData(data); } Feel free to propose a different option. > + > private: > explicit Camera(const std::string &name); > ~Camera(); > > std::string name_; > + std::unique_ptr<CameraData> data_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index acf912b..1e2c858 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -33,6 +33,26 @@ > > namespace libcamera { > > +/** > + * \class CameraData > + * \brief Base class for platform specific data associated with a camera > + * > + * The CameraData base abstract class represents platform specific data > + * a pipeline handler might want to associate with a Camera to access them > + * at a later time. > + * > + * Pipeline handlers are expected to extend this base class with platform > + * specific implementation, associate instances of the derived classes > + * using the Camera::setCameraData() method, and access them at a later time > + * with Camera::cameraData(). > + * > + * Once associated with a camera, lifetime of derived classes instances will > + * be tied to the one of the Camera instance itself. > + * > + * \sa Camera::setCameraData() > + * \sa Camera::cameraData() > + */ > + > /** > * \class Camera > * \brief Camera device > @@ -83,6 +103,36 @@ const std::string &Camera::name() const > return name_; > } > > +/** > + * \fn CameraData *Camera::cameraData() > + * \brief Retrieve the pipeline specific data s/pipeline specific/pipeline-specific/ > + * > + * Borrow a reference to the platform specific data, associated to a camera > + * by pipeline handlers using the setCameraData() method. How about * \return A pointer to the pipeline-specific data set by * setCameraData(). The returned pointer is guaranteed to be valid until the * reference to the Camera object is released. > + */ > + > +/** > + * \brief Set pipeline specific data in the camera s/pipeline specific/pipeline-specific/ > + * > + * Pipeline handlers might need to associate platform-specific informations to s/platform-specific/pipeline-specific/ s/informations to/information with/ > + * camera instances, this method allows them to do so while also transferring s/, this/. This/ > + * ownership of the \a cameraData to the Camera instance. "allows them to do so" sounds a bit heavy. How about * This method allows pipeline handlers to associate pipeline-specific * information with the camera. Ownership of the \a data is transferred to the * Camera, and the data will be deleted when the camera is destroyed. * * If pipeline-specific data has already been associated with the camera by a * previous call to this method, is it replaced by \a data and the previous data * are deleted, rendering all references to them invalid. * * The data can be retrieved by pipeline handlers using the cameraData() method. > + * > + * Pipeline specific data are stored as unique_ptr<> to guarantee its > + * destruction at Camera deletion time. > + * > + * Pipeline specific data can be accessed again by pipeline handlers by > + * borrowing a (mutable) reference using the cameraData() method. > + * > + * \sa Camera::cameraData() > + * \sa CameraData Do we need these given that both the CameraData class and the cameraData() function are referenced from the text, and should thus generate html links already ? > + */ > +void Camera::setCameraData(CameraData *data) > +{ > + data_ = std::unique_ptr<CameraData>(data); data_ = std::move(data); after changing the function prototype to pass an std::unique_ptr<>. > + Unneeded blank line. > +} > + > Camera::Camera(const std::string &name) > : name_(name) > {
Hi Laurent, On Wed, Jan 23, 2019 at 12:29:15PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Jan 22, 2019 at 07:12:24PM +0100, Jacopo Mondi wrote: > > Add abstract base class CameraData for pipeline handlers to store > > pipeline specific data in Camera class instances. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/camera.h | 13 ++++++++++ > > src/libcamera/camera.cpp | 50 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 63 insertions(+) > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > index 2ea1a68..50041f1 100644 > > --- a/include/libcamera/camera.h > > +++ b/include/libcamera/camera.h > > @@ -12,6 +12,15 @@ > > > > namespace libcamera { > > > > +class CameraData > > +{ > > +public: > > + virtual ~CameraData() { } > > Missing blank line. > > > +protected: > > + CameraData() { } > > + CameraData(CameraData &) = delete; > > Note that the copy constructor is CameraData(const CameraData &). I > think you also want to delete the operator=(const CameraData &). Yes, thanks > > > +}; > > + > > class Camera final > > { > > public: > > @@ -22,11 +31,15 @@ public: > > > > const std::string &name() const; > > > > + CameraData *cameraData() const { return data_.get(); } > > + void setCameraData(CameraData *data); > > You should define this as > > void setCameraData(std::unique_ptr<CameraData> data); > > to show that the function takes ownership of the object. we're then going through 2 moves... Is it worth it? I agree that forcing a move in the caller clarifies the reference is not valid afterwards, but it also forces the caller to a very specific pattern: unique_ptr u = make_unique<T>() camera->setData(move(u)) while the caller might have the data to set not stored as unique_ptr. Is this good in your opinion? > > Bikeshedding, this is pipeline-specific data tied to a camera instance, > should the function be called setCameraData or setPipelineData ? I think > the "camera" part of the name is redundant as the method is in the > Camera class, so I'd go for setPipelineData(), or even setData(). > setData() is good > The next bikeshedding question is whether the class should be called > CameraData or PipelineData, and I'm not sure about which option is best. > Maybe CameraPipelineData to make sure everybody is happy ? :-) > I sincerely do not find disturbing having a member field of a Camera class of type CameraData, seems quite natural to me. True, those are pipeline specific data, as platform data are embedded in a struct device... let's go with PipelineData > We should make those two functions available to pipeline handlers only. > One option is to make them private and add the PipelineHandler class as > a friend. Classes deriving from PipelineHandler won't be able to access > them directly though, so you will need a method in PipelineHandler to do > so: > > static CameraData *cameraData(Camera *camera) > { > return camera->cameraData(); > } > > static void setCameraData(Camera *camera, CameraData *data) > { > camera->setCameraData(data); > } > > Feel free to propose a different option. > I think this is good and I will take this in > > + > > private: > > explicit Camera(const std::string &name); > > ~Camera(); > > > > std::string name_; > > + std::unique_ptr<CameraData> data_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index acf912b..1e2c858 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -33,6 +33,26 @@ > > > > namespace libcamera { > > > > +/** > > + * \class CameraData > > + * \brief Base class for platform specific data associated with a camera > > + * > > + * The CameraData base abstract class represents platform specific data > > + * a pipeline handler might want to associate with a Camera to access them > > + * at a later time. > > + * > > + * Pipeline handlers are expected to extend this base class with platform > > + * specific implementation, associate instances of the derived classes > > + * using the Camera::setCameraData() method, and access them at a later time > > + * with Camera::cameraData(). > > + * > > + * Once associated with a camera, lifetime of derived classes instances will > > + * be tied to the one of the Camera instance itself. > > + * > > + * \sa Camera::setCameraData() > > + * \sa Camera::cameraData() > > + */ > > + > > /** > > * \class Camera > > * \brief Camera device > > @@ -83,6 +103,36 @@ const std::string &Camera::name() const > > return name_; > > } > > > > +/** > > + * \fn CameraData *Camera::cameraData() > > + * \brief Retrieve the pipeline specific data > > s/pipeline specific/pipeline-specific/ > > > + * > > + * Borrow a reference to the platform specific data, associated to a camera > > + * by pipeline handlers using the setCameraData() method. > > How about > > * \return A pointer to the pipeline-specific data set by > * setCameraData(). The returned pointer is guaranteed to be valid until the > * reference to the Camera object is released. > > > + */ > > + > > +/** > > + * \brief Set pipeline specific data in the camera > > s/pipeline specific/pipeline-specific/ > > > + * > > + * Pipeline handlers might need to associate platform-specific informations to > > s/platform-specific/pipeline-specific/ > s/informations to/information with/ > > > + * camera instances, this method allows them to do so while also transferring > > s/, this/. This/ > > > + * ownership of the \a cameraData to the Camera instance. > > "allows them to do so" sounds a bit heavy. How about > > * This method allows pipeline handlers to associate pipeline-specific > * information with the camera. Ownership of the \a data is transferred to the > * Camera, and the data will be deleted when the camera is destroyed. > * > * If pipeline-specific data has already been associated with the camera by a > * previous call to this method, is it replaced by \a data and the previous data > * are deleted, rendering all references to them invalid. > * > * The data can be retrieved by pipeline handlers using the cameraData() method. > > > + * > > + * Pipeline specific data are stored as unique_ptr<> to guarantee its > > + * destruction at Camera deletion time. > > + * > > + * Pipeline specific data can be accessed again by pipeline handlers by > > + * borrowing a (mutable) reference using the cameraData() method. > > + * > > + * \sa Camera::cameraData() > > + * \sa CameraData > > Do we need these given that both the CameraData class and the > cameraData() function are referenced from the text, and should thus > generate html links already ? > > > + */ > > +void Camera::setCameraData(CameraData *data) > > +{ > > + data_ = std::unique_ptr<CameraData>(data); > > data_ = std::move(data); > > after changing the function prototype to pass an std::unique_ptr<>. > > > + > > Unneeded blank line. Thanks j > > > +} > > + > > Camera::Camera(const std::string &name) > > : name_(name) > > { > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Wed, Jan 23, 2019 at 02:55:05PM +0100, Jacopo Mondi wrote: > On Wed, Jan 23, 2019 at 12:29:15PM +0200, Laurent Pinchart wrote: > > On Tue, Jan 22, 2019 at 07:12:24PM +0100, Jacopo Mondi wrote: > >> Add abstract base class CameraData for pipeline handlers to store > >> pipeline specific data in Camera class instances. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> include/libcamera/camera.h | 13 ++++++++++ > >> src/libcamera/camera.cpp | 50 ++++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 63 insertions(+) > >> > >> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > >> index 2ea1a68..50041f1 100644 > >> --- a/include/libcamera/camera.h > >> +++ b/include/libcamera/camera.h > >> @@ -12,6 +12,15 @@ > >> > >> namespace libcamera { > >> > >> +class CameraData > >> +{ > >> +public: > >> + virtual ~CameraData() { } > > > > Missing blank line. > > > >> +protected: > >> + CameraData() { } > >> + CameraData(CameraData &) = delete; > > > > Note that the copy constructor is CameraData(const CameraData &). I > > think you also want to delete the operator=(const CameraData &). > > Yes, thanks > > >> +}; > >> + > >> class Camera final > >> { > >> public: > >> @@ -22,11 +31,15 @@ public: > >> > >> const std::string &name() const; > >> > >> + CameraData *cameraData() const { return data_.get(); } > >> + void setCameraData(CameraData *data); > > > > You should define this as > > > > void setCameraData(std::unique_ptr<CameraData> data); > > > > to show that the function takes ownership of the object. > > we're then going through 2 moves... Is it worth it? I agree that > forcing a move in the caller clarifies the reference is not valid > afterwards, but it also forces the caller to a very specific pattern: > > unique_ptr u = make_unique<T>() > camera->setData(move(u)) > > while the caller might have the data to set not stored as unique_ptr. > Is this good in your opinion? I think that clarifying object ownership is worth it. The setData() function takes ownership of the object, and while it could be useful for the caller to access it after calling setData(), it can only do so if the setData() function allows the caller to borrow the object. We have no way in the API to express this at the level of the setData() function, so calling data() afterwards is the best way I found to express the ownership contract. > > Bikeshedding, this is pipeline-specific data tied to a camera instance, > > should the function be called setCameraData or setPipelineData ? I think > > the "camera" part of the name is redundant as the method is in the > > Camera class, so I'd go for setPipelineData(), or even setData(). > > setData() is good > > > The next bikeshedding question is whether the class should be called > > CameraData or PipelineData, and I'm not sure about which option is best. > > Maybe CameraPipelineData to make sure everybody is happy ? :-) > > > > I sincerely do not find disturbing having a member field of a Camera > class of type CameraData, seems quite natural to me. The CameraData type isn't disturbing as such, it was the cameraData() function name that bothered me. > True, those are pipeline specific data, as platform data are embedded > in a struct device... let's go with PipelineData > > > We should make those two functions available to pipeline handlers only. > > One option is to make them private and add the PipelineHandler class as > > a friend. Classes deriving from PipelineHandler won't be able to access > > them directly though, so you will need a method in PipelineHandler to do > > so: > > > > static CameraData *cameraData(Camera *camera) > > { > > return camera->cameraData(); > > } > > > > static void setCameraData(Camera *camera, CameraData *data) > > { > > camera->setCameraData(data); > > } > > > > Feel free to propose a different option. > > I think this is good and I will take this in > > >> + > >> private: > >> explicit Camera(const std::string &name); > >> ~Camera(); > >> > >> std::string name_; > >> + std::unique_ptr<CameraData> data_; > >> }; > >> > >> } /* namespace libcamera */ > >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > >> index acf912b..1e2c858 100644 > >> --- a/src/libcamera/camera.cpp > >> +++ b/src/libcamera/camera.cpp > >> @@ -33,6 +33,26 @@ > >> > >> namespace libcamera { > >> > >> +/** > >> + * \class CameraData > >> + * \brief Base class for platform specific data associated with a camera > >> + * > >> + * The CameraData base abstract class represents platform specific data > >> + * a pipeline handler might want to associate with a Camera to access them > >> + * at a later time. > >> + * > >> + * Pipeline handlers are expected to extend this base class with platform > >> + * specific implementation, associate instances of the derived classes > >> + * using the Camera::setCameraData() method, and access them at a later time > >> + * with Camera::cameraData(). > >> + * > >> + * Once associated with a camera, lifetime of derived classes instances will > >> + * be tied to the one of the Camera instance itself. > >> + * > >> + * \sa Camera::setCameraData() > >> + * \sa Camera::cameraData() > >> + */ > >> + > >> /** > >> * \class Camera > >> * \brief Camera device > >> @@ -83,6 +103,36 @@ const std::string &Camera::name() const > >> return name_; > >> } > >> > >> +/** > >> + * \fn CameraData *Camera::cameraData() > >> + * \brief Retrieve the pipeline specific data > > > > s/pipeline specific/pipeline-specific/ > > > >> + * > >> + * Borrow a reference to the platform specific data, associated to a camera > >> + * by pipeline handlers using the setCameraData() method. > > > > How about > > > > * \return A pointer to the pipeline-specific data set by > > * setCameraData(). The returned pointer is guaranteed to be valid until the > > * reference to the Camera object is released. > > > >> + */ > >> + > >> +/** > >> + * \brief Set pipeline specific data in the camera > > > > s/pipeline specific/pipeline-specific/ > > > >> + * > >> + * Pipeline handlers might need to associate platform-specific informations to > > > > s/platform-specific/pipeline-specific/ > > s/informations to/information with/ > > > >> + * camera instances, this method allows them to do so while also transferring > > > > s/, this/. This/ > > > >> + * ownership of the \a cameraData to the Camera instance. > > > > "allows them to do so" sounds a bit heavy. How about > > > > * This method allows pipeline handlers to associate pipeline-specific > > * information with the camera. Ownership of the \a data is transferred to the > > * Camera, and the data will be deleted when the camera is destroyed. > > * > > * If pipeline-specific data has already been associated with the camera by a > > * previous call to this method, is it replaced by \a data and the previous data > > * are deleted, rendering all references to them invalid. > > * > > * The data can be retrieved by pipeline handlers using the cameraData() method. > > > >> + * > >> + * Pipeline specific data are stored as unique_ptr<> to guarantee its > >> + * destruction at Camera deletion time. > >> + * > >> + * Pipeline specific data can be accessed again by pipeline handlers by > >> + * borrowing a (mutable) reference using the cameraData() method. > >> + * > >> + * \sa Camera::cameraData() > >> + * \sa CameraData > > > > Do we need these given that both the CameraData class and the > > cameraData() function are referenced from the text, and should thus > > generate html links already ? > > > >> + */ > >> +void Camera::setCameraData(CameraData *data) > >> +{ > >> + data_ = std::unique_ptr<CameraData>(data); > > > > data_ = std::move(data); > > > > after changing the function prototype to pass an std::unique_ptr<>. > > > >> + > > > > Unneeded blank line. > > > >> +} > >> + > >> Camera::Camera(const std::string &name) > >> : name_(name) > >> {
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 2ea1a68..50041f1 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -12,6 +12,15 @@ namespace libcamera { +class CameraData +{ +public: + virtual ~CameraData() { } +protected: + CameraData() { } + CameraData(CameraData &) = delete; +}; + class Camera final { public: @@ -22,11 +31,15 @@ public: const std::string &name() const; + CameraData *cameraData() const { return data_.get(); } + void setCameraData(CameraData *data); + private: explicit Camera(const std::string &name); ~Camera(); std::string name_; + std::unique_ptr<CameraData> data_; }; } /* namespace libcamera */ diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index acf912b..1e2c858 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -33,6 +33,26 @@ namespace libcamera { +/** + * \class CameraData + * \brief Base class for platform specific data associated with a camera + * + * The CameraData base abstract class represents platform specific data + * a pipeline handler might want to associate with a Camera to access them + * at a later time. + * + * Pipeline handlers are expected to extend this base class with platform + * specific implementation, associate instances of the derived classes + * using the Camera::setCameraData() method, and access them at a later time + * with Camera::cameraData(). + * + * Once associated with a camera, lifetime of derived classes instances will + * be tied to the one of the Camera instance itself. + * + * \sa Camera::setCameraData() + * \sa Camera::cameraData() + */ + /** * \class Camera * \brief Camera device @@ -83,6 +103,36 @@ const std::string &Camera::name() const return name_; } +/** + * \fn CameraData *Camera::cameraData() + * \brief Retrieve the pipeline specific data + * + * Borrow a reference to the platform specific data, associated to a camera + * by pipeline handlers using the setCameraData() method. + */ + +/** + * \brief Set pipeline specific data in the camera + * + * Pipeline handlers might need to associate platform-specific informations to + * camera instances, this method allows them to do so while also transferring + * ownership of the \a cameraData to the Camera instance. + * + * Pipeline specific data are stored as unique_ptr<> to guarantee its + * destruction at Camera deletion time. + * + * Pipeline specific data can be accessed again by pipeline handlers by + * borrowing a (mutable) reference using the cameraData() method. + * + * \sa Camera::cameraData() + * \sa CameraData + */ +void Camera::setCameraData(CameraData *data) +{ + data_ = std::unique_ptr<CameraData>(data); + +} + Camera::Camera(const std::string &name) : name_(name) {
Add abstract base class CameraData for pipeline handlers to store pipeline specific data in Camera class instances. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/camera.h | 13 ++++++++++ src/libcamera/camera.cpp | 50 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+)