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

Jacopo Mondi jacopo at jmondi.org
Sat Dec 29 12:09:04 CET 2018


Hi Laurent, Niklas,

On Sat, Dec 29, 2018 at 02:06:24AM +0100, Niklas S??derlund wrote:
> Hi Laurent,
>
> Thanks for your feedback.
>
> 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.

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.

Thanks
  j

> >
> > > 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(+)
> > > >
> > > > diff --git a/src/libcamera/deviceenumerator.cpp
> > > > b/src/libcamera/deviceenumerator.cpp index
> > > > 7c44a65b45472ef3..17b6874d229c791c 100644
> > > > --- a/src/libcamera/deviceenumerator.cpp
> > > > +++ b/src/libcamera/deviceenumerator.cpp
> > > > @@ -75,4 +75,58 @@ bool DeviceInfo::lookup(const std::string &name,
> > > > std::string &devnode) const
> > > >  	return true;
> > > >  }
> > > >
> > > > +/* ----------------------------------------------------------------------
> > > > + * DeviceMatch
> > > > + */
> > > > +
> > > > +DeviceMatch::DeviceMatch(const std::string &driver)
> > > > +	: driver_(driver)
> > > > +{
> > > > +}
> > >
> > > Is it worth placing constructors with list initializers only in the
> > > .cpp file?
> > >
> > > > +
> > > > +void DeviceMatch::add(const std::string &entity)
> > > > +{
> > > > +	entities_.push_back(std::string(entity));
> >
> > Doesn't entities_.push_back(entity); work as expected ?
>
> It does, this is a mistake on my side. Thanks for pointing it out.
>
> >
> > > > +}
> > > > +
> > > > +bool DeviceMatch::match(const DeviceInfo *info) const
> > > > +{
> > > > +	std::vector<std::string> entities;
> > >
> > > That's weird. This is clearly unused, but the compiler does not
> > > complain. Actually I can keep it or remove it without any message from
> > > g++. Was it here intentionally?
> > >
> > > > +
> > > > +	if (!matchInfo(info->info()))
> > > > +		return false;
> > > > +
> > > > +	if (!matchEntities(info->entities()))
> > > > +		return false;
> > > > +
> > > > +	return true;
> >
> > Given how simple the code is I would inline both matchInfo() and
> > matchEntities() here.
>
> OK I yield :-) Will inline in v2.
>
> >
> > > > +}
> > > > +
> > > > +bool DeviceMatch::matchInfo(const struct media_device_info &info) const
> > > > +{
> > > > +	/* TODO: Add more optinal matching pairs from struct media_device_info
> > > > */
> > >
> > > optional
> > >
> > > > +	/* TODO: Allow for empty driver in DeviceMatch */
> > > > +	return driver_ == info.driver;
> > > > +}
> > > > +
> > > > +bool DeviceMatch::matchEntities(const std::vector<std::string> &entities)
> > > > const
> > > > +{
> > > > +	for (const std::string &name : entities_) {
> > > > +		bool found = false;
> > > > +
> > > > +		for (const std::string &entity : entities) {
> > > > +
> > >
> > > nit: unnecessary empty line
> > >
> > > > +			if (name == entity) {
> > > > +				found = true;
> > > > +				break;
> > > > +			}
> > > > +		}
> > > > +
> > > > +		if (!found)
> > > > +			return false;
> > > > +	}
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > >
> > > >  } /* namespace libcamera */
> > > >
> > > > diff --git a/src/libcamera/include/deviceenumerator.h
> > > > b/src/libcamera/include/deviceenumerator.h index
> > > > 0136ed6ea23bf65e..fb412b8840cb2ffe 100644
> > > > --- a/src/libcamera/include/deviceenumerator.h
> > > > +++ b/src/libcamera/include/deviceenumerator.h
> > > > @@ -39,6 +39,23 @@ private:
> > > >  	std::map<std::string, std::string> entities_;
> > > >  };
> > > >
> > > > +class DeviceMatch
> > > > +{
> > > > +public:
> > > > +	DeviceMatch(const std::string &driver);
> > > > +
> > > > +	void add(const std::string &entity);
> > > > +
> > > > +	bool match(const DeviceInfo *info) const;
> > > > +
> > > > +private:
> > > > +	std::string driver_;
> > > > +	std::vector<std::string> entities_;
> > > > +
> > > > +	bool matchInfo(const struct media_device_info &info) const;
> > > > +	bool matchEntities(const std::vector<std::string> &entities) const;
> > > > +};
> > > > +
> > > >
> > > >  } /* namespace libcamera */
> > > >
> > > >  #endif	/* __LIBCAMERA_DEVICEENUMERATE_H__ */
> > >
> > > This is less than nit, but shouldn't this be DEVICEENUMERATOR ? :)
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> >
> >
>
> --
> Regards,
> Niklas Söderlund
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20181229/4ee9490a/attachment.sig>


More information about the libcamera-devel mailing list