[libcamera-devel] [PATCH 3/4] libcamera: camera_manager: Register cameras with the camera manager

[Laurent Pinchart]

Hi Niklas,

On Fri, Jan 18, 2019 at 05:06:04PM +0100, Niklas Söderlund wrote:
> On 2019-01-18 01:59:15 +0200, Laurent Pinchart wrote:
> > Cameras are listed through a double indirection, first iterating over
> > all available pipeline handlers, and then listing the cameras they each
> > support. To simplify the API make the pipeline handlers register the
> > cameras with the camera manager directly, which lets the camera manager
> > easily expose the list of all available cameras.
> > 
> > The PipelineHandler API gets simplified as the handlers don't need to
> > expose the list of cameras they have created.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/camera_manager.h       |  5 ++-
> >  src/libcamera/camera_manager.cpp         | 44 +++++++++++-------------
> >  src/libcamera/include/pipeline_handler.h |  6 ++--
> >  src/libcamera/pipeline/vimc.cpp          | 30 ++++------------
> >  src/libcamera/pipeline_handler.cpp       | 21 +++--------
> >  test/list-cameras.cpp                    |  5 +--
> >  6 files changed, 40 insertions(+), 71 deletions(-)

[snip]

> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index 9806e399f92b..852e5ed70fe3 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp

[snip]

> > +/**
> > + * \brief Add a camera to the camera manager
> > + * \param[in] camera The camera to be added
> > + *
> > + * This function is called by pipeline handlers to register the cameras they
> > + * handle with the camera manager. Registered cameras are immediately made
> > + * available to the system.
> > + */
> > +void CameraManager::addCamera(Camera *camera)
> > +{
> > +	cameras_.push_back(camera);
> 
> I wonder if we should not add a runtime check here to make sure the 
> camera name is unique before adding it to the vector here? The get() 
> function retrieves a Camera object by name there might be odd 
> consequences otherwise :-)

Good point. I'll add

        for (Camera *c : cameras_) {
                if (c->name() == camera->name()) {
                        LOG(Warning) << "Registering camera with duplicate name '"
                                     << camera->name() << "'";
                        break;
                }
        }

Or should it be LOG(Error) ? I don't think it should be a fatal error in
any case (so no ASSERT) as it doesn't prevent from using the other
cameras, or even the one with a duplicate name when retrieved directly
from the vector of cameras (introduced in patch 3/4).

> With this addressed,
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> > +}

[snip]

-- 
Regards,

Laurent Pinchart