| Message ID | 20190417135858.23914-7-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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! > for (auto const &pair : bufferMap_) { > Buffer *buffer = pair.second; > pending_.insert(buffer); > -- > 2.21.0 >
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@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@ideasonboard.com> :-) > > for (auto const &pair : bufferMap_) { > > Buffer *buffer = pair.second; > > pending_.insert(buffer);
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; + } + for (auto const &pair : bufferMap_) { Buffer *buffer = pair.second; pending_.insert(buffer);
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 | 11 ++++++++--- src/libcamera/request.cpp | 12 +++++++++++- 2 files changed, 19 insertions(+), 4 deletions(-)