| Message ID | 20201127133738.880859-3-tomi.valkeinen@iki.fi |
|---|---|
| State | RFC |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi, On 27/11/2020 13:37, Tomi Valkeinen wrote: > Py bindings need to find out which camera is the source of the completed > Request. Request already has a private field for the Camera, so we can > just expose it via a getter. Interestingly, some time ago I posted a similar (but simpler?) patch for this. [PATCH] libcamera: request: Facilitate retrieval of the camera https://lists.libcamera.org/pipermail/libcamera-devel/2020-August/012064.html Which was partially rejected by Niklas, however he did give me an RB tag. I sort of thought this was beneficial as it helps get directly to the 'correct' Camera in any callbacks. Interesting that you've used a shared_ptr rather than where I directly pass the pointer, and I think that's likely more correct as the expected use might be to take the event in a callback, and immediately pass it on to another thread - so a shared pointer makes a bit more sense then to keep things safe. > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > include/libcamera/request.h | 1 + > src/libcamera/request.cpp | 5 +++++ > 2 files changed, 6 insertions(+) > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index 655b1324..f98ef767 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -56,6 +56,7 @@ public: > > bool hasPendingBuffers() const { return !pending_.empty(); } > > + std::shared_ptr<Camera> camera() const; > private: > friend class PipelineHandler; > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index a68684ef..5a50ec6b 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -216,6 +216,11 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const > return it->second; > } > Perhaps just add this ;-) (stolen/adapted from my patch) +/** + * \fn Request::camera() + * \brief Retrieve the camera that owns the request + * + * \return A shared pointer to the camera associated with the request + */ With a version with that, I'd give this a: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> But given previous push-back, I'll hope for further acceptance from the others before we consider dropping the 'HACK:' prefix ;-) -- Kieran > +std::shared_ptr<Camera> Request::camera() const > +{ > + return camera_->shared_from_this(); > +} > + > /** > * \fn Request::metadata() > * \brief Retrieve the request's metadata >
Hi Kieran and Tomi, On Sun, Nov 29, 2020 at 09:08:28PM +0000, Kieran Bingham wrote: > On 27/11/2020 13:37, Tomi Valkeinen wrote: > > Py bindings need to find out which camera is the source of the completed > > Request. Request already has a private field for the Camera, so we can > > just expose it via a getter. Note that the camera_ member is actually not used. We should either use it, or remove it :-) > Interestingly, some time ago I posted a similar (but simpler?) patch for > this. > > [PATCH] libcamera: request: Facilitate retrieval of the camera > > https://lists.libcamera.org/pipermail/libcamera-devel/2020-August/012064.html > > Which was partially rejected by Niklas, however he did give me an RB tag. > > I sort of thought this was beneficial as it helps get directly to the > 'correct' Camera in any callbacks. > > Interesting that you've used a shared_ptr rather than where I directly > pass the pointer, and I think that's likely more correct as the expected > use might be to take the event in a callback, and immediately pass it on > to another thread - so a shared pointer makes a bit more sense then to > keep things safe. I'm not entirely sure about that. shared_ptr<> has a tendency to spread, and sometimes borrowed references are enough. In this particular case, requests should all complete before the application receives the camera disconnection signal. Borrowed references to the camera in the completion handler should thus be valid. Request processing is then typically deferred to the main thread, but for cancelled requests, a different code path can be used. I'll review the patch that makes use of this to see what's best. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > > --- > > include/libcamera/request.h | 1 + > > src/libcamera/request.cpp | 5 +++++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > index 655b1324..f98ef767 100644 > > --- a/include/libcamera/request.h > > +++ b/include/libcamera/request.h > > @@ -56,6 +56,7 @@ public: > > > > bool hasPendingBuffers() const { return !pending_.empty(); } > > > > + std::shared_ptr<Camera> camera() const; Doesn't checkstyle.py warn about spaces uses for indentation ? There's also a missing blank line. > > private: > > friend class PipelineHandler; > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > index a68684ef..5a50ec6b 100644 > > --- a/src/libcamera/request.cpp > > +++ b/src/libcamera/request.cpp > > @@ -216,6 +216,11 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const > > return it->second; > > } > > > > Perhaps just add this ;-) (stolen/adapted from my patch) > > +/** > + * \fn Request::camera() > + * \brief Retrieve the camera that owns the request > + * > + * \return A shared pointer to the camera associated with the request > + */ > > With a version with that, I'd give this a: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > But given previous push-back, I'll hope for further acceptance from the > others before we consider dropping the 'HACK:' prefix ;-) > > > +std::shared_ptr<Camera> Request::camera() const > > +{ > > + return camera_->shared_from_this(); > > +} > > + > > /** > > * \fn Request::metadata() > > * \brief Retrieve the request's metadata
On 30/11/2020 01:42, Laurent Pinchart wrote: > Hi Kieran and Tomi, > > On Sun, Nov 29, 2020 at 09:08:28PM +0000, Kieran Bingham wrote: >> On 27/11/2020 13:37, Tomi Valkeinen wrote: >>> Py bindings need to find out which camera is the source of the completed >>> Request. Request already has a private field for the Camera, so we can >>> just expose it via a getter. > > Note that the camera_ member is actually not used. We should either use > it, or remove it :-) > >> Interestingly, some time ago I posted a similar (but simpler?) patch for >> this. >> >> [PATCH] libcamera: request: Facilitate retrieval of the camera >> >> https://lists.libcamera.org/pipermail/libcamera-devel/2020-August/012064.html >> >> Which was partially rejected by Niklas, however he did give me an RB tag. >> >> I sort of thought this was beneficial as it helps get directly to the >> 'correct' Camera in any callbacks. >> >> Interesting that you've used a shared_ptr rather than where I directly >> pass the pointer, and I think that's likely more correct as the expected >> use might be to take the event in a callback, and immediately pass it on >> to another thread - so a shared pointer makes a bit more sense then to >> keep things safe. > > I'm not entirely sure about that. shared_ptr<> has a tendency to spread, > and sometimes borrowed references are enough. In this particular case, > requests should all complete before the application receives the camera > disconnection signal. Borrowed references to the camera in the > completion handler should thus be valid. Request processing is then > typically deferred to the main thread, but for cancelled requests, a > different code path can be used. We can return Camera* here, and I can convert it to a shared_ptr in the binding code. I don't know what you mean with "has tendency to spread". The camera is allocated as shared_ptr, so generally speaking, it should always be shared_ptr<Camera>. In some cases we could pass it as Camera* as an optimization, e.g. for a function that we know only does a thing X, and does not store the Camera pointer anywhere. Storing a Camera* is not correct in my opinion, although it can of course work if the lifetime or that pointer is well defined and enforced. But it's pretty easy to get that wrong, and it should be considered if such optimization is worth it (maybe it is here with Requests). But when talking about the python bindings, a Camera* is not ok, we can't pass such a thing to python. Either python owns the instance (unique_ptr, which we can create from a pointer) or it doesn't (shared_ptr). Well, we can also build more elaborate ownership containers, but the rules must be clear and defined in the container code. In this case, as Camera is a shared_ptr, and we store the Camera to a list which is processed later, I think it's clear that we should use shared_ptr (either as I do in this patch, or convert Camera* to shared_ptr<Camera> in the binding code). > I'll review the patch that makes use of this to see what's best. > >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >>> --- >>> include/libcamera/request.h | 1 + >>> src/libcamera/request.cpp | 5 +++++ >>> 2 files changed, 6 insertions(+) >>> >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h >>> index 655b1324..f98ef767 100644 >>> --- a/include/libcamera/request.h >>> +++ b/include/libcamera/request.h >>> @@ -56,6 +56,7 @@ public: >>> >>> bool hasPendingBuffers() const { return !pending_.empty(); } >>> >>> + std::shared_ptr<Camera> camera() const; > > Doesn't checkstyle.py warn about spaces uses for indentation ? There's > also a missing blank line. It does, I just didn't run it... Not sure how I managed to indent with spaces, though. Tomi
Hi Tomi, On Mon, Nov 30, 2020 at 02:48:11PM +0200, Tomi Valkeinen wrote: > On 30/11/2020 01:42, Laurent Pinchart wrote: > > On Sun, Nov 29, 2020 at 09:08:28PM +0000, Kieran Bingham wrote: > >> On 27/11/2020 13:37, Tomi Valkeinen wrote: > >>> Py bindings need to find out which camera is the source of the completed > >>> Request. Request already has a private field for the Camera, so we can > >>> just expose it via a getter. > > > > Note that the camera_ member is actually not used. We should either use > > it, or remove it :-) > > > >> Interestingly, some time ago I posted a similar (but simpler?) patch for > >> this. > >> > >> [PATCH] libcamera: request: Facilitate retrieval of the camera > >> > >> https://lists.libcamera.org/pipermail/libcamera-devel/2020-August/012064.html > >> > >> Which was partially rejected by Niklas, however he did give me an RB tag. > >> > >> I sort of thought this was beneficial as it helps get directly to the > >> 'correct' Camera in any callbacks. > >> > >> Interesting that you've used a shared_ptr rather than where I directly > >> pass the pointer, and I think that's likely more correct as the expected > >> use might be to take the event in a callback, and immediately pass it on > >> to another thread - so a shared pointer makes a bit more sense then to > >> keep things safe. > > > > I'm not entirely sure about that. shared_ptr<> has a tendency to spread, > > and sometimes borrowed references are enough. In this particular case, > > requests should all complete before the application receives the camera > > disconnection signal. Borrowed references to the camera in the > > completion handler should thus be valid. Request processing is then > > typically deferred to the main thread, but for cancelled requests, a > > different code path can be used. > > We can return Camera* here, and I can convert it to a shared_ptr in the binding code. > > I don't know what you mean with "has tendency to spread". The camera is allocated as shared_ptr, so > generally speaking, it should always be shared_ptr<Camera>. > > In some cases we could pass it as Camera* as an optimization, e.g. for a function that we know only > does a thing X, and does not store the Camera pointer anywhere. Storing a Camera* is not correct in > my opinion, although it can of course work if the lifetime or that pointer is well defined and > enforced. But it's pretty easy to get that wrong, and it should be considered if such optimization > is worth it (maybe it is here with Requests). > > But when talking about the python bindings, a Camera* is not ok, we can't pass such a thing to > python. Either python owns the instance (unique_ptr, which we can create from a pointer) or it > doesn't (shared_ptr). Well, we can also build more elaborate ownership containers, but the rules > must be clear and defined in the container code. > > In this case, as Camera is a shared_ptr, and we store the Camera to a list which is processed later, > I think it's clear that we should use shared_ptr (either as I do in this patch, or convert Camera* > to shared_ptr<Camera> in the binding code). All these are very good points. I'd propose revisiting this once we move forward some more with the bindings. > > I'll review the patch that makes use of this to see what's best. > > > >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > >>> --- > >>> include/libcamera/request.h | 1 + > >>> src/libcamera/request.cpp | 5 +++++ > >>> 2 files changed, 6 insertions(+) > >>> > >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h > >>> index 655b1324..f98ef767 100644 > >>> --- a/include/libcamera/request.h > >>> +++ b/include/libcamera/request.h > >>> @@ -56,6 +56,7 @@ public: > >>> > >>> bool hasPendingBuffers() const { return !pending_.empty(); } > >>> > >>> + std::shared_ptr<Camera> camera() const; > > > > Doesn't checkstyle.py warn about spaces uses for indentation ? There's > > also a missing blank line. > > It does, I just didn't run it... Not sure how I managed to indent with spaces, though. cp utils/hooks/post-commit .git/hooks/ :-)
diff --git a/include/libcamera/request.h b/include/libcamera/request.h index 655b1324..f98ef767 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -56,6 +56,7 @@ public: bool hasPendingBuffers() const { return !pending_.empty(); } + std::shared_ptr<Camera> camera() const; private: friend class PipelineHandler; diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index a68684ef..5a50ec6b 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -216,6 +216,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
Py bindings need to find out which camera is the source of the completed Request. Request already has a private field for the Camera, so we can just expose it via a getter. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- include/libcamera/request.h | 1 + src/libcamera/request.cpp | 5 +++++ 2 files changed, 6 insertions(+)