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

[Laurent Pinchart]

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

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

-- 
Regards,

Laurent Pinchart