[libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator: add DeviceMatch class

[Laurent Pinchart]

Hi Jacopo,

On Saturday, 29 December 2018 13:09:04 EET Jacopo Mondi wrote:
> On Sat, Dec 29, 2018 at 02:06:24AM +0100, Niklas S??derlund wrote:
> > On 2018-12-29 00:02:41 +0200, Laurent Pinchart wrote:
> >> On Monday, 24 December 2018 12:55:30 EET Jacopo Mondi wrote:
> >>> Hi Niklas,
> >>> 
> >>> as I've commented on 00/12, I think the DeviceMatcher should be
> >>> replaced by a MediaDeviceDesc and have the MediaDevice do the matching
> >>> on it. Nonethless, I have some questions here, as they might apply
> >>> generally to all our code base.
> >> 
> >> How about doing it the other way around, having the match() function in
> >> the MediaDeviceDesc class ? That would keep MediaDevice focused on
> >> handling media devices, and MediaDeviceDesc on handling matching. We
> >> could perhaps rename MediaDeviceDesc to MediaDeviceMatch (I don't
> >> dislike the MediaDeviceDesc name as such, but I don't really like
> >> abbreviating Desc).
> > 
> > I agree it would be good if the MedaDevice could focus on operating
> > media device and have the enumeration and matching in other objects.
> 
> I'm sorry, I don't agree.
> 
> I struggled to keep the MediaDevice as opaque as possible and having a
> (friend?) class that goes and look into the media device to see if it
> matches a description goes against this direction.

But why does it need to be as opaque as possible ? I see no reason not to 
expose the full list of entities for instance. Let's not artificially restrict 
what the MediaDevice users can do.

> Matching a media graphs at the moment means matching (part of) it's
> media_device_info and a list of entities names. I really feel this is
> something the MediaDevice should do, otherwise those informations are
> exposed to the extern, which makes then possible for all components in
> the system access the single entities (as you might notice, in my
> proposed implementation 'getEntityByName()' is private) and the
> internal device informations as returned by MEDIA_IOC_DEVICE_INFO.
> 
> I really would like to keep the MediaDevice self-contained, exposing a
> minimal API to handle linking/unlinking and in future retrieve entities
> to access their subdevices to set/get formats, controls and all V4L2
> operations from the pipeline manager. But it my opinion that's should
> be it. Everything else should be opaque and incapsulated by the
> MediaDevice class.
> 
> I fail to see why creating a dependency on the MediaDevice interface
> (how you access the media_device iformation, how you retrieve entities
> etc) in another object is better than define a 'description' object
> along side the MediaDevice definition itself, and have the MediaDevice
> consume it and match with it internals informations.
> 
> For the current patch series this is not a big deal though, but it's a
> discussion I would like to continue in future.

As we've discussed on IRC, we will have code that will need to access entities 
without much a priori knowledge of the graph. Two such examples are generic 
helpers that are not device-specific, and pipeline handlers that have no a 
priori knowledge of which sensor(s) can be connected to ISPs.

> >>> On Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote:
> >>>> Provide a DeviceMatch class which represents all properties of a
> >>>> media device a pipeline hander can specify when searching for a
> >>>> device to use in its pipeline.
> >>>> 
> >>>> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >>>> ---
> >>>> 
> >>>>  src/libcamera/deviceenumerator.cpp       | 54 ++++++++++++++++++++++++
> >>>>  src/libcamera/include/deviceenumerator.h | 17 ++++++++
> >>>>  2 files changed, 71 insertions(+)

[snip]

-- 
Regards,

Laurent Pinchart