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

[Jacopo Mondi]

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 at 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
>
>
>
-------------- 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/ec3e49b7/attachment.sig>