| Message ID | 20190122181225.12922-1-jacopo@jmondi.org |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. 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. 2. Use the pointer as a cookie to look-up any camera specific data in its private data structures. I think that is quiet neat as it uses infrastructure we need anyhow to properly manage the interactions between the two objects. > > Thanks > j > > 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(+) > > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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 :-) > 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. > 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(+) > >
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
Hi Niklas, On Wed, Jan 23, 2019 at 11:31:01AM +0100, Niklas Söderlund wrote: > On 2019-01-23 12:09:22 +0200, Laurent Pinchart wrote: > > 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 haven't thought about that, it's a good point. > 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'll let you discuss the pros and cons with Jacopo :-) It could possibly help to sketch how this would look like in the IPU3 pipeline handler (with the helper method in the PipelineHandler class) to verify that lifetime management is sound, and compare the two options. > >> 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(+)