[libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator: add DeviceInfo class
Niklas S??derlund
niklas.soderlund at ragnatech.se
Fri Dec 28 04:06:57 CET 2018
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?
>
> > +}
> > +
> > +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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list