| Message ID | 20240616163910.5506-12-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 4ce519cdc5f62a8b17861242f8ea830c9a8df1a1 |
| Headers | show |
| Series |
|
| Related | show |
Quoting Laurent Pinchart (2024-06-16 17:39:09) > The sensor's maximum shutter speed is clamped by the maximum frame > duration specified in requests. If the requested maximum frame duration > is lower than the sensor's minimum shutter speed, the Agc::process() > function will pass a minimum value higher than the maximum to the > setLimits() function, resulting in an assertion failure. Fix it by > clamping the value to both the lower and the upper bounds. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 5b917557b887..0018c43f18cf 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -420,8 +420,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > [](uint32_t x) { return x >> 4; }); > expMeans_ = { params->ae.exp_mean, context.hw->numAeCells }; > > - utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed, > - frameContext.agc.maxFrameDuration); > + utils::Duration maxShutterSpeed = > + std::clamp(frameContext.agc.maxFrameDuration, > + context.configuration.sensor.minShutterSpeed, > + context.configuration.sensor.maxShutterSpeed); > setLimits(context.configuration.sensor.minShutterSpeed, > maxShutterSpeed, I'm somehow confused here, as we're passing sensor.minShutterSpeed in here as well. Does that now become redundant? Or could/should this clamping be done within setLimits? Does the other call site of setLimits experience the same issue? > context.configuration.sensor.minAnalogueGain, > -- > Regards, > > Laurent Pinchart >
On Mon, Jun 17, 2024 at 10:48:39AM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2024-06-16 17:39:09) > > The sensor's maximum shutter speed is clamped by the maximum frame > > duration specified in requests. If the requested maximum frame duration > > is lower than the sensor's minimum shutter speed, the Agc::process() > > function will pass a minimum value higher than the maximum to the > > setLimits() function, resulting in an assertion failure. Fix it by > > clamping the value to both the lower and the upper bounds. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/agc.cpp | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > index 5b917557b887..0018c43f18cf 100644 > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > @@ -420,8 +420,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > [](uint32_t x) { return x >> 4; }); > > expMeans_ = { params->ae.exp_mean, context.hw->numAeCells }; > > > > - utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed, > > - frameContext.agc.maxFrameDuration); > > + utils::Duration maxShutterSpeed = > > + std::clamp(frameContext.agc.maxFrameDuration, > > + context.configuration.sensor.minShutterSpeed, > > + context.configuration.sensor.maxShutterSpeed); > > setLimits(context.configuration.sensor.minShutterSpeed, > > maxShutterSpeed, > > I'm somehow confused here, as we're passing sensor.minShutterSpeed in > here as well. Does that now become redundant? Or could/should this > clamping be done within setLimits? Does the other call site of setLimits > experience the same issue? setLimits() is to configure the ExposureModeHelper, so that you can use it to split the exposure duration into gain and shutter time. sensor.minShutterSpeed is the minimum shutter speed for the exposure mode helper, and the maxShutterSpeed is, well, the maximum. Looks fine to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > context.configuration.sensor.minAnalogueGain,
On Mon, Jun 17, 2024 at 08:52:01PM +0900, Paul Elder wrote: > On Mon, Jun 17, 2024 at 10:48:39AM +0100, Kieran Bingham wrote: > > Quoting Laurent Pinchart (2024-06-16 17:39:09) > > > The sensor's maximum shutter speed is clamped by the maximum frame > > > duration specified in requests. If the requested maximum frame duration > > > is lower than the sensor's minimum shutter speed, the Agc::process() > > > function will pass a minimum value higher than the maximum to the > > > setLimits() function, resulting in an assertion failure. Fix it by > > > clamping the value to both the lower and the upper bounds. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/ipa/rkisp1/algorithms/agc.cpp | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > index 5b917557b887..0018c43f18cf 100644 > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > @@ -420,8 +420,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > > [](uint32_t x) { return x >> 4; }); > > > expMeans_ = { params->ae.exp_mean, context.hw->numAeCells }; > > > > > > - utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed, > > > - frameContext.agc.maxFrameDuration); > > > + utils::Duration maxShutterSpeed = > > > + std::clamp(frameContext.agc.maxFrameDuration, > > > + context.configuration.sensor.minShutterSpeed, > > > + context.configuration.sensor.maxShutterSpeed); > > > setLimits(context.configuration.sensor.minShutterSpeed, > > > maxShutterSpeed, > > > > I'm somehow confused here, as we're passing sensor.minShutterSpeed in > > here as well. Does that now become redundant? Or could/should this > > clamping be done within setLimits? Does the other call site of setLimits > > experience the same issue? > > setLimits() is to configure the ExposureModeHelper, so that you can use > it to split the exposure duration into gain and shutter time. > sensor.minShutterSpeed is the minimum shutter speed for the exposure > mode helper, and the maxShutterSpeed is, well, the maximum. I got confused too, and implemented code to using the minimum FrameDurationLimits to compute the minShutterSpeed. The result was interesting, but clearly wrong :-) That's when I realized that we're doing with shutter speeds here, not frame durations. That prompted me to rename the variable in patch 11/12. > Looks fine to me. > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > context.configuration.sensor.minAnalogueGain,
On Mon, Jun 17, 2024 at 03:09:01PM +0300, Laurent Pinchart wrote: > On Mon, Jun 17, 2024 at 08:52:01PM +0900, Paul Elder wrote: > > On Mon, Jun 17, 2024 at 10:48:39AM +0100, Kieran Bingham wrote: > > > Quoting Laurent Pinchart (2024-06-16 17:39:09) > > > > The sensor's maximum shutter speed is clamped by the maximum frame > > > > duration specified in requests. If the requested maximum frame duration > > > > is lower than the sensor's minimum shutter speed, the Agc::process() > > > > function will pass a minimum value higher than the maximum to the > > > > setLimits() function, resulting in an assertion failure. Fix it by > > > > clamping the value to both the lower and the upper bounds. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > src/ipa/rkisp1/algorithms/agc.cpp | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > > index 5b917557b887..0018c43f18cf 100644 > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > > @@ -420,8 +420,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > > > [](uint32_t x) { return x >> 4; }); > > > > expMeans_ = { params->ae.exp_mean, context.hw->numAeCells }; > > > > > > > > - utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed, > > > > - frameContext.agc.maxFrameDuration); > > > > + utils::Duration maxShutterSpeed = > > > > + std::clamp(frameContext.agc.maxFrameDuration, > > > > + context.configuration.sensor.minShutterSpeed, > > > > + context.configuration.sensor.maxShutterSpeed); > > > > setLimits(context.configuration.sensor.minShutterSpeed, > > > > maxShutterSpeed, > > > > > > I'm somehow confused here, as we're passing sensor.minShutterSpeed in > > > here as well. Does that now become redundant? Or could/should this > > > clamping be done within setLimits? Does the other call site of setLimits > > > experience the same issue? > > > > setLimits() is to configure the ExposureModeHelper, so that you can use > > it to split the exposure duration into gain and shutter time. > > sensor.minShutterSpeed is the minimum shutter speed for the exposure > > mode helper, and the maxShutterSpeed is, well, the maximum. > > I got confused too, and implemented code to using the minimum > FrameDurationLimits to compute the minShutterSpeed. The result was > interesting, but clearly wrong :-) That's when I realized that we're > doing with shutter speeds here, not frame durations. That prompted me to > rename the variable in patch 11/12. I meant 10/12, sorry > > Looks fine to me. > > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > context.configuration.sensor.minAnalogueGain,
Quoting Laurent Pinchart (2024-06-17 13:10:08) > On Mon, Jun 17, 2024 at 03:09:01PM +0300, Laurent Pinchart wrote: > > On Mon, Jun 17, 2024 at 08:52:01PM +0900, Paul Elder wrote: > > > On Mon, Jun 17, 2024 at 10:48:39AM +0100, Kieran Bingham wrote: > > > > Quoting Laurent Pinchart (2024-06-16 17:39:09) > > > > > The sensor's maximum shutter speed is clamped by the maximum frame > > > > > duration specified in requests. If the requested maximum frame duration > > > > > is lower than the sensor's minimum shutter speed, the Agc::process() > > > > > function will pass a minimum value higher than the maximum to the > > > > > setLimits() function, resulting in an assertion failure. Fix it by > > > > > clamping the value to both the lower and the upper bounds. > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > src/ipa/rkisp1/algorithms/agc.cpp | 6 ++++-- > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > > > index 5b917557b887..0018c43f18cf 100644 > > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > > > @@ -420,8 +420,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > > > > [](uint32_t x) { return x >> 4; }); > > > > > expMeans_ = { params->ae.exp_mean, context.hw->numAeCells }; > > > > > > > > > > - utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed, > > > > > - frameContext.agc.maxFrameDuration); > > > > > + utils::Duration maxShutterSpeed = > > > > > + std::clamp(frameContext.agc.maxFrameDuration, > > > > > + context.configuration.sensor.minShutterSpeed, > > > > > + context.configuration.sensor.maxShutterSpeed); > > > > > setLimits(context.configuration.sensor.minShutterSpeed, > > > > > maxShutterSpeed, > > > > > > > > I'm somehow confused here, as we're passing sensor.minShutterSpeed in > > > > here as well. Does that now become redundant? Or could/should this > > > > clamping be done within setLimits? Does the other call site of setLimits > > > > experience the same issue? > > > > > > setLimits() is to configure the ExposureModeHelper, so that you can use > > > it to split the exposure duration into gain and shutter time. > > > sensor.minShutterSpeed is the minimum shutter speed for the exposure > > > mode helper, and the maxShutterSpeed is, well, the maximum. > > > > I got confused too, and implemented code to using the minimum > > FrameDurationLimits to compute the minShutterSpeed. The result was > > interesting, but clearly wrong :-) That's when I realized that we're > > doing with shutter speeds here, not frame durations. That prompted me to > > rename the variable in patch 11/12. > > I meant 10/12, sorry I've checked the other setLimits caller, and there's no equivalent required there. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Looks fine to me. > > > > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > > > context.configuration.sensor.minAnalogueGain, > > -- > Regards, > > Laurent Pinchart
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 5b917557b887..0018c43f18cf 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -420,8 +420,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, [](uint32_t x) { return x >> 4; }); expMeans_ = { params->ae.exp_mean, context.hw->numAeCells }; - utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed, - frameContext.agc.maxFrameDuration); + utils::Duration maxShutterSpeed = + std::clamp(frameContext.agc.maxFrameDuration, + context.configuration.sensor.minShutterSpeed, + context.configuration.sensor.maxShutterSpeed); setLimits(context.configuration.sensor.minShutterSpeed, maxShutterSpeed, context.configuration.sensor.minAnalogueGain,
The sensor's maximum shutter speed is clamped by the maximum frame duration specified in requests. If the requested maximum frame duration is lower than the sensor's minimum shutter speed, the Agc::process() function will pass a minimum value higher than the maximum to the setLimits() function, resulting in an assertion failure. Fix it by clamping the value to both the lower and the upper bounds. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/ipa/rkisp1/algorithms/agc.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)