[libcamera-devel,v5,1/7] libcamera: properties: Add model property
diff mbox series

Message ID 20200925150743.1822226-2-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • libcamera: Allow for user-friendly names in applications
Related show

Commit Message

Niklas Söderlund Sept. 25, 2020, 3:07 p.m. UTC 
A user-friendly camera identification model name. The model name must
to the extent possible describe the camera sensor model. For most
devices this is the model name of the sensor. While for some devices the
sensor model is unavailable as the sensor or the entire camera is part
of a larger unit and exposed as a black-box to the system. In such cases
the model name of the smallest component closest to the sensor must be
used.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
* Changes since v4
- Expand description.

* Changes since v3
- s/as ASCII/in ASCII/
---
 src/libcamera/property_ids.yaml | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Laurent Pinchart Sept. 28, 2020, 4:53 p.m. UTC | #1 
Hi Niklas,

Thank you for the patch.

On Fri, Sep 25, 2020 at 05:07:37PM +0200, Niklas Söderlund wrote:
> A user-friendly camera identification model name. The model name must
> to the extent possible describe the camera sensor model. For most
> devices this is the model name of the sensor. While for some devices the
> sensor model is unavailable as the sensor or the entire camera is part
> of a larger unit and exposed as a black-box to the system. In such cases
> the model name of the smallest component closest to the sensor must be
> used.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> * Changes since v4
> - Expand description.
> 
> * Changes since v3
> - s/as ASCII/in ASCII/
> ---
>  src/libcamera/property_ids.yaml | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 74ad0195d6310367..b53e850c8b647c71 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -387,6 +387,26 @@ controls:
>                                |                    |
>                                +--------------------+
>  
> +  - Model:
> +      type: string
> +      description: |
> +        A user-friendly camera identification model name. The model name must
> +        to the extent possible describe the camera sensor model. For most
> +        devices this is the model name of the sensor. While for some devices the
> +        sensor model is unavailable as the sensor or the entire camera is part
> +        of a larger unit and exposed as a black-box to the system. In such cases
> +        the model name of the smallest component closest to the sensor must be
> +        used.

This is clearer than in v4, but I'm still not very enthousiastic :-( The
camera sensor model isn't very end-user-friendly in general. And what
will happen when we will have multi-sensors cameras ?

I understand that this property isn't mean to be a camera name displayed
to the end-user, but should instead be combined with other camera
information (such as the location for instance) to create a camera name.
Maybe that should be documented here too. There's still something
ill-defined about the property that bothers me.

> +
> +        The model name is not guaranteed to be unique in the system nor does
> +        it guarantee to be stable or have any other properties required to make

s/does it guarantee/is it guaranteed/

> +        it a good candidate to be used as a permanent identifier of a camera.
> +
> +        The model name shall describe the camera in a human readable format and
> +        be shall be encoded in ASCII.

s/be shall be/shall be/

> +
> +        Example model names are 'ov5670', 'imx219' or 'Logitech Webcam C930e'.
> +
>    - UnitCellSize:
>        type: Size
>        description: |
Niklas Söderlund Sept. 29, 2020, 1:48 p.m. UTC | #2 
Hi Laurent,

Thanks for your persisting in reviewing this.

On 2020-09-28 19:53:33 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Fri, Sep 25, 2020 at 05:07:37PM +0200, Niklas Söderlund wrote:
> > A user-friendly camera identification model name. The model name must
> > to the extent possible describe the camera sensor model. For most
> > devices this is the model name of the sensor. While for some devices the
> > sensor model is unavailable as the sensor or the entire camera is part
> > of a larger unit and exposed as a black-box to the system. In such cases
> > the model name of the smallest component closest to the sensor must be
> > used.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> > * Changes since v4
> > - Expand description.
> > 
> > * Changes since v3
> > - s/as ASCII/in ASCII/
> > ---
> >  src/libcamera/property_ids.yaml | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > index 74ad0195d6310367..b53e850c8b647c71 100644
> > --- a/src/libcamera/property_ids.yaml
> > +++ b/src/libcamera/property_ids.yaml
> > @@ -387,6 +387,26 @@ controls:
> >                                |                    |
> >                                +--------------------+
> >  
> > +  - Model:
> > +      type: string
> > +      description: |
> > +        A user-friendly camera identification model name. The model name must
> > +        to the extent possible describe the camera sensor model. For most
> > +        devices this is the model name of the sensor. While for some devices the
> > +        sensor model is unavailable as the sensor or the entire camera is part
> > +        of a larger unit and exposed as a black-box to the system. In such cases
> > +        the model name of the smallest component closest to the sensor must be
> > +        used.
> 
> This is clearer than in v4, but I'm still not very enthousiastic :-( The
> camera sensor model isn't very end-user-friendly in general. And what
> will happen when we will have multi-sensors cameras ?

I assume it will have to go thru the same transformation as other camera 
properties (UnitCellSize, PixelArraySize, 
PixelArrayOpticalBlackRectangles and PixelArrayActiveAreas)? 

My current feeling is that we will make these and Model a stream 
property instead of a camera property as today. Or possibly we need to 
introduce a new concept between Camera and Stream to be able to model 
Stream constraints for multi-senors cameras?

> 
> I understand that this property isn't mean to be a camera name displayed
> to the end-user, but should instead be combined with other camera
> information (such as the location for instance) to create a camera name.
> Maybe that should be documented here too. There's still something
> ill-defined about the property that bothers me.

I understand the feeling this is bothering me but I don't know why :-) 
Unfortunately it's just as frustrating on the other end ;-P

I have tried to capture what I understanding is the top concern here but 
I'm sure it will not clear you assert line. Would it help if me made 
this a draft control to be able to move forward or shall I abandon the 
control (and the setting of the TIFFTAG_MODEL) and just post a patch to 
cam to print the Location property for now?

If it's any help the TIFFTAG_MODEL is defined as,

    "The model name or number of the scanner, video digitizer, or other 
    type of equipment used to generate the image."

> 
> > +
> > +        The model name is not guaranteed to be unique in the system nor does
> > +        it guarantee to be stable or have any other properties required to make
> 
> s/does it guarantee/is it guaranteed/
> 
> > +        it a good candidate to be used as a permanent identifier of a camera.
> > +
> > +        The model name shall describe the camera in a human readable format and
> > +        be shall be encoded in ASCII.
> 
> s/be shall be/shall be/
> 
> > +
> > +        Example model names are 'ov5670', 'imx219' or 'Logitech Webcam C930e'.
> > +
> >    - UnitCellSize:
> >        type: Size
> >        description: |
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
index 74ad0195d6310367..b53e850c8b647c71 100644
--- a/src/libcamera/property_ids.yaml
+++ b/src/libcamera/property_ids.yaml
@@ -387,6 +387,26 @@  controls:
                               |                    |
                               +--------------------+
 
+  - Model:
+      type: string
+      description: |
+        A user-friendly camera identification model name. The model name must
+        to the extent possible describe the camera sensor model. For most
+        devices this is the model name of the sensor. While for some devices the
+        sensor model is unavailable as the sensor or the entire camera is part
+        of a larger unit and exposed as a black-box to the system. In such cases
+        the model name of the smallest component closest to the sensor must be
+        used.
+
+        The model name is not guaranteed to be unique in the system nor does
+        it guarantee to be stable or have any other properties required to make
+        it a good candidate to be used as a permanent identifier of a camera.
+
+        The model name shall describe the camera in a human readable format and
+        be shall be encoded in ASCII.
+
+        Example model names are 'ov5670', 'imx219' or 'Logitech Webcam C930e'.
+
   - UnitCellSize:
       type: Size
       description: |