[v1] libcamera: pipeline: uvcvideo: Silently ignore `AeEnable`
diff mbox series

Message ID 20250611134118.585044-1-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [v1] libcamera: pipeline: uvcvideo: Silently ignore `AeEnable`
Related show

Commit Message

Barnabás Pőcze June 11, 2025, 1:41 p.m. UTC 
The `AeEnable` control is handled in `Camera::queueRequest()`
but it still reaches the pipeline handler, so ignore it silently.

Fixes: ffcecda4d5b9b5 ("libcamera: pipeline: uvcvideo: Report new AeEnable control as available")
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++
 1 file changed, 2 insertions(+)

--
2.49.0

Comments

Kieran Bingham June 11, 2025, 2:19 p.m. UTC | #1 
Quoting Barnabás Pőcze (2025-06-11 14:41:18)
> The `AeEnable` control is handled in `Camera::queueRequest()`
> but it still reaches the pipeline handler, so ignore it silently.
> 
> Fixes: ffcecda4d5b9b5 ("libcamera: pipeline: uvcvideo: Report new AeEnable control as available")
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 58aa0eb4c..d52b88042 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -331,6 +331,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
>                 cid = V4L2_CID_GAIN;
>         else if (id == controls::Gamma)
>                 cid = V4L2_CID_GAMMA;
> +       else if (id == controls::AeEnable)
> +               return 0; /* Handled in `Camera::queueRequest()`. */

I think this is probably the best thing to do here - I wonder if we
should have a way to 'remove' the AeEnable control during
Camera::queueRequest so that it doesn't have to have these special
cases, but I'm not sure we can do that at the moment.

So,


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>         else
>                 return -EINVAL;
> 
> --
> 2.49.0
Laurent Pinchart June 11, 2025, 2:23 p.m. UTC | #2 
On Wed, Jun 11, 2025 at 03:19:35PM +0100, Kieran Bingham wrote:
> Quoting Barnabás Pőcze (2025-06-11 14:41:18)
> > The `AeEnable` control is handled in `Camera::queueRequest()`
> > but it still reaches the pipeline handler, so ignore it silently.
> > 
> > Fixes: ffcecda4d5b9b5 ("libcamera: pipeline: uvcvideo: Report new AeEnable control as available")
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 58aa0eb4c..d52b88042 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -331,6 +331,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
> >                 cid = V4L2_CID_GAIN;
> >         else if (id == controls::Gamma)
> >                 cid = V4L2_CID_GAMMA;
> > +       else if (id == controls::AeEnable)
> > +               return 0; /* Handled in `Camera::queueRequest()`. */
> 
> I think this is probably the best thing to do here - I wonder if we
> should have a way to 'remove' the AeEnable control during
> Camera::queueRequest so that it doesn't have to have these special
> cases, but I'm not sure we can do that at the moment.

We should, but we can't ATM :-/

> So,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >         else
> >                 return -EINVAL;
> >
Laurent Pinchart June 11, 2025, 2:23 p.m. UTC | #3 
On Wed, Jun 11, 2025 at 05:23:35PM +0300, Laurent Pinchart wrote:
> On Wed, Jun 11, 2025 at 03:19:35PM +0100, Kieran Bingham wrote:
> > Quoting Barnabás Pőcze (2025-06-11 14:41:18)
> > > The `AeEnable` control is handled in `Camera::queueRequest()`
> > > but it still reaches the pipeline handler, so ignore it silently.
> > > 
> > > Fixes: ffcecda4d5b9b5 ("libcamera: pipeline: uvcvideo: Report new AeEnable control as available")
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index 58aa0eb4c..d52b88042 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -331,6 +331,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
> > >                 cid = V4L2_CID_GAIN;
> > >         else if (id == controls::Gamma)
> > >                 cid = V4L2_CID_GAMMA;
> > > +       else if (id == controls::AeEnable)
> > > +               return 0; /* Handled in `Camera::queueRequest()`. */
> > 
> > I think this is probably the best thing to do here - I wonder if we
> > should have a way to 'remove' the AeEnable control during
> > Camera::queueRequest so that it doesn't have to have these special
> > cases, but I'm not sure we can do that at the moment.
> 
> We should, but we can't ATM :-/

It would be nice to explain this in the commit message.

> > So,
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > >         else
> > >                 return -EINVAL;
> > >
Barnabás Pőcze June 11, 2025, 2:27 p.m. UTC | #4 
2025. 06. 11. 16:19 keltezéssel, Kieran Bingham írta:
> Quoting Barnabás Pőcze (2025-06-11 14:41:18)
>> The `AeEnable` control is handled in `Camera::queueRequest()`
>> but it still reaches the pipeline handler, so ignore it silently.
>>
>> Fixes: ffcecda4d5b9b5 ("libcamera: pipeline: uvcvideo: Report new AeEnable control as available")
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> index 58aa0eb4c..d52b88042 100644
>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> @@ -331,6 +331,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
>>                  cid = V4L2_CID_GAIN;
>>          else if (id == controls::Gamma)
>>                  cid = V4L2_CID_GAMMA;
>> +       else if (id == controls::AeEnable)
>> +               return 0; /* Handled in `Camera::queueRequest()`. */
> 
> I think this is probably the best thing to do here - I wonder if we
> should have a way to 'remove' the AeEnable control during
> Camera::queueRequest so that it doesn't have to have these special
> cases, but I'm not sure we can do that at the moment.

It should be simple to add `extract()/remove()` to `ControlList`, but I
don't know why it hasn't been done so far.


Regards,
Barnabás Pőcze

> 
> So,
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>>          else
>>                  return -EINVAL;
>>
>> --
>> 2.49.0
Barnabás Pőcze June 11, 2025, 2:31 p.m. UTC | #5 
2025. 06. 11. 16:23 keltezéssel, Laurent Pinchart írta:
> On Wed, Jun 11, 2025 at 05:23:35PM +0300, Laurent Pinchart wrote:
>> On Wed, Jun 11, 2025 at 03:19:35PM +0100, Kieran Bingham wrote:
>>> Quoting Barnabás Pőcze (2025-06-11 14:41:18)
>>>> The `AeEnable` control is handled in `Camera::queueRequest()`
>>>> but it still reaches the pipeline handler, so ignore it silently.
>>>>
>>>> Fixes: ffcecda4d5b9b5 ("libcamera: pipeline: uvcvideo: Report new AeEnable control as available")
>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>> ---
>>>>   src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>> index 58aa0eb4c..d52b88042 100644
>>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>> @@ -331,6 +331,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
>>>>                  cid = V4L2_CID_GAIN;
>>>>          else if (id == controls::Gamma)
>>>>                  cid = V4L2_CID_GAMMA;
>>>> +       else if (id == controls::AeEnable)
>>>> +               return 0; /* Handled in `Camera::queueRequest()`. */
>>>
>>> I think this is probably the best thing to do here - I wonder if we
>>> should have a way to 'remove' the AeEnable control during
>>> Camera::queueRequest so that it doesn't have to have these special
>>> cases, but I'm not sure we can do that at the moment.
>>
>> We should, but we can't ATM :-/
> 
> It would be nice to explain this in the commit message.

I will modify it as follows:

   The `AeEnable` control is handled in `Camera::queueRequest()` but it
   still reaches the pipeline handler because a single element cannot be
   removed from a `ControlList`. So ignore it silently.



> 
>>> So,
>>>
>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>>>>          else
>>>>                  return -EINVAL;
>>>>
>
Laurent Pinchart June 11, 2025, 2:45 p.m. UTC | #6 
On Wed, Jun 11, 2025 at 04:31:01PM +0200, Barnabás Pőcze wrote:
> 2025. 06. 11. 16:23 keltezéssel, Laurent Pinchart írta:
> > On Wed, Jun 11, 2025 at 05:23:35PM +0300, Laurent Pinchart wrote:
> >> On Wed, Jun 11, 2025 at 03:19:35PM +0100, Kieran Bingham wrote:
> >>> Quoting Barnabás Pőcze (2025-06-11 14:41:18)
> >>>> The `AeEnable` control is handled in `Camera::queueRequest()`
> >>>> but it still reaches the pipeline handler, so ignore it silently.
> >>>>
> >>>> Fixes: ffcecda4d5b9b5 ("libcamera: pipeline: uvcvideo: Report new AeEnable control as available")
> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >>>> ---
> >>>>   src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++
> >>>>   1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> >>>> index 58aa0eb4c..d52b88042 100644
> >>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> >>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> >>>> @@ -331,6 +331,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
> >>>>                  cid = V4L2_CID_GAIN;
> >>>>          else if (id == controls::Gamma)
> >>>>                  cid = V4L2_CID_GAMMA;
> >>>> +       else if (id == controls::AeEnable)
> >>>> +               return 0; /* Handled in `Camera::queueRequest()`. */
> >>>
> >>> I think this is probably the best thing to do here - I wonder if we
> >>> should have a way to 'remove' the AeEnable control during
> >>> Camera::queueRequest so that it doesn't have to have these special
> >>> cases, but I'm not sure we can do that at the moment.
> >>
> >> We should, but we can't ATM :-/
> > 
> > It would be nice to explain this in the commit message.
> 
> I will modify it as follows:
> 
>    The `AeEnable` control is handled in `Camera::queueRequest()` but it
>    still reaches the pipeline handler because a single element cannot be
>    removed from a `ControlList`. So ignore it silently.

Perfect.

> >>> So,
> >>>
> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>
> >>>>          else
> >>>>                  return -EINVAL;
> >>>>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index 58aa0eb4c..d52b88042 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -331,6 +331,8 @@  int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
 		cid = V4L2_CID_GAIN;
 	else if (id == controls::Gamma)
 		cid = V4L2_CID_GAMMA;
+	else if (id == controls::AeEnable)
+		return 0; /* Handled in `Camera::queueRequest()`. */
 	else
 		return -EINVAL;