[libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator: add DeviceInfo class
Jacopo Mondi
jacopo at jmondi.org
Fri Dec 28 09:21:48 CET 2018
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 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=/
> >
> > > +}
> > > +
> > > +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 at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
>
>
> --
> 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/20181228/b523e85c/attachment-0001.sig>
More information about the libcamera-devel
mailing list