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

[Laurent Pinchart]

Hi Kieran,

On Fri, Jan 18, 2019 at 11:12:56AM +0000, Kieran Bingham wrote:
> On 18/01/2019 00:30, Laurent Pinchart wrote:
> > 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 we're going to have our 'own' implementation of make_unique - could
> you add a test to test/meson.build::internal_tests please?

That's a good point. I'm however not sure how to test it meaningfully
other than making sure it compiles, which is already handled by the
libcamera code base. Feel free to propose a test :-)

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