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