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

998 diff mbox series

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@jmondi.org>
---
 src/libcamera/camera.cpp | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hello Jacopo,

Thank you for the patch.

In the subject line, s/befor/before/

On Tue, Apr 16, 2019 at 01:56:43AM +0200, Niklas Söderlund wrote:
> 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@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.

 * This method validates and queues the \a request to the camera 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 by this method 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.
> > @@ -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.
> > +	 */

Isn't the Camera class as central as it can get ? :-) I would drop the
comment.

> > +	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 would be fine with that. I would then remove the error message below,
and add an "Invalid request due to missing buffers" message to
Request::prepare().

> 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.

completed() isn't a good name, as the request doesn't complete
automatically when all buffers complete, pipeline handlers can delay
completion. How about hasPendingBuffers() ? A tad longer than I would
like, but still manageable, and I think it conveys the right meaning.

> > +
> >  	int ret = request->prepare();
> >  	if (ret) {
> >  		LOG(Camera, Error) << "Failed to prepare request";