| Message ID | 20250204150912.962121-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the patch. On Tue, 4 Feb 2025 at 15:09, Naushir Patuck <naush@raspberrypi.com> wrote: > > The existing IPA initialisation code did not set default values for > some sensor related controls. This caused a crash using libcamerify > when the it was trying to access the default value for > controls::FrameDurationLimits as part of a recent change. > > Ensure controls::FrameDurationLimits, controls::AnalogueGain and > controls::ExposureTime advertise default values along with the existing > min/max values. The default is set to the min value. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253 > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> I suppose there is a bit of a question as to what these defaults mean, if anything. And if they don't mean much, then why do we need them. But this is obviously better than crashing, so: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Though it also feels like we need some better automated testing!! :) David > --- > src/ipa/rpi/common/ipa_base.cpp | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index bd3c22000df5..5924df4c1125 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -259,15 +259,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa > ControlInfoMap::Map ctrlMap = ipaControls; > ctrlMap[&controls::FrameDurationLimits] = > ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()), > - static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>())); > + static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()), > + static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>())); > > ctrlMap[&controls::AnalogueGain] = > ControlInfo(static_cast<float>(mode_.minAnalogueGain), > - static_cast<float>(mode_.maxAnalogueGain)); > + static_cast<float>(mode_.maxAnalogueGain), > + static_cast<float>(mode_.minAnalogueGain)); > > ctrlMap[&controls::ExposureTime] = > ControlInfo(static_cast<int32_t>(mode_.minExposureTime.get<std::micro>()), > - static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>())); > + static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>()), > + static_cast<int32_t>(mode_.minExposureTime.get<std::micro>())); > > /* Declare colour processing related controls for non-mono sensors. */ > if (!monoSensor_) > -- > 2.43.0 >
Quoting David Plowman (2025-02-04 16:05:06) > Hi Naush > > Thanks for the patch. > > On Tue, 4 Feb 2025 at 15:09, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > The existing IPA initialisation code did not set default values for > > some sensor related controls. This caused a crash using libcamerify > > when the it was trying to access the default value for > > controls::FrameDurationLimits as part of a recent change. > > > > Ensure controls::FrameDurationLimits, controls::AnalogueGain and > > controls::ExposureTime advertise default values along with the existing > > min/max values. The default is set to the min value. > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253 > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > I suppose there is a bit of a question as to what these defaults mean, > if anything. And if they don't mean much, then why do we need them. > But this is obviously better than crashing, so: > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Yup, I agree, it's hard to know what the 'right' default should be in all of these cases, and I think that's why defaults are not enforced on the Control constructors, because sometimes I'm sure in some controls it's really not possible to have a default. But as this is better than crashing I think it's fine to merge this patch - and we should try to determine which controls /require/ a default value - and find a way to specify (and validate) such things. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Though it also feels like we need some better automated testing!! :) > > David > > > --- > > src/ipa/rpi/common/ipa_base.cpp | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > index bd3c22000df5..5924df4c1125 100644 > > --- a/src/ipa/rpi/common/ipa_base.cpp > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > @@ -259,15 +259,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa > > ControlInfoMap::Map ctrlMap = ipaControls; > > ctrlMap[&controls::FrameDurationLimits] = > > ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()), > > - static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>())); > > + static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()), > > + static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>())); > > > > ctrlMap[&controls::AnalogueGain] = > > ControlInfo(static_cast<float>(mode_.minAnalogueGain), > > - static_cast<float>(mode_.maxAnalogueGain)); > > + static_cast<float>(mode_.maxAnalogueGain), > > + static_cast<float>(mode_.minAnalogueGain)); > > > > ctrlMap[&controls::ExposureTime] = > > ControlInfo(static_cast<int32_t>(mode_.minExposureTime.get<std::micro>()), > > - static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>())); > > + static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>()), > > + static_cast<int32_t>(mode_.minExposureTime.get<std::micro>())); > > > > /* Declare colour processing related controls for non-mono sensors. */ > > if (!monoSensor_) > > -- > > 2.43.0 > >
On Tue, Feb 04, 2025 at 06:39:45PM +0000, Kieran Bingham wrote: > Quoting David Plowman (2025-02-04 16:05:06) > > Hi Naush > > > > Thanks for the patch. > > > > On Tue, 4 Feb 2025 at 15:09, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > The existing IPA initialisation code did not set default values for > > > some sensor related controls. This caused a crash using libcamerify > > > when the it was trying to access the default value for > > > controls::FrameDurationLimits as part of a recent change. > > > > > > Ensure controls::FrameDurationLimits, controls::AnalogueGain and > > > controls::ExposureTime advertise default values along with the existing > > > min/max values. The default is set to the min value. > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253 > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > I suppose there is a bit of a question as to what these defaults mean, > > if anything. And if they don't mean much, then why do we need them. > > But this is obviously better than crashing, so: > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Yup, I agree, it's hard to know what the 'right' default should be in > all of these cases, and I think that's why defaults are not enforced on > the Control constructors, because sometimes I'm sure in some controls > it's really not possible to have a default. But the IPA module will use a default when no value is specified. If the camera starts with AGC disabled, without specifying manual gain and exposure time, then the IPA will pick a value. Sometimes the default will make clear sense, sometimes it will just be arbitrary, but that's what the default reported in ControlInfo should be. Does this patch report the correct values ? If so, it looks good to me. > But as this is better than crashing I think it's fine to merge this > patch - and we should try to determine which controls /require/ a > default value - and find a way to specify (and validate) such things. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Though it also feels like we need some better automated testing!! :) > > > > David > > > > > --- > > > src/ipa/rpi/common/ipa_base.cpp | 9 ++++++--- > > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > index bd3c22000df5..5924df4c1125 100644 > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > @@ -259,15 +259,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa > > > ControlInfoMap::Map ctrlMap = ipaControls; > > > ctrlMap[&controls::FrameDurationLimits] = > > > ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()), > > > - static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>())); > > > + static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()), > > > + static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>())); > > > > > > ctrlMap[&controls::AnalogueGain] = > > > ControlInfo(static_cast<float>(mode_.minAnalogueGain), > > > - static_cast<float>(mode_.maxAnalogueGain)); > > > + static_cast<float>(mode_.maxAnalogueGain), > > > + static_cast<float>(mode_.minAnalogueGain)); > > > > > > ctrlMap[&controls::ExposureTime] = > > > ControlInfo(static_cast<int32_t>(mode_.minExposureTime.get<std::micro>()), > > > - static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>())); > > > + static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>()), > > > + static_cast<int32_t>(mode_.minExposureTime.get<std::micro>())); > > > > > > /* Declare colour processing related controls for non-mono sensors. */ > > > if (!monoSensor_)
On Tue, Feb 04, 2025 at 09:27:26PM +0200, Laurent Pinchart wrote: > On Tue, Feb 04, 2025 at 06:39:45PM +0000, Kieran Bingham wrote: > > Quoting David Plowman (2025-02-04 16:05:06) > > > Hi Naush > > > > > > Thanks for the patch. > > > > > > On Tue, 4 Feb 2025 at 15:09, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > > > The existing IPA initialisation code did not set default values for > > > > some sensor related controls. This caused a crash using libcamerify > > > > when the it was trying to access the default value for > > > > controls::FrameDurationLimits as part of a recent change. > > > > > > > > Ensure controls::FrameDurationLimits, controls::AnalogueGain and > > > > controls::ExposureTime advertise default values along with the existing > > > > min/max values. The default is set to the min value. > > > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253 > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > I suppose there is a bit of a question as to what these defaults mean, > > > if anything. And if they don't mean much, then why do we need them. > > > But this is obviously better than crashing, so: > > > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > Yup, I agree, it's hard to know what the 'right' default should be in > > all of these cases, and I think that's why defaults are not enforced on > > the Control constructors, because sometimes I'm sure in some controls > > it's really not possible to have a default. > > But the IPA module will use a default when no value is specified. If the > camera starts with AGC disabled, without specifying manual gain and > exposure time, then the IPA will pick a value. Sometimes the default > will make clear sense, sometimes it will just be arbitrary, but that's > what the default reported in ControlInfo should be. > > Does this patch report the correct values ? If so, it looks good to me. Looking at AgcChannel::switchMode() initial fixed exposure time and analogue gain are calculated and seem to produce different values than the ones below. I think more work is needed. > > But as this is better than crashing I think it's fine to merge this > > patch - and we should try to determine which controls /require/ a > > default value - and find a way to specify (and validate) such things. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Though it also feels like we need some better automated testing!! :) > > > > > > David > > > > > > > --- > > > > src/ipa/rpi/common/ipa_base.cpp | 9 ++++++--- > > > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > index bd3c22000df5..5924df4c1125 100644 > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > @@ -259,15 +259,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa > > > > ControlInfoMap::Map ctrlMap = ipaControls; > > > > ctrlMap[&controls::FrameDurationLimits] = > > > > ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()), > > > > - static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>())); > > > > + static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()), > > > > + static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>())); > > > > > > > > ctrlMap[&controls::AnalogueGain] = > > > > ControlInfo(static_cast<float>(mode_.minAnalogueGain), > > > > - static_cast<float>(mode_.maxAnalogueGain)); > > > > + static_cast<float>(mode_.maxAnalogueGain), > > > > + static_cast<float>(mode_.minAnalogueGain)); > > > > > > > > ctrlMap[&controls::ExposureTime] = > > > > ControlInfo(static_cast<int32_t>(mode_.minExposureTime.get<std::micro>()), > > > > - static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>())); > > > > + static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>()), > > > > + static_cast<int32_t>(mode_.minExposureTime.get<std::micro>())); > > > > > > > > /* Declare colour processing related controls for non-mono sensors. */ > > > > if (!monoSensor_)
Hi Laurent, On Tue, 4 Feb 2025 at 19:37, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Feb 04, 2025 at 09:27:26PM +0200, Laurent Pinchart wrote: > > On Tue, Feb 04, 2025 at 06:39:45PM +0000, Kieran Bingham wrote: > > > Quoting David Plowman (2025-02-04 16:05:06) > > > > Hi Naush > > > > > > > > Thanks for the patch. > > > > > > > > On Tue, 4 Feb 2025 at 15:09, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > > > > > The existing IPA initialisation code did not set default values for > > > > > some sensor related controls. This caused a crash using libcamerify > > > > > when the it was trying to access the default value for > > > > > controls::FrameDurationLimits as part of a recent change. > > > > > > > > > > Ensure controls::FrameDurationLimits, controls::AnalogueGain and > > > > > controls::ExposureTime advertise default values along with the existing > > > > > min/max values. The default is set to the min value. > > > > > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253 > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > > I suppose there is a bit of a question as to what these defaults mean, > > > > if anything. And if they don't mean much, then why do we need them. > > > > But this is obviously better than crashing, so: > > > > > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > > > Yup, I agree, it's hard to know what the 'right' default should be in > > > all of these cases, and I think that's why defaults are not enforced on > > > the Control constructors, because sometimes I'm sure in some controls > > > it's really not possible to have a default. > > > > But the IPA module will use a default when no value is specified. If the > > camera starts with AGC disabled, without specifying manual gain and > > exposure time, then the IPA will pick a value. Sometimes the default > > will make clear sense, sometimes it will just be arbitrary, but that's > > what the default reported in ControlInfo should be. > > > > Does this patch report the correct values ? If so, it looks good to me. > > Looking at AgcChannel::switchMode() initial fixed exposure time and > analogue gain are calculated and seem to produce different values than > the ones below. I think more work is needed. I'm not sure that's correct, but It could be because of my interpretation of the default values... AgcChannel::switchMode() is called at the time of Camera::start(). That point seems too late to set the ControlInfo default value. From a user's perspective, fetching a control default after the camera has started does not seem intuitive to me. Also noting that the default value here is the default in manual mode, which is not the default AGC mode :) But again, this might just be my interpretation of how/when default values are accessed by the user. However, I have missed something in this patch - there are specific default analogue gain, shutter and frame duration values that are global to the IPA that get setup in ipa::configure(). Instead of using the min values as default, I should explicitly use those values. I'll post a v2 patch for folks to consider. Regards, Naush > > > > But as this is better than crashing I think it's fine to merge this > > > patch - and we should try to determine which controls /require/ a > > > default value - and find a way to specify (and validate) such things. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > Though it also feels like we need some better automated testing!! :) > > > > > > > > David > > > > > > > > > --- > > > > > src/ipa/rpi/common/ipa_base.cpp | 9 ++++++--- > > > > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > > index bd3c22000df5..5924df4c1125 100644 > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > > @@ -259,15 +259,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa > > > > > ControlInfoMap::Map ctrlMap = ipaControls; > > > > > ctrlMap[&controls::FrameDurationLimits] = > > > > > ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()), > > > > > - static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>())); > > > > > + static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()), > > > > > + static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>())); > > > > > > > > > > ctrlMap[&controls::AnalogueGain] = > > > > > ControlInfo(static_cast<float>(mode_.minAnalogueGain), > > > > > - static_cast<float>(mode_.maxAnalogueGain)); > > > > > + static_cast<float>(mode_.maxAnalogueGain), > > > > > + static_cast<float>(mode_.minAnalogueGain)); > > > > > > > > > > ctrlMap[&controls::ExposureTime] = > > > > > ControlInfo(static_cast<int32_t>(mode_.minExposureTime.get<std::micro>()), > > > > > - static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>())); > > > > > + static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>()), > > > > > + static_cast<int32_t>(mode_.minExposureTime.get<std::micro>())); > > > > > > > > > > /* Declare colour processing related controls for non-mono sensors. */ > > > > > if (!monoSensor_) > > -- > Regards, > > Laurent Pinchart
diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index bd3c22000df5..5924df4c1125 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -259,15 +259,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa ControlInfoMap::Map ctrlMap = ipaControls; ctrlMap[&controls::FrameDurationLimits] = ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()), - static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>())); + static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()), + static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>())); ctrlMap[&controls::AnalogueGain] = ControlInfo(static_cast<float>(mode_.minAnalogueGain), - static_cast<float>(mode_.maxAnalogueGain)); + static_cast<float>(mode_.maxAnalogueGain), + static_cast<float>(mode_.minAnalogueGain)); ctrlMap[&controls::ExposureTime] = ControlInfo(static_cast<int32_t>(mode_.minExposureTime.get<std::micro>()), - static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>())); + static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>()), + static_cast<int32_t>(mode_.minExposureTime.get<std::micro>())); /* Declare colour processing related controls for non-mono sensors. */ if (!monoSensor_)
The existing IPA initialisation code did not set default values for some sensor related controls. This caused a crash using libcamerify when the it was trying to access the default value for controls::FrameDurationLimits as part of a recent change. Ensure controls::FrameDurationLimits, controls::AnalogueGain and controls::ExposureTime advertise default values along with the existing min/max values. The default is set to the min value. Bug: https://bugs.libcamera.org/show_bug.cgi?id=253 Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/rpi/common/ipa_base.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)