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

[Laurent Pinchart]

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.

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

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

-- 
Regards,

Laurent Pinchart