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

[Ricky Liang]

Hi Laurent,

On Tue, Jan 15, 2019 at 10:57 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?

I believe as the code base grows there may be cases where we'll need
to store a reference to a pointer. When that happens we can document
in comments what makes it safe to store and access the pointer.

Otherwise your summary looks good to me.

>
> > >>>>                       if (pipe)
> > >>>>                               pipes_.push_back(pipe);
> > >>>>               } while (pipe);
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>