[libcamera-devel,00/12] Add basic camera enumeration

Message ID 20181222230041.29999-1-niklas.soderlund@ragnatech.se
Headers show
Series
  • Add basic camera enumeration
Related show

Message

Niklas Söderlund Dec. 22, 2018, 11 p.m. UTC
Hi,

This series adds basic camera enumeration to libcamera. The idea is to 
provide something which can server as a starting point to solve other 
problems around and enhance the enumeration process once the overall 
design is properly reviewed and requirements on the features missing 
becomes a bit more clear. The code in this series is fully functional 
and allows to add more pipeline handlers and enumerate cameras.

In this series there are however a few known limitations which I find 
acceptable to get started and which can be solved one by one once the 
basic structure of enumeration is reviewed and hopefully upstream.  The 
known limitations are:

- No sysfs based device enumerator. This will be needed so libcamera 
  have something to fallback on if udev enumeration fails or is not 
  available at all. The rational to focus on udev first is that it's 
  more complex and will support more features then sysfs (most notably 
  hot-(un)plug support.

- The camera object (1/12) in this series is a mix of Jacopo's and 
  Laurent's previous patches with modifications by me. By no means would 
  I like to claim authorship of all work in this patch but as I modified 
  it I did claim authorship of it. If any of you wish to be claim 
  authorship for it let me know, I don't want you to be unprepared when
  you get (git) blamed for my mistakes :-)

- The DeviceInfo class should probably down the road be replaced by a 
  more comprehensive MediaDevice class. For now i feel this solution 
  works as it allows us to move forward with a MediaDevice class focused 
  on the needs of the Camera class and later extend it with the limited 
  features needed to replace DeviceInfo. For this reason I chose the 
  name DeciceInfo to not cause conflicts and allow the two to live in 
  parallel for a short wile if needed.

- There is no support for hot-plug or hot-unplug. The state of the 
  system is assumed to be static for the entire execution. Some care 
  have been taken to facilitate addition of hot-plug support.

- There is only one pipeline handler included in this series for the 
  vimc driver. It should be fairly trivial to add one for the Soraka, I 
  chose not to do so to reduce the number of patches and to increase the 
  number of potential testers as anyone can do 'modprobe vimc' and have 
  the test case for this series passing.

Patch 1/12 adds a mock for a Camera class and stubs in lifetime 
management features needed to allow the camera manger to try and control 
the lifetime of objects. Patch 2/12 adds a build requirement on libudev.

Patch 3/12 -- 7/12 adds the device enumerator and accompanying classes 
to allow storing and searching for media devices by a pipeline handler.  
Patch 8/12 adds the documentation for all enumerator related classes. I 
chose to split this in multiple patches to ease review, I'm open to 
squashing them together if someone feels it's better for some reason or 
other.

Patch 9/12 adds a base class for pipeline handlers and a global list of 
pipe handlers in the system which each pipeline implementation can 
register it self with to be included in the enumeration of cameras. This 
approach allows the pipeline implementations to be self contained in a 
single .cpp file, thanks Kieran for pointing me in the right direction 
for how to do this in C++ ;-)

Patch 10/12 adds the camera manger which tracks lifetime management of 
cameras in the system and is the interface to applications to list and 
get cameras out of the library. Patch 11/12 adds a pipeline handler for 
the vimc driver and lastly 12/12 adds a test case which simply uses most 
features of the series to list all cameras in the system.

This series is based on top of the current master branch and the test 
case have been run under valgrind and no memory issues are found.

valgrind --tool=memcheck --leak-check=full ./test/list
==29822== Memcheck, a memory error detector
==29822== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==29822== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==29822== Command: ./test/list
==29822==
[18:23:20.377682367] INFO deviceenumerator.cpp:70 Device: /dev/media0 Entity: 'Integrated Camera: Integrated C' -> /dev/video1
[18:23:20.422617627] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Debayer A' -> /dev/v4l-subdev2
[18:23:20.423131606] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Debayer B' -> /dev/v4l-subdev3
[18:23:20.423351904] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'RGB/YUV Capture' -> /dev/video4
[18:23:20.423484244] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'RGB/YUV Input' -> /dev/v4l-subdev4
[18:23:20.423736672] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Raw Capture 0' -> /dev/video2
[18:23:20.423962866] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Raw Capture 1' -> /dev/video3
[18:23:20.424090475] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Scaler' -> /dev/v4l-subdev5
[18:23:20.424215875] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Sensor A' -> /dev/v4l-subdev0
[18:23:20.424337060] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Sensor B' -> /dev/v4l-subdev1
[18:23:20.458610283]  DBG camera.cpp:57 Camera Constructed for Dummy VIMC Camera
- Dummy VIMC Camera
[18:23:20.478303363]  DBG camera.cpp:65 Camera Destroyed
==29822==
==29822== HEAP SUMMARY:
==29822==     in use at exit: 0 bytes in 0 blocks
==29822==   total heap usage: 756 allocs, 756 frees, 418,671 bytes allocated
==29822==
==29822== All heap blocks were freed -- no leaks are possible
==29822==
==29822== For counts of detected and suppressed errors, rerun with: -v
==29822== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Niklas Söderlund (12):
  libcamera: Add Camera class
  libcamera: add dependency on libudev
  libcamera: deviceenumerator: add DeviceInfo class
  libcamera: deviceenumerator: add DeviceMatch class
  libcamera: deviceenumerator: add DeviceEnumerator class
  libcamera: deviceenumerator: add DeviceEnumeratorUdev class
  libcamera: deviceenumerator: add factory for DeviceEnumerators
  libcamera: deviceenumerator: add documentation
  libcamera: pipelinehandler: add PipelineHandler class
  libcamera: cameramanager: add CameraManager class
  libcamera: pipe-vimc: add pipeline handler for vimc
  tests: add test to list all cameras in the system

 include/libcamera/camera.h               |  31 ++
 include/libcamera/cameramanager.h        |  38 ++
 include/libcamera/libcamera.h            |   3 +
 include/libcamera/meson.build            |   2 +
 src/libcamera/camera.cpp                 |  97 ++++
 src/libcamera/cameramanager.cpp          | 173 +++++++
 src/libcamera/deviceenumerator.cpp       | 622 +++++++++++++++++++++++
 src/libcamera/include/deviceenumerator.h | 100 ++++
 src/libcamera/include/pipelinehandler.h  |  71 +++
 src/libcamera/meson.build                |  12 +-
 src/libcamera/pipe-vimc.cpp              |  92 ++++
 src/libcamera/pipelinehandler.cpp        | 122 +++++
 test/list.cpp                            |  54 ++
 test/meson.build                         |   5 +
 14 files changed, 1421 insertions(+), 1 deletion(-)
 create mode 100644 include/libcamera/camera.h
 create mode 100644 include/libcamera/cameramanager.h
 create mode 100644 src/libcamera/camera.cpp
 create mode 100644 src/libcamera/cameramanager.cpp
 create mode 100644 src/libcamera/deviceenumerator.cpp
 create mode 100644 src/libcamera/include/deviceenumerator.h
 create mode 100644 src/libcamera/include/pipelinehandler.h
 create mode 100644 src/libcamera/pipe-vimc.cpp
 create mode 100644 src/libcamera/pipelinehandler.cpp
 create mode 100644 test/list.cpp

Comments

Jacopo Mondi Dec. 24, 2018, 10:08 a.m. UTC | #1
Hi Niklas,
    thanks for the series, there's quite a bunch of stuff there to
look into :) great!

Before going in patch-per-patch discussions, I would like to discuss a
bit the overall design, and the possible overlap with the MediaDevice
objects I proposed a few days ago.

On Sun, Dec 23, 2018 at 12:00:29AM +0100, Niklas Söderlund wrote:
> Hi,
>
> This series adds basic camera enumeration to libcamera. The idea is to
> provide something which can server as a starting point to solve other
> problems around and enhance the enumeration process once the overall
> design is properly reviewed and requirements on the features missing
> becomes a bit more clear. The code in this series is fully functional
> and allows to add more pipeline handlers and enumerate cameras.
>
> In this series there are however a few known limitations which I find
> acceptable to get started and which can be solved one by one once the
> basic structure of enumeration is reviewed and hopefully upstream.  The
> known limitations are:
>
> - No sysfs based device enumerator. This will be needed so libcamera
>   have something to fallback on if udev enumeration fails or is not
>   available at all. The rational to focus on udev first is that it's
>   more complex and will support more features then sysfs (most notably
>   hot-(un)plug support.
>
> - The camera object (1/12) in this series is a mix of Jacopo's and
>   Laurent's previous patches with modifications by me. By no means would
>   I like to claim authorship of all work in this patch but as I modified
>   it I did claim authorship of it. If any of you wish to be claim
>   authorship for it let me know, I don't want you to be unprepared when
>   you get (git) blamed for my mistakes :-)
>
> - The DeviceInfo class should probably down the road be replaced by a
>   more comprehensive MediaDevice class. For now i feel this solution
>   works as it allows us to move forward with a MediaDevice class focused
>   on the needs of the Camera class and later extend it with the limited
>   features needed to replace DeviceInfo. For this reason I chose the
>   name DeciceInfo to not cause conflicts and allow the two to live in
>   parallel for a short wile if needed.

I don't think the overlap between this series and the MediaDevice
objects hierarchy is just with DeviceInfo, but possibly with
DeviceMatch as well.

From a top-level perspective, what happens here is the following
(correct me if I'm wrong).

1) The camera manager creates a DeviceEnumerator through a factory
(only udev based enumerator is supported atm). The device enumerator
starts, retrieves the media devices in the system through udev, and
collects their topology in the form of entities names, their devnode
paths and the media device info (creating a MediaDeviceInfo)

2) The camera manager walks the list of pipeline handlers registered
in the system, and calls their 'match()' methods providing them an
enumerator.

3) The pipeline manager enumerator fills a 'DeviceMatch' object, which
collects entities names and the media device name and pass them to the
enumerator's 'search()' method.

4) The DeviceEnumerator's search() method which receives a DeviceMatch
goes through all the DeviceInfo created at step 1) and call the
DeviceMatch()'s 'match()' method on them.

While I think this is quite neat and the pipeline handler registration
part is very nice, I feel we're duplicating quite some work,
specifically with the enumeration of media entities that happens at
MediaEnumerator::addDevice() time.

Let me expand this point: after a pipeline handler gets a match with a
device enumerator, it is expected to to handle the media device to
link and configure it opportunely for the requested capture use case.
What should it do to do that? Enumerate all media entities in the
media graph to get their representation. This mean it has to go
through G_TOPOLOGY again to do what the media enumerator partially did
at enumeration time in step 1)

I would consider instead the possibility to have the MediaEnumerator
instantiate a MediaDevice for each media device udev reports to be
present in the system and add your matching logic to the MediaDevice
itself, instead of having the DeviceMatch do the matching on the
MediaDevice (which is now a DeviceInfo).

The code I submitted has everything in place to retrieve entities by
name or id, so I think it would be pretty easy to implement a
MediaDevice::match(DeviceMatch) which goes through the content of the
pipeline handler provided DeviceMatch and verifies it can satisfy it
or not. If the MediaEnumerator finds a MediaDevice that satisfies a
DeviceMatch (s/DeviceMatch/MediaDeviceDesc ?) it returns it to the
pipeline handler, which receives an already constructed MediaDevice
and can simply start using it to setup links etc.

Using the IPU3 example, what I think would be a possible design is:

1) The enumerator starts, for each media device returned by udev
creates a MediaDevice. For IPU3 they will be the CIO2 MediaDevice and
the IMGU MediaDevice.

2) The camera manager goes through the list of pipeline handlers, and
finds the IPU3 Pipeline handler. It calls its match() method providing
it the enumerator created at step 1)

3) The IPU3 pipeline manager creates a CIO2Desc MediaDeviceDesc and
provides it to the enumerator's search() method

4) The enumerator's search() method, goes through the list of
MediaDevice constructed at step 1), and for each of them calls they're
match() method, providing them the MediaDeviceDesc created by the
pipeline manager at step 3)

5) If the MediaDevice matches the MediaDeviceDesc, the enumerator
returns it to the Pipeline manager, which in this way receives an
already constructed MediaDevice and does not have to go through
topology enumeration again

6) The pipeline manager is happy if the CIO2 MediaDeviceDesc is
matched and returned, and repeats step 3 with the IMGU MediaDeviceDesc

7) If the enumerator returns a MediaDevice for the IMGU too we're
actually running on an IPU3 and the IPU3 Pipeline Manager is selected.

I still have to figure out how how bad would it look in this way the
devnode path association with entities, as in your series the udev
enumerator creates entities, and then associates them with devnode
paths. In my proposal the MediaDevice has to enumerate its components,
and call into the udev/sysfs/whatever enumerator to help with the
association of entities/devnode path. I'm sure it's doable in a clean
way though...

>
> - There is no support for hot-plug or hot-unplug. The state of the
>   system is assumed to be static for the entire execution. Some care
>   have been taken to facilitate addition of hot-plug support.
>
> - There is only one pipeline handler included in this series for the
>   vimc driver. It should be fairly trivial to add one for the Soraka, I
>   chose not to do so to reduce the number of patches and to increase the
>   number of potential testers as anyone can do 'modprobe vimc' and have
>   the test case for this series passing.
>
> Patch 1/12 adds a mock for a Camera class and stubs in lifetime
> management features needed to allow the camera manger to try and control
> the lifetime of objects. Patch 2/12 adds a build requirement on libudev.
>
> Patch 3/12 -- 7/12 adds the device enumerator and accompanying classes
> to allow storing and searching for media devices by a pipeline handler.
> Patch 8/12 adds the documentation for all enumerator related classes. I
> chose to split this in multiple patches to ease review, I'm open to
> squashing them together if someone feels it's better for some reason or
> other.

Separating the deviceenumerator parts is fine, it quite weird to have a
patch that adds comments on top of them though.

Having comments in place when looking at the single patch is helpful to
understand what's going on. Otherwise I had to look at the "naked"
patch and jump back and forth to the comment patch to get an idea of
what the method/class purpose was. Comments are not just helpful for
people reading the code after it went in, but also during review, so I
would appreciate to receive them with the code they apply to :)

>
> Patch 9/12 adds a base class for pipeline handlers and a global list of
> pipe handlers in the system which each pipeline implementation can
> register it self with to be included in the enumeration of cameras. This
> approach allows the pipeline implementations to be self contained in a
> single .cpp file, thanks Kieran for pointing me in the right direction
> for how to do this in C++ ;-)
>
> Patch 10/12 adds the camera manger which tracks lifetime management of
> cameras in the system and is the interface to applications to list and
> get cameras out of the library. Patch 11/12 adds a pipeline handler for
> the vimc driver and lastly 12/12 adds a test case which simply uses most
> features of the series to list all cameras in the system.
>
> This series is based on top of the current master branch and the test
> case have been run under valgrind and no memory issues are found.
>

Should we have a valgrind wrapper script, as we do have one for
checkstyle?

> valgrind --tool=memcheck --leak-check=full ./test/list
> ==29822== Memcheck, a memory error detector
> ==29822== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
> ==29822== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
> ==29822== Command: ./test/list
> ==29822==
> [18:23:20.377682367] INFO deviceenumerator.cpp:70 Device: /dev/media0 Entity: 'Integrated Camera: Integrated C' -> /dev/video1
> [18:23:20.422617627] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Debayer A' -> /dev/v4l-subdev2
> [18:23:20.423131606] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Debayer B' -> /dev/v4l-subdev3
> [18:23:20.423351904] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'RGB/YUV Capture' -> /dev/video4
> [18:23:20.423484244] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'RGB/YUV Input' -> /dev/v4l-subdev4
> [18:23:20.423736672] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Raw Capture 0' -> /dev/video2
> [18:23:20.423962866] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Raw Capture 1' -> /dev/video3
> [18:23:20.424090475] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Scaler' -> /dev/v4l-subdev5
> [18:23:20.424215875] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Sensor A' -> /dev/v4l-subdev0
> [18:23:20.424337060] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Sensor B' -> /dev/v4l-subdev1
> [18:23:20.458610283]  DBG camera.cpp:57 Camera Constructed for Dummy VIMC Camera
> - Dummy VIMC Camera
> [18:23:20.478303363]  DBG camera.cpp:65 Camera Destroyed
> ==29822==
> ==29822== HEAP SUMMARY:
> ==29822==     in use at exit: 0 bytes in 0 blocks
> ==29822==   total heap usage: 756 allocs, 756 frees, 418,671 bytes allocated
> ==29822==
> ==29822== All heap blocks were freed -- no leaks are possible
> ==29822==
> ==29822== For counts of detected and suppressed errors, rerun with: -v
> ==29822== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
>
> Niklas Söderlund (12):
>   libcamera: Add Camera class
>   libcamera: add dependency on libudev
>   libcamera: deviceenumerator: add DeviceInfo class
>   libcamera: deviceenumerator: add DeviceMatch class
>   libcamera: deviceenumerator: add DeviceEnumerator class
>   libcamera: deviceenumerator: add DeviceEnumeratorUdev class
>   libcamera: deviceenumerator: add factory for DeviceEnumerators
>   libcamera: deviceenumerator: add documentation
>   libcamera: pipelinehandler: add PipelineHandler class
>   libcamera: cameramanager: add CameraManager class
>   libcamera: pipe-vimc: add pipeline handler for vimc
>   tests: add test to list all cameras in the system
>
>  include/libcamera/camera.h               |  31 ++
>  include/libcamera/cameramanager.h        |  38 ++
>  include/libcamera/libcamera.h            |   3 +
>  include/libcamera/meson.build            |   2 +
>  src/libcamera/camera.cpp                 |  97 ++++
>  src/libcamera/cameramanager.cpp          | 173 +++++++
>  src/libcamera/deviceenumerator.cpp       | 622 +++++++++++++++++++++++
>  src/libcamera/include/deviceenumerator.h | 100 ++++
>  src/libcamera/include/pipelinehandler.h  |  71 +++
>  src/libcamera/meson.build                |  12 +-
>  src/libcamera/pipe-vimc.cpp              |  92 ++++
>  src/libcamera/pipelinehandler.cpp        | 122 +++++

That's a minor, but why did you decide not to use underscores in
between file names? I mean, camera_manager.cpp is easier to read than
cameramanager.cpp, as device_enumerator or vimc_pipeline_handler would
be.

Thanks
  j


>  test/list.cpp                            |  54 ++
>  test/meson.build                         |   5 +
>  14 files changed, 1421 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/camera.h
>  create mode 100644 include/libcamera/cameramanager.h
>  create mode 100644 src/libcamera/camera.cpp
>  create mode 100644 src/libcamera/cameramanager.cpp
>  create mode 100644 src/libcamera/deviceenumerator.cpp
>  create mode 100644 src/libcamera/include/deviceenumerator.h
>  create mode 100644 src/libcamera/include/pipelinehandler.h
>  create mode 100644 src/libcamera/pipe-vimc.cpp
>  create mode 100644 src/libcamera/pipelinehandler.cpp
>  create mode 100644 test/list.cpp
>
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund Dec. 28, 2018, 2:52 a.m. UTC | #2
Hi Jacopo,

Thanks for your feedback.

On 2018-12-24 11:08:44 +0100, Jacopo Mondi wrote:
> Hi Niklas,
>     thanks for the series, there's quite a bunch of stuff there to
> look into :) great!
> 
> Before going in patch-per-patch discussions, I would like to discuss a
> bit the overall design, and the possible overlap with the MediaDevice
> objects I proposed a few days ago.
> 
> On Sun, Dec 23, 2018 at 12:00:29AM +0100, Niklas Söderlund wrote:
> > Hi,
> >
> > This series adds basic camera enumeration to libcamera. The idea is to
> > provide something which can server as a starting point to solve other
> > problems around and enhance the enumeration process once the overall
> > design is properly reviewed and requirements on the features missing
> > becomes a bit more clear. The code in this series is fully functional
> > and allows to add more pipeline handlers and enumerate cameras.
> >
> > In this series there are however a few known limitations which I find
> > acceptable to get started and which can be solved one by one once the
> > basic structure of enumeration is reviewed and hopefully upstream.  The
> > known limitations are:
> >
> > - No sysfs based device enumerator. This will be needed so libcamera
> >   have something to fallback on if udev enumeration fails or is not
> >   available at all. The rational to focus on udev first is that it's
> >   more complex and will support more features then sysfs (most notably
> >   hot-(un)plug support.
> >
> > - The camera object (1/12) in this series is a mix of Jacopo's and
> >   Laurent's previous patches with modifications by me. By no means would
> >   I like to claim authorship of all work in this patch but as I modified
> >   it I did claim authorship of it. If any of you wish to be claim
> >   authorship for it let me know, I don't want you to be unprepared when
> >   you get (git) blamed for my mistakes :-)
> >
> > - The DeviceInfo class should probably down the road be replaced by a
> >   more comprehensive MediaDevice class. For now i feel this solution
> >   works as it allows us to move forward with a MediaDevice class focused
> >   on the needs of the Camera class and later extend it with the limited
> >   features needed to replace DeviceInfo. For this reason I chose the
> >   name DeciceInfo to not cause conflicts and allow the two to live in
> >   parallel for a short wile if needed.
> 
> I don't think the overlap between this series and the MediaDevice
> objects hierarchy is just with DeviceInfo, but possibly with
> DeviceMatch as well.
> 
> From a top-level perspective, what happens here is the following
> (correct me if I'm wrong).
> 
> 1) The camera manager creates a DeviceEnumerator through a factory
> (only udev based enumerator is supported atm). The device enumerator
> starts, retrieves the media devices in the system through udev, and
> collects their topology in the form of entities names, their devnode
> paths and the media device info (creating a MediaDeviceInfo)
> 
> 2) The camera manager walks the list of pipeline handlers registered
> in the system, and calls their 'match()' methods providing them an
> enumerator.
> 
> 3) The pipeline manager enumerator fills a 'DeviceMatch' object, which
> collects entities names and the media device name and pass them to the
> enumerator's 'search()' method.
> 
> 4) The DeviceEnumerator's search() method which receives a DeviceMatch
> goes through all the DeviceInfo created at step 1) and call the
> DeviceMatch()'s 'match()' method on them.
> 
> While I think this is quite neat and the pipeline handler registration
> part is very nice, I feel we're duplicating quite some work,
> specifically with the enumeration of media entities that happens at
> MediaEnumerator::addDevice() time.

Agreed, but as no MediaDevice object is upstream something is needed to 
fill the void to get the enumeration working. In the long run I'm sure 
things could be joined to reduce code duplication. For the moment I feel 
get things running trumps a perfect design.

> 
> Let me expand this point: after a pipeline handler gets a match with a
> device enumerator, it is expected to to handle the media device to
> link and configure it opportunely for the requested capture use case.
> What should it do to do that? Enumerate all media entities in the
> media graph to get their representation. This mean it has to go
> through G_TOPOLOGY again to do what the media enumerator partially did
> at enumeration time in step 1)

Yes, I'm sure we can improve this over time. Currently we have no 
enumeration and no MediaDevice. If we solve this on the ML or patches 
don't matter to me. I just want to gain momentum at this point.

> 
> I would consider instead the possibility to have the MediaEnumerator
> instantiate a MediaDevice for each media device udev reports to be
> present in the system and add your matching logic to the MediaDevice
> itself, instead of having the DeviceMatch do the matching on the
> MediaDevice (which is now a DeviceInfo).
> 
> The code I submitted has everything in place to retrieve entities by
> name or id, so I think it would be pretty easy to implement a
> MediaDevice::match(DeviceMatch) which goes through the content of the
> pipeline handler provided DeviceMatch and verifies it can satisfy it
> or not. If the MediaEnumerator finds a MediaDevice that satisfies a
> DeviceMatch (s/DeviceMatch/MediaDeviceDesc ?) it returns it to the
> pipeline handler, which receives an already constructed MediaDevice
> and can simply start using it to setup links etc.

I do not agree. I feel the matching logic is not part of a future 
MediaDevice object. The output of the enumeration should as you point 
out above be a MediaDevice which a PiplelineManger/Camera object could 
use.

> 
> Using the IPU3 example, what I think would be a possible design is:
> 
> 1) The enumerator starts, for each media device returned by udev
> creates a MediaDevice. For IPU3 they will be the CIO2 MediaDevice and
> the IMGU MediaDevice.
> 
> 2) The camera manager goes through the list of pipeline handlers, and
> finds the IPU3 Pipeline handler. It calls its match() method providing
> it the enumerator created at step 1)
> 
> 3) The IPU3 pipeline manager creates a CIO2Desc MediaDeviceDesc and
> provides it to the enumerator's search() method
> 
> 4) The enumerator's search() method, goes through the list of
> MediaDevice constructed at step 1), and for each of them calls they're
> match() method, providing them the MediaDeviceDesc created by the
> pipeline manager at step 3)
> 
> 5) If the MediaDevice matches the MediaDeviceDesc, the enumerator
> returns it to the Pipeline manager, which in this way receives an
> already constructed MediaDevice and does not have to go through
> topology enumeration again
> 
> 6) The pipeline manager is happy if the CIO2 MediaDeviceDesc is
> matched and returned, and repeats step 3 with the IMGU MediaDeviceDesc
> 
> 7) If the enumerator returns a MediaDevice for the IMGU too we're
> actually running on an IPU3 and the IPU3 Pipeline Manager is selected.
> 
> I still have to figure out how how bad would it look in this way the
> devnode path association with entities, as in your series the udev
> enumerator creates entities, and then associates them with devnode
> paths. In my proposal the MediaDevice has to enumerate its components,
> and call into the udev/sysfs/whatever enumerator to help with the
> association of entities/devnode path. I'm sure it's doable in a clean
> way though...

I'm open to this, but for now I lets keep it simple so we can move 
forward. I'm sure there is potential to merge some classes to reduce 
code duplication. For now lets get things going. I see no real problem 
with some duplication in a initial stage. This is of course not the 
target or default mode of operation. To get the ball rolling I see no 
issue in enumerating edge devices twice.

> 
> >
> > - There is no support for hot-plug or hot-unplug. The state of the
> >   system is assumed to be static for the entire execution. Some care
> >   have been taken to facilitate addition of hot-plug support.
> >
> > - There is only one pipeline handler included in this series for the
> >   vimc driver. It should be fairly trivial to add one for the Soraka, I
> >   chose not to do so to reduce the number of patches and to increase the
> >   number of potential testers as anyone can do 'modprobe vimc' and have
> >   the test case for this series passing.
> >
> > Patch 1/12 adds a mock for a Camera class and stubs in lifetime
> > management features needed to allow the camera manger to try and control
> > the lifetime of objects. Patch 2/12 adds a build requirement on libudev.
> >
> > Patch 3/12 -- 7/12 adds the device enumerator and accompanying classes
> > to allow storing and searching for media devices by a pipeline handler.
> > Patch 8/12 adds the documentation for all enumerator related classes. I
> > chose to split this in multiple patches to ease review, I'm open to
> > squashing them together if someone feels it's better for some reason or
> > other.
> 
> Separating the deviceenumerator parts is fine, it quite weird to have a
> patch that adds comments on top of them though.
> 
> Having comments in place when looking at the single patch is helpful to
> understand what's going on. Otherwise I had to look at the "naked"
> patch and jump back and forth to the comment patch to get an idea of
> what the method/class purpose was. Comments are not just helpful for
> people reading the code after it went in, but also during review, so I
> would appreciate to receive them with the code they apply to :)
> 

Yes and no :-)

The documentation reference the different objects in a none coherent 
manner. I'm not against squashing all the enumeration patches. I just 
thought this was easier to review.

> >
> > Patch 9/12 adds a base class for pipeline handlers and a global list of
> > pipe handlers in the system which each pipeline implementation can
> > register it self with to be included in the enumeration of cameras. This
> > approach allows the pipeline implementations to be self contained in a
> > single .cpp file, thanks Kieran for pointing me in the right direction
> > for how to do this in C++ ;-)
> >
> > Patch 10/12 adds the camera manger which tracks lifetime management of
> > cameras in the system and is the interface to applications to list and
> > get cameras out of the library. Patch 11/12 adds a pipeline handler for
> > the vimc driver and lastly 12/12 adds a test case which simply uses most
> > features of the series to list all cameras in the system.
> >
> > This series is based on top of the current master branch and the test
> > case have been run under valgrind and no memory issues are found.
> >
> 
> Should we have a valgrind wrapper script, as we do have one for
> checkstyle?

Not sure, valgrind is not a tool which solves all problems. It's one of 
many things you can use to check your code.

> 
> > valgrind --tool=memcheck --leak-check=full ./test/list
> > ==29822== Memcheck, a memory error detector
> > ==29822== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
> > ==29822== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
> > ==29822== Command: ./test/list
> > ==29822==
> > [18:23:20.377682367] INFO deviceenumerator.cpp:70 Device: /dev/media0 Entity: 'Integrated Camera: Integrated C' -> /dev/video1
> > [18:23:20.422617627] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Debayer A' -> /dev/v4l-subdev2
> > [18:23:20.423131606] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Debayer B' -> /dev/v4l-subdev3
> > [18:23:20.423351904] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'RGB/YUV Capture' -> /dev/video4
> > [18:23:20.423484244] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'RGB/YUV Input' -> /dev/v4l-subdev4
> > [18:23:20.423736672] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Raw Capture 0' -> /dev/video2
> > [18:23:20.423962866] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Raw Capture 1' -> /dev/video3
> > [18:23:20.424090475] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Scaler' -> /dev/v4l-subdev5
> > [18:23:20.424215875] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Sensor A' -> /dev/v4l-subdev0
> > [18:23:20.424337060] INFO deviceenumerator.cpp:70 Device: /dev/media1 Entity: 'Sensor B' -> /dev/v4l-subdev1
> > [18:23:20.458610283]  DBG camera.cpp:57 Camera Constructed for Dummy VIMC Camera
> > - Dummy VIMC Camera
> > [18:23:20.478303363]  DBG camera.cpp:65 Camera Destroyed
> > ==29822==
> > ==29822== HEAP SUMMARY:
> > ==29822==     in use at exit: 0 bytes in 0 blocks
> > ==29822==   total heap usage: 756 allocs, 756 frees, 418,671 bytes allocated
> > ==29822==
> > ==29822== All heap blocks were freed -- no leaks are possible
> > ==29822==
> > ==29822== For counts of detected and suppressed errors, rerun with: -v
> > ==29822== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
> >
> > Niklas Söderlund (12):
> >   libcamera: Add Camera class
> >   libcamera: add dependency on libudev
> >   libcamera: deviceenumerator: add DeviceInfo class
> >   libcamera: deviceenumerator: add DeviceMatch class
> >   libcamera: deviceenumerator: add DeviceEnumerator class
> >   libcamera: deviceenumerator: add DeviceEnumeratorUdev class
> >   libcamera: deviceenumerator: add factory for DeviceEnumerators
> >   libcamera: deviceenumerator: add documentation
> >   libcamera: pipelinehandler: add PipelineHandler class
> >   libcamera: cameramanager: add CameraManager class
> >   libcamera: pipe-vimc: add pipeline handler for vimc
> >   tests: add test to list all cameras in the system
> >
> >  include/libcamera/camera.h               |  31 ++
> >  include/libcamera/cameramanager.h        |  38 ++
> >  include/libcamera/libcamera.h            |   3 +
> >  include/libcamera/meson.build            |   2 +
> >  src/libcamera/camera.cpp                 |  97 ++++
> >  src/libcamera/cameramanager.cpp          | 173 +++++++
> >  src/libcamera/deviceenumerator.cpp       | 622 +++++++++++++++++++++++
> >  src/libcamera/include/deviceenumerator.h | 100 ++++
> >  src/libcamera/include/pipelinehandler.h  |  71 +++
> >  src/libcamera/meson.build                |  12 +-
> >  src/libcamera/pipe-vimc.cpp              |  92 ++++
> >  src/libcamera/pipelinehandler.cpp        | 122 +++++
> 
> That's a minor, but why did you decide not to use underscores in
> between file names? I mean, camera_manager.cpp is easier to read than
> cameramanager.cpp, as device_enumerator or vimc_pipeline_handler would
> be.

I like file names without _, but as discussed on IRC I have no real 
preference as long as it's coherent in the project.

> 
> Thanks
>   j
> 
> 
> >  test/list.cpp                            |  54 ++
> >  test/meson.build                         |   5 +
> >  14 files changed, 1421 insertions(+), 1 deletion(-)
> >  create mode 100644 include/libcamera/camera.h
> >  create mode 100644 include/libcamera/cameramanager.h
> >  create mode 100644 src/libcamera/camera.cpp
> >  create mode 100644 src/libcamera/cameramanager.cpp
> >  create mode 100644 src/libcamera/deviceenumerator.cpp
> >  create mode 100644 src/libcamera/include/deviceenumerator.h
> >  create mode 100644 src/libcamera/include/pipelinehandler.h
> >  create mode 100644 src/libcamera/pipe-vimc.cpp
> >  create mode 100644 src/libcamera/pipelinehandler.cpp
> >  create mode 100644 test/list.cpp
> >
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel