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

[Niklas Söderlund]

Hi Laurent,

On 2019-01-19 00:57:50 +0200, Laurent Pinchart wrote:
> 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.

I'm fine with moving forward with the current solution until we get the 
CameraManager API in a more stable state.

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

-- 
Regards,
Niklas Söderlund