[libcamera-devel] [PATCH v4 08/12] libcamera: camera: Refuse empty requests

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Apr 15, 2019 at 02:40:36PM +0200, Jacopo Mondi wrote:
> On Sun, Apr 14, 2019 at 10:08:44PM +0200, Niklas Söderlund wrote:
> > On 2019-04-09 21:25:44 +0200, Jacopo Mondi wrote:
> > > Requests that do not contain any Stream should not be forwarded to
> > > the pipeline handlers. Return -EINVAL if the request does not contain
> > > any Stream to capture from.

When we'll add support for controls this will not necessarily hold true
anymore (although I'm not sure what the use cases would be). In any case
I don't think it's a reason to block this patch.

> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/libcamera/camera.cpp | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index cb392ca3b7e7..bd73ff69c3da 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -728,6 +728,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 does not contain any stream to capture from

In the future I'm sure we'll have other types of invalid requests,
should we already just state that the request is invalid without
detailing why ? It may make sense to add a new paragraph to the
documentation of this function to explain what a valid request is (or
move that to the documentation of Request::valid() if you decide to go
that route).

> > >   */
> > >  int Camera::queueRequest(Request *request)
> > >  {
> > > @@ -743,6 +744,9 @@ int Camera::queueRequest(Request *request)
> > >  		return ret;
> > >  	}
> > >
> > > +	if (request->empty())

I think you should log a debug message.

> > > +		return -EINVAL;
> > > +
> >
> > Should this be moved inside Request::prepare() ?
> 
> I considered doing that, but this sees to me like something that
> should live in a 'validate()' method more than in 'prepare()'.
> 
> As long as this is only validation we have I think it could live
> here, to be later moved to a 'validate()' method if we see any need
> for that. What do you think?

Maybe you could rename Request::empty() to Request::valid() ?

> > >  	return pipe_->queueRequest(this, request);
> > >  }
> > >

-- 
Regards,

Laurent Pinchart