| Message ID | 20190817105937.29353-3-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your patch. 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@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? > + CAMERA_LOCATION_FRONT, > + CAMERA_LOCATION_BACK, > + CAMERA_LOCATION_EXTERNAL, > +}; > + > enum ControlId { > AwbEnable, > Brightness, > @@ -19,6 +25,7 @@ enum ControlId { > Saturation, > ManualExposure, > ManualGain, > + Location, > }; > > } /* namespace libcamera */ > -- > 2.22.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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". > > + CAMERA_LOCATION_FRONT, > > + CAMERA_LOCATION_BACK, > > + CAMERA_LOCATION_EXTERNAL, > > +}; I'm afraid I'll ask you to document CameraLocation and the new Location property :-) > > + > > enum ControlId { > > AwbEnable, > > Brightness, > > @@ -19,6 +25,7 @@ enum ControlId { > > Saturation, > > ManualExposure, > > ManualGain, > > + Location, Should we move this to a new PropertyId enum ? > > }; > > > > } /* namespace libcamera */
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@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
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@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 */
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 { + CAMERA_LOCATION_FRONT, + CAMERA_LOCATION_BACK, + CAMERA_LOCATION_EXTERNAL, +}; + enum ControlId { AwbEnable, Brightness, @@ -19,6 +25,7 @@ enum ControlId { Saturation, ManualExposure, ManualGain, + Location, }; } /* namespace libcamera */
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@jmondi.org> --- include/libcamera/control_ids.h | 7 +++++++ 1 file changed, 7 insertions(+)