[libcamera-devel] [RFC 0/2] Add support for pipeline specific data to Cameras

[Niklas Söderlund]

Hi Laurent,

On 2019-01-23 12:09:22 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Wed, Jan 23, 2019 at 01:07:58AM +0100, Niklas Söderlund wrote:
> > On 2019-01-22 19:12:23 +0100, Jacopo Mondi wrote:
> > > RFC as I'm not sure about the idea of delegating ownership of platform data
> > > to Cameras, as that requires the pipeline handler to dynamically allocate the
> > > resources (if they go away at pipeline handler destruction time, is pointless
> > > to store them in Camera).
> > > 
> > > The other way around is the idea of borrowing pipeline handler data to Cameras,
> > > but as Cameras are shared objects, they might stay around longer that pipeline
> > > handlers, and thus I felt it is safer to tie the CameraData lifetime to the one
> > > of the Camera instance they're associated to.
> > 
> > While working with the Camera API and it's interaction with the pipeline 
> > handler I felt a need for something similar you have here. My initial 
> > idea to avoid lifetime management issues which you deal with in this 
> > series was for the PipelineHandler to be able to set a unsigned int as a 
> > cookie on the Camera at creation time, which it could retrieve with it's 
> > later interaction with a Camera object.
> > 
> > In the end I went a different way, as it became clear we would need an 
> > two way association between the Camera and the PipelineHandler anyhow.  
> > That is the PipelineHandler would need to keep a reference to the 
> > Camera, so I ended up using the pointer to the Camera object as the 
> > cookie as this like a numerical cookie avoids all lifetime issues. That 
> > way whenever a PipelineHandler is handed a Camera object it can use the 
> > pointer to
> > 
> > 1. Validate that the Camera object in question really is its 
> >    responsibility to handle.
> 
> Given that all calls to the pipeline handler will go through the Camera
> object, I think any such validation should be done there to avoid
> duplicating code in pipeline handlers. Furthermore, as the Camera object
> will need to store a pointer to the pipeline handler in order to
> firmward the calls, I don't see how we could get it wrong :-)

That is a good point indeed, maybe we can drop the validation for now 
then :-) I feel a bit stupid not thinking about it...

> 
> > 2. Use the pointer as a cookie to look-up any camera specific data in 
> >    its private data structures.
> 
> This would require all pipeline handlers to implement the equivalent of
> a std::map<Camera *, PrivateData *>. Isn't Jacopo's approach simpler as
> it moves this to a pointer in the Camera object itself, avoiding lookup
> code duplication in the pipeline handlers.

One option is to create a common implementation in the PipelineHandler 
base class to remove the code duplication. I'm not against Jacopos 
suggestion categorically, I only feel the same thing could be achieved 
with less complexity. Or maybe my fear of object lifetime issues is 
bordering on the insane.

> 
> > I think that is quiet neat as it uses infrastructure we need anyhow to 
> > properly manage the interactions between the two objects.
> > 
> > > Jacopo Mondi (2):
> > >   libcamera: camera: Add CameraData
> > >   libcamera: ipu3: Create CIO2 V4L2 devices
> > > 
> > >  include/libcamera/camera.h           | 13 ++++++++
> > >  src/libcamera/camera.cpp             | 50 ++++++++++++++++++++++++++++
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 42 +++++++++++++++++++++++
> > >  3 files changed, 105 insertions(+)
> > > 
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund