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

[Laurent Pinchart]

Hi Ricky,

On Tuesday, 15 January 2019 05:16:00 EET Ricky Liang wrote:
> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart 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]

> >>>> Not directly related to this patch, but have we considered making
> >>>> these pointers std::unique_ptr? From our experience working in
> >>>> Chromium we find it informative, easier to follow the code, and less
> >>>> error-prone if an object's ownership can be clearly identified
> >>>> through an std::unique_ptr.
> >>>> 
> >>>> For example, in the current libcamera code base I noticed there's a
> >>>> potential leak in DeviceEnumerator::create() if enumerator->init()
> >>>> fails:
> >>>> 
> >>>> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/device_enume
> >>>> rator.cpp#n137
> >>>> 
> >>>> If we instead only create and pass std::unique_ptr<DeviceEnumerator>
> >>>> around then we'd avoid leak like this, and people can also look at
> >>>> the code and clearly understands that CameraManager has the ownership
> >>>> of enumerator_.
> >>> 
> >>> I agree with you, std::unique_ptr<> would here both provide the
> >>> advantages of a scoped pointer with automatic deletion, and make it
> >>> clear who owns the object. I gave it a try for enumerator_, and here
> >>> is what I ended up with (quote characters added to comment inline).

[snip]

> >>>> diff --git a/src/libcamera/camera_manager.cpp
> >>>> b/src/libcamera/camera_manager.cpp index be327f5d5638..432f2b0a11c5
> >>>> 100644
> >>>> --- a/src/libcamera/camera_manager.cpp
> >>>> +++ b/src/libcamera/camera_manager.cpp

[snip]

> >>>> @@ -84,7 +83,7 @@ int CameraManager::start()
> >>>>                * all pipelines it can provide.
> >>>>                */
> >>>>               do {
> >>>> -                     pipe = PipelineHandlerFactory::create(handler,
> >>>> enumerator_);
> >>>> +                     pipe = PipelineHandlerFactory::create(handler,
> >>>> enumerator_.get());
> >>> 
> >>> We already break the unique_ptr<> promise :-) If this acceptable
> >>> according to you, or is there a better way ?
> >> 
> >> If we're not going to change the internal state of enumerator_, then
> >> we can make PipelineHandlerFactory::create take a const reference
> >> instead of a pointer. In general we use const reference for
> >> method/function arguments that stay unchanged after calling the
> >> method/function, and pointer for output arguments.
> >> 
> >> Wdyt?
> > 
> > While we don't have to change the state of the enumerator strictly
> > speaking, we need to change the state of the MediaDevice instances it
> > stores (as pointers in a vector). We call the following method for that
> > purpose:
> > 
> > MediaDevice *DeviceEnumerator::search(const DeviceMatch &dm) const
> > 
> > which I think should not be const as it allows changing the state of an
> > object owned by the enumerator.
> 
> I see. Thanks for the detail!
> 
> > What we really need to convey through the API is that the called function
> > PipelineHandlerFactory::create() receive a borrowed references to the
> > enumerator and may not access it after it returns. As far as I know
> > there's no simple construct in C++ that is universally understood as
> > expressing this, unlike passing a unique_ptr to express that ownership is
> > transferred. We could possibly standardize on using references for that
> > purpose (const and non- const), but in some cases we still need pointers
> > when passing nullptr is valid, so it wouldn't be a great solution. What's
> > your opinion on this, could we set in stone the rule that a reference
> > received by a function means that the reference is borrowed for the
> > duration of the function only ?
> 
> Personally I'm not a big fan of non-const reference. I find it easily
> confused when reviewing the code as it has value syntax with pointer
> semantics. Having raw pointer arguments and/or return values is fine
> and often necessary.
> 
> I'd suggest we use std::unique_ptr<> whenever possible to establish
> clear object ownership, and const reference whenever we don't plan to
> alter the state of the object. We then can put most of our attention
> to the remaining raw pointers.
> 
> As we're following the Google C++ coding style I'd suggest we follow:
> 
> https://google.github.io/styleguide/cppguide.html#Reference_Argument

We already do, with one exception that is just an oversight and can easily be 
fixed (I'll submit a patch).

To recap, the idea woulc be to standardize on the following semantics:

- Passing ownership: std::unique_ptr<>
- Passing a reference to a shared object: std::shared_ptr<>
- Passing a borrowed reference when the object doesn't need to be modified: 
const &
- Passing a borrowed reference when the object can be modified: pointer
- Passing a borrowed reference to an output parameter: pointer
- Passing a borrowed reference to an object that can be null: pointer

This implies that the callee can never store a reference to a pointer it 
receives, as all the use cases for pointers pass borrowed references.

Am I missing something ?

> >>>>                       if (pipe)
> >>>>                               pipes_.push_back(pipe);
> >>>>               } while (pipe);

[snip]

-- 
Regards,

Laurent Pinchart