[v1] v4l2_camera_proxy: Fix for getting deafult controls::FrameDurationLimits

25623 diff mbox series

The default values for controls::FrameDurationLimits is now an array but
the v4l2 proxy is fetching it as a scalar value, causing a runtime
error. Fix this by templating the getter with the correct
Span<const int64_t, 2> type.

Also related, fix the RPi IPA's initial default value for
controls::FrameDurationLimits to be an array.

Fixes: 4e9be7d11b9df ("ipa: ipu3, mali-c55, rkisp1, rpi: Fix reporting non-scalar controls")
Closes: https://github.com/raspberrypi/libcamera/issues/321
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp | 3 ++-
 src/v4l2/v4l2_camera_proxy.cpp  | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

2026. 01. 05. 11:34 keltezéssel, Naushir Patuck írta:
> The default values for controls::FrameDurationLimits is now an array but
> the v4l2 proxy is fetching it as a scalar value, causing a runtime
> error. Fix this by templating the getter with the correct
> Span<const int64_t, 2> type.
> 
> Also related, fix the RPi IPA's initial default value for
> controls::FrameDurationLimits to be an array.
> 
> Fixes: 4e9be7d11b9df ("ipa: ipu3, mali-c55, rkisp1, rpi: Fix reporting non-scalar controls")
> Closes: https://github.com/raspberrypi/libcamera/issues/321
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>   src/ipa/rpi/common/ipa_base.cpp | 3 ++-
>   src/v4l2/v4l2_camera_proxy.cpp  | 4 ++--
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 14aba4500ae4..9f8667a2284a 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -85,7 +85,8 @@ const ControlInfoMap::Map ipaControls{
>   	{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
>   	{ &controls::FrameDurationLimits,
>   	  ControlInfo(INT64_C(33333), INT64_C(120000),
> -		      static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>())) },
> +		      Span<const int64_t, 2>{ { static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()),
> +						static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()) } }) },

Why the hard-coded min/max values if there is `default{Min,Max}FrameDuration` ?

And somewhat unrelated and not rpi specific, but why do we want the default value to be a
"singleton range"? Wouldn't it make more sense for the default value to encompass the entire
possible range so as to give the algorithms the most freedom in choosing the parameters?
(Admittedly limiting the max to 1-5 fps is probably more in-line with user expectations,
  but still, setting the default frame duration to a very specific single value does not
  seem to me to be the most optimal choice.)


Regards,
Barnabás Pőcze


>   	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>   	{ &controls::rpi::StatsOutputEnable, ControlInfo(false, true, false) },
>   };
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 559ffc6170b1..03cd4810cc05 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -200,9 +200,9 @@ void V4L2CameraProxy::setFmtFromConfig(const StreamConfiguration &streamConfig)
>   	const auto &it = controls.find(&controls::FrameDurationLimits);
>   
>   	if (it != controls.end()) {
> -		const int64_t duration = it->second.def().get<int64_t>();
> +		Span<const int64_t, 2> duration = it->second.def().get<Span<const int64_t, 2>>();
>   
> -		v4l2TimePerFrame_.numerator = duration;
> +		v4l2TimePerFrame_.numerator = duration[0];
>   		v4l2TimePerFrame_.denominator = 1000000;
>   	} else {
>   		/*

On Mon, 5 Jan 2026 at 10:54, Barnabás Pőcze
<barnabas.pocze@ideasonboard.com> wrote:
>
> 2026. 01. 05. 11:34 keltezéssel, Naushir Patuck írta:
> > The default values for controls::FrameDurationLimits is now an array but
> > the v4l2 proxy is fetching it as a scalar value, causing a runtime
> > error. Fix this by templating the getter with the correct
> > Span<const int64_t, 2> type.
> >
> > Also related, fix the RPi IPA's initial default value for
> > controls::FrameDurationLimits to be an array.
> >
> > Fixes: 4e9be7d11b9df ("ipa: ipu3, mali-c55, rkisp1, rpi: Fix reporting non-scalar controls")
> > Closes: https://github.com/raspberrypi/libcamera/issues/321
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >   src/ipa/rpi/common/ipa_base.cpp | 3 ++-
> >   src/v4l2/v4l2_camera_proxy.cpp  | 4 ++--
> >   2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > index 14aba4500ae4..9f8667a2284a 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -85,7 +85,8 @@ const ControlInfoMap::Map ipaControls{
> >       { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> >       { &controls::FrameDurationLimits,
> >         ControlInfo(INT64_C(33333), INT64_C(120000),
> > -                   static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>())) },
> > +                   Span<const int64_t, 2>{ { static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()),
> > +                                             static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()) } }) },
>
> Why the hard-coded min/max values if there is `default{Min,Max}FrameDuration` ?

TBH the min/max here are inconsequential as they do get updated as
soon as a camera is configured.  I can update the patch to use the
same default constants in the next revision if needed.

>
> And somewhat unrelated and not rpi specific, but why do we want the default value to be a
> "singleton range"? Wouldn't it make more sense for the default value to encompass the entire
> possible range so as to give the algorithms the most freedom in choosing the parameters?
> (Admittedly limiting the max to 1-5 fps is probably more in-line with user expectations,
>   but still, setting the default frame duration to a very specific single value does not
>   seem to me to be the most optimal choice.)

I think this is more of a legacy thing and how most users view frame
rate as a scalar value rather than a range.  If we do want to change
this, it should probably be done across all the IPAs.

Thanks,
Naush

>
>
> Regards,
> Barnabás Pőcze
>
>
> >       { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> >       { &controls::rpi::StatsOutputEnable, ControlInfo(false, true, false) },
> >   };
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index 559ffc6170b1..03cd4810cc05 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -200,9 +200,9 @@ void V4L2CameraProxy::setFmtFromConfig(const StreamConfiguration &streamConfig)
> >       const auto &it = controls.find(&controls::FrameDurationLimits);
> >
> >       if (it != controls.end()) {
> > -             const int64_t duration = it->second.def().get<int64_t>();
> > +             Span<const int64_t, 2> duration = it->second.def().get<Span<const int64_t, 2>>();
> >
> > -             v4l2TimePerFrame_.numerator = duration;
> > +             v4l2TimePerFrame_.numerator = duration[0];
> >               v4l2TimePerFrame_.denominator = 1000000;
> >       } else {
> >               /*
>

Hi

2026. 01. 05. 12:04 keltezéssel, Naushir Patuck írta:
> On Mon, 5 Jan 2026 at 10:54, Barnabás Pőcze
> <barnabas.pocze@ideasonboard.com> wrote:
>>
>> 2026. 01. 05. 11:34 keltezéssel, Naushir Patuck írta:
>>> The default values for controls::FrameDurationLimits is now an array but
>>> the v4l2 proxy is fetching it as a scalar value, causing a runtime
>>> error. Fix this by templating the getter with the correct
>>> Span<const int64_t, 2> type.
>>>
>>> Also related, fix the RPi IPA's initial default value for
>>> controls::FrameDurationLimits to be an array.
>>>
>>> Fixes: 4e9be7d11b9df ("ipa: ipu3, mali-c55, rkisp1, rpi: Fix reporting non-scalar controls")
>>> Closes: https://github.com/raspberrypi/libcamera/issues/321
>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>>> ---
>>>    src/ipa/rpi/common/ipa_base.cpp | 3 ++-
>>>    src/v4l2/v4l2_camera_proxy.cpp  | 4 ++--
>>>    2 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
>>> index 14aba4500ae4..9f8667a2284a 100644
>>> --- a/src/ipa/rpi/common/ipa_base.cpp
>>> +++ b/src/ipa/rpi/common/ipa_base.cpp
>>> @@ -85,7 +85,8 @@ const ControlInfoMap::Map ipaControls{
>>>        { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
>>>        { &controls::FrameDurationLimits,
>>>          ControlInfo(INT64_C(33333), INT64_C(120000),
>>> -                   static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>())) },
>>> +                   Span<const int64_t, 2>{ { static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()),
>>> +                                             static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()) } }) },
>>
>> Why the hard-coded min/max values if there is `default{Min,Max}FrameDuration` ?
> 
> TBH the min/max here are inconsequential as they do get updated as
> soon as a camera is configured.  I can update the patch to use the
> same default constants in the next revision if needed.

I think it would be nice if the already defined constants could be used.


> 
>>
>> And somewhat unrelated and not rpi specific, but why do we want the default value to be a
>> "singleton range"? Wouldn't it make more sense for the default value to encompass the entire
>> possible range so as to give the algorithms the most freedom in choosing the parameters?
>> (Admittedly limiting the max to 1-5 fps is probably more in-line with user expectations,
>>    but still, setting the default frame duration to a very specific single value does not
>>    seem to me to be the most optimal choice.)
> 
> I think this is more of a legacy thing and how most users view frame
> rate as a scalar value rather than a range.  If we do want to change
> this, it should probably be done across all the IPAs.
> 
> Thanks,
> Naush
> 
>>
>>
>> Regards,
>> Barnabás Pőcze
>>
>>
>>>        { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>>>        { &controls::rpi::StatsOutputEnable, ControlInfo(false, true, false) },
>>>    };
>>> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
>>> index 559ffc6170b1..03cd4810cc05 100644
>>> --- a/src/v4l2/v4l2_camera_proxy.cpp
>>> +++ b/src/v4l2/v4l2_camera_proxy.cpp
>>> @@ -200,9 +200,9 @@ void V4L2CameraProxy::setFmtFromConfig(const StreamConfiguration &streamConfig)
>>>        const auto &it = controls.find(&controls::FrameDurationLimits);
>>>
>>>        if (it != controls.end()) {
>>> -             const int64_t duration = it->second.def().get<int64_t>();
>>> +             Span<const int64_t, 2> duration = it->second.def().get<Span<const int64_t, 2>>();
>>>
>>> -             v4l2TimePerFrame_.numerator = duration;
>>> +             v4l2TimePerFrame_.numerator = duration[0];

This part looks OK. I wanted to suggest using `(duration[0] + duration[1]) / 2`,
but I suppose if the maximum is quite large (e.g. seconds), then the simple mean
is probably less useful than just the minimum. And in any case, this initial value
is likely inconsequential most of the time.

So for this part:

Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


>>>                v4l2TimePerFrame_.denominator = 1000000;
>>>        } else {
>>>                /*
>>