[libcamera-devel] [RFC PATCH v2 6/9] libcamera: request: Add a ControlList

[Laurent Pinchart]

Hi Kieran,

On Wed, Jun 26, 2019 at 12:30:21PM +0100, Kieran Bingham wrote:
> On 23/06/2019 23:31, Laurent Pinchart wrote:
> > On Sun, Jun 23, 2019 at 05:36:52PM +0200, Niklas Söderlund wrote:
> >> On 2019-06-21 17:13:58 +0100, Kieran Bingham wrote:
> >>> Provide a ControlList on request objects to facilitate setting controls
> >>>
> >>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>> ---
> >>>  include/libcamera/request.h |  3 +++
> >>>  src/libcamera/request.cpp   | 10 ++++++++++
> >>>  2 files changed, 13 insertions(+)
> >>>
> >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> >>> index 58de6f00a554..8075270a9a12 100644
> >>> --- a/include/libcamera/request.h
> >>> +++ b/include/libcamera/request.h
> >>> @@ -11,6 +11,7 @@
> >>>  #include <unordered_set>
> >>>  
> >>>  #include <libcamera/signal.h>
> >>> +#include <libcamera/controls.h>
> >>
> >> Alphabetical order please.
> >>
> >> With this fixed,
> >>
> >> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >>
> >>>  
> >>>  namespace libcamera {
> >>>  
> >>> @@ -32,6 +33,7 @@ public:
> >>>  	Request(const Request &) = delete;
> >>>  	Request &operator=(const Request &) = delete;
> >>>  
> >>> +	ControlList &controls() { return controls_; }
> >>>  	const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }
> >>>  	int setBuffers(const std::map<Stream *, Buffer *> &streamMap);
> >>>  	Buffer *findBuffer(Stream *stream) const;
> >>> @@ -50,6 +52,7 @@ private:
> >>>  	bool completeBuffer(Buffer *buffer);
> >>>  
> >>>  	Camera *camera_;
> >>> +	ControlList controls_;
> >>>  	std::map<Stream *, Buffer *> bufferMap_;
> >>>  	std::unordered_set<Buffer *> pending_;
> >>>  
> >>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> >>> index fa3ee46da440..9a0409b46017 100644
> >>> --- a/src/libcamera/request.cpp
> >>> +++ b/src/libcamera/request.cpp
> >>> @@ -52,6 +52,16 @@ Request::Request(Camera *camera)
> >>>  {
> >>>  }
> >>>  
> >>> +/**
> >>> + * \fn Request::controls()
> >>> + * \brief Retrieve the request's ControlList
> >>> + *
> >>> + * Return a reference to the ControlList that stores all the controls relevant
> >>> + * to this request.
> >>> + *
> >>> + * \return A reference to the ControlList in this request
> >>> + */
> > 
> > We need more documentation, with a focus on how this API should be used
> > by applications.
> 
> Sure. RFC==WIP right? Maybe I am using RFC too strongly?

I wasn't blaming you, just pointing out a missing piece to make sure it
wouldn't be forgotten :-) But for me RFC signals a more advanced state
than WIP.

> I'm trying to get the underlying implementation right before fully
> documenting (and re-documenting for every change)
> 
> This exposes the list, so I think is a good place for documenting how
> the Controls should be added to the request.

I agree.

> I wonder if we're going to need some more 'higher level' documentation
> somewhere describing how to write a libcamera application in the first
> place... (as well, I'm not saying instead of this).

Yes, I think we will.

> >  I also expect that you will need to reset the controls
> > once the request complete, as request objects can be reused.
> 
> I don't think requests can be reused currently:
> 
> /**
>  * \brief Handle request completion and notify application
>  * \param[in] request The request that has completed
>  *
>  * This function is called by the pipeline handler to notify the camera that
>  * the request has completed. It emits the requestCompleted signal and deletes
>  * the request.
>  */
> void Camera::requestComplete(Request *request)
> {
> 	std::map<Stream *, Buffer *> buffers(std::move(request->bufferMap_));
> 	requestCompleted.emit(request, buffers);
> 	delete request;
> }
> 
> 
> If that changes, then probably yes, the ControlList should be cleared at
> some point during requestComplete();

You're right, my bad.

-- 
Regards,

Laurent Pinchart