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

[Laurent Pinchart]

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.

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.

> 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 ?

> >>
> >>>
> >>>>>> 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