[libcamera-devel] [PATCH v6 5/7] libcamera: camera: Validate Request befor queueing it

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2019-04-16 15:42:08 +0200, Jacopo Mondi wrote:
> Validate the Request before proceeding to prepare it at
> Camera::queueRequest() time.
> 
> For now limit the validation to making sure the Request contains at
> least one Stream to capture from.

I think you should reword the commit message, it feels it have diverged 
a bit from the change it self. I would at least drop the mentioning of 
Camera::queueRequest().

> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/camera.cpp  | 10 ++++++++--
>  src/libcamera/request.cpp | 15 ++++++++++++++-
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 2d0a80664214..0709047341d7 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -714,8 +714,14 @@ Request *Camera::createRequest()
>   * \param[in] request The request to queue to the camera
>   *
>   * This method queues a \a request allocated with createRequest() to the camera
> - * for capture. Once the request has been queued, the camera will notify its
> - * completion through the \ref requestCompleted signal.
> + * for capture.
> + *
> + * After allocating the request with createRequest(), the application shall
> + * fill it with at least one capture buffer before queuing it. Requests that
> + * contain no buffers are invalid and are rejected without being queued.
> + *
> + * Once the request has been queued, the camera will notify its completion
> + * through the \ref requestCompleted signal.
>   *
>   * Ownership of the request is transferred to the camera. It will be deleted
>   * automatically after it completes.
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 5e86c8e10128..12a3c5204f24 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -115,10 +115,23 @@ Buffer *Request::findBuffer(Stream *stream) const
>   */
>  
>  /**
> - * \brief Prepare the resources for the completion handler
> + * \brief Prepare and validate the request for the completion handler
> + *
> + * This method prepares resources and validates the request to prepare it for
> + * capture operations.

I think you can drop this sentence.

> + *
> + * Requests that contain no buffers are invalid and are rejected by this method.

s/by this method//

> + *
> + * \return 0 on success a negative error code otherwise

s/success a/success or a/

> + * \retval -EVINAL The request is invalid
>   */
>  int Request::prepare()
>  {
> +	if (!hasPendingBuffers()) {
> +		LOG(Request, Error) << "Invalid request due to missing buffers";
> +		return -EINVAL;
> +	}
> +
>  	for (auto const &pair : bufferMap_) {
>  		Buffer *buffer = pair.second;
>  		pending_.insert(buffer);
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund