[libcamera-devel] [PATCH v2 3/7] libcamera: controls: Add camera properties IDs

[Laurent Pinchart]

Hi Niklas,

On Wed, Aug 28, 2019 at 01:08:20PM +0200, Niklas Söderlund wrote:
> On 2019-08-27 11:50:03 +0200, Jacopo Mondi wrote:
> > Add the PropertyID enumeration where to list the camera static
> > properties supported by libcamera. Initially add the Location and
> > Rotation properties
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/control_ids.h | 11 +++++++
> >  src/libcamera/controls.cpp      | 55 +++++++++++++++++++++++++++++++++
> >  2 files changed, 66 insertions(+)
> > 
> > diff --git a/include/libcamera/control_ids.h b/include/libcamera/control_ids.h
> > index 75b6a2d5cafe..d8c3c3265ee6 100644
> > --- a/include/libcamera/control_ids.h
> > +++ b/include/libcamera/control_ids.h
> > @@ -21,6 +21,17 @@ enum ControlId {
> >  	ManualGain,
> >  };
> >  
> > +enum CameraLocation {
> > +	CAMERA_LOCATION_EXTERNAL,
> > +	CAMERA_LOCATION_FRONT,
> > +	CAMERA_LOCATION_BACK,
> > +};
> > +
> > +enum PropertyId {
> > +	Location,
> > +	Rotation,
> > +};
> > +
> >  } /* namespace libcamera */
> >  
> >  namespace std {
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 9adc3badc254..9562ecc189bb 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -555,4 +555,59 @@ void ControlList::update(const ControlList &other)
> >   * Specify a fixed gain parameter
> >   */
> >  
> > +/**
> > + * \enum CameraLocation
> > + * \brief List the supported mounting location of a camera
> > + */
> > +
> > +/**
> > + * \var CameraLocation::CAMERA_LOCATION_EXTERNAL
> > + * \brief Identify an external camera
> > + *
> > + * The camera is connected to the main device through extension cables
> > + * and is freely movable. Typical examples of externally mounted cameras are
> > + * webcams and digital camera devices.
> > + */
> 
> Maybe this can be rephrased to not mention cables, maybe one day there 
> will be a Bluetooth enabled camera which would be external ;-)

I think I pointed this out during review of the kernel side patches. The
documentation of the properties in libcamera will likely copy the one
from the kernel, so I think we should wait until the kernel side is
settled and then respin this patch.

> > +
> > +/**
> > + * \var CameraLocation::CAMERA_LOCATION_FRONT
> > + * \brief Identify a camera mounted in the device front location
> > + *
> > + * The camera is mounted on the front side of the device, which is typically
> > + * the user facing side.
> > + */
> > +
> > +/**
> > + * \var CameraLocation::CAMERA_LOCATION_BACK
> > + * \brief Identify a camera mounted in the device back location
> > + *
> > + * The camera is mounted on the back side of the device, which is typically
> > + * the side opposed to the front one.
> > + */
> > +
> > +/**
> > + * \enum PropertyId
> > + * \brief Numerical properties ID
> > + *
> > + * List the identifiers of the static properties exposed by a Camera.
> > + */
> > +
> > +/**
> > + * \var PropertyId::Location
> > + * \brief The camera mounting location
> > + *
> > + * The Camera device location is expressed as the position relative to the
> > + * device intended usage orientation. Possible values are identified by the
> > + * libcamera::CameraLocation enumeration.
> > + */
> > +
> > +/**
> > + * \var PropertyId::Rotation
> > + * \brief The camera sensor rotation
> > + *
> > + * The Camera rotation is expressed as counter-clockwise rotation in degrees
> > + * in respect to the intended device orientation. Typical values are 0 for
> > + * upright mounted cameras, and 180 for cameras mounted upside down.
> > + */
> 
> Out of curiosity why counter-clockwise and not clockwise? ;-)
> 
> I think this could be improved by explicitly stating where the observe 
> is when examining the rotation. Am I looking into the camera when I 
> rotate it or am "I" the camera and rotate. Maybe this is a bit over 
> zealous, bit I always find this type of information lacking when I read 
> this type of information and have to find out by train and error ;-)

I've had a loooong discussion with Jacopo regarding how to express
rotations. Please see "[PATCH v2 03/10] media: v4l2-ctrl: Document
V4L2_CID_CAMERA_SENSOR_ROTATION". Rotations are very tricky and
counter-intuitive, and we need very detailed documentation that
describes

- the rotation axis (which is a vector, and thus has a direction)
- the rotation angle (unit and meaning of positive/negative values)

Regarding the angle, I think we should follow the usual practices of
geometry and trigonometry, I see no reason to depart from that.
Regarding the axis, it should be perpendicular to the sensor plane, and
the direction is currently being debated in the abovementioned mail
thread.

> > +
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart