[libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager: Make the class a singleton

Shik Chen shik at google.com
Fri Jan 18 08:42:28 CET 2019 

Hi Laurent,

On Fri, Jan 18, 2019 at 8:30 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Shik,
>
> On Wed, Jan 16, 2019 at 09:46:30PM +0800, Shik Chen wrote:
> > On Tue, Jan 15, 2019 at 10:26 PM Ricky Liang <jcliang at chromium.org> wrote:
> > > On Tue, Jan 15, 2019 at 11:16 AM Ricky Liang <jcliang at chromium.org> wrote:
> > >> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart
> > >> <laurent.pinchart at ideasonboard.com> wrote:
> > >>> On Monday, 14 January 2019 10:19:50 EET Ricky Liang wrote:
> > >>>> On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart wrote:
> > >>>>> On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:
> > >>>>>> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:
> > >>>>>>> There can only be a single camera manager instance in the application.
> > >>>>>>> Creating it as a singleton helps avoiding mistakes. It also allows the
> > >>>>>>> camera manager to be used as a storage of global data, such as the
> > >>>>>>> future event dispatcher.
> > >>>>>>>
> > >>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > >>>>>>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > >>>>>>> ---
> > >>>>>>> Changes since v1:
> > >>>>>>>
> > >>>>>>> - Delete copy constructor and assignment operator
> > >>>>>>> - Fix documentation style issue
> > >>>>>>> ---
> > >>>>>>>
> > >>>>>>> include/libcamera/camera_manager.h |  8 ++++++--
> > >>>>>>> src/libcamera/camera_manager.cpp   | 15 +++++++++++++++
> > >>>>>>> test/list.cpp                      |  7 +------
> > >>>>>>> 3 files changed, 22 insertions(+), 8 deletions(-)
>
> [snip]
>
> > >>>>>> diff --git a/src/libcamera/device_enumerator.cpp
> > >>>>>> b/src/libcamera/device_enumerator.cpp index 0d18e75525af..2b03b191fa5d
> > >>>>>> 100644
> > >>>>>> --- a/src/libcamera/device_enumerator.cpp
> > >>>>>> +++ b/src/libcamera/device_enumerator.cpp
> > >>>>>> @@ -14,6 +14,7 @@
> > >>>>>> #include "device_enumerator.h"
> > >>>>>> #include "log.h"
> > >>>>>> #include "media_device.h"
> > >>>>>> +#include "utils.h"
> > >>>>>>
> > >>>>>> /**
> > >>>>>> * \file device_enumerator.h
> > >>>>>> @@ -128,20 +129,20 @@ bool DeviceMatch::match(const MediaDevice *device)
> > >>>>>> const
> > >>>>>> * \return A pointer to the newly created device enumerator on success,
> > >>>>>> or
> > >>>>>> * nullptr if an error occurs
> > >>>>>> */
> > >>>>>> -DeviceEnumerator *DeviceEnumerator::create()
> > >>>>>> +std::unique_ptr<DeviceEnumerator> DeviceEnumerator::create()
> > >>>>>> {
> > >>>>>> -     DeviceEnumerator *enumerator;
> > >>>>>> +     std::unique_ptr<DeviceEnumerator> enumerator;
> > >>>>>>
> > >>>>>> /**
> > >>>>>> * \todo Add compile time checks to only try udev enumerator if
> > >>>>>> libudev
> > >>>>>> * is available.
> > >>>>>> */
> > >>>>>> -     enumerator = new DeviceEnumeratorUdev();
> > >>>>>> +     enumerator = std::make_unique<DeviceEnumeratorUdev>(); /* [1] */
> > >>>>>> +     enumerator = std::unique_ptr<DeviceEnumeratorUdev>(new
> > >>>>>> DeviceEnumeratorUdev()); /* [2] */
> > >>>>>> +     enumerator.reset(new> DeviceEnumeratorUdev()); /* [3] */
> > >>>>>
> > >>>>> Here are three different ways of implementing the same operation.
> > >>>>> std::make_unique<> doesn't exist in C++11, so I've defined it in utils.c.
> > >>>>> Adding functions to the std namespace could be considered a big of a hack
> > >>>>> though, so two other implementations are proposed. The second option is
> > >>>>> explicit, but a bit too long for my taste, while the third option is short
> > >>>>> but uses reset() which sounds a bit strange for an assignment. Do you
> > >>>>> have any advice ?
> > >>>>
> > >>>> Before we have C++11 in Chromium we also had a base::MakeUnique<>
> > >>>> template in the Chromium libbase handling creation of unique pointers.
> > >>>> If we have to stick with C++11 then we might consider doing the same,
> > >>>> probably with a utils:: namespace. Hacking the std:: namespace is
> > >>>> indeed a bad idea an can cause build errors.
> > >>>>
> > >>>> Semantically I'd say [2] is more accurate than [3], but I don't really
> > >>>> have strong opinions between these two. If we don't want to define our
> > >>>> own make_unique template then we can use either.
> > >>>
> > >>> I'm tempted to add our own make_unique implementation then, most likely in the
> > >>> libcamera:: or libcamera::utils:: namespace though.
> > >>
> > >> Sounds good!
> > >>
> > >>>
> > >>>> Do we restrict ourselves in C++11 for compatibility reason?
> > >>>
> > >>> That was the rationale, but it could be reconsidered if needed.
> > >>
> > >> I suppose C++11 shall be sufficient. We can indeed reconsider if we
> > >> have strong use cases for newer standards one day.
> >
> > Implementing make_unique or anything already defined in the newer standards need
> > be done with extra care. Users would expect it to work in the same way, so any
> > inconsistency with the standards might be a hidden trap. Making things forward
> > compatible is not an easy task. For example, the implementation below does not
> > provide the make_unique<T[]>(std::size_t) overload defined in C++14.
> >
> > I'd suggest not to reinvent the wheel if there is no strong objection. Is it
> > possible to bump the C++ version to C++14/17? We can still limit the scope of
> > new language features as we did in [1], so we can opt-in the features gradually.
>
> This worries me for two reasons. The first one is that depending on
> C++14/C++17 lock us out of platform that don't support those newer
> standards. This is more of a general concern, I don't have any data to
> tell whether this is a valid concern or not.

Yes it's hard to make decision without having the concrete target platforms. The
compiler support for C++14 are complete and stable enough in GCC/Clang, and
it's the default C++ standard version since GCC 5 and Clang 6. But some
popularish Linux distros might ship with older compilers that does not support
C++14 well, such as Ubuntu 14.04 and RHEL 6 although they are approaching EOL.

>
> The second concern is that we may start using features of C++14/C++17
> without noticing, possibly creating problems later if we want to support
> C++11-based systems.

This is actually a good point. Note that the compiler flag "-std=c++XX" might
not strictly align with the standard. We are already using at least one C++17
feature "nested namespace" in utils.h, which uses "namespace libcamera::utils"
instead of "namespace libcamera { namespace utils". I found this because it fail
to compile on one of my machine (Ubuntu 17.10).

>
> > Another possibility is adopting some existing library such as abseil [2] which
> > includes make_unique and many other goodies for projects in C++11.
> >
> > [1] http://www.libcamera.org/coding-style.html#c-specific-rules
> > [2] https://abseil.io/
>
> Thank you for the pointer, that's an interesting library. However, at
> this point, I would like to avoid adding an external dependency just to
> provide make_unique support. I'm thus tempted to keep our in-house
> implementation (which by the way is proposed by
> https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique), and
> revisit this topic if/when we will need more feature of newer C++
> versions. Does this sound acceptable to you ?

It's reasonable that not to add an external dependency just for make_unique. We
could revisit this topic as we need it.

>
> > >>
> > >>>
> > >>>>>> if (!enumerator->init())
> > >>>>>> return enumerator;
> > >>>>>>
> > >>>>>> -     delete enumerator;
> > >>>>>> -
> > >>>>>> /*
> > >>>>>> * Either udev is not available or udev initialization failed.
> > >>>>>> Fall back
> > >>>>>> * on the sysfs enumerator.
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart

Sincerely,
Shik

More information about the libcamera-devel mailing list