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

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2019-04-16 01:18:57 +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.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/camera.cpp | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 21caa24b90b5..e93e7b3b8477 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -715,8 +715,9 @@ 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 and validates it by making sure it contains at least one stream
> + * where to capture from. 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.
> @@ -724,6 +725,7 @@ Request *Camera::createRequest()
>   * \return 0 on success or a negative error code otherwise
>   * \retval -ENODEV The camera has been disconnected from the system
>   * \retval -EACCES The camera is not running so requests can't be queued
> + * \retval -EINVAL The request is not valid
>   */
>  int Camera::queueRequest(Request *request)
>  {
> @@ -733,6 +735,15 @@ int Camera::queueRequest(Request *request)
>  	if (!stateIs(CameraRunning))
>  		return -EACCES;
>  
> +	/*
> +	 * \todo: Centralize validation if it gets more complex and update
> +	 * the documentation.
> +	 */
> +	if (request->empty()) {
> +		LOG(Camera, Error) << "Invalid request";
> +		return -EINVAL;
> +	}

I know this is still debated and there are 3 different options on the 
topic ;-)

I still think this should be moved to Request::prepare(), as it can't be 
a valid operation to prepare a request for capture if it contains no 
buffers.

I also know the name is under debate and empty() is a work in progress.  
Looking at what empty() actually do maybe name isWaiting(), isPending(),
havePending() or something indicating that it checks if the request have 
any pending buffers. completed() might even be an option.

> +
>  	int ret = request->prepare();
>  	if (ret) {
>  		LOG(Camera, Error) << "Failed to prepare request";
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund