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

[Laurent Pinchart]

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 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.

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

-- 
Regards,

Laurent Pinchart