| Message ID | 20190409192548.20325-9-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. 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. > > Signed-off-by: Jacopo Mondi <jacopo@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 > */ > int Camera::queueRequest(Request *request) > { > @@ -743,6 +744,9 @@ int Camera::queueRequest(Request *request) > return ret; > } > > + if (request->empty()) > + return -EINVAL; > + Should this be moved inside Request::prepare() ? > return pipe_->queueRequest(this, request); > } > > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Sun, Apr 14, 2019 at 10:08:44PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > 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. > > > > Signed-off-by: Jacopo Mondi <jacopo@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 > > */ > > int Camera::queueRequest(Request *request) > > { > > @@ -743,6 +744,9 @@ int Camera::queueRequest(Request *request) > > return ret; > > } > > > > + if (request->empty()) > > + 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? Thanks j > > return pipe_->queueRequest(this, request); > > } > > > > -- > > 2.21.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
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@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); > > > } > > >
Hi Laurent, On Mon, Apr 15, 2019 at 05:19:29PM +0300, Laurent Pinchart wrote: > 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@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... Thanks j > > > > return pipe_->queueRequest(this, request); > > > > } > > > > > > -- > Regards, > > 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@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); > >>>> } > >>>>
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 */ int Camera::queueRequest(Request *request) { @@ -743,6 +744,9 @@ int Camera::queueRequest(Request *request) return ret; } + if (request->empty()) + return -EINVAL; + return pipe_->queueRequest(this, request); }
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. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/camera.cpp | 4 ++++ 1 file changed, 4 insertions(+)