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

[Niklas S??derlund]

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.

> 
> > 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