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

[Laurent Pinchart]

Hi Tomasz,

On Sat, Sep 26, 2020 at 03:53:26PM +0200, Tomasz Figa wrote:
> On Sat, Sep 26, 2020 at 3:47 PM Niklas Söderlund wrote:
> > On 2020-09-26 14:24:20 +0200, Tomasz Figa wrote:
> > > On Fri, Sep 25, 2020 at 5:08 PM Niklas Söderlund 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?
> 
> How about the sensor resolutions, as in the examples I listed? I think
> it's really hard to tell which camera is which for the user by looking
> at the sensor model, but in most of the cases, the resolutions are
> very well specified and understood.
> 
> Of course the location is critical too, but it doesn't solve the
> problem fully either - we may have multiple sensors at the same
> location. Actually, one could imagine something like "Internal front
> wide-angle camera" and "Internal front telephoto camera". Maybe these
> user-friendly names should be configurable somehow?

The design idea here (and it should really be documented somewhere, like
in the Camera class documentation) is that libcamera should provide
enough information to applications to construct meaningful names for
end-users, with the option to localize them. This patch series
demonstrates how this can be done in cam and qcam. The exact scheme to
construct camera names is left to applications (or frameworks), as
different applications have different types of end-users.

The examples you gave above, with wide-angle and telephoto are very
good, and that's what I would expect the camera service in a phone or
tablet to create. libcamera needs to provide the location of the camera
and its resolution (we do so already, although it's not plumbed to the
firmware on all devices we support). Lens information is also needed,
and that's currently missing. We will add it, but I don't think it
should be a blocker for this series :-)

Now the real question is what cam should output. cam is both a sample
application (it's thus important to showcase how to create a camera
name), but also a developer-oriented swiss army knife tool for
libcamera. Its expected audience is thus developers, and the sensor
names can thus be more relevant than for end-users of a phone. Maybe we
could go for something like "Internal front 5MP camera (ov5693)", to
both showcase how to create a camera name from different components, but
also keep the sensor name given the tool's audience ?

> > > >
> > > >     $ 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(-)

-- 
Regards,

Laurent Pinchart