[libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it extensible

[Kieran Bingham]

Hi Laurent,

On 20/04/2021 23:18, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Tue, Apr 20, 2021 at 02:07:41PM +0100, Kieran Bingham wrote:
>> Provide an extensible private object for the Request class.
>> This allows us to make modifications to the private API and storage of
>> requests without affecting the public API or ABI.
>>
>> The D pointer is obtained in all Request functions implemented in the
>> request.cpp file along with an assertion that the D pointer was valid to
>> provide extra validation checking that the Request has not been
>> deleted, while in use as it is 'owned' by the application.
> 
> s/, while in use/ while in use,/
> 
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>> ---
>> The assertions added in findBuffer, complete() and completeBuffer()
>> allow us to ensure that the Request is still valid while asynchronous
>> actions occur on the Request internally in libcamera, and provide
>> (almost) equivalent functionality as the Request Canary previously
>> proposed.
> 
> Does std::unique_ptr<> guarantees that it will reset its internal
> pointer when deleted ? libc++ calls reset() in the destructor, and
> stdlibc++ seems to set the pointer to nullptr manually, but that doesn't
> seem to be guaranteed by the C++ standard.

I've manually tested that it gets reset, but that doesn't offer any
guarantee beyond that.




>> The additions in reuse() and addBuffer() are called from applications,
>> so the assertions may be less helpful there, but I've added them for
>> consistency.
>>
>>  include/libcamera/request.h |  4 +++-
>>  src/libcamera/request.cpp   | 34 ++++++++++++++++++++++++++++++++--
>>  2 files changed, 35 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
>> index 4cf5ff3f7d3b..909a1aebc2d2 100644
>> --- a/include/libcamera/request.h
>> +++ b/include/libcamera/request.h
>> @@ -24,8 +24,10 @@ class CameraControlValidator;
>>  class FrameBuffer;
>>  class Stream;
>>  
>> -class Request
>> +class Request : public Extensible
>>  {
>> +	LIBCAMERA_DECLARE_PRIVATE(Request)
>> +
>>  public:
>>  	enum Status {
>>  		RequestPending,
>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
>> index ce2dd7b17f10..977bfac4fce6 100644
>> --- a/src/libcamera/request.cpp
>> +++ b/src/libcamera/request.cpp
>> @@ -28,6 +28,19 @@ namespace libcamera {
>>  
>>  LOG_DEFINE_CATEGORY(Request)
>>  
>> +class Request::Private : public Extensible::Private
>> +{
>> +	LIBCAMERA_DECLARE_PUBLIC(Request)
>> +
>> +public:
>> +	Private(Request *request);
>> +};
>> +
>> +Request::Private::Private(Request *request)
>> +	: Extensible::Private(request)
>> +{
>> +}
>> +
>>  /**
>>   * \enum Request::Status
>>   * Request completion status
>> @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request)
>>   *
>>   */
>>  Request::Request(Camera *camera, uint64_t cookie)
>> -	: camera_(camera), sequence_(0), cookie_(cookie),
>> -	  status_(RequestPending), cancelled_(false)
>> +	: Extensible(new Private(this)), camera_(camera), sequence_(0),
>> +	  cookie_(cookie), status_(RequestPending), cancelled_(false)
> 
> Should we move some of the data to Private (in a subsequent patch) ? As
> an exercise, how about moving the member data that we think will be
> subject to change when we'll rework the request completion API, and see
> if we manage to complete that rework without breaking the API of the
> request class ?

Of course, the goal of this patch isn't just to add assertions, it's to
make it extensible so we can extend it in subsequent patches ;-)


> A subsequent patch should also move the public functions that are not
> called by applications to the Private class.

Agreed, I wanted to keep this patch as simple as possible to start with.

> 
>>  {
>>  	/**
>>  	 * \todo Should the Camera expose a validator instance, to avoid
>> @@ -114,6 +127,9 @@ Request::~Request()
>>   */
>>  void Request::reuse(ReuseFlag flags)
>>  {
>> +	Private *const d = LIBCAMERA_D_PTR();
>> +	ASSERT(d);
>> +
>>  	LIBCAMERA_TRACEPOINT(request_reuse, this);
>>  
>>  	pending_.clear();
>> @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags)
>>   */
>>  int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
>>  {
>> +	Private *const d = LIBCAMERA_D_PTR();
>> +	ASSERT(d);
>> +
>>  	if (!stream) {
>>  		LOG(Request, Error) << "Invalid stream reference";
>>  		return -EINVAL;
>> @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
>>   */
>>  FrameBuffer *Request::findBuffer(const Stream *stream) const
>>  {
>> +	const Private *const d = LIBCAMERA_D_PTR();
>> +	ASSERT(d);
>> +
>>  	const auto it = bufferMap_.find(stream);
>>  	if (it == bufferMap_.end())
>>  		return nullptr;
>> @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
>>   */
>>  void Request::complete()
>>  {
>> +	Private *const d = LIBCAMERA_D_PTR();
>> +	ASSERT(d);
>>  	ASSERT(status_ == RequestPending);
>>  	ASSERT(!hasPendingBuffers());
>>  
>> @@ -307,6 +331,9 @@ void Request::complete()
>>   */
>>  bool Request::completeBuffer(FrameBuffer *buffer)
>>  {
>> +	Private *const d = LIBCAMERA_D_PTR();
>> +	ASSERT(d);
>> +
>>  	LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);
>>  
>>  	int ret = pending_.erase(buffer);
>> @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer)
>>   */
>>  std::string Request::toString() const
>>  {
>> +	const Private *const d = LIBCAMERA_D_PTR();
>> +	ASSERT(d);
>> +
>>  	std::stringstream ss;
>>  
>>  	/* Pending, Completed, Cancelled(X). */
> 

-- 
Regards
--
Kieran