[libcamera-devel] [PATCH v7 6/8] libcamera: camera: Validate Request before queueing it

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 17, 2019 at 06:13:22PM +0200, Jacopo Mondi wrote:
> On Wed, Apr 17, 2019 at 03:58:56PM +0200, Jacopo Mondi wrote:
> > 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 at jmondi.org>
> > ---
> >  src/libcamera/camera.cpp  | 11 ++++++++---
> >  src/libcamera/request.cpp | 12 +++++++++++-
> >  2 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 2d0a80664214..75a21008070b 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -713,9 +713,14 @@ Request *Camera::createRequest()
> >   * \brief Queue a request to the camera
> >   * \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.
> > + * This method queues a \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 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 e5c25d2c6988..9fc8c6845578 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -115,10 +115,20 @@ Buffer *Request::findBuffer(Stream *stream) const
> >   */
> >
> >  /**
> > - * \brief Prepare the resources for the completion handler
> > + * \brief Validate the request and prepare it for the completion handler
> > + *
> > + * Requests that contain no buffers are invalid and are rejected.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EINVAL The request is invalid
> >   */
> >  int Request::prepare()
> >  {
> > +	if (!hasPendingBuffers()) {
> > +		LOG(Request, Error) << "Invalid request due to missing buffers";
> > +		return -EINVAL;
> > +	}
> > +
> 
> Oh, this is so stupid!
> hasPendingBuffers() checks for pending_ to be empty, and pending_ is
> here below populated :(
> 
> Sorry about this, I moved it up to skip the below loop if no buffers
> were available, not a great idea it seems!

Let's keep it below (or check bufferMap_.empty() instead, that may be
even better), and you'll have my

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

:-)

> >  	for (auto const &pair : bufferMap_) {
> >  		Buffer *buffer = pair.second;
> >  		pending_.insert(buffer);

-- 
Regards,

Laurent Pinchart