| Message ID | 20250808141315.413839-10-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2025-08-08 15:12:47) > There are several occations where quantization can lead to visible > effects. > > In WDR mode it can happen that exposure times get set to very low values > (Sometimes 2-3 lines). This intentionally introduced underexposure is > corrected by the GWDR module. As exposure time is quantized by lines, > the smallest possible change in exposure time now results in a quite > visible change in perceived brightness. > > Sometimes the possible gain steps are also quite large leading to > visible jumps if e.g. if the exposure time is fixed. > > Mitigate that by applying a global gain to account for the error > introduced by the exposure quantization. > > ToDo: This needs perfect frame synchronous control of the sensor to work > properly which is not guaranteed in all cases. It still improves the > behavior with the current regulation and can easily be skipped, when > there is no compress algorithm. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 24 +++++++++++++++++++++--- > src/ipa/rkisp1/ipa_context.h | 2 ++ > 2 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 0a29326841fb..d34c041f9fe1 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -199,6 +199,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width; > context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height; > > + AgcMeanLuminance::configure(context.configuration.sensor.lineDuration, > + context.camHelper.get()); It probably doesn't matter at the moment, but could/should the camHelper be initialiased at 'init' time ? It shouldn't ever change during configure() right ? It's only the lineDuration that could change at configure time. > + There's /so/ much (potential) duplication of code here in Agc::{init,configure,queueRequest} - that I have started to try to work out how to defer most of the work to the AgcMeanLumince::{init,configure,queueRequest} too. (See https://git.uk.ideasonboard.com/kbingham/libcamera/pulls/18 for my ongoing development branch of that) But I don't think I should block this work on the work I'm doing. I think we need to do better to keep IPU3,RKISP1,Mali-C55,SoftIPA AEGC and AWB implementations aligned for features and capabilities and controls. I think it should be possible that only the conversion of statistics and conversion will occur in the platform specific IPA for those platforms. > setLimits(context.configuration.sensor.minExposureTime, > context.configuration.sensor.maxExposureTime, > context.configuration.sensor.minAnalogueGain, > @@ -283,6 +286,10 @@ void Agc::queueRequest(IPAContext &context, > if (!frameContext.agc.autoGainEnabled) > frameContext.agc.gain = agc.manual.gain; > > + if (!frameContext.agc.autoExposureEnabled && > + !frameContext.agc.autoGainEnabled) > + frameContext.agc.quantizationGain = 1.0; > + > const auto &meteringMode = controls.get(controls::AeMeteringMode); > if (meteringMode) { > frameContext.agc.updateMetering = agc.meteringMode != *meteringMode; > @@ -336,12 +343,17 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > { > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure; > double activeAutoGain = context.activeState.agc.automatic.gain; > + double activeAutoQGain = context.activeState.agc.automatic.quantizationGain; > > /* Populate exposure and gain in auto mode */ > - if (frameContext.agc.autoExposureEnabled) > + if (frameContext.agc.autoExposureEnabled) { > frameContext.agc.exposure = activeAutoExposure; > - if (frameContext.agc.autoGainEnabled) > + frameContext.agc.quantizationGain = activeAutoQGain; > + } > + if (frameContext.agc.autoGainEnabled) { > frameContext.agc.gain = activeAutoGain; > + frameContext.agc.quantizationGain = activeAutoQGain; > + } > > /* > * Populate manual exposure and gain from the active auto values when > @@ -354,6 +366,12 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > if (!frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange) { > context.activeState.agc.manual.gain = activeAutoGain; > frameContext.agc.gain = activeAutoGain; > + frameContext.agc.quantizationGain = activeAutoQGain; > + } > + > + if (context.activeState.compress.supported) { > + frameContext.compress.enable = true; > + frameContext.compress.gain = frameContext.agc.quantizationGain; > } > > if (frame > 0 && !frameContext.agc.updateMetering) > @@ -582,7 +600,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > /* Update the estimated exposure and gain. */ > activeState.agc.automatic.exposure = newExposureTime / lineDuration; > activeState.agc.automatic.gain = aGain; > - > + activeState.agc.automatic.quantizationGain = qGain; > /* > * Expand the target frame duration so that we do not run faster than > * the minimum frame duration when we have short exposures. > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index 37a74215ce19..362ab2fda5fe 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -77,6 +77,7 @@ struct IPAActiveState { > struct { > uint32_t exposure; > double gain; > + double quantizationGain; > } automatic; > > bool autoExposureEnabled; > @@ -135,6 +136,7 @@ struct IPAFrameContext : public FrameContext { > uint32_t exposure; > double gain; > double exposureValue; > + double quantizationGain; > uint32_t vblank; > bool autoExposureEnabled; > bool autoGainEnabled; > -- > 2.48.1 >
Hi Kieran, Thank you for the review. Quoting Kieran Bingham (2025-08-11 16:43:39) > Quoting Stefan Klug (2025-08-08 15:12:47) > > There are several occations where quantization can lead to visible > > effects. > > > > In WDR mode it can happen that exposure times get set to very low values > > (Sometimes 2-3 lines). This intentionally introduced underexposure is > > corrected by the GWDR module. As exposure time is quantized by lines, > > the smallest possible change in exposure time now results in a quite > > visible change in perceived brightness. > > > > Sometimes the possible gain steps are also quite large leading to > > visible jumps if e.g. if the exposure time is fixed. > > > > Mitigate that by applying a global gain to account for the error > > introduced by the exposure quantization. > > > > ToDo: This needs perfect frame synchronous control of the sensor to work > > properly which is not guaranteed in all cases. It still improves the > > behavior with the current regulation and can easily be skipped, when > > there is no compress algorithm. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/agc.cpp | 24 +++++++++++++++++++++--- > > src/ipa/rkisp1/ipa_context.h | 2 ++ > > 2 files changed, 23 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > index 0a29326841fb..d34c041f9fe1 100644 > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > @@ -199,6 +199,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width; > > context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height; > > > > + AgcMeanLuminance::configure(context.configuration.sensor.lineDuration, > > + context.camHelper.get()); > > It probably doesn't matter at the moment, but could/should the camHelper > be initialiased at 'init' time ? It shouldn't ever change during > configure() right ? It's only the lineDuration that could change at > configure time. Yes, you're right that would be possible, but it also means another function and a wider interface. I'm undecided... > > > + > > There's /so/ much (potential) duplication of code here in > Agc::{init,configure,queueRequest} - that I have started to try to work > out how to defer most of the work to the > AgcMeanLumince::{init,configure,queueRequest} too. (See > https://git.uk.ideasonboard.com/kbingham/libcamera/pulls/18 for my > ongoing development branch of that) > > But I don't think I should block this work on the work I'm doing. > > I think we need to do better to keep IPU3,RKISP1,Mali-C55,SoftIPA > AEGC and AWB implementations aligned for features and capabilities and > controls. > > I think it should be possible that only the conversion of statistics and > conversion will occur in the platform specific IPA for those platforms. Yes, I think that development is sensible. It is maybe a bit of a philosophical question: Does libipa provide basic building blocks and every IPA creates it's own "camera" or is there a roughly defined libipa "camera" and the IPA contains only glue code. We are now moving into the second direction which I think makes sense. libipa can still serve as the basic block provider in case anyone wants to implement a completely different "camera" (which I doubt will happpen). If possible I'd like to still keep those topics separate to not pull in too many architectural changes into this series. Best regards, Stefan > > > setLimits(context.configuration.sensor.minExposureTime, > > context.configuration.sensor.maxExposureTime, > > context.configuration.sensor.minAnalogueGain, > > @@ -283,6 +286,10 @@ void Agc::queueRequest(IPAContext &context, > > if (!frameContext.agc.autoGainEnabled) > > frameContext.agc.gain = agc.manual.gain; > > > > + if (!frameContext.agc.autoExposureEnabled && > > + !frameContext.agc.autoGainEnabled) > > + frameContext.agc.quantizationGain = 1.0; > > + > > const auto &meteringMode = controls.get(controls::AeMeteringMode); > > if (meteringMode) { > > frameContext.agc.updateMetering = agc.meteringMode != *meteringMode; > > @@ -336,12 +343,17 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > > { > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure; > > double activeAutoGain = context.activeState.agc.automatic.gain; > > + double activeAutoQGain = context.activeState.agc.automatic.quantizationGain; > > > > /* Populate exposure and gain in auto mode */ > > - if (frameContext.agc.autoExposureEnabled) > > + if (frameContext.agc.autoExposureEnabled) { > > frameContext.agc.exposure = activeAutoExposure; > > - if (frameContext.agc.autoGainEnabled) > > + frameContext.agc.quantizationGain = activeAutoQGain; > > + } > > + if (frameContext.agc.autoGainEnabled) { > > frameContext.agc.gain = activeAutoGain; > > + frameContext.agc.quantizationGain = activeAutoQGain; > > + } > > > > /* > > * Populate manual exposure and gain from the active auto values when > > @@ -354,6 +366,12 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > > if (!frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange) { > > context.activeState.agc.manual.gain = activeAutoGain; > > frameContext.agc.gain = activeAutoGain; > > + frameContext.agc.quantizationGain = activeAutoQGain; > > + } > > + > > + if (context.activeState.compress.supported) { > > + frameContext.compress.enable = true; > > + frameContext.compress.gain = frameContext.agc.quantizationGain; > > } > > > > if (frame > 0 && !frameContext.agc.updateMetering) > > @@ -582,7 +600,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > /* Update the estimated exposure and gain. */ > > activeState.agc.automatic.exposure = newExposureTime / lineDuration; > > activeState.agc.automatic.gain = aGain; > > - > > + activeState.agc.automatic.quantizationGain = qGain; > > /* > > * Expand the target frame duration so that we do not run faster than > > * the minimum frame duration when we have short exposures. > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > index 37a74215ce19..362ab2fda5fe 100644 > > --- a/src/ipa/rkisp1/ipa_context.h > > +++ b/src/ipa/rkisp1/ipa_context.h > > @@ -77,6 +77,7 @@ struct IPAActiveState { > > struct { > > uint32_t exposure; > > double gain; > > + double quantizationGain; > > } automatic; > > > > bool autoExposureEnabled; > > @@ -135,6 +136,7 @@ struct IPAFrameContext : public FrameContext { > > uint32_t exposure; > > double gain; > > double exposureValue; > > + double quantizationGain; > > uint32_t vblank; > > bool autoExposureEnabled; > > bool autoGainEnabled; > > -- > > 2.48.1 > >
Quoting Stefan Klug (2025-08-13 10:26:38) > Hi Kieran, > > Thank you for the review. > > Quoting Kieran Bingham (2025-08-11 16:43:39) > > Quoting Stefan Klug (2025-08-08 15:12:47) > > > There are several occations where quantization can lead to visible > > > effects. > > > > > > In WDR mode it can happen that exposure times get set to very low values > > > (Sometimes 2-3 lines). This intentionally introduced underexposure is > > > corrected by the GWDR module. As exposure time is quantized by lines, > > > the smallest possible change in exposure time now results in a quite > > > visible change in perceived brightness. > > > > > > Sometimes the possible gain steps are also quite large leading to > > > visible jumps if e.g. if the exposure time is fixed. > > > > > > Mitigate that by applying a global gain to account for the error > > > introduced by the exposure quantization. > > > > > > ToDo: This needs perfect frame synchronous control of the sensor to work > > > properly which is not guaranteed in all cases. It still improves the > > > behavior with the current regulation and can easily be skipped, when > > > there is no compress algorithm. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > --- > > > src/ipa/rkisp1/algorithms/agc.cpp | 24 +++++++++++++++++++++--- > > > src/ipa/rkisp1/ipa_context.h | 2 ++ > > > 2 files changed, 23 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > index 0a29326841fb..d34c041f9fe1 100644 > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > @@ -199,6 +199,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > > context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width; > > > context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height; > > > > > > + AgcMeanLuminance::configure(context.configuration.sensor.lineDuration, > > > + context.camHelper.get()); > > > > It probably doesn't matter at the moment, but could/should the camHelper > > be initialiased at 'init' time ? It shouldn't ever change during > > configure() right ? It's only the lineDuration that could change at > > configure time. > > Yes, you're right that would be possible, but it also means another > function and a wider interface. I'm undecided... I think the common controls that AgcMeanLumince supports/creates should already move to the corresponding AgcMeanLuminance::init() anyway so I don't see this as a wider interface... https://git.uk.ideasonboard.com/kbingham/libcamera/src/commit/829f07e27b707a1696873c3d9109ff17c8dd0ff7/src/ipa/libipa/agc_mean_luminance.cpp#L396 But it doesn't have to be part of this series... > > > + > > > > There's /so/ much (potential) duplication of code here in > > Agc::{init,configure,queueRequest} - that I have started to try to work > > out how to defer most of the work to the > > AgcMeanLumince::{init,configure,queueRequest} too. (See > > https://git.uk.ideasonboard.com/kbingham/libcamera/pulls/18 for my > > ongoing development branch of that) > > > > But I don't think I should block this work on the work I'm doing. > > > > I think we need to do better to keep IPU3,RKISP1,Mali-C55,SoftIPA > > AEGC and AWB implementations aligned for features and capabilities and > > controls. > > > > I think it should be possible that only the conversion of statistics and > > conversion will occur in the platform specific IPA for those platforms. > > Yes, I think that development is sensible. It is maybe a bit of a > philosophical question: Does libipa provide basic building blocks and > every IPA creates it's own "camera" or is there a roughly defined libipa > "camera" and the IPA contains only glue code. > > We are now moving into the second direction which I think makes sense. > libipa can still serve as the basic block provider in case anyone wants > to implement a completely different "camera" (which I doubt will > happpen). > > If possible I'd like to still keep those topics separate to not pull in > too many architectural changes into this series. > Absolutely - I don't want to block this series on a huge refactor. I think libipa can serve as helpers and reference implementations alike. Having a full featured 'agc_mean_luminace' doesn't prevent someone making an IPA use helpers and implement things differently - my goal is just to reduce the burden on 'us' re-implementing the same thing 5 times in different places :D (and make it easy for new platforms to just 'slot' in an algorithm when desired). I'll leave it to you to decide for this series: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Best regards, > Stefan > > > > > > setLimits(context.configuration.sensor.minExposureTime, > > > context.configuration.sensor.maxExposureTime, > > > context.configuration.sensor.minAnalogueGain, > > > @@ -283,6 +286,10 @@ void Agc::queueRequest(IPAContext &context, > > > if (!frameContext.agc.autoGainEnabled) > > > frameContext.agc.gain = agc.manual.gain; > > > > > > + if (!frameContext.agc.autoExposureEnabled && > > > + !frameContext.agc.autoGainEnabled) > > > + frameContext.agc.quantizationGain = 1.0; > > > + > > > const auto &meteringMode = controls.get(controls::AeMeteringMode); > > > if (meteringMode) { > > > frameContext.agc.updateMetering = agc.meteringMode != *meteringMode; > > > @@ -336,12 +343,17 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > > > { > > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure; > > > double activeAutoGain = context.activeState.agc.automatic.gain; > > > + double activeAutoQGain = context.activeState.agc.automatic.quantizationGain; > > > > > > /* Populate exposure and gain in auto mode */ > > > - if (frameContext.agc.autoExposureEnabled) > > > + if (frameContext.agc.autoExposureEnabled) { > > > frameContext.agc.exposure = activeAutoExposure; > > > - if (frameContext.agc.autoGainEnabled) > > > + frameContext.agc.quantizationGain = activeAutoQGain; > > > + } > > > + if (frameContext.agc.autoGainEnabled) { > > > frameContext.agc.gain = activeAutoGain; > > > + frameContext.agc.quantizationGain = activeAutoQGain; > > > + } > > > > > > /* > > > * Populate manual exposure and gain from the active auto values when > > > @@ -354,6 +366,12 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > > > if (!frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange) { > > > context.activeState.agc.manual.gain = activeAutoGain; > > > frameContext.agc.gain = activeAutoGain; > > > + frameContext.agc.quantizationGain = activeAutoQGain; > > > + } > > > + > > > + if (context.activeState.compress.supported) { > > > + frameContext.compress.enable = true; > > > + frameContext.compress.gain = frameContext.agc.quantizationGain; > > > } > > > > > > if (frame > 0 && !frameContext.agc.updateMetering) > > > @@ -582,7 +600,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > > /* Update the estimated exposure and gain. */ > > > activeState.agc.automatic.exposure = newExposureTime / lineDuration; > > > activeState.agc.automatic.gain = aGain; > > > - > > > + activeState.agc.automatic.quantizationGain = qGain; > > > /* > > > * Expand the target frame duration so that we do not run faster than > > > * the minimum frame duration when we have short exposures. > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > index 37a74215ce19..362ab2fda5fe 100644 > > > --- a/src/ipa/rkisp1/ipa_context.h > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > @@ -77,6 +77,7 @@ struct IPAActiveState { > > > struct { > > > uint32_t exposure; > > > double gain; > > > + double quantizationGain; > > > } automatic; > > > > > > bool autoExposureEnabled; > > > @@ -135,6 +136,7 @@ struct IPAFrameContext : public FrameContext { > > > uint32_t exposure; > > > double gain; > > > double exposureValue; > > > + double quantizationGain; > > > uint32_t vblank; > > > bool autoExposureEnabled; > > > bool autoGainEnabled; > > > -- > > > 2.48.1 > > >
Hi Stefan On 08/08/2025 15:12, Stefan Klug wrote: > There are several occations where quantization can lead to visible > effects. > > In WDR mode it can happen that exposure times get set to very low values > (Sometimes 2-3 lines). This intentionally introduced underexposure is > corrected by the GWDR module. As exposure time is quantized by lines, > the smallest possible change in exposure time now results in a quite > visible change in perceived brightness. > > Sometimes the possible gain steps are also quite large leading to > visible jumps if e.g. if the exposure time is fixed. > > Mitigate that by applying a global gain to account for the error > introduced by the exposure quantization. > > ToDo: This needs perfect frame synchronous control of the sensor to work > properly which is not guaranteed in all cases. It still improves the > behavior with the current regulation and can easily be skipped, when > there is no compress algorithm. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> Looks ok to me: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 24 +++++++++++++++++++++--- > src/ipa/rkisp1/ipa_context.h | 2 ++ > 2 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 0a29326841fb..d34c041f9fe1 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -199,6 +199,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width; > context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height; > > + AgcMeanLuminance::configure(context.configuration.sensor.lineDuration, > + context.camHelper.get()); > + > setLimits(context.configuration.sensor.minExposureTime, > context.configuration.sensor.maxExposureTime, > context.configuration.sensor.minAnalogueGain, > @@ -283,6 +286,10 @@ void Agc::queueRequest(IPAContext &context, > if (!frameContext.agc.autoGainEnabled) > frameContext.agc.gain = agc.manual.gain; > > + if (!frameContext.agc.autoExposureEnabled && > + !frameContext.agc.autoGainEnabled) > + frameContext.agc.quantizationGain = 1.0; > + > const auto &meteringMode = controls.get(controls::AeMeteringMode); > if (meteringMode) { > frameContext.agc.updateMetering = agc.meteringMode != *meteringMode; > @@ -336,12 +343,17 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > { > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure; > double activeAutoGain = context.activeState.agc.automatic.gain; > + double activeAutoQGain = context.activeState.agc.automatic.quantizationGain; > > /* Populate exposure and gain in auto mode */ > - if (frameContext.agc.autoExposureEnabled) > + if (frameContext.agc.autoExposureEnabled) { > frameContext.agc.exposure = activeAutoExposure; > - if (frameContext.agc.autoGainEnabled) > + frameContext.agc.quantizationGain = activeAutoQGain; > + } > + if (frameContext.agc.autoGainEnabled) { > frameContext.agc.gain = activeAutoGain; > + frameContext.agc.quantizationGain = activeAutoQGain; > + } > > /* > * Populate manual exposure and gain from the active auto values when > @@ -354,6 +366,12 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > if (!frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange) { > context.activeState.agc.manual.gain = activeAutoGain; > frameContext.agc.gain = activeAutoGain; > + frameContext.agc.quantizationGain = activeAutoQGain; > + } > + > + if (context.activeState.compress.supported) { > + frameContext.compress.enable = true; > + frameContext.compress.gain = frameContext.agc.quantizationGain; > } > > if (frame > 0 && !frameContext.agc.updateMetering) > @@ -582,7 +600,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > /* Update the estimated exposure and gain. */ > activeState.agc.automatic.exposure = newExposureTime / lineDuration; > activeState.agc.automatic.gain = aGain; > - > + activeState.agc.automatic.quantizationGain = qGain; > /* > * Expand the target frame duration so that we do not run faster than > * the minimum frame duration when we have short exposures. > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index 37a74215ce19..362ab2fda5fe 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -77,6 +77,7 @@ struct IPAActiveState { > struct { > uint32_t exposure; > double gain; > + double quantizationGain; > } automatic; > > bool autoExposureEnabled; > @@ -135,6 +136,7 @@ struct IPAFrameContext : public FrameContext { > uint32_t exposure; > double gain; > double exposureValue; > + double quantizationGain; > uint32_t vblank; > bool autoExposureEnabled; > bool autoGainEnabled;
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 0a29326841fb..d34c041f9fe1 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -199,6 +199,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width; context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height; + AgcMeanLuminance::configure(context.configuration.sensor.lineDuration, + context.camHelper.get()); + setLimits(context.configuration.sensor.minExposureTime, context.configuration.sensor.maxExposureTime, context.configuration.sensor.minAnalogueGain, @@ -283,6 +286,10 @@ void Agc::queueRequest(IPAContext &context, if (!frameContext.agc.autoGainEnabled) frameContext.agc.gain = agc.manual.gain; + if (!frameContext.agc.autoExposureEnabled && + !frameContext.agc.autoGainEnabled) + frameContext.agc.quantizationGain = 1.0; + const auto &meteringMode = controls.get(controls::AeMeteringMode); if (meteringMode) { frameContext.agc.updateMetering = agc.meteringMode != *meteringMode; @@ -336,12 +343,17 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, { uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure; double activeAutoGain = context.activeState.agc.automatic.gain; + double activeAutoQGain = context.activeState.agc.automatic.quantizationGain; /* Populate exposure and gain in auto mode */ - if (frameContext.agc.autoExposureEnabled) + if (frameContext.agc.autoExposureEnabled) { frameContext.agc.exposure = activeAutoExposure; - if (frameContext.agc.autoGainEnabled) + frameContext.agc.quantizationGain = activeAutoQGain; + } + if (frameContext.agc.autoGainEnabled) { frameContext.agc.gain = activeAutoGain; + frameContext.agc.quantizationGain = activeAutoQGain; + } /* * Populate manual exposure and gain from the active auto values when @@ -354,6 +366,12 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, if (!frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange) { context.activeState.agc.manual.gain = activeAutoGain; frameContext.agc.gain = activeAutoGain; + frameContext.agc.quantizationGain = activeAutoQGain; + } + + if (context.activeState.compress.supported) { + frameContext.compress.enable = true; + frameContext.compress.gain = frameContext.agc.quantizationGain; } if (frame > 0 && !frameContext.agc.updateMetering) @@ -582,7 +600,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, /* Update the estimated exposure and gain. */ activeState.agc.automatic.exposure = newExposureTime / lineDuration; activeState.agc.automatic.gain = aGain; - + activeState.agc.automatic.quantizationGain = qGain; /* * Expand the target frame duration so that we do not run faster than * the minimum frame duration when we have short exposures. diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index 37a74215ce19..362ab2fda5fe 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -77,6 +77,7 @@ struct IPAActiveState { struct { uint32_t exposure; double gain; + double quantizationGain; } automatic; bool autoExposureEnabled; @@ -135,6 +136,7 @@ struct IPAFrameContext : public FrameContext { uint32_t exposure; double gain; double exposureValue; + double quantizationGain; uint32_t vblank; bool autoExposureEnabled; bool autoGainEnabled;
There are several occations where quantization can lead to visible effects. In WDR mode it can happen that exposure times get set to very low values (Sometimes 2-3 lines). This intentionally introduced underexposure is corrected by the GWDR module. As exposure time is quantized by lines, the smallest possible change in exposure time now results in a quite visible change in perceived brightness. Sometimes the possible gain steps are also quite large leading to visible jumps if e.g. if the exposure time is fixed. Mitigate that by applying a global gain to account for the error introduced by the exposure quantization. ToDo: This needs perfect frame synchronous control of the sensor to work properly which is not guaranteed in all cases. It still improves the behavior with the current regulation and can easily be skipped, when there is no compress algorithm. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/algorithms/agc.cpp | 24 +++++++++++++++++++++--- src/ipa/rkisp1/ipa_context.h | 2 ++ 2 files changed, 23 insertions(+), 3 deletions(-)