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

[Jacopo Mondi]

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..

> > +		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