[libcamera-devel] [PATCH v5 1/7] libcamera: properties: Add model property

[Niklas Söderlund]

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 at ragnatech.se>
> > Reviewed-by: Kieran Bingham <kieran.bingham at 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

-- 
Regards,
Niklas Söderlund