[libcamera-devel] [PATCH 2/5] libcamera: controls: Add camera location control

[Laurent Pinchart]

Hi Jacopo,

On Mon, Aug 19, 2019 at 09:29:43AM +0200, Jacopo Mondi wrote:
> On Sat, Aug 17, 2019 at 06:56:58PM +0300, Laurent Pinchart wrote:
> > On Sat, Aug 17, 2019 at 05:54:24PM +0200, Niklas Söderlund wrote:
> > > On 2019-08-17 12:59:34 +0200, Jacopo Mondi wrote:
> > > > Define a new libcamera control used to specify the camera location.
> > > > Provide an enumeration of possible values to describe the "front",
> > > > "back" and "external" camera locations.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > >  include/libcamera/control_ids.h | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/control_ids.h b/include/libcamera/control_ids.h
> > > > index 75b6a2d5cafe..97ba11ec5f7b 100644
> > > > --- a/include/libcamera/control_ids.h
> > > > +++ b/include/libcamera/control_ids.h
> > > > @@ -12,6 +12,12 @@
> > > >
> > > >  namespace libcamera {
> > > >
> > > > +enum CameraLocation {
> > >
> > > Do we need a CAMERA_LOCATION_UNKNOWN to handle cases where this
> > > information is not available?
> >
> > That, or move CAMERA_LOCATION_EXTERNAL as the first entry and use it as
> > a default.
> >
> > I don't like UNKNOWN much as it gives a way for pipeline handler authors
> > to be lazy and "fix this later".
> 
> I agree, I would not provide a way to skip providing the camera
> location by setting it to unknown.
> 
> > > > +	CAMERA_LOCATION_FRONT,
> > > > +	CAMERA_LOCATION_BACK,
> > > > +	CAMERA_LOCATION_EXTERNAL,
> > > > +};
> >
> > I'm afraid I'll ask you to document CameraLocation and the new Location
> > property :-)
> 
> Ah yes indeed. I'll add documentation in controls.cpp
> 
> > > > +
> > > >  enum ControlId {
> > > >  	AwbEnable,
> > > >  	Brightness,
> > > > @@ -19,6 +25,7 @@ enum ControlId {
> > > >  	Saturation,
> > > >  	ManualExposure,
> > > >  	ManualGain,
> > > > +	Location,
> >
> > Should we move this to a new PropertyId enum ?
> 
> Would you keep controls and property separated? I'm debated... Looking
> at android metadata tags, most of the controls have an associated
> 'controlAvailble' static property assigned and I wonder if that's a
> good idea or we can re-use the same id but in different context...

Android has decided to build everything around a single metadata
concept, I assume to give easy serialisation. I don't think we have to
do the same. The data structures underlying controls and properties
should be the same, with the same serialisation format, but I think it
would make sense to keep the definition of controls and properties
separate, as they're not the same.

> For sure Location would not be something that application could change with
> a control.
> 
> > > >  };
> > > >
> > > >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart