[libcamera-devel] [RFC v2 2/4] HACK: expose Camera* from Request

[Laurent Pinchart]

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 at 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/

:-)

-- 
Regards,

Laurent Pinchart