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

[Kieran Bingham]

Hi Laurent,

On 18/01/2019 00:30, Laurent Pinchart 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.
> 
> 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?




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

-- 
Regards
--
Kieran