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

1006 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  | 10 ++++++++--
 src/libcamera/request.cpp | 15 ++++++++++++++-
 2 files changed, 22 insertions(+), 3 deletions(-)

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

Hi Jacopo,

Thank you for the patch.

On Tue, Apr 16, 2019 at 09:40:54PM +0200, Niklas Söderlund wrote:
> On 2019-04-16 15:42:08 +0200, Jacopo Mondi wrote:

In the subject line, s/befor/before/

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

"Extend the Request::prepare() operation to validate the request before
preparing it. Return an error if the request is invalid, which for now
is limited to ensuring that the request contains at least one buffer."

> > Signed-off-by: Jacopo Mondi <jacopo@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

I would s/allocated with createRequest() // as it is mentioned in the
next paragraph.

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

"Validate the request and prepare it for the completion handler" as the
request is validated first and then prepared ?

> > + *
> > + * 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/

Those three comments look good to me.

> > + * \retval -EVINAL The request is invalid

s/-EVINAL/-EINVAL/

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >   */
> >  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);