[libcamera-devel] [PATCH 08/10] libcamera: camera: Add Camera properties

[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 at 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 at ideasonboard.com>
> >
> > > +}
> > > +
> > >  /**
> > >   * \brief Retrieve all the camera's stream information
> > >   *

-- 
Regards,

Laurent Pinchart