| Message ID | 20211209092906.37303-3-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Tomi Valkeinen (2021-12-09 09:29:03) > Request has Camera as a private member. Expose this so that users can > more easily associate a received Request to a Camera. > Aha, I created a very similar patch in the past, but only got one ack so it didn't get in: https://patchwork.libcamera.org/patch/9328/ That one only returned a direct pointer to the Camera, and I think given it's the public API, returning a shared_ptr is safer, as it keeps the camera alive while the application still has a reference. Thinking futher to really horrible corner cases, if we're shutting down perhaps - the Camera might be destructed while the Request is still available. In that instance I think the Camera would then be invalid. It would be helpful if we could somehow make sure we invalidate the pointers in the Request when the camera is destroyed. (I think once created, the ownership of the Request goes to the application, so they could be 'kept' in the application after the destruction of the Camera... of course that's really corner cases as it would certainly be invalid to access Requests after shutting down the CameraManager or such). Anyway, I think this is probably better than mine for the shared_ptr reasoning so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > include/libcamera/request.h | 2 ++ > src/libcamera/request.cpp | 5 +++++ > 2 files changed, 7 insertions(+) > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index f434335b..6e8987b6 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -60,6 +60,8 @@ public: > > std::string toString() const; > > + std::shared_ptr<Camera> camera() const; > + > private: > LIBCAMERA_DISABLE_COPY(Request) > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index 17fefab7..3651e8ca 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -218,6 +218,11 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const > return it->second; > } > > +std::shared_ptr<Camera> Request::camera() const > +{ > + return camera_->shared_from_this(); > +} > + > /** > * \fn Request::metadata() > * \brief Retrieve the request's metadata > -- > 2.25.1 >
Hi Tomi, Thank you for the patch. On Thu, Dec 09, 2021 at 11:29:03AM +0200, Tomi Valkeinen wrote: > Request has Camera as a private member. Expose this so that users can > more easily associate a received Request to a Camera. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > include/libcamera/request.h | 2 ++ > src/libcamera/request.cpp | 5 +++++ > 2 files changed, 7 insertions(+) > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index f434335b..6e8987b6 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -60,6 +60,8 @@ public: > > std::string toString() const; > > + std::shared_ptr<Camera> camera() const; I'm not thrilled by this, as it would prevent us from removing the camera pointer stored in the Request class, which I've been tempted to do already. Why do we need this, and do we have any alternative ? > + > private: > LIBCAMERA_DISABLE_COPY(Request) > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index 17fefab7..3651e8ca 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -218,6 +218,11 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const > return it->second; > } > > +std::shared_ptr<Camera> Request::camera() const > +{ > + return camera_->shared_from_this(); > +} > + > /** > * \fn Request::metadata() > * \brief Retrieve the request's metadata
On 09/12/2021 18:50, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Thu, Dec 09, 2021 at 11:29:03AM +0200, Tomi Valkeinen wrote: >> Request has Camera as a private member. Expose this so that users can >> more easily associate a received Request to a Camera. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> include/libcamera/request.h | 2 ++ >> src/libcamera/request.cpp | 5 +++++ >> 2 files changed, 7 insertions(+) >> >> diff --git a/include/libcamera/request.h b/include/libcamera/request.h >> index f434335b..6e8987b6 100644 >> --- a/include/libcamera/request.h >> +++ b/include/libcamera/request.h >> @@ -60,6 +60,8 @@ public: >> >> std::string toString() const; >> >> + std::shared_ptr<Camera> camera() const; > > I'm not thrilled by this, as it would prevent us from removing the > camera pointer stored in the Request class, which I've been tempted to > do already. > > Why do we need this, and do we have any alternative ? I use it to associate the completed Request with a camera. But I guess the same can be done with the cookie with a few more lines. Tomi
Hi Tomi, On Fri, Dec 10, 2021 at 01:42:17PM +0200, Tomi Valkeinen wrote: > On 09/12/2021 18:50, Laurent Pinchart wrote: > > On Thu, Dec 09, 2021 at 11:29:03AM +0200, Tomi Valkeinen wrote: > >> Request has Camera as a private member. Expose this so that users can > >> more easily associate a received Request to a Camera. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >> --- > >> include/libcamera/request.h | 2 ++ > >> src/libcamera/request.cpp | 5 +++++ > >> 2 files changed, 7 insertions(+) > >> > >> diff --git a/include/libcamera/request.h b/include/libcamera/request.h > >> index f434335b..6e8987b6 100644 > >> --- a/include/libcamera/request.h > >> +++ b/include/libcamera/request.h > >> @@ -60,6 +60,8 @@ public: > >> > >> std::string toString() const; > >> > >> + std::shared_ptr<Camera> camera() const; > > > > I'm not thrilled by this, as it would prevent us from removing the > > camera pointer stored in the Request class, which I've been tempted to > > do already. > > > > Why do we need this, and do we have any alternative ? > > I use it to associate the completed Request with a camera. But I guess > the same can be done with the cookie with a few more lines. I've also commented in the review of the bindings that we could move the completion handling mechanism from the CameraManager to the Camera class, maybe that would also help avoiding the need to expose this function ?
On 10/12/2021 13:42, Tomi Valkeinen wrote: > On 09/12/2021 18:50, Laurent Pinchart wrote: >> Hi Tomi, >> >> Thank you for the patch. >> >> On Thu, Dec 09, 2021 at 11:29:03AM +0200, Tomi Valkeinen wrote: >>> Request has Camera as a private member. Expose this so that users can >>> more easily associate a received Request to a Camera. >>> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>> --- >>> include/libcamera/request.h | 2 ++ >>> src/libcamera/request.cpp | 5 +++++ >>> 2 files changed, 7 insertions(+) >>> >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h >>> index f434335b..6e8987b6 100644 >>> --- a/include/libcamera/request.h >>> +++ b/include/libcamera/request.h >>> @@ -60,6 +60,8 @@ public: >>> std::string toString() const; >>> + std::shared_ptr<Camera> camera() const; >> >> I'm not thrilled by this, as it would prevent us from removing the >> camera pointer stored in the Request class, which I've been tempted to >> do already. >> >> Why do we need this, and do we have any alternative ? > > I use it to associate the completed Request with a camera. But I guess > the same can be done with the cookie with a few more lines. Oh, there's another use. I have Request.set_control(), which takes a control name (string) and a value. I need the camera instance to find the control IDs. That's currently the only way to set controls. I need to figure out some other way to handle it. Tomi
diff --git a/include/libcamera/request.h b/include/libcamera/request.h index f434335b..6e8987b6 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -60,6 +60,8 @@ public: std::string toString() const; + std::shared_ptr<Camera> camera() const; + private: LIBCAMERA_DISABLE_COPY(Request) diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index 17fefab7..3651e8ca 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -218,6 +218,11 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const return it->second; } +std::shared_ptr<Camera> Request::camera() const +{ + return camera_->shared_from_this(); +} + /** * \fn Request::metadata() * \brief Retrieve the request's metadata
Request has Camera as a private member. Expose this so that users can more easily associate a received Request to a Camera. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- include/libcamera/request.h | 2 ++ src/libcamera/request.cpp | 5 +++++ 2 files changed, 7 insertions(+)