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

[Jean-Michel Hautbois]

Hi Kieran,

Thanks for the patch !

On 20/04/2021 15:07, 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.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois 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.
> 
> 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)
>  {
>  	/**
>  	 * \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). */
>