[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:47:31PM +0100, Niklas Söderlund wrote:
>  On 2019-01-18 18:30:10 +0200, Laurent Pinchart wrote:
> > 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).
>  
>  I think this should be LOG(Error) and that we should not add the Camera 
>  object to the list but instead return after LOG().
>  
>  I don't think it's a good idea to try and function in a state which have 
>  a possibility of unknown behavior when we can in a safe way inform the 
>  user that something is wrong and keep the fully functional previous 
>  state. That being said, I'm not totally against your solution here and 
>  if you can think of a good use-case where it's more desirable to do so I 
>  would be OK with that.

This would require returning an error to the pipeline handler and
propagating (until where ?), while we can still function normally when
multiple cameras have the same name. Only the CameraManager::get()
function will be affected, and will return the first camera with the
requested name. That's why I think it would be simpler for pipeline
managers if the addCamera() call didn't fail. We should certainly log a
warning or error as the problem should be fixed, but it won't cause us
to fail (which is why I went for a warning originally, to denote an
issue that is recoverable but still needs to be fixed).

I expect the camera lookup API offered by the CameraManager to still
evolve, so I think this will be revisited anyway. If you strongly think
it needs to be addressed now then I'll see what can be done.

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

-- 
Regards,

Laurent Pinchart