| Message ID | 20191204132106.21582-9-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Wed, Dec 04, 2019 at 02:21:04PM +0100, Jacopo Mondi wrote: > Add an operation to the Camera class to retrive the Camera properties s/an operation/a method/ s/retrive/retrieve/ > registered by the pipeline handler. > > While at it, rewords the Camera::controls() operation documentation to s/rewords/reword/ > specify that the camera control information are constant during the > camera lifetime not their value, while the camera properties value are > the actually static information. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/camera.h | 1 + > src/libcamera/camera.cpp | 16 +++++++++++++++- > 2 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index ef6a37bb142c..72d5d62cc902 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -85,6 +85,7 @@ public: > int release(); > > const ControlInfoMap &controls(); > + const ControlList &properties(); > > const std::set<Stream *> &streams() const; > std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles); > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index e810fb725d81..44031ffdb4b3 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -551,7 +551,8 @@ int Camera::release() > /** > * \brief Retrieve the list of controls supported by the camera > * > - * Camera controls remain constant through the lifetime of the camera. > + * The list of controls supported by the camera and their associated > + * constraints remain constant through the lifetime of the camera. Maybe s/camera/Camera object/ ? > * > * \return A ControlInfoMap listing the controls supported by the camera > */ > @@ -560,6 +561,19 @@ const ControlInfoMap &Camera::controls() > return pipe_->controls(this); > } > > +/** > + * \brief Retrieve the list of properties of the camera > + * > + * Camera properties are static information that remain constant through the > + * lifetime of the camera. How about * Camera properties are static information that describe the capabilities of * the camera. They remain constant through the lifetime of the Camera object. > + * > + * \return A ControlList of properties supported by the camera > + */ > +const ControlList &Camera::properties() > +{ > + return pipe_->properties(this); I think it would make sense at some point to let the Camera access CameraData, at which point we can short-circuit the pipeline handler for this. It's out of scope for this patch series anyway. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > +} > + > /** > * \brief Retrieve all the camera's stream information > *
Hi Laurent, two minor notes on wording On Wed, Dec 04, 2019 at 06:20:37PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Dec 04, 2019 at 02:21:04PM +0100, Jacopo Mondi wrote: > > Add an operation to the Camera class to retrive the Camera properties > > s/an operation/a method/ I thought we stabilizd on 'operation' instead of 'method' > s/retrive/retrieve/ > > > registered by the pipeline handler. > > > > While at it, rewords the Camera::controls() operation documentation to > > s/rewords/reword/ > > > specify that the camera control information are constant during the > > camera lifetime not their value, while the camera properties value are > > the actually static information. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/camera.h | 1 + > > src/libcamera/camera.cpp | 16 +++++++++++++++- > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > index ef6a37bb142c..72d5d62cc902 100644 > > --- a/include/libcamera/camera.h > > +++ b/include/libcamera/camera.h > > @@ -85,6 +85,7 @@ public: > > int release(); > > > > const ControlInfoMap &controls(); > > + const ControlList &properties(); > > > > const std::set<Stream *> &streams() const; > > std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles); > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index e810fb725d81..44031ffdb4b3 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -551,7 +551,8 @@ int Camera::release() > > /** > > * \brief Retrieve the list of controls supported by the camera > > * > > - * Camera controls remain constant through the lifetime of the camera. > > + * The list of controls supported by the camera and their associated > > + * constraints remain constant through the lifetime of the camera. > > Maybe s/camera/Camera object/ ? > > > * > > * \return A ControlInfoMap listing the controls supported by the camera > > */ > > @@ -560,6 +561,19 @@ const ControlInfoMap &Camera::controls() > > return pipe_->controls(this); > > } > > > > +/** > > + * \brief Retrieve the list of properties of the camera > > + * > > + * Camera properties are static information that remain constant through the > > + * lifetime of the camera. > > How about > > * Camera properties are static information that describe the capabilities of > * the camera. They remain constant through the lifetime of the Camera object. I thought we use '$class instance' instead of 'object' as object is a Java terminology Thanks j > > > + * > > + * \return A ControlList of properties supported by the camera > > + */ > > +const ControlList &Camera::properties() > > +{ > > + return pipe_->properties(this); > > I think it would make sense at some point to let the Camera access > CameraData, at which point we can short-circuit the pipeline handler for > this. It's out of scope for this patch series anyway. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > +} > > + > > /** > > * \brief Retrieve all the camera's stream information > > * > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Dec 05, 2019 at 02:55:59PM +0100, Jacopo Mondi wrote: > On Wed, Dec 04, 2019 at 06:20:37PM +0200, Laurent Pinchart wrote: > > On Wed, Dec 04, 2019 at 02:21:04PM +0100, Jacopo Mondi wrote: > > > Add an operation to the Camera class to retrive the Camera properties > > > > s/an operation/a method/ > > I thought we stabilizd on 'operation' instead of 'method' If we have I haven't noticed :-) We use the word 'operation' extensively for IPA operations, but when it comes to C++ member methods, they're just called methods as far as I know. > > s/retrive/retrieve/ > > > > > registered by the pipeline handler. > > > > > > While at it, rewords the Camera::controls() operation documentation to > > > > s/rewords/reword/ > > > > > specify that the camera control information are constant during the > > > camera lifetime not their value, while the camera properties value are > > > the actually static information. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > include/libcamera/camera.h | 1 + > > > src/libcamera/camera.cpp | 16 +++++++++++++++- > > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > > index ef6a37bb142c..72d5d62cc902 100644 > > > --- a/include/libcamera/camera.h > > > +++ b/include/libcamera/camera.h > > > @@ -85,6 +85,7 @@ public: > > > int release(); > > > > > > const ControlInfoMap &controls(); > > > + const ControlList &properties(); > > > > > > const std::set<Stream *> &streams() const; > > > std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles); > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index e810fb725d81..44031ffdb4b3 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -551,7 +551,8 @@ int Camera::release() > > > /** > > > * \brief Retrieve the list of controls supported by the camera > > > * > > > - * Camera controls remain constant through the lifetime of the camera. > > > + * The list of controls supported by the camera and their associated > > > + * constraints remain constant through the lifetime of the camera. > > > > Maybe s/camera/Camera object/ ? > > > > > * > > > * \return A ControlInfoMap listing the controls supported by the camera > > > */ > > > @@ -560,6 +561,19 @@ const ControlInfoMap &Camera::controls() > > > return pipe_->controls(this); > > > } > > > > > > +/** > > > + * \brief Retrieve the list of properties of the camera > > > + * > > > + * Camera properties are static information that remain constant through the > > > + * lifetime of the camera. > > > > How about > > > > * Camera properties are static information that describe the capabilities of > > * the camera. They remain constant through the lifetime of the Camera object. > > I thought we use '$class instance' instead of 'object' as object is a > Java terminology 'instance' works too, I don't care much, I think 'object' applies equally well to C++. > > > + * > > > + * \return A ControlList of properties supported by the camera > > > + */ > > > +const ControlList &Camera::properties() > > > +{ > > > + return pipe_->properties(this); > > > > I think it would make sense at some point to let the Camera access > > CameraData, at which point we can short-circuit the pipeline handler for > > this. It's out of scope for this patch series anyway. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > +} > > > + > > > /** > > > * \brief Retrieve all the camera's stream information > > > *
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index ef6a37bb142c..72d5d62cc902 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -85,6 +85,7 @@ public: int release(); const ControlInfoMap &controls(); + const ControlList &properties(); const std::set<Stream *> &streams() const; std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles); diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index e810fb725d81..44031ffdb4b3 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -551,7 +551,8 @@ int Camera::release() /** * \brief Retrieve the list of controls supported by the camera * - * Camera controls remain constant through the lifetime of the camera. + * The list of controls supported by the camera and their associated + * constraints remain constant through the lifetime of the camera. * * \return A ControlInfoMap listing the controls supported by the camera */ @@ -560,6 +561,19 @@ const ControlInfoMap &Camera::controls() return pipe_->controls(this); } +/** + * \brief Retrieve the list of properties of the camera + * + * Camera properties are static information that remain constant through the + * lifetime of the camera. + * + * \return A ControlList of properties supported by the camera + */ +const ControlList &Camera::properties() +{ + return pipe_->properties(this); +} + /** * \brief Retrieve all the camera's stream information *
Add an operation to the Camera class to retrive the Camera properties registered by the pipeline handler. While at it, rewords the Camera::controls() operation documentation to specify that the camera control information are constant during the camera lifetime not their value, while the camera properties value are the actually static information. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/camera.h | 1 + src/libcamera/camera.cpp | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-)