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

[Kieran Bingham]

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?

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