[libcamera-devel] [PATCH v2 02/10] libcamera: pipeline: uvcvideo: Report control errors

[Utkarsh Tiwari]

On Wed, Aug 17, 2022 at 5:28 PM Kieran Bingham via libcamera-devel <
libcamera-devel at 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 at 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 at ideasonboard.com>
> > > > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> > > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > Signed-off-by: Jacopo Mondi <jacopo at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220817/b27ea255/attachment.htm>