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

[Jacopo Mondi]

Hi Laurent, Niklas,

On Sat, Aug 17, 2019 at 06:56:58PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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...

For sure Location would not be something that application could change with
a control.

> > >  };
> > >
> > >  } /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190819/b1ac237c/attachment.sig>