[libcamera-devel,03/12] libcamera: deviceenumerator: add DeviceInfo class

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

Commit Message

Niklas Söderlund Dec. 22, 2018, 11 p.m. UTC
Provide a DeviceInfo class which holds all information from the initial
enumeration of a media device. Not all information available at a media
device is stored, only the information needed for a pipeline handler to
find a specific device.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/deviceenumerator.cpp       | 78 ++++++++++++++++++++++++
 src/libcamera/include/deviceenumerator.h | 44 +++++++++++++
 src/libcamera/meson.build                |  2 +
 3 files changed, 124 insertions(+)
 create mode 100644 src/libcamera/deviceenumerator.cpp
 create mode 100644 src/libcamera/include/deviceenumerator.h

Comments

Jacopo Mondi Dec. 24, 2018, 10:42 a.m. UTC | #1
Hi Niklas,

On Sun, Dec 23, 2018 at 12:00:32AM +0100, Niklas S??derlund wrote:
> Provide a DeviceInfo class which holds all information from the initial
> enumeration of a media device. Not all information available at a media
> device is stored, only the information needed for a pipeline handler to
> find a specific device.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/deviceenumerator.cpp       | 78 ++++++++++++++++++++++++
>  src/libcamera/include/deviceenumerator.h | 44 +++++++++++++
>  src/libcamera/meson.build                |  2 +
>  3 files changed, 124 insertions(+)
>  create mode 100644 src/libcamera/deviceenumerator.cpp
>  create mode 100644 src/libcamera/include/deviceenumerator.h
>
> diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp
> new file mode 100644
> index 0000000000000000..7c44a65b45472ef3
> --- /dev/null
> +++ b/src/libcamera/deviceenumerator.cpp
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * deviceenumerator.cpp - Enumeration and matching
> + */
> +
> +#include "deviceenumerator.h"
> +#include "log.h"
> +
> +namespace libcamera {
> +
> +/* -----------------------------------------------------------------------------
> + * DeviceInfo
> + */
> +
> +DeviceInfo::DeviceInfo(const std::string &devnode, const struct media_device_info &info,
> +		       const std::map<std::string, std::string> &entities)
> +	: acquired_(false), devnode_(devnode), info_(info), entities_(entities)
> +{
> +	for (const auto &entity : entities_)
> +		LOG(Info) << "Device: " << devnode_ << " Entity: '" << entity.first << "' -> " << entity.second;
> +}
> +
> +int DeviceInfo::acquire()
> +{
> +	if (acquired_)
> +		return -EBUSY;
> +
> +	acquired_ = true;
> +
> +	return 0;
> +}
> +
> +void DeviceInfo::release()
> +{
> +	acquired_ = false;
> +}
> +
> +bool DeviceInfo::busy() const
> +{
> +	return acquired_;
> +}
> +
> +const std::string &DeviceInfo::devnode() const
> +{
> +	return devnode_;
> +}
> +
> +const struct media_device_info &DeviceInfo::info() const
> +{
> +	return info_;
> +}
> +
> +std::vector<std::string> DeviceInfo::entities() const
> +{
> +	std::vector<std::string> entities;
> +
> +	for (const auto &entity : entities_)
> +		entities.push_back(entity.first);
> +
> +	return entities;

Are you returning (by copy) a variable with local scope?
Why coldn't you just return (as const reference maybe) entities_ ?

> +}
> +
> +bool DeviceInfo::lookup(const std::string &name, std::string &devnode) const
> +{
> +	auto it = entities_.find(name);
> +
> +	if (it == entities_.end()) {
> +		LOG(Error) << "Trying to lookup entity '" << name << "' which do not exist";

nit: "does not exist"

> +		return false;
> +	}
> +
> +	devnode = it->second;
> +	return true;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h
> new file mode 100644
> index 0000000000000000..0136ed6ea23bf65e
> --- /dev/null
> +++ b/src/libcamera/include/deviceenumerator.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * deviceenumerator.h - API to enumerate and find media devices
> + */
> +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__
> +#define __LIBCAMERA_DEVICEENUMERATE_H__
> +
> +#include <map>
> +#include <string>
> +#include <vector>
> +
> +#include <linux/media.h>
> +
> +namespace libcamera {
> +
> +class DeviceInfo
> +{
> +public:
> +	DeviceInfo(const std::string &devnode, const struct media_device_info &info,
> +		   const std::map<std::string, std::string> &entities);
> +
> +	int acquire();
> +	void release();
> +	bool busy() const;

Trivial methods might be inlined here.

> +
> +	const std::string &devnode() const;
> +	const struct media_device_info &info() const;
> +	std::vector<std::string> entities() const;
> +
> +	bool lookup(const std::string &name, std::string &devnode) const;
> +
> +private:
> +	bool acquired_;
> +
> +	std::string devnode_;
> +	struct media_device_info info_;
> +	std::map<std::string, std::string> entities_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif	/* __LIBCAMERA_DEVICEENUMERATE_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 52b556a8ed4050cb..17cdf06dd2bedfb3 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -1,10 +1,12 @@
>  libcamera_sources = files([
>      'camera.cpp',
> +    'deviceenumerator.cpp',
>      'log.cpp',
>      'main.cpp',
>  ])
>
>  libcamera_headers = files([
> +    'include/deviceenumerator.h',
>      'include/log.h',
>      'include/utils.h',
>  ])
> --
> 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, 3:06 a.m. UTC | #2
Hi Jacopo,

Thanks for your feedback.

On 2018-12-24 11:42:16 +0100, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Sun, Dec 23, 2018 at 12:00:32AM +0100, Niklas S??derlund wrote:
> > Provide a DeviceInfo class which holds all information from the initial
> > enumeration of a media device. Not all information available at a media
> > device is stored, only the information needed for a pipeline handler to
> > find a specific device.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/deviceenumerator.cpp       | 78 ++++++++++++++++++++++++
> >  src/libcamera/include/deviceenumerator.h | 44 +++++++++++++
> >  src/libcamera/meson.build                |  2 +
> >  3 files changed, 124 insertions(+)
> >  create mode 100644 src/libcamera/deviceenumerator.cpp
> >  create mode 100644 src/libcamera/include/deviceenumerator.h
> >
> > diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp
> > new file mode 100644
> > index 0000000000000000..7c44a65b45472ef3
> > --- /dev/null
> > +++ b/src/libcamera/deviceenumerator.cpp
> > @@ -0,0 +1,78 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * deviceenumerator.cpp - Enumeration and matching
> > + */
> > +
> > +#include "deviceenumerator.h"
> > +#include "log.h"
> > +
> > +namespace libcamera {
> > +
> > +/* -----------------------------------------------------------------------------
> > + * DeviceInfo
> > + */
> > +
> > +DeviceInfo::DeviceInfo(const std::string &devnode, const struct media_device_info &info,
> > +		       const std::map<std::string, std::string> &entities)
> > +	: acquired_(false), devnode_(devnode), info_(info), entities_(entities)
> > +{
> > +	for (const auto &entity : entities_)
> > +		LOG(Info) << "Device: " << devnode_ << " Entity: '" << entity.first << "' -> " << entity.second;
> > +}
> > +
> > +int DeviceInfo::acquire()
> > +{
> > +	if (acquired_)
> > +		return -EBUSY;
> > +
> > +	acquired_ = true;
> > +
> > +	return 0;
> > +}
> > +
> > +void DeviceInfo::release()
> > +{
> > +	acquired_ = false;
> > +}
> > +
> > +bool DeviceInfo::busy() const
> > +{
> > +	return acquired_;
> > +}
> > +
> > +const std::string &DeviceInfo::devnode() const
> > +{
> > +	return devnode_;
> > +}
> > +
> > +const struct media_device_info &DeviceInfo::info() const
> > +{
> > +	return info_;
> > +}
> > +
> > +std::vector<std::string> DeviceInfo::entities() const
> > +{
> > +	std::vector<std::string> entities;
> > +
> > +	for (const auto &entity : entities_)
> > +		entities.push_back(entity.first);
> > +
> > +	return entities;
> 
> Are you returning (by copy) a variable with local scope?
> Why coldn't you just return (as const reference maybe) entities_ ?

This is by design. The idea is that a new copy of the entities in a 
vector instead of the class internal object of the model of entities as 
the use-case here is to do matching. Why would it be beneficial to 
return a internal data structure?

> 
> > +}
> > +
> > +bool DeviceInfo::lookup(const std::string &name, std::string &devnode) const
> > +{
> > +	auto it = entities_.find(name);
> > +
> > +	if (it == entities_.end()) {
> > +		LOG(Error) << "Trying to lookup entity '" << name << "' which do not exist";
> 
> nit: "does not exist"

My comprehension of the Queens English is lacking so I trust you on 
this.  For my education is not 'does' used when there is more then one 
while 'do' is used when there is exactly one?

> 
> > +		return false;
> > +	}
> > +
> > +	devnode = it->second;
> > +	return true;
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h
> > new file mode 100644
> > index 0000000000000000..0136ed6ea23bf65e
> > --- /dev/null
> > +++ b/src/libcamera/include/deviceenumerator.h
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * deviceenumerator.h - API to enumerate and find media devices
> > + */
> > +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__
> > +#define __LIBCAMERA_DEVICEENUMERATE_H__
> > +
> > +#include <map>
> > +#include <string>
> > +#include <vector>
> > +
> > +#include <linux/media.h>
> > +
> > +namespace libcamera {
> > +
> > +class DeviceInfo
> > +{
> > +public:
> > +	DeviceInfo(const std::string &devnode, const struct media_device_info &info,
> > +		   const std::map<std::string, std::string> &entities);
> > +
> > +	int acquire();
> > +	void release();
> > +	bool busy() const;
> 
> Trivial methods might be inlined here.

There are other methods who can't be inlined for this class. Why spread 
the implementation around?

> 
> > +
> > +	const std::string &devnode() const;
> > +	const struct media_device_info &info() const;
> > +	std::vector<std::string> entities() const;
> > +
> > +	bool lookup(const std::string &name, std::string &devnode) const;
> > +
> > +private:
> > +	bool acquired_;
> > +
> > +	std::string devnode_;
> > +	struct media_device_info info_;
> > +	std::map<std::string, std::string> entities_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif	/* __LIBCAMERA_DEVICEENUMERATE_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 52b556a8ed4050cb..17cdf06dd2bedfb3 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -1,10 +1,12 @@
> >  libcamera_sources = files([
> >      'camera.cpp',
> > +    'deviceenumerator.cpp',
> >      'log.cpp',
> >      'main.cpp',
> >  ])
> >
> >  libcamera_headers = files([
> > +    'include/deviceenumerator.h',
> >      'include/log.h',
> >      'include/utils.h',
> >  ])
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Dec. 28, 2018, 8:21 a.m. UTC | #3
Hi Niklas,
   just replying to a few things

On Fri, Dec 28, 2018 at 04:06:57AM +0100, Niklas S??derlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2018-12-24 11:42:16 +0100, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Sun, Dec 23, 2018 at 12:00:32AM +0100, Niklas S??derlund wrote:
> > > Provide a DeviceInfo class which holds all information from the initial
> > > enumeration of a media device. Not all information available at a media
> > > device is stored, only the information needed for a pipeline handler to
> > > find a specific device.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > ---
> > >  src/libcamera/deviceenumerator.cpp       | 78 ++++++++++++++++++++++++
> > >  src/libcamera/include/deviceenumerator.h | 44 +++++++++++++
> > >  src/libcamera/meson.build                |  2 +
> > >  3 files changed, 124 insertions(+)
> > >  create mode 100644 src/libcamera/deviceenumerator.cpp
> > >  create mode 100644 src/libcamera/include/deviceenumerator.h
> > >
> > > diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp
> > > new file mode 100644
> > > index 0000000000000000..7c44a65b45472ef3
> > > --- /dev/null
> > > +++ b/src/libcamera/deviceenumerator.cpp
> > > @@ -0,0 +1,78 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2018, Google Inc.
> > > + *
> > > + * deviceenumerator.cpp - Enumeration and matching
> > > + */
> > > +
> > > +#include "deviceenumerator.h"
> > > +#include "log.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +/* -----------------------------------------------------------------------------
> > > + * DeviceInfo
> > > + */
> > > +
> > > +DeviceInfo::DeviceInfo(const std::string &devnode, const struct media_device_info &info,
> > > +		       const std::map<std::string, std::string> &entities)
> > > +	: acquired_(false), devnode_(devnode), info_(info), entities_(entities)
> > > +{
> > > +	for (const auto &entity : entities_)
> > > +		LOG(Info) << "Device: " << devnode_ << " Entity: '" << entity.first << "' -> " << entity.second;
> > > +}
> > > +
> > > +int DeviceInfo::acquire()
> > > +{
> > > +	if (acquired_)
> > > +		return -EBUSY;
> > > +
> > > +	acquired_ = true;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void DeviceInfo::release()
> > > +{
> > > +	acquired_ = false;
> > > +}
> > > +
> > > +bool DeviceInfo::busy() const
> > > +{
> > > +	return acquired_;
> > > +}
> > > +
> > > +const std::string &DeviceInfo::devnode() const
> > > +{
> > > +	return devnode_;
> > > +}
> > > +
> > > +const struct media_device_info &DeviceInfo::info() const
> > > +{
> > > +	return info_;
> > > +}
> > > +
> > > +std::vector<std::string> DeviceInfo::entities() const
> > > +{
> > > +	std::vector<std::string> entities;
> > > +
> > > +	for (const auto &entity : entities_)
> > > +		entities.push_back(entity.first);
> > > +
> > > +	return entities;
> >
> > Are you returning (by copy) a variable with local scope?
> > Why coldn't you just return (as const reference maybe) entities_ ?
>
> This is by design. The idea is that a new copy of the entities in a
> vector instead of the class internal object of the model of entities as
> the use-case here is to do matching. Why would it be beneficial to
> return a internal data structure?
>

It would avoid moving around a vector by copy. You only use
the returned vector to compare entities names, so you could just
simply return a const reference to your internal data structure.

I would actually ask why would it be beneficial to copy all entities
names in a new vector and copy it back, if that's not going to be
modified anyhow. Furthermore, you could have used operator= to copy
the internal one: http://www.cplusplus.com/reference/vector/vector/operator=/

> >
> > > +}
> > > +
> > > +bool DeviceInfo::lookup(const std::string &name, std::string &devnode) const
> > > +{
> > > +	auto it = entities_.find(name);
> > > +
> > > +	if (it == entities_.end()) {
> > > +		LOG(Error) << "Trying to lookup entity '" << name << "' which do not exist";
> >
> > nit: "does not exist"
>
> My comprehension of the Queens English is lacking so I trust you on

Don't!

> this.  For my education is not 'does' used when there is more then one
> while 'do' is used when there is exactly one?
>

You might be right, let's wait for some more educated (or native)
speaker to school us.

> >
> > > +		return false;
> > > +	}
> > > +
> > > +	devnode = it->second;
> > > +	return true;
> > > +}
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h
> > > new file mode 100644
> > > index 0000000000000000..0136ed6ea23bf65e
> > > --- /dev/null
> > > +++ b/src/libcamera/include/deviceenumerator.h
> > > @@ -0,0 +1,44 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2018, Google Inc.
> > > + *
> > > + * deviceenumerator.h - API to enumerate and find media devices
> > > + */
> > > +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__
> > > +#define __LIBCAMERA_DEVICEENUMERATE_H__
> > > +
> > > +#include <map>
> > > +#include <string>
> > > +#include <vector>
> > > +
> > > +#include <linux/media.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +class DeviceInfo
> > > +{
> > > +public:
> > > +	DeviceInfo(const std::string &devnode, const struct media_device_info &info,
> > > +		   const std::map<std::string, std::string> &entities);
> > > +
> > > +	int acquire();
> > > +	void release();
> > > +	bool busy() const;
> >
> > Trivial methods might be inlined here.
>
> There are other methods who can't be inlined for this class. Why spread
> the implementation around?
>

I'm in favour of inlining single line and trivial methods/constructors as
much as we could. It reduce the size of the cpp file, and makes clear
from reading the header the method is trivial. I'm not much concerned about
spreading per se, as we all navigate tags don't we?

(let alone the discussion about the fact inlining in class declaration
is a hint for the compiler to inline the method in the generated code.
I think the compiler is smart enough to find that out by itself
anyhow though)

Thanks
   j

> >
> > > +
> > > +	const std::string &devnode() const;
> > > +	const struct media_device_info &info() const;
> > > +	std::vector<std::string> entities() const;
> > > +
> > > +	bool lookup(const std::string &name, std::string &devnode) const;
> > > +
> > > +private:
> > > +	bool acquired_;
> > > +
> > > +	std::string devnode_;
> > > +	struct media_device_info info_;
> > > +	std::map<std::string, std::string> entities_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif	/* __LIBCAMERA_DEVICEENUMERATE_H__ */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index 52b556a8ed4050cb..17cdf06dd2bedfb3 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -1,10 +1,12 @@
> > >  libcamera_sources = files([
> > >      'camera.cpp',
> > > +    'deviceenumerator.cpp',
> > >      'log.cpp',
> > >      'main.cpp',
> > >  ])
> > >
> > >  libcamera_headers = files([
> > > +    'include/deviceenumerator.h',
> > >      'include/log.h',
> > >      'include/utils.h',
> > >  ])
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
>
>
> --
> Regards,
> Niklas Söderlund
Laurent Pinchart Dec. 28, 2018, 5:15 p.m. UTC | #4
Hi Jacopo,

On Friday, 28 December 2018 10:21:48 EET Jacopo Mondi wrote:
> On Fri, Dec 28, 2018 at 04:06:57AM +0100, Niklas S??derlund wrote:
> > On 2018-12-24 11:42:16 +0100, Jacopo Mondi wrote:
> > > On Sun, Dec 23, 2018 at 12:00:32AM +0100, Niklas S??derlund wrote:
> > > > Provide a DeviceInfo class which holds all information from the
> > > > initial enumeration of a media device. Not all information available
> > > > at a media device is stored, only the information needed for a
> > > > pipeline handler to find a specific device.
> > > > 
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > ---
> > > > 
> > > >  src/libcamera/deviceenumerator.cpp       | 78 +++++++++++++++++++++++
> > > >  src/libcamera/include/deviceenumerator.h | 44 +++++++++++++
> > > >  src/libcamera/meson.build                |  2 +
> > > >  3 files changed, 124 insertions(+)
> > > >  create mode 100644 src/libcamera/deviceenumerator.cpp
> > > >  create mode 100644 src/libcamera/include/deviceenumerator.h
> > > > 
> > > > diff --git a/src/libcamera/deviceenumerator.cpp
> > > > b/src/libcamera/deviceenumerator.cpp new file mode 100644
> > > > index 0000000000000000..7c44a65b45472ef3
> > > > --- /dev/null
> > > > +++ b/src/libcamera/deviceenumerator.cpp
> > > > @@ -0,0 +1,78 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2018, Google Inc.
> > > > + *
> > > > + * deviceenumerator.cpp - Enumeration and matching
> > > > + */
> > > > +
> > > > +#include "deviceenumerator.h"
> > > > +#include "log.h"
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +/*
> > > > ---------------------------------------------------------------------
> > > > -------- + * DeviceInfo
> > > > + */
> > > > +
> > > > +DeviceInfo::DeviceInfo(const std::string &devnode, const struct
> > > > media_device_info &info, +		       const std::map<std::string,
> > > > std::string> &entities)
> > > > +	: acquired_(false), devnode_(devnode), info_(info),
> > > > entities_(entities)
> > > > +{
> > > > +	for (const auto &entity : entities_)
> > > > +		LOG(Info) << "Device: " << devnode_ << " Entity: '" << 
entity.first
> > > > << "' -> " << entity.second; +}
> > > > +
> > > > +int DeviceInfo::acquire()
> > > > +{
> > > > +	if (acquired_)
> > > > +		return -EBUSY;
> > > > +
> > > > +	acquired_ = true;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +void DeviceInfo::release()
> > > > +{
> > > > +	acquired_ = false;
> > > > +}
> > > > +
> > > > +bool DeviceInfo::busy() const
> > > > +{
> > > > +	return acquired_;
> > > > +}
> > > > +
> > > > +const std::string &DeviceInfo::devnode() const
> > > > +{
> > > > +	return devnode_;
> > > > +}
> > > > +
> > > > +const struct media_device_info &DeviceInfo::info() const
> > > > +{
> > > > +	return info_;
> > > > +}
> > > > +
> > > > +std::vector<std::string> DeviceInfo::entities() const
> > > > +{
> > > > +	std::vector<std::string> entities;
> > > > +
> > > > +	for (const auto &entity : entities_)
> > > > +		entities.push_back(entity.first);
> > > > +
> > > > +	return entities;
> > > 
> > > Are you returning (by copy) a variable with local scope?
> > > Why coldn't you just return (as const reference maybe) entities_ ?
> > 
> > This is by design. The idea is that a new copy of the entities in a
> > vector instead of the class internal object of the model of entities as
> > the use-case here is to do matching. Why would it be beneficial to
> > return a internal data structure?
> 
> It would avoid moving around a vector by copy. You only use
> the returned vector to compare entities names, so you could just
> simply return a const reference to your internal data structure.
> 
> I would actually ask why would it be beneficial to copy all entities
> names in a new vector and copy it back, if that's not going to be
> modified anyhow. Furthermore, you could have used operator= to copy
> the internal one:
> http://www.cplusplus.com/reference/vector/vector/operator=/

What do you mean by "copying it back" ? Have you overlooked the fact that 
entities_ is a map, not a vector ?

> > > > +}
> > > > +
> > > > +bool DeviceInfo::lookup(const std::string &name, std::string
> > > > &devnode) const +{
> > > > +	auto it = entities_.find(name);
> > > > +
> > > > +	if (it == entities_.end()) {
> > > > +		LOG(Error) << "Trying to lookup entity '" << name << "' which do
> > > > not exist";
> > > > 
> > > nit: "does not exist"
> > 
> > My comprehension of the Queens English is lacking so I trust you on
> 
> Don't!
> 
> > this.  For my education is not 'does' used when there is more then one
> > while 'do' is used when there is exactly one?
> 
> You might be right, let's wait for some more educated (or native)
> speaker to school us.

I'm certainly not native, and wouldn't claim to be more educated, but I think 
the subject of the verb refers to "entity", which is a third person singular, 
so "does" is the right form.

> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	devnode = it->second;
> > > > +	return true;
> > > > +}
> > > > +
> > > > +} /* namespace libcamera */
> > > > diff --git a/src/libcamera/include/deviceenumerator.h
> > > > b/src/libcamera/include/deviceenumerator.h new file mode 100644
> > > > index 0000000000000000..0136ed6ea23bf65e
> > > > --- /dev/null
> > > > +++ b/src/libcamera/include/deviceenumerator.h
> > > > @@ -0,0 +1,44 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2018, Google Inc.
> > > > + *
> > > > + * deviceenumerator.h - API to enumerate and find media devices
> > > > + */
> > > > +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__
> > > > +#define __LIBCAMERA_DEVICEENUMERATE_H__
> > > > +
> > > > +#include <map>
> > > > +#include <string>
> > > > +#include <vector>
> > > > +
> > > > +#include <linux/media.h>
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +class DeviceInfo
> > > > +{
> > > > +public:
> > > > +	DeviceInfo(const std::string &devnode, const struct
> > > > media_device_info &info, +		   const std::map<std::string,
> > > > std::string> &entities);
> > > > +
> > > > +	int acquire();
> > > > +	void release();
> > > > +	bool busy() const;
> > > 
> > > Trivial methods might be inlined here.
> > 
> > There are other methods who can't be inlined for this class. Why spread
> > the implementation around?
> 
> I'm in favour of inlining single line and trivial methods/constructors as
> much as we could. It reduce the size of the cpp file, and makes clear
> from reading the header the method is trivial. I'm not much concerned about
> spreading per se, as we all navigate tags don't we?

Methods should only be inlined when the code size is roughly equivalent to a 
function call. Trivial getters are good candidates. Constructors may be, but 
only if they don't have a large list of member initializers. For destructors, 
even if the function is empty, it might call other destructors for members, 
and the destructor of the base class. Inlining destructors is thus usually a 
bad idea. Inlinline in C++ uses a different syntax than in C, but the result 
is similar, the code is inlined, and may thus increase the binary size. Please 
see the Google C++ style guide section about this.

> (let alone the discussion about the fact inlining in class declaration
> is a hint for the compiler to inline the method in the generated code.
> I think the compiler is smart enough to find that out by itself
> anyhow though)
> 
> > > > +
> > > > +	const std::string &devnode() const;
> > > > +	const struct media_device_info &info() const;
> > > > +	std::vector<std::string> entities() const;
> > > > +
> > > > +	bool lookup(const std::string &name, std::string &devnode) const;
> > > > +
> > > > +private:
> > > > +	bool acquired_;
> > > > +
> > > > +	std::string devnode_;
> > > > +	struct media_device_info info_;
> > > > +	std::map<std::string, std::string> entities_;
> > > > +};
> > > > +
> > > > +} /* namespace libcamera */
> > > > +
> > > > +#endif	/* __LIBCAMERA_DEVICEENUMERATE_H__ */

[snip]
Laurent Pinchart Dec. 28, 2018, 5:20 p.m. UTC | #5
Hi Niklas,

Thank you for the patch.

On Sunday, 23 December 2018 01:00:32 EET Niklas Söderlund wrote:
> Provide a DeviceInfo class which holds all information from the initial
> enumeration of a media device. Not all information available at a media
> device is stored, only the information needed for a pipeline handler to
> find a specific device.

Do I understand correctly that this will be replaced with MediaDevice ? If so 
I only have a small comment below, and for the rest,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/deviceenumerator.cpp       | 78 ++++++++++++++++++++++++
>  src/libcamera/include/deviceenumerator.h | 44 +++++++++++++
>  src/libcamera/meson.build                |  2 +
>  3 files changed, 124 insertions(+)
>  create mode 100644 src/libcamera/deviceenumerator.cpp
>  create mode 100644 src/libcamera/include/deviceenumerator.h
> 
> diff --git a/src/libcamera/deviceenumerator.cpp
> b/src/libcamera/deviceenumerator.cpp new file mode 100644
> index 0000000000000000..7c44a65b45472ef3
> --- /dev/null
> +++ b/src/libcamera/deviceenumerator.cpp
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * deviceenumerator.cpp - Enumeration and matching
> + */
> +
> +#include "deviceenumerator.h"
> +#include "log.h"
> +
> +namespace libcamera {
> +
> +/* ------------------------------------------------------------------------
> + * DeviceInfo
> + */
> +
> +DeviceInfo::DeviceInfo(const std::string &devnode, const struct
> media_device_info &info,
> +		       const std::map<std::string, std::string>
> &entities)
> +	: acquired_(false), devnode_(devnode), info_(info), entities_(entities)
> +{
> +	for (const auto &entity : entities_)
> +		LOG(Info) << "Device: " << devnode_ << " Entity: '" << entity.first << 
"'
> -> " << entity.second;
> +}
> +
> +int DeviceInfo::acquire()
> +{
> +	if (acquired_)
> +		return -EBUSY;
> +
> +	acquired_ = true;
> +
> +	return 0;
> +}
> +
> +void DeviceInfo::release()
> +{
> +	acquired_ = false;
> +}
> +
> +bool DeviceInfo::busy() const
> +{
> +	return acquired_;
> +}
> +
> +const std::string &DeviceInfo::devnode() const
> +{
> +	return devnode_;
> +}
> +
> +const struct media_device_info &DeviceInfo::info() const
> +{
> +	return info_;
> +}
> +
> +std::vector<std::string> DeviceInfo::entities() const
> +{
> +	std::vector<std::string> entities;
> +
> +	for (const auto &entity : entities_)
> +		entities.push_back(entity.first);
> +
> +	return entities;
> +}
> +
> +bool DeviceInfo::lookup(const std::string &name, std::string &devnode)
> const
> +{
> +	auto it = entities_.find(name);
> +
> +	if (it == entities_.end()) {
> +		LOG(Error) << "Trying to lookup entity '" << name << "' which do not
> exist";
> +		return false;
> +	}
> +
> +	devnode = it->second;
> +	return true;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/include/deviceenumerator.h
> b/src/libcamera/include/deviceenumerator.h new file mode 100644
> index 0000000000000000..0136ed6ea23bf65e
> --- /dev/null
> +++ b/src/libcamera/include/deviceenumerator.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * deviceenumerator.h - API to enumerate and find media devices
> + */
> +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__

This doesn't match the file name.

> +#define __LIBCAMERA_DEVICEENUMERATE_H__
> +
> +#include <map>
> +#include <string>
> +#include <vector>
> +
> +#include <linux/media.h>
> +
> +namespace libcamera {
> +
> +class DeviceInfo
> +{
> +public:
> +	DeviceInfo(const std::string &devnode, const struct media_device_info
> &info,
> +		   const std::map<std::string, std::string> &entities);
> +
> +	int acquire();
> +	void release();
> +	bool busy() const;
> +
> +	const std::string &devnode() const;
> +	const struct media_device_info &info() const;
> +	std::vector<std::string> entities() const;
> +
> +	bool lookup(const std::string &name, std::string &devnode) const;
> +
> +private:
> +	bool acquired_;
> +
> +	std::string devnode_;
> +	struct media_device_info info_;
> +	std::map<std::string, std::string> entities_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif	/* __LIBCAMERA_DEVICEENUMERATE_H__ */

[snip]
Niklas Söderlund Dec. 29, 2018, 12:46 a.m. UTC | #6
Hi Laurent,

Thanks for your comments.

On 2018-12-28 19:20:59 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sunday, 23 December 2018 01:00:32 EET Niklas Söderlund wrote:
> > Provide a DeviceInfo class which holds all information from the initial
> > enumeration of a media device. Not all information available at a media
> > device is stored, only the information needed for a pipeline handler to
> > find a specific device.
> 
> Do I understand correctly that this will be replaced with MediaDevice ? If so 
> I only have a small comment below, and for the rest,

Yes the plan is to replace this with MediaDevice.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/deviceenumerator.cpp       | 78 ++++++++++++++++++++++++
> >  src/libcamera/include/deviceenumerator.h | 44 +++++++++++++
> >  src/libcamera/meson.build                |  2 +
> >  3 files changed, 124 insertions(+)
> >  create mode 100644 src/libcamera/deviceenumerator.cpp
> >  create mode 100644 src/libcamera/include/deviceenumerator.h
> > 
> > diff --git a/src/libcamera/deviceenumerator.cpp
> > b/src/libcamera/deviceenumerator.cpp new file mode 100644
> > index 0000000000000000..7c44a65b45472ef3
> > --- /dev/null
> > +++ b/src/libcamera/deviceenumerator.cpp
> > @@ -0,0 +1,78 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * deviceenumerator.cpp - Enumeration and matching
> > + */
> > +
> > +#include "deviceenumerator.h"
> > +#include "log.h"
> > +
> > +namespace libcamera {
> > +
> > +/* ------------------------------------------------------------------------
> > + * DeviceInfo
> > + */
> > +
> > +DeviceInfo::DeviceInfo(const std::string &devnode, const struct
> > media_device_info &info,
> > +		       const std::map<std::string, std::string>
> > &entities)
> > +	: acquired_(false), devnode_(devnode), info_(info), entities_(entities)
> > +{
> > +	for (const auto &entity : entities_)
> > +		LOG(Info) << "Device: " << devnode_ << " Entity: '" << entity.first << 
> "'
> > -> " << entity.second;
> > +}
> > +
> > +int DeviceInfo::acquire()
> > +{
> > +	if (acquired_)
> > +		return -EBUSY;
> > +
> > +	acquired_ = true;
> > +
> > +	return 0;
> > +}
> > +
> > +void DeviceInfo::release()
> > +{
> > +	acquired_ = false;
> > +}
> > +
> > +bool DeviceInfo::busy() const
> > +{
> > +	return acquired_;
> > +}
> > +
> > +const std::string &DeviceInfo::devnode() const
> > +{
> > +	return devnode_;
> > +}
> > +
> > +const struct media_device_info &DeviceInfo::info() const
> > +{
> > +	return info_;
> > +}
> > +
> > +std::vector<std::string> DeviceInfo::entities() const
> > +{
> > +	std::vector<std::string> entities;
> > +
> > +	for (const auto &entity : entities_)
> > +		entities.push_back(entity.first);
> > +
> > +	return entities;
> > +}
> > +
> > +bool DeviceInfo::lookup(const std::string &name, std::string &devnode)
> > const
> > +{
> > +	auto it = entities_.find(name);
> > +
> > +	if (it == entities_.end()) {
> > +		LOG(Error) << "Trying to lookup entity '" << name << "' which do not
> > exist";
> > +		return false;
> > +	}
> > +
> > +	devnode = it->second;
> > +	return true;
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/include/deviceenumerator.h
> > b/src/libcamera/include/deviceenumerator.h new file mode 100644
> > index 0000000000000000..0136ed6ea23bf65e
> > --- /dev/null
> > +++ b/src/libcamera/include/deviceenumerator.h
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * deviceenumerator.h - API to enumerate and find media devices
> > + */
> > +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__
> 
> This doesn't match the file name.

Thanks for noticing, fixed.

> 
> > +#define __LIBCAMERA_DEVICEENUMERATE_H__
> > +
> > +#include <map>
> > +#include <string>
> > +#include <vector>
> > +
> > +#include <linux/media.h>
> > +
> > +namespace libcamera {
> > +
> > +class DeviceInfo
> > +{
> > +public:
> > +	DeviceInfo(const std::string &devnode, const struct media_device_info
> > &info,
> > +		   const std::map<std::string, std::string> &entities);
> > +
> > +	int acquire();
> > +	void release();
> > +	bool busy() const;
> > +
> > +	const std::string &devnode() const;
> > +	const struct media_device_info &info() const;
> > +	std::vector<std::string> entities() const;
> > +
> > +	bool lookup(const std::string &name, std::string &devnode) const;
> > +
> > +private:
> > +	bool acquired_;
> > +
> > +	std::string devnode_;
> > +	struct media_device_info info_;
> > +	std::map<std::string, std::string> entities_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif	/* __LIBCAMERA_DEVICEENUMERATE_H__ */
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
>
Jacopo Mondi Dec. 29, 2018, 9:29 a.m. UTC | #7
Hi Laurent,

On Fri, Dec 28, 2018 at 07:15:20PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Friday, 28 December 2018 10:21:48 EET Jacopo Mondi wrote:
> > On Fri, Dec 28, 2018 at 04:06:57AM +0100, Niklas S??derlund wrote:
> > > On 2018-12-24 11:42:16 +0100, Jacopo Mondi wrote:
> > > > On Sun, Dec 23, 2018 at 12:00:32AM +0100, Niklas S??derlund wrote:
> > > > > Provide a DeviceInfo class which holds all information from the
> > > > > initial enumeration of a media device. Not all information available
> > > > > at a media device is stored, only the information needed for a
> > > > > pipeline handler to find a specific device.
> > > > >
> > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > > ---
> > > > >
> > > > >  src/libcamera/deviceenumerator.cpp       | 78 +++++++++++++++++++++++
> > > > >  src/libcamera/include/deviceenumerator.h | 44 +++++++++++++
> > > > >  src/libcamera/meson.build                |  2 +
> > > > >  3 files changed, 124 insertions(+)
> > > > >  create mode 100644 src/libcamera/deviceenumerator.cpp
> > > > >  create mode 100644 src/libcamera/include/deviceenumerator.h
> > > > >
> > > > > diff --git a/src/libcamera/deviceenumerator.cpp
> > > > > b/src/libcamera/deviceenumerator.cpp new file mode 100644
> > > > > index 0000000000000000..7c44a65b45472ef3
> > > > > --- /dev/null
> > > > > +++ b/src/libcamera/deviceenumerator.cpp
> > > > > @@ -0,0 +1,78 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2018, Google Inc.
> > > > > + *
> > > > > + * deviceenumerator.cpp - Enumeration and matching
> > > > > + */
> > > > > +
> > > > > +#include "deviceenumerator.h"
> > > > > +#include "log.h"
> > > > > +
> > > > > +namespace libcamera {
> > > > > +
> > > > > +/*
> > > > > ---------------------------------------------------------------------
> > > > > -------- + * DeviceInfo
> > > > > + */
> > > > > +
> > > > > +DeviceInfo::DeviceInfo(const std::string &devnode, const struct
> > > > > media_device_info &info, +		       const std::map<std::string,
> > > > > std::string> &entities)
> > > > > +	: acquired_(false), devnode_(devnode), info_(info),
> > > > > entities_(entities)
> > > > > +{
> > > > > +	for (const auto &entity : entities_)
> > > > > +		LOG(Info) << "Device: " << devnode_ << " Entity: '" <<
> entity.first
> > > > > << "' -> " << entity.second; +}
> > > > > +
> > > > > +int DeviceInfo::acquire()
> > > > > +{
> > > > > +	if (acquired_)
> > > > > +		return -EBUSY;
> > > > > +
> > > > > +	acquired_ = true;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +void DeviceInfo::release()
> > > > > +{
> > > > > +	acquired_ = false;
> > > > > +}
> > > > > +
> > > > > +bool DeviceInfo::busy() const
> > > > > +{
> > > > > +	return acquired_;
> > > > > +}
> > > > > +
> > > > > +const std::string &DeviceInfo::devnode() const
> > > > > +{
> > > > > +	return devnode_;
> > > > > +}
> > > > > +
> > > > > +const struct media_device_info &DeviceInfo::info() const
> > > > > +{
> > > > > +	return info_;
> > > > > +}
> > > > > +
> > > > > +std::vector<std::string> DeviceInfo::entities() const
> > > > > +{
> > > > > +	std::vector<std::string> entities;
> > > > > +
> > > > > +	for (const auto &entity : entities_)
> > > > > +		entities.push_back(entity.first);
> > > > > +
> > > > > +	return entities;
> > > >
> > > > Are you returning (by copy) a variable with local scope?
> > > > Why coldn't you just return (as const reference maybe) entities_ ?
> > >
> > > This is by design. The idea is that a new copy of the entities in a
> > > vector instead of the class internal object of the model of entities as
> > > the use-case here is to do matching. Why would it be beneficial to
> > > return a internal data structure?
> >
> > It would avoid moving around a vector by copy. You only use
> > the returned vector to compare entities names, so you could just
> > simply return a const reference to your internal data structure.
> >
> > I would actually ask why would it be beneficial to copy all entities
> > names in a new vector and copy it back, if that's not going to be
> > modified anyhow. Furthermore, you could have used operator= to copy
> > the internal one:
> > http://www.cplusplus.com/reference/vector/vector/operator=/
>
> What do you mean by "copying it back" ? Have you overlooked the fact that
> entities_ is a map, not a vector ?
>

Indeed I did.
Sorry for the noise.

Thanks
   j

> > > > > +}
> > > > > +
> > > > > +bool DeviceInfo::lookup(const std::string &name, std::string
> > > > > &devnode) const +{
> > > > > +	auto it = entities_.find(name);
> > > > > +
> > > > > +	if (it == entities_.end()) {
> > > > > +		LOG(Error) << "Trying to lookup entity '" << name << "' which do
> > > > > not exist";
> > > > >
> > > > nit: "does not exist"
> > >
> > > My comprehension of the Queens English is lacking so I trust you on
> >
> > Don't!
> >
> > > this.  For my education is not 'does' used when there is more then one
> > > while 'do' is used when there is exactly one?
> >
> > You might be right, let's wait for some more educated (or native)
> > speaker to school us.
>
> I'm certainly not native, and wouldn't claim to be more educated, but I think
> the subject of the verb refers to "entity", which is a third person singular,
> so "does" is the right form.
>
> > > > > +		return false;
> > > > > +	}
> > > > > +
> > > > > +	devnode = it->second;
> > > > > +	return true;
> > > > > +}
> > > > > +
> > > > > +} /* namespace libcamera */
> > > > > diff --git a/src/libcamera/include/deviceenumerator.h
> > > > > b/src/libcamera/include/deviceenumerator.h new file mode 100644
> > > > > index 0000000000000000..0136ed6ea23bf65e
> > > > > --- /dev/null
> > > > > +++ b/src/libcamera/include/deviceenumerator.h
> > > > > @@ -0,0 +1,44 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2018, Google Inc.
> > > > > + *
> > > > > + * deviceenumerator.h - API to enumerate and find media devices
> > > > > + */
> > > > > +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__
> > > > > +#define __LIBCAMERA_DEVICEENUMERATE_H__
> > > > > +
> > > > > +#include <map>
> > > > > +#include <string>
> > > > > +#include <vector>
> > > > > +
> > > > > +#include <linux/media.h>
> > > > > +
> > > > > +namespace libcamera {
> > > > > +
> > > > > +class DeviceInfo
> > > > > +{
> > > > > +public:
> > > > > +	DeviceInfo(const std::string &devnode, const struct
> > > > > media_device_info &info, +		   const std::map<std::string,
> > > > > std::string> &entities);
> > > > > +
> > > > > +	int acquire();
> > > > > +	void release();
> > > > > +	bool busy() const;
> > > >
> > > > Trivial methods might be inlined here.
> > >
> > > There are other methods who can't be inlined for this class. Why spread
> > > the implementation around?
> >
> > I'm in favour of inlining single line and trivial methods/constructors as
> > much as we could. It reduce the size of the cpp file, and makes clear
> > from reading the header the method is trivial. I'm not much concerned about
> > spreading per se, as we all navigate tags don't we?
>
> Methods should only be inlined when the code size is roughly equivalent to a
> function call. Trivial getters are good candidates. Constructors may be, but
> only if they don't have a large list of member initializers. For destructors,
> even if the function is empty, it might call other destructors for members,
> and the destructor of the base class. Inlining destructors is thus usually a
> bad idea. Inlinline in C++ uses a different syntax than in C, but the result
> is similar, the code is inlined, and may thus increase the binary size. Please
> see the Google C++ style guide section about this.
>
> > (let alone the discussion about the fact inlining in class declaration
> > is a hint for the compiler to inline the method in the generated code.
> > I think the compiler is smart enough to find that out by itself
> > anyhow though)
> >
> > > > > +
> > > > > +	const std::string &devnode() const;
> > > > > +	const struct media_device_info &info() const;
> > > > > +	std::vector<std::string> entities() const;
> > > > > +
> > > > > +	bool lookup(const std::string &name, std::string &devnode) const;
> > > > > +
> > > > > +private:
> > > > > +	bool acquired_;
> > > > > +
> > > > > +	std::string devnode_;
> > > > > +	struct media_device_info info_;
> > > > > +	std::map<std::string, std::string> entities_;
> > > > > +};
> > > > > +
> > > > > +} /* namespace libcamera */
> > > > > +
> > > > > +#endif	/* __LIBCAMERA_DEVICEENUMERATE_H__ */
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Patch

diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp
new file mode 100644
index 0000000000000000..7c44a65b45472ef3
--- /dev/null
+++ b/src/libcamera/deviceenumerator.cpp
@@ -0,0 +1,78 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2018, Google Inc.
+ *
+ * deviceenumerator.cpp - Enumeration and matching
+ */
+
+#include "deviceenumerator.h"
+#include "log.h"
+
+namespace libcamera {
+
+/* -----------------------------------------------------------------------------
+ * DeviceInfo
+ */
+
+DeviceInfo::DeviceInfo(const std::string &devnode, const struct media_device_info &info,
+		       const std::map<std::string, std::string> &entities)
+	: acquired_(false), devnode_(devnode), info_(info), entities_(entities)
+{
+	for (const auto &entity : entities_)
+		LOG(Info) << "Device: " << devnode_ << " Entity: '" << entity.first << "' -> " << entity.second;
+}
+
+int DeviceInfo::acquire()
+{
+	if (acquired_)
+		return -EBUSY;
+
+	acquired_ = true;
+
+	return 0;
+}
+
+void DeviceInfo::release()
+{
+	acquired_ = false;
+}
+
+bool DeviceInfo::busy() const
+{
+	return acquired_;
+}
+
+const std::string &DeviceInfo::devnode() const
+{
+	return devnode_;
+}
+
+const struct media_device_info &DeviceInfo::info() const
+{
+	return info_;
+}
+
+std::vector<std::string> DeviceInfo::entities() const
+{
+	std::vector<std::string> entities;
+
+	for (const auto &entity : entities_)
+		entities.push_back(entity.first);
+
+	return entities;
+}
+
+bool DeviceInfo::lookup(const std::string &name, std::string &devnode) const
+{
+	auto it = entities_.find(name);
+
+	if (it == entities_.end()) {
+		LOG(Error) << "Trying to lookup entity '" << name << "' which do not exist";
+		return false;
+	}
+
+	devnode = it->second;
+	return true;
+}
+
+} /* namespace libcamera */
diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h
new file mode 100644
index 0000000000000000..0136ed6ea23bf65e
--- /dev/null
+++ b/src/libcamera/include/deviceenumerator.h
@@ -0,0 +1,44 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2018, Google Inc.
+ *
+ * deviceenumerator.h - API to enumerate and find media devices
+ */
+#ifndef __LIBCAMERA_DEVICEENUMERATE_H__
+#define __LIBCAMERA_DEVICEENUMERATE_H__
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include <linux/media.h>
+
+namespace libcamera {
+
+class DeviceInfo
+{
+public:
+	DeviceInfo(const std::string &devnode, const struct media_device_info &info,
+		   const std::map<std::string, std::string> &entities);
+
+	int acquire();
+	void release();
+	bool busy() const;
+
+	const std::string &devnode() const;
+	const struct media_device_info &info() const;
+	std::vector<std::string> entities() const;
+
+	bool lookup(const std::string &name, std::string &devnode) const;
+
+private:
+	bool acquired_;
+
+	std::string devnode_;
+	struct media_device_info info_;
+	std::map<std::string, std::string> entities_;
+};
+
+} /* namespace libcamera */
+
+#endif	/* __LIBCAMERA_DEVICEENUMERATE_H__ */
diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
index 52b556a8ed4050cb..17cdf06dd2bedfb3 100644
--- a/src/libcamera/meson.build
+++ b/src/libcamera/meson.build
@@ -1,10 +1,12 @@ 
 libcamera_sources = files([
     'camera.cpp',
+    'deviceenumerator.cpp',
     'log.cpp',
     'main.cpp',
 ])
 
 libcamera_headers = files([
+    'include/deviceenumerator.h',
     'include/log.h',
     'include/utils.h',
 ])