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

[Kieran Bingham]

Hi Laurent,

On 23/06/2019 23:31, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> 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'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 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).


>  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();


-- 
Regards
--
Kieran