| Message ID | 20220805135312.47497-3-jacopo@jmondi.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo and Kieran, Thank you for the patch. On Fri, Aug 05, 2022 at 03:53:04PM +0200, Jacopo Mondi via libcamera-devel wrote: > From: Kieran Bingham via libcamera-devel <libcamera-devel@lists.libcamera.org> This isn't right. > Report an error when failing to process controls, but still allow the > request to process and complete where possible. > > The Request ControlError flag is raised on the request. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=135 > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index fbe02cdcd520..8ffa27b1337b 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -26,6 +26,7 @@ > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/pipeline_handler.h" > +#include "libcamera/internal/request.h" > #include "libcamera/internal/sysfs.h" > #include "libcamera/internal/v4l2_videodevice.h" > > @@ -373,8 +374,10 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) > } > > int ret = processControls(data, request); > - if (ret < 0) > - return ret; > + if (ret < 0) { > + LOG(UVC, Error) << "Failed to process controls"; Have you ever noticed this ? I'm wondering if we would need to rate-limit this message. > + request->_d()->setError(Request::ControlError); There's a bit of a mismatch between this and the definition of ControlError in patch 01/10, as it recommends checking the metadata to see what happened, and the UVC pipeline handler doesn't generate any metadata. Generally speaking, checking metadata seems cumbersome (at best) for any camera that would have a non-zero max sync latency. This should be considered to figure out how an application would handle errors in that case. > + } > > ret = data->video_->queueBuffer(buffer); > if (ret < 0)
Hi Laurent On Tue, Aug 16, 2022 at 06:23:35AM +0300, Laurent Pinchart wrote: > Hi Jacopo and Kieran, > > Thank you for the patch. > > On Fri, Aug 05, 2022 at 03:53:04PM +0200, Jacopo Mondi via libcamera-devel wrote: > > From: Kieran Bingham via libcamera-devel <libcamera-devel@lists.libcamera.org> > > This isn't right. I -hate- this, it makes applying patches from email more painful than necessary. How can we fix this ? > > > Report an error when failing to process controls, but still allow the > > request to process and complete where possible. > > > > The Request ControlError flag is raised on the request. > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=135 > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > index fbe02cdcd520..8ffa27b1337b 100644 > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > @@ -26,6 +26,7 @@ > > #include "libcamera/internal/device_enumerator.h" > > #include "libcamera/internal/media_device.h" > > #include "libcamera/internal/pipeline_handler.h" > > +#include "libcamera/internal/request.h" > > #include "libcamera/internal/sysfs.h" > > #include "libcamera/internal/v4l2_videodevice.h" > > > > @@ -373,8 +374,10 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) > > } > > > > int ret = processControls(data, request); > > - if (ret < 0) > > - return ret; > > + if (ret < 0) { > > + LOG(UVC, Error) << "Failed to process controls"; > > Have you ever noticed this ? I'm wondering if we would need to > rate-limit this message. > I didn't, but we have a bug for that, so I assume it happens.. > > + request->_d()->setError(Request::ControlError); > > There's a bit of a mismatch between this and the definition of > ControlError in patch 01/10, as it recommends checking the metadata to > see what happened, and the UVC pipeline handler doesn't generate any > metadata. Generally speaking, checking metadata seems cumbersome (at > best) for any camera that would have a non-zero max sync latency. This And comparing the metadata with the requested controls is indeed expensive and error-prone > should be considered to figure out how an application would handle > errors in that case. > If they have max-latency > 0 it means they don't support per-frame control so indeed the returned metadata can possibily not match the requested controls and finding out which ones will converge in the next requests and which one have failed won't be possible. Is the Request's control list still populated in a completed Request ? If that's the case we can add a flag to the Control<> instances themselves ? (not excited by that tbh) > > + } > > > > ret = data->video_->queueBuffer(buffer); > > if (ret < 0) > > -- > Regards, > > Laurent Pinchart
Quoting Jacopo Mondi via libcamera-devel (2022-08-17 10:18:18) > Hi Laurent > > On Tue, Aug 16, 2022 at 06:23:35AM +0300, Laurent Pinchart wrote: > > Hi Jacopo and Kieran, > > > > Thank you for the patch. > > > > On Fri, Aug 05, 2022 at 03:53:04PM +0200, Jacopo Mondi via libcamera-devel wrote: > > > From: Kieran Bingham via libcamera-devel <libcamera-devel@lists.libcamera.org> > > > > This isn't right. > > I -hate- this, it makes applying patches from email more painful than > necessary. How can we fix this ? > > > > > > Report an error when failing to process controls, but still allow the > > > request to process and complete where possible. > > > > > > The Request ControlError flag is raised on the request. > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=135 > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > index fbe02cdcd520..8ffa27b1337b 100644 > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > @@ -26,6 +26,7 @@ > > > #include "libcamera/internal/device_enumerator.h" > > > #include "libcamera/internal/media_device.h" > > > #include "libcamera/internal/pipeline_handler.h" > > > +#include "libcamera/internal/request.h" > > > #include "libcamera/internal/sysfs.h" > > > #include "libcamera/internal/v4l2_videodevice.h" > > > > > > @@ -373,8 +374,10 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) > > > } > > > > > > int ret = processControls(data, request); > > > - if (ret < 0) > > > - return ret; > > > + if (ret < 0) { > > > + LOG(UVC, Error) << "Failed to process controls"; > > > > Have you ever noticed this ? I'm wondering if we would need to > > rate-limit this message. > > > > I didn't, but we have a bug for that, so I assume it happens.. Yes, it can quite easily be caused to happen (demonstrated by Utkarsh's control settings window implementation). It will occur once per request that contains a control that fails ... Does it need to be rate limited more than that? The catch is that if the request fails, in Utkarsh's implementation - the control is still in the list - so it gets resubmitted. We might have to make sure applications are checking that they don't continuously queue invalid controls... But perhaps that's up to the app. > > > + request->_d()->setError(Request::ControlError); > > > > There's a bit of a mismatch between this and the definition of > > ControlError in patch 01/10, as it recommends checking the metadata to > > see what happened, and the UVC pipeline handler doesn't generate any > > metadata. Generally speaking, checking metadata seems cumbersome (at That's a separate issue in the UVC pipeline handler right ? > > best) for any camera that would have a non-zero max sync latency. This > > And comparing the metadata with the requested controls is indeed > expensive and error-prone Agreed, I did already start looking into getting the control lists to return 'which' control fails ... but that's a WIP, and changes the underlying error handling of setting control lists. > > should be considered to figure out how an application would handle > > errors in that case. > > > > If they have max-latency > 0 it means they don't support per-frame > control so indeed the returned metadata can possibily not match the > requested controls and finding out which ones will converge in the > next requests and which one have failed won't be possible. > > Is the Request's control list still populated in a completed Request ? It still exists there yes ... > If that's the case we can add a flag to the Control<> instances > themselves ? (not excited by that tbh) Me neither. Do we need to return a control list which contains a list of any controls that failed ? (separately to the metadata, and the incoming controls ?) -- Kieran > > > > + } > > > > > > ret = data->video_->queueBuffer(buffer); > > > if (ret < 0) > > > > -- > > Regards, > > > > Laurent Pinchart
On Wed, Aug 17, 2022 at 5:28 PM Kieran Bingham via libcamera-devel < libcamera-devel@lists.libcamera.org> wrote: > Quoting Jacopo Mondi via libcamera-devel (2022-08-17 10:18:18) > > Hi Laurent > > > > On Tue, Aug 16, 2022 at 06:23:35AM +0300, Laurent Pinchart wrote: > > > Hi Jacopo and Kieran, > > > > > > Thank you for the patch. > > > > > > On Fri, Aug 05, 2022 at 03:53:04PM +0200, Jacopo Mondi via > libcamera-devel wrote: > > > > From: Kieran Bingham via libcamera-devel < > libcamera-devel@lists.libcamera.org> > > > > > > This isn't right. > > > > I -hate- this, it makes applying patches from email more painful than > > necessary. How can we fix this ? > > > > > > > > > Report an error when failing to process controls, but still allow the > > > > request to process and complete where possible. > > > > > > > > The Request ControlError flag is raised on the request. > > > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=135 > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 +++++-- > > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > > index fbe02cdcd520..8ffa27b1337b 100644 > > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > > @@ -26,6 +26,7 @@ > > > > #include "libcamera/internal/device_enumerator.h" > > > > #include "libcamera/internal/media_device.h" > > > > #include "libcamera/internal/pipeline_handler.h" > > > > +#include "libcamera/internal/request.h" > > > > #include "libcamera/internal/sysfs.h" > > > > #include "libcamera/internal/v4l2_videodevice.h" > > > > > > > > @@ -373,8 +374,10 @@ int > PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) > > > > } > > > > > > > > int ret = processControls(data, request); > > > > - if (ret < 0) > > > > - return ret; > > > > + if (ret < 0) { > > > > + LOG(UVC, Error) << "Failed to process controls"; > > > > > > Have you ever noticed this ? I'm wondering if we would need to > > > rate-limit this message. > > > > > > > I didn't, but we have a bug for that, so I assume it happens.. > > Yes, it can quite easily be caused to happen (demonstrated by Utkarsh's > control settings window implementation). > > It will occur once per request that contains a control that fails ... > Does it need to be rate limited more than that? > I don't think so, each request that fails generates this error message. Helps in easier debugging. > > The catch is that if the request fails, in Utkarsh's implementation - > the control is still in the list - so it gets resubmitted. > > We might have to make sure applications are checking that they don't > continuously queue invalid controls... But perhaps that's up to the app. > > Yeah libcamera should just inform that some controls have failed (possibly which ones) what happens next depends upon the application. > > > > > + request->_d()->setError(Request::ControlError); > > > > > > There's a bit of a mismatch between this and the definition of > > > ControlError in patch 01/10, as it recommends checking the metadata to > > > see what happened, and the UVC pipeline handler doesn't generate any > > > metadata. Generally speaking, checking metadata seems cumbersome (at > > That's a separate issue in the UVC pipeline handler right ? > > > > best) for any camera that would have a non-zero max sync latency. This > > > > And comparing the metadata with the requested controls is indeed > > expensive and error-prone > > Agreed, I did already start looking into getting the control lists to > return 'which' control fails ... but that's a WIP, and changes the > underlying error handling of setting control lists. > > > > > should be considered to figure out how an application would handle > > > errors in that case. > > > > > > > If they have max-latency > 0 it means they don't support per-frame > > control so indeed the returned metadata can possibily not match the > > requested controls and finding out which ones will converge in the > > next requests and which one have failed won't be possible. > > > > Is the Request's control list still populated in a completed Request ? > > It still exists there yes ... > > > If that's the case we can add a flag to the Control<> instances > > themselves ? (not excited by that tbh) > > Me neither. Do we need to return a control list which contains a list of > any controls that failed ? (separately to the metadata, and the incoming > controls ?) > > I like this idea, it would allow the app to exactly know which controls have Failed. > -- > Kieran > > > > > > > > + } > > > > > > > > ret = data->video_->queueBuffer(buffer); > > > > if (ret < 0) > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index fbe02cdcd520..8ffa27b1337b 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -26,6 +26,7 @@ #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/pipeline_handler.h" +#include "libcamera/internal/request.h" #include "libcamera/internal/sysfs.h" #include "libcamera/internal/v4l2_videodevice.h" @@ -373,8 +374,10 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) } int ret = processControls(data, request); - if (ret < 0) - return ret; + if (ret < 0) { + LOG(UVC, Error) << "Failed to process controls"; + request->_d()->setError(Request::ControlError); + } ret = data->video_->queueBuffer(buffer); if (ret < 0)