| Message ID | 20260105143206.115011-1-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Typo in the subject line, s/deafult/default/. While at it, I'd add a "v4l2: " prefix: v4l2: v4l2_camera_proxy: Fix for getting default FrameDurationLimits On Mon, Jan 05, 2026 at 02:31:45PM +0000, Naushir Patuck wrote: > 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 and use the default const > values. > > 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> > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/ipa/rpi/common/ipa_base.cpp | 6 ++++-- > src/v4l2/v4l2_camera_proxy.cpp | 4 ++-- I'd have split the patch in two. > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index 14aba4500ae4..322694985e72 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -84,8 +84,10 @@ const ControlInfoMap::Map ipaControls{ > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > { &controls::FrameDurationLimits, > - ControlInfo(INT64_C(33333), INT64_C(120000), This is a change in behaviour, moving from 120ms to 250s. Is it intended ? > - static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>())) }, > + ControlInfo(static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()), > + static_cast<int64_t>(defaultMaxFrameDuration.get<std::micro>()), > + Span<const int64_t, 2>{ { static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()), > + static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()) } }) }, > { &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>>(); We need a better API to make this kind of error impossible by propagating the type from the Control<T> passed to find() to the get() function. Not a candidate for this patch of course. > > - v4l2TimePerFrame_.numerator = duration; > + v4l2TimePerFrame_.numerator = duration[0]; > v4l2TimePerFrame_.denominator = 1000000; > } else { > /*
On Tue, Jan 06, 2026 at 03:07:13AM +0200, Laurent Pinchart wrote: > Typo in the subject line, s/deafult/default/. While at it, I'd add a > "v4l2: " prefix: > > v4l2: v4l2_camera_proxy: Fix for getting default FrameDurationLimits > > On Mon, Jan 05, 2026 at 02:31:45PM +0000, Naushir Patuck wrote: > > 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 and use the default const > > values. > > > > 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> > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > --- > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++-- > > src/v4l2/v4l2_camera_proxy.cpp | 4 ++-- > > I'd have split the patch in two. > > > 2 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > index 14aba4500ae4..322694985e72 100644 > > --- a/src/ipa/rpi/common/ipa_base.cpp > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > @@ -84,8 +84,10 @@ const ControlInfoMap::Map ipaControls{ > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > { &controls::FrameDurationLimits, > > - ControlInfo(INT64_C(33333), INT64_C(120000), > > This is a change in behaviour, moving from 120ms to 250s. Is it intended > ? I've now seen the discussion in v1. Could you update the commit message to explain this ? If you decide not to split the patch then I can update the commit message when applying (or you can send a v3 if it's equally easy). > > - static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>())) }, > > + ControlInfo(static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()), > > + static_cast<int64_t>(defaultMaxFrameDuration.get<std::micro>()), > > + Span<const int64_t, 2>{ { static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()), > > + static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()) } }) }, > > { &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>>(); > > We need a better API to make this kind of error impossible by > propagating the type from the Control<T> passed to find() to the get() > function. Not a candidate for this patch of course. > > > - v4l2TimePerFrame_.numerator = duration; > > + v4l2TimePerFrame_.numerator = duration[0]; > > v4l2TimePerFrame_.denominator = 1000000; > > } else { > > /*
Hi Laurent, On Tue, 6 Jan 2026 at 03:51, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Jan 06, 2026 at 03:07:13AM +0200, Laurent Pinchart wrote: > > Typo in the subject line, s/deafult/default/. While at it, I'd add a > > "v4l2: " prefix: > > > > v4l2: v4l2_camera_proxy: Fix for getting default FrameDurationLimits > > > > On Mon, Jan 05, 2026 at 02:31:45PM +0000, Naushir Patuck wrote: > > > 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 and use the default const > > > values. > > > > > > 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> > > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++-- > > > src/v4l2/v4l2_camera_proxy.cpp | 4 ++-- > > > > I'd have split the patch in two. > > > > > 2 files changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > index 14aba4500ae4..322694985e72 100644 > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > @@ -84,8 +84,10 @@ const ControlInfoMap::Map ipaControls{ > > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > > > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > > { &controls::FrameDurationLimits, > > > - ControlInfo(INT64_C(33333), INT64_C(120000), > > > > This is a change in behaviour, moving from 120ms to 250s. Is it intended > > ? > > I've now seen the discussion in v1. Could you update the commit message > to explain this ? If you decide not to split the patch then I can update > the commit message when applying (or you can send a v3 if it's equally > easy). I think let's keep both changes in one patch this time as both are relevant for fixing this issue. I'll update the commit message to be more clear and post a v3 shortly. Regards, Naush > > > > - static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>())) }, > > > + ControlInfo(static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()), > > > + static_cast<int64_t>(defaultMaxFrameDuration.get<std::micro>()), > > > + Span<const int64_t, 2>{ { static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()), > > > + static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()) } }) }, > > > { &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>>(); > > > > We need a better API to make this kind of error impossible by > > propagating the type from the Control<T> passed to find() to the get() > > function. Not a candidate for this patch of course. > > > > > - v4l2TimePerFrame_.numerator = duration; > > > + v4l2TimePerFrame_.numerator = duration[0]; > > > v4l2TimePerFrame_.denominator = 1000000; > > > } else { > > > /* > > -- > Regards, > > Laurent Pinchart
diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index 14aba4500ae4..322694985e72 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -84,8 +84,10 @@ const ControlInfoMap::Map ipaControls{ { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, { &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>())) }, + ControlInfo(static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()), + static_cast<int64_t>(defaultMaxFrameDuration.get<std::micro>()), + Span<const int64_t, 2>{ { static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()), + static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()) } }) }, { &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 { /*
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 and use the default const values. 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> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/ipa/rpi/common/ipa_base.cpp | 6 ++++-- src/v4l2/v4l2_camera_proxy.cpp | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-)