[libcamera-devel] [PATCH v2 08/14] libcamera: request: Add RequestData

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Aug 30, 2019 at 01:26:47AM +0200, Niklas Söderlund wrote:
> Add a RequestData pointer to the Request class, this is intended to be
> used by pipeline handlers while handling the request and is invalid
> outside the pipeline handler context.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/request.h |  5 +++++
>  src/libcamera/request.cpp   | 10 +++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 9edf1cedc054014f..570924c5ef5e2425 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -21,6 +21,9 @@ class Buffer;
>  class Camera;
>  class Stream;
>  
> +class RequestData
> +{
> +};

A forward declaration is enough here, let's not expose this class to
applications.

>  
>  class Request
>  {
> @@ -46,6 +49,8 @@ public:
>  
>  	bool hasPendingBuffers() const { return !pending_.empty(); }
>  
> +	RequestData *data;

As a public member ? No way :-)

This will create issues to access the data pointer from pipeline
handlers though, so I wonder if you wouldn't let pipeline handlers
subclass Request instead, given that all requests are created by
Camera::createRequest() that could easily be forwarded to pipeline
handlers (with a default implementation in the base PipelineHandler
class that creates a Request). Bonus points if you can make the Request
constructor private.

Feel free to propose an alternative scheme if you think that the Request
class should be final.

> +
>  private:
>  	friend class Camera;
>  	friend class PipelineHandler;
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index ee2158fc7a9cf0b9..b8b4dfb4b7549163 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -55,7 +55,7 @@ LOG_DEFINE_CATEGORY(Request)
>   *
>   */
>  Request::Request(Camera *camera, uint64_t cookie)
> -	: camera_(camera), controls_(camera), cookie_(cookie),
> +	: data(nullptr), camera_(camera), controls_(camera), cookie_(cookie),
>  	  status_(RequestPending), cancelled_(false)
>  {
>  }
> @@ -178,6 +178,14 @@ Buffer *Request::findBuffer(Stream *stream) const
>   * otherwise
>   */
>  
> +/**
> + * \var Request::data
> + * \brief Pipeline handler specific data

Maybe "Pipeline handler-specific request data" ?

> + *
> + * Pipeline handlers may associate private data with with an request that

s/with with/with/
s/an request/a request/

> + * is valid for the requests lifetime inside the pipeline handler.

s/requests/request's/

This isn't very clear. I would explicitly state when this member becomes
and stops being valid, or rather when the pipeline handler can touch it.
I would also mention that nothing else is allowed to touch this.

> + */
> +
>  /**
>   * \brief Validate the request and prepare it for the completion handler
>   *

-- 
Regards,

Laurent Pinchart