[libcamera-devel] [PATCH 0/9] libcamera: camera: Add camera ID
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jul 27 02:34:50 CEST 2020
Hi Niklas,
On Sat, Jul 25, 2020 at 10:35:26AM +0200, Niklas Söderlund wrote:
> On 2020-07-25 02:44:07 +0300, Laurent Pinchart wrote:
> > On Fri, Jul 24, 2020 at 12:08:35PM +0200, Niklas Söderlund wrote:
> >> On 2020-07-20 15:07:07 +0200, Jacopo Mondi wrote:
> >>> On Sat, Jul 18, 2020 at 03:23:15PM +0200, Niklas Söderlund wrote:
> >>>> Hello,
> >>>
> >>> minors apart that I will reply to patch-by-patch, I have two questions
> >>> on the series in general
> >>>
> >>> 1) The series introduce and 'id' to be used alongside and
> >>> alternatively to the camera 'name'. This might just be a matter of
> >>> terminology, but I find this a bit confusing. Ideally, the 'name'
> >>> should be the unique part, to which a 'model' (to mimic the API we
> >>> have for camera sensors) could be added.
> >>
> >> I'm not against this change. But maybe removing the name() and adding a
> >> model() could be done on top as this series main goal is to add a id()
> >> field?
> >
> > We're entering the bikeshedding territory a little bit, but I think it's
> > an important question. We may want to drop "name" altogether, as it's a
> > confusing name (no pun intended). Using friendlyName() and uniqueName(),
> > for instance, would make the API clearer. We can continue bikeshedding
> > on the name, but I think whatever function names we pick, they should be
> > fairly explicit.
>
> I want to avoid bikeshedding so I will only focus of the addition of the
> new id()/uniequeName() in this series. Then once that is in we can think
> about how we wish to evolve or remove name().
>
> Some bikeshedding is of course needed, in my first private incarnation
> of this series I had named the new property uniqueName(). It became very
> cumbersome in the documentation and felt none obvious, switching to id()
> felt like the right thing. So while we hash this out I will keep id() as
> a working name and then I will switch it to whatever the majority likes.
I think this is important bikeshedding :-) I'm fine continuing to
develop the patch series without a rename until we reach a consensus,
but I think we need to sort out the naming before the final version.
This is also not just about the function names, but also about making
sure we have a solid scheme for camera identification. Not everything
needs to be implemented as part of this patch series, but the design
needs to cover the whole problem space.
> >>> 2) The API and the cam implementation allow to use 'name' and 'id'
> >>> interchangibily. Is this a good thing ? Applications should always use
> >>> 'id' when interfacing to libcamera, and ideally 'name' should be a
> >>> shortcut for users, to easily select a camera (provided it is unique).
> >>> If I'm not mistaken it is currently possible to ask libcamera for a
> >>> 'camera' name, should we drop this and implement that part in the
> >>> application ? 'cam' and alike can and should use mnemonic names to
> >>> users, but should libcamera do the same? Do we want an API that allows
> >>> selecting camera with a name which is not guaranteed to be unique and
> >>> consistent ? I would say we don't...
> >>
> >> Also I'm not against this change and I think if we all agree this series
> >> can be modified to only allow selecting cameras based on id() instead of
> >> id() or name() as this version of this series allows.
> >
> > I'm with Jacopo on this, I'd prefer if libcamera pushed applications to
> > use a safer API, while still offering the option of an application-side
> > manual implementation based on the friendly name.
>
> Then we all agree on this point and this series will modify the get()
> function to only operate on the new id() property.
>
> >>> What do you think ?
> >>>
> >>>> This series aims to add a ID to each camera in addition to it's more
> >>>> user-friendly name. The ID is unique and persistent between reboots of
> >>>> the same system. The use-case for this is to create a single
> >>>> machine-friendly ID that can be stored and used to always resolve to the
> >>>> same camera.
> >>>>
> >>>> The idea on how to generate a ID is to take the sysfs path of the sensor
> >>>> device which is part of each camera pipeline. As the path describes the
> >>>> location of the sensor hardware it is persistent across reboots and as
> >>>> the path is read from sysfs it's guaranteed to be unique in the system.
> >>>>
> >>>> For pipelines that do not have a sensor (UVC) the sysfs path of the main
> >>>> video device is used instead. That path resolves to the USB device and
> >>>> includes the USB bus information so it satisfy the ID requirements.
> >>>>
> >>>> While working with this problem it became apparent that two pipelines
> >>>> diverge from the others on how they name their cameras, raspberrypi and
> >>>> vimc. This series aligns these two and adds a helper to avoid such
> >>>> situations in the future. Unfortunately this means the user-friendly
> >>>> name of the sensor changes but this proves the need for a
> >>>> machine-friendly ID which luckily this series also adds :-)
> >>>>
> >>>> Before this series camera user-friendly names on different systems
> >>>> looked like this (I do not have access to a simple pipeline device):
> >>>>
> >>>> - ipu3
> >>>> ov13858 8-0010
> >>>> ov5670 10-0036
> >>>> - raspberrypi
> >>>> imx219
> >>>> - rkisp1
> >>>> ov5695 7-0036
> >>>> ov2685 7-003c
> >>>> - uvcvideo
> >>>> Logitech Webcam C930e
> >>>> - vimc
> >>>> VIMC Sensor B
> >>>>
> >>>> With this series applied the user-friendly names machine-friendly ID on
> >>>> the same systems look like this:
> >>>>
> >>>> The format is:
> >>>> <user-friendly name> (<machine-friendly ID>)
> >>>>
> >>>> - ipu3
> >>>> ov13858 8-0010 (pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-OVTID858:00)
> >>>> ov5670 10-0036 (pci0000:00/0000:00:19.2/i2c_designware.5/i2c-10/i2c-INT3479:00)
> >>>> - raspberrypi
> >>>> imx219 10-0010 (platform/soc/3f205000.i2c/i2c-11/i2c-10/10-0010)
> >>>> - rkisp1
> >>>> ov5695 7-0036 (platform/ff160000.i2c/i2c-7/7-0036)
> >>>> ov2685 7-003c (platform/ff160000.i2c/i2c-7/7-003c)
> >>>> - uvcvideo
> >>>> Logitech Webcam C930e (pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0)
> >>>> - vimc
> >>>> Sensor B (platform/vimc.0)
> >>>>
> >>>> Where it previously where possible to select a camera by its
> >>>> user-friendly name its now possible to also select it using its
> >>>> machine-friendly one. The following is therefor two equivalent
> >>>> commands:
> >>>>
> >>>> $ cam -c "Logitech Webcam C930e" -C
> >>>> $ cam -c "pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0" -C
> >>>>
> >>>> Niklas Söderlund (9):
> >>>> libcamera: v4l2_device: Add method to lookup device path
> >>>> libcamera: camera_sensor: Expose a sensor ID
> >>>> libcamera: camera: Add camera ID
> >>>> libcamera: camera_manager: Enforce unique camera IDs
> >>>> libcamera: camera_manager: Try to match camera IDs first
> >>>> libcamera: pipeline: vimc: Align camera name
> >>>> libcamera: pipeline: raspberrypi: Align camera name
> >>>> libcamera: camera: Add create() that operates on CameraSensor
> >>>> cam: Print camera IDs when listing cameras
> >>>>
> >>>> include/libcamera/camera.h | 11 +++-
> >>>> include/libcamera/internal/camera_sensor.h | 2 +
> >>>> include/libcamera/internal/v4l2_device.h | 1 +
> >>>> src/cam/main.cpp | 3 +-
> >>>> src/libcamera/camera.cpp | 54 +++++++++++++++----
> >>>> src/libcamera/camera_manager.cpp | 13 +++++
> >>>> src/libcamera/camera_sensor.cpp | 17 ++++++
> >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++--
> >>>> .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-
> >>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> >>>> src/libcamera/pipeline/simple/simple.cpp | 3 +-
> >>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++-
> >>>> src/libcamera/pipeline/vimc/vimc.cpp | 4 +-
> >>>> src/libcamera/v4l2_device.cpp | 27 ++++++++++
> >>>> test/camera/buffer_import.cpp | 2 +-
> >>>> test/camera/capture.cpp | 2 +-
> >>>> test/camera/configuration_default.cpp | 2 +-
> >>>> test/camera/configuration_set.cpp | 2 +-
> >>>> test/camera/statemachine.cpp | 2 +-
> >>>> test/controls/control_info_map.cpp | 2 +-
> >>>> test/controls/control_list.cpp | 2 +-
> >>>> 21 files changed, 138 insertions(+), 32 deletions(-)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list