[libcamera-devel] [PATCH] libcamera: CameraManager, PipelineHandler: Allow multiple devnums per camera

[Laurent Pinchart]

Hi Paul,

On Sat, Jun 06, 2020 at 04:48:34PM +0900, Paul Elder wrote:
> Hi Laurent,
> 
> Thank you for the review.
> 
> On Fri, Jun 05, 2020 at 10:02:37PM +0300, Laurent Pinchart wrote:
> > Hi Paul,
> > 
> > Thank you for the patch.
> > 
> 
> <snip>
> 
> > >  /**
> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > > index 53aeebdc..b3824a5f 100644
> > > --- a/src/libcamera/pipeline_handler.cpp
> > > +++ b/src/libcamera/pipeline_handler.cpp
> > > @@ -502,7 +502,17 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
> > >  	data->camera_ = camera.get();
> > >  	cameraData_[camera.get()] = std::move(data);
> > >  	cameras_.push_back(camera);
> > > -	manager_->addCamera(std::move(camera), devnum);
> > > +
> > > +	std::vector<dev_t> devnums;
> > > +	if (devnum != 0)
> > > +		devnums.push_back(devnum);
> > 
> > Should we allow this, or always use all the capture video nodes ?
> 
> It doesn't seem to me that applications are very likely to use
> CameraManager::get(devnum) to get cameras, and only those that deal with
> V4L2 to a certain extent (eg. the V4L2 compatibility layer), will. In
> that sense, I think that it won't be bad to disallow declaring a devnum
> and always map the devnums automatically like below. In addition, having
> all the capture video nodes mapped to a camera would be prevent
> confusion on the rules surrounding which "video node is mapped to which
> camera when using the V4L2 compatibility layer".
> 
> On the other hand, I think it's good to still provide the ability to
> pipeline handlers to declare their own mapping if they want to. The
> default (ie. less work) is the auto-mapping anyway. This might cause
> confusion to users of the V4L2 compatilibity layer, if they expect that
> all capture nodes can be used when the pipeline handler has disagreed.
> 
> So it's ease of use for the users of the V4L2 compatiblity layer, or
> freedom to pipeline handlers. Maybe user-friendliness is more important,
> and the pipeline handlers don't care to declare devnum mappings. I think
> I'll go with the former then, until there are objections.

Sounds good to me. We may reconsider this later when (if) we'll have
pipelines supporting multiple cameras concurrently, but for now I think
it's totally fine.

> > > +	else
> > > +		for (const std::shared_ptr<MediaDevice> media : mediaDevices_)
> > > +			for (const MediaEntity *entity : media->entities())
> > > +				devnums.push_back(makedev(entity->deviceMajor(),
> > > +							  entity->deviceMinor()));
> > 
> > You need to limit entities to the capture video nodes. You can handle
> > that through a combination of entity flags (to check if it's a video
> > node) and pad flags (to check if it's a capture video node, by looking
> > at the direction of the pad).
> > 
> > A short comment to explain what's going on would be useful.
> 
> ack
> 
> > > +
> > > +	manager_->addCamera(std::move(camera), devnums);
> > >  }
> > >  
> > >  /**

-- 
Regards,

Laurent Pinchart