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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Apr 15, 2019 at 04:55:50PM +0200, Jacopo Mondi wrote:
> On Mon, Apr 15, 2019 at 05:19:29PM +0300, Laurent Pinchart wrote:
> > 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() ?
> >
> 
> This would be very convienent, but I'm not sure a request should be
> self-validating honestly... Although a Request::valid() function might
> make sense to perform the basic checks, I think there should be a
> validation performed by the Camera class too (and posibly a hook to
> pipeline handler too? as some control might be valid for one
> implementation but not for the other? That's for later thuough...)
> 
> I'll change empty() to valid() and perform a first validity check,
> such as making sure the request has at least one Stream where to
> capture from...

Actually, after reading the rest of the series, I realized you use
->empty() is various other places, and those usages don't match a
valid() function. I would thus perform the validate check in
Camera::queueRequest() as done in this patch, with an additional
paragraph in the function documentation to explain what a valid request
is, and keep the existing semantics for the empty() method. I would
however rename empty() to something else, as what we really want to
check is if the request still has uncompleted buffers. Maybe
isReadyToComplete() ? It's a bit long, I'm sure a better name exists.

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

-- 
Regards,

Laurent Pinchart