| Message ID | 20241014154747.2295253-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch On 14/10/24 9:17 pm, Kieran Bingham wrote: > The configured line duration of the sensor is used frequently throughout > the AGC implementation. > > It's available in the IPA context through the rather long: > context.configuration.sensor.lineDuration > > Take a copy of the lineDuration early in the call and replace the two > current usages of the reference with the shorter copy to manage line > length and ease readibility. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 33f902862c4a..e23ab120b3e2 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -400,6 +400,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > return; > } > > + utils::Duration lineDuration = context.configuration.sensor.lineDuration; > + > /* > * \todo Verify that the exposure and gain applied by the sensor for > * this frame match what has been requested. This isn't a hard > @@ -429,8 +431,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > * The Agc algorithm needs to know the effective exposure value that was > * applied to the sensor when the statistics were collected. > */ > - utils::Duration exposureTime = context.configuration.sensor.lineDuration > - * frameContext.sensor.exposure; > + utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure; > double analogueGain = frameContext.sensor.gain; > utils::Duration effectiveExposureValue = exposureTime * analogueGain; > > @@ -447,7 +448,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > IPAActiveState &activeState = context.activeState; > /* Update the estimated exposure and gain. */ > - activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration; > + activeState.agc.automatic.exposure = shutterTime / lineDuration; > activeState.agc.automatic.gain = aGain; > > fillMetadata(context, frameContext, metadata);
On 14/10/2024 16:47, Kieran Bingham wrote: > The configured line duration of the sensor is used frequently throughout > the AGC implementation. > > It's available in the IPA context through the rather long: > context.configuration.sensor.lineDuration > > Take a copy of the lineDuration early in the call and replace the two > current usages of the reference with the shorter copy to manage line > length and ease readibility. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > src/ipa/rkisp1/algorithms/agc.cpp | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 33f902862c4a..e23ab120b3e2 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -400,6 +400,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > return; > } > > + utils::Duration lineDuration = context.configuration.sensor.lineDuration; > + > /* > * \todo Verify that the exposure and gain applied by the sensor for > * this frame match what has been requested. This isn't a hard > @@ -429,8 +431,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > * The Agc algorithm needs to know the effective exposure value that was > * applied to the sensor when the statistics were collected. > */ > - utils::Duration exposureTime = context.configuration.sensor.lineDuration > - * frameContext.sensor.exposure; > + utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure; > double analogueGain = frameContext.sensor.gain; > utils::Duration effectiveExposureValue = exposureTime * analogueGain; > > @@ -447,7 +448,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > IPAActiveState &activeState = context.activeState; > /* Update the estimated exposure and gain. */ > - activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration; > + activeState.agc.automatic.exposure = shutterTime / lineDuration; > activeState.agc.automatic.gain = aGain; > > fillMetadata(context, frameContext, metadata);
Hi Kieran On Mon, Oct 14, 2024 at 04:47:46PM +0100, Kieran Bingham wrote: > The configured line duration of the sensor is used frequently throughout > the AGC implementation. > > It's available in the IPA context through the rather long: > context.configuration.sensor.lineDuration > > Take a copy of the lineDuration early in the call and replace the two > current usages of the reference with the shorter copy to manage line > length and ease readibility. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 33f902862c4a..e23ab120b3e2 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -400,6 +400,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > return; > } > > + utils::Duration lineDuration = context.configuration.sensor.lineDuration; > + If you want to enforce this is read-only you can take a const & maybe > /* > * \todo Verify that the exposure and gain applied by the sensor for > * this frame match what has been requested. This isn't a hard > @@ -429,8 +431,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > * The Agc algorithm needs to know the effective exposure value that was > * applied to the sensor when the statistics were collected. > */ > - utils::Duration exposureTime = context.configuration.sensor.lineDuration > - * frameContext.sensor.exposure; > + utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure; > double analogueGain = frameContext.sensor.gain; > utils::Duration effectiveExposureValue = exposureTime * analogueGain; > > @@ -447,7 +448,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > IPAActiveState &activeState = context.activeState; > /* Update the estimated exposure and gain. */ > - activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration; > + activeState.agc.automatic.exposure = shutterTime / lineDuration; > activeState.agc.automatic.gain = aGain; > > fillMetadata(context, frameContext, metadata); > -- > 2.34.1 >
On Thu, Oct 24, 2024 at 05:39:48PM +0200, Jacopo Mondi wrote: > Hi Kieran > > On Mon, Oct 14, 2024 at 04:47:46PM +0100, Kieran Bingham wrote: > > The configured line duration of the sensor is used frequently throughout > > the AGC implementation. > > > > It's available in the IPA context through the rather long: > > context.configuration.sensor.lineDuration > > > > Take a copy of the lineDuration early in the call and replace the two > > current usages of the reference with the shorter copy to manage line > > length and ease readibility. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/agc.cpp | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > index 33f902862c4a..e23ab120b3e2 100644 > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > @@ -400,6 +400,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > return; > > } > > > > + utils::Duration lineDuration = context.configuration.sensor.lineDuration; > > + > > If you want to enforce this is read-only you can take a const & maybe const sounds good. It doesn't have to be a reference as the type is small, it will only copy an integer. With an added const, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > /* > > * \todo Verify that the exposure and gain applied by the sensor for > > * this frame match what has been requested. This isn't a hard > > @@ -429,8 +431,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > * The Agc algorithm needs to know the effective exposure value that was > > * applied to the sensor when the statistics were collected. > > */ > > - utils::Duration exposureTime = context.configuration.sensor.lineDuration > > - * frameContext.sensor.exposure; > > + utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure; > > double analogueGain = frameContext.sensor.gain; > > utils::Duration effectiveExposureValue = exposureTime * analogueGain; > > > > @@ -447,7 +448,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > > > IPAActiveState &activeState = context.activeState; > > /* Update the estimated exposure and gain. */ > > - activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration; > > + activeState.agc.automatic.exposure = shutterTime / lineDuration; > > activeState.agc.automatic.gain = aGain; > > > > fillMetadata(context, frameContext, metadata);
On Mon, Oct 14, 2024 at 04:47:46PM +0100, Kieran Bingham wrote: > The configured line duration of the sensor is used frequently throughout > the AGC implementation. > > It's available in the IPA context through the rather long: > context.configuration.sensor.lineDuration > > Take a copy of the lineDuration early in the call and replace the two > current usages of the reference with the shorter copy to manage line > length and ease readibility. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 33f902862c4a..e23ab120b3e2 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -400,6 +400,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > return; > } > > + utils::Duration lineDuration = context.configuration.sensor.lineDuration; > + > /* > * \todo Verify that the exposure and gain applied by the sensor for > * this frame match what has been requested. This isn't a hard > @@ -429,8 +431,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > * The Agc algorithm needs to know the effective exposure value that was > * applied to the sensor when the statistics were collected. > */ > - utils::Duration exposureTime = context.configuration.sensor.lineDuration > - * frameContext.sensor.exposure; > + utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure; > double analogueGain = frameContext.sensor.gain; > utils::Duration effectiveExposureValue = exposureTime * analogueGain; > > @@ -447,7 +448,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > IPAActiveState &activeState = context.activeState; > /* Update the estimated exposure and gain. */ > - activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration; > + activeState.agc.automatic.exposure = shutterTime / lineDuration; > activeState.agc.automatic.gain = aGain; > > fillMetadata(context, frameContext, metadata); > -- > 2.34.1 >
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 33f902862c4a..e23ab120b3e2 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -400,6 +400,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, return; } + utils::Duration lineDuration = context.configuration.sensor.lineDuration; + /* * \todo Verify that the exposure and gain applied by the sensor for * this frame match what has been requested. This isn't a hard @@ -429,8 +431,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, * The Agc algorithm needs to know the effective exposure value that was * applied to the sensor when the statistics were collected. */ - utils::Duration exposureTime = context.configuration.sensor.lineDuration - * frameContext.sensor.exposure; + utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure; double analogueGain = frameContext.sensor.gain; utils::Duration effectiveExposureValue = exposureTime * analogueGain; @@ -447,7 +448,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, IPAActiveState &activeState = context.activeState; /* Update the estimated exposure and gain. */ - activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration; + activeState.agc.automatic.exposure = shutterTime / lineDuration; activeState.agc.automatic.gain = aGain; fillMetadata(context, frameContext, metadata);
The configured line duration of the sensor is used frequently throughout the AGC implementation. It's available in the IPA context through the rather long: context.configuration.sensor.lineDuration Take a copy of the lineDuration early in the call and replace the two current usages of the reference with the shorter copy to manage line length and ease readibility. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/ipa/rkisp1/algorithms/agc.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)