[libcamera-devel] [PATCH v5 0/7] libcamera: Allow for user-friendly names in applications

[Niklas Söderlund]

Hi Tomasz,

Thanks for your feedback.

On 2020-09-26 14:24:20 +0200, Tomasz Figa wrote:
> Hi Niklas,
> 
> On Fri, Sep 25, 2020 at 5:08 PM Niklas Söderlund
> <niklas.soderlund at ragnatech.se> wrote:
> >
> > Hello,
> >
> > This series trues to advance the topic of user-friendly names in
> > applications.
> >
> > Patch 1/7 adds a new camera property 'Model' that may be used to record
> > a cameras model. Patch 2/7, 3/7, 4/7 and 5/7 then implements this new
> > property for all pipeline handlers.
> >
> > Patch 6/7 introduce user-friendly names to the cam utility.
> >
> > The final patch 7/7 records the model information in the DNG files
> > created by qcam. More work is needed on top of this series to create
> > user-friendly names for qcam. My intention is to do this in a follow up
> > series once this is merged as I fear some bikeshedding on the style of
> > the user-friendly name so lets battle that out in cam first :-)
> >
> > Example of listing cameras with and without this series,
> >
> > Without:
> >     $ cam -l
> >     Available cameras:
> >     1: \_SB_.PCI0.RP05.PXSX-2.4:1.0-046d:0843
> >     2: platform/vimc.0 Sensor B
> >
> >     $ cam -l
> >     Available cameras:
> >     1: /base/i2c at ff160000/camera at 36
> >     2: /base/i2c at ff160000/camera at 3c
> >
> >     $ cam -l
> >     Available cameras:
> >     1: /base/soc/i2c0mux/i2c at 1/imx219 at 10
> >
> >     $ cam -l
> >     Available cameras:
> >     1: \_SB_.PCI0.I2C2.CAM0
> >     2: \_SB_.PCI0.I2C4.CAM1
> >
> >
> > With:
> >     $ cam -l
> >     Available cameras:
> >     1: External camera Logitech Webcam C930e (\_SB_.PCI0.RP05.PXSX-2.4:1.0-046d:0843)
> >     2: Internal front camera Sensor B (platform/vimc.0 Sensor B)
> >
> >     $ cam -l
> >     Available cameras:
> >     1: Internal front camera ov5695 (/base/i2c at ff160000/camera at 36)
> >     2: Internal front camera ov2685 (/base/i2c at ff160000/camera at 3c)
> 
> First of all, thanks for working on this. The new names are indeed
> much more friendly. I still wonder, though, if for the user the exact
> sensor model of an internal camera has any meaning. Perhaps something
> like "Internal 5 megapixel front camera" and "Internal 2 megapixel
> front camera" would be even friendlier? What do you think?

I agree with you that user-friendly names can be enhanced even further 
to make them more useful for humans. On the devices I have access to the 
location property is not set in firmware (yet), and I expect this to be 
the case for most firmware as the property is a rather new. Until we 
have that sorted and common place in most firmware I think the sensor 
model adds value. But as you point out going forward a true location 
brings much more value.

Do you think keeping the sensor model in the name for now is a good idea 
or should we drop it and try to add pressure to get the location 
reporting correct?

> 
> Best regards,
> Tomasz
> 
> >
> >     $ cam -l
> >     Available cameras:
> >     1: Internal front camera imx219 (/base/soc/i2c0mux/i2c at 1/imx219 at 10)
> >
> >     $ cam -l
> >     Available cameras:
> >     1: Internal front camera ov13858 (\_SB_.PCI0.I2C2.CAM0)
> >     2: Internal front camera ov5670 (\_SB_.PCI0.I2C4.CAM1)
> >
> > It can be observed above that all Cameras that report the Location
> > property do so by stating they face the front. We know this is not true
> > for some devices. This is however not a fault of this series as it only
> > prints what is reported by the Camera. Once we teach the Camera to
> > report true values for these properties cam will print the correct
> > information.
> >
> > Niklas Söderlund (7):
> >   libcamera: properties: Add model property
> >   libcamera: utils: Add method to remove non-ASCII characters
> >   libcamera: camera_sensor: Set sensor model property
> >   libcamera: pipeline: uvcvideo: Initialize CameraData from MediaDevice
> >   libcamera: pipeline: uvcvideo: Set sensor model property
> >   cam: Print user-friendly camera names
> >   qcam: dng_writer: Record camera model
> >
> >  include/libcamera/internal/utils.h           |  2 ++
> >  src/cam/main.cpp                             | 31 +++++++++++++++++++-
> >  src/libcamera/camera_sensor.cpp              |  2 ++
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 31 ++++++++++----------
> >  src/libcamera/property_ids.yaml              | 20 +++++++++++++
> >  src/libcamera/utils.cpp                      | 17 +++++++++++
> >  src/qcam/dng_writer.cpp                      | 13 ++++++--
> >  7 files changed, 97 insertions(+), 19 deletions(-)
> >
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund