| Message ID | 20241016170348.715993-5-jacopo.mondi@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote: > The RkISP1 AGC algorithms assumes the metering mode to be > "MeteringMatrix" and pre-fill an array of weights associated > with it stored in meteringModes_[MeteringMatrix] when intializing > the algorithm in parseMeteringModes(). > > It laters fetches the weights when computing parameters using the > FrameContext.agc.meteringMode as index of the meteringModes_ array. > > When a Request gets queued to the algorithm, the > FrameContext.agc.meteringMode index is populated from the algorithm's > active state, and optionally updated if the application has specified > a different metering mode. > > In case of Request underrun however, a non-initialized FrameContext > gets provided to the algorithm, causing an (unhandled) exception when > accessing the meteringModes_ array. > > Fix this by intializing the AGC metering mode to a supported value > coming from the ActiveState in IPAFrameContext::init(). > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 2 ++ > src/ipa/rkisp1/ipa_context.cpp | 5 +++++ > 2 files changed, 7 insertions(+) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 17d074d9c03e..dd7e9468741e 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first); > context.activeState.agc.exposureMode = > static_cast<controls::AeExposureModeEnum>(exposureModeHelpers().begin()->first); > + > + /* Use the metering matrix mode by default. */ Strictly speaking, this isn't where the metering mode is defaulted to matrix; it's in parseMeteringModes(). > context.activeState.agc.meteringMode = > static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > index 2dad42b3154f..e88609137345 100644 > --- a/src/ipa/rkisp1/ipa_context.cpp > +++ b/src/ipa/rkisp1/ipa_context.cpp > @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum, > const ActiveState &activeState) > { > FrameContext::init(frameNum, activeState); > + > + const IPAActiveState *rkisp1ActiveState = > + reinterpret_cast<const IPAActiveState *>(&activeState); > + > + agc.meteringMode = rkisp1ActiveState->agc.meteringMode; But yes this looks like the right solution. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > } > > /** > -- > 2.47.0 >
Hi Jacopo, Thank you for the patch. On Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote: > The RkISP1 AGC algorithms assumes the metering mode to be > "MeteringMatrix" and pre-fill an array of weights associated > with it stored in meteringModes_[MeteringMatrix] when intializing > the algorithm in parseMeteringModes(). > > It laters fetches the weights when computing parameters using the > FrameContext.agc.meteringMode as index of the meteringModes_ array. > > When a Request gets queued to the algorithm, the > FrameContext.agc.meteringMode index is populated from the algorithm's > active state, and optionally updated if the application has specified > a different metering mode. > > In case of Request underrun however, a non-initialized FrameContext > gets provided to the algorithm, causing an (unhandled) exception when > accessing the meteringModes_ array. > > Fix this by intializing the AGC metering mode to a supported value > coming from the ActiveState in IPAFrameContext::init(). > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 2 ++ > src/ipa/rkisp1/ipa_context.cpp | 5 +++++ > 2 files changed, 7 insertions(+) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 17d074d9c03e..dd7e9468741e 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first); > context.activeState.agc.exposureMode = > static_cast<controls::AeExposureModeEnum>(exposureModeHelpers().begin()->first); > + > + /* Use the metering matrix mode by default. */ > context.activeState.agc.meteringMode = > static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > index 2dad42b3154f..e88609137345 100644 > --- a/src/ipa/rkisp1/ipa_context.cpp > +++ b/src/ipa/rkisp1/ipa_context.cpp > @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum, > const ActiveState &activeState) > { > FrameContext::init(frameNum, activeState); > + > + const IPAActiveState *rkisp1ActiveState = > + reinterpret_cast<const IPAActiveState *>(&activeState); > + > + agc.meteringMode = rkisp1ActiveState->agc.meteringMode; I don't particularly like that the IPAFrameContext needs that knowledge which should be internal to the agc algorithm. We could add a initContext() function to the algorithm base class. But then we need to loop over all algorithms here, which doesn't feel efficient for a single case. But on the other hand if we assume that request underruns become more common, then the other algorithms need to be able to continue with current data without a prior queueRequest() call. While writing this, yes I think we should forward that to every algorithm. Otherwise we might end up with strange effects because the algorithms use the data from the frame context that was reused without proper initialization. I hope it is somehow understandable. Cheers, Stefan > } > > /** > -- > 2.47.0 >
Hi Stefan On Mon, Oct 28, 2024 at 05:47:28PM +0100, Stefan Klug wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote: > > The RkISP1 AGC algorithms assumes the metering mode to be > > "MeteringMatrix" and pre-fill an array of weights associated > > with it stored in meteringModes_[MeteringMatrix] when intializing > > the algorithm in parseMeteringModes(). > > > > It laters fetches the weights when computing parameters using the > > FrameContext.agc.meteringMode as index of the meteringModes_ array. > > > > When a Request gets queued to the algorithm, the > > FrameContext.agc.meteringMode index is populated from the algorithm's > > active state, and optionally updated if the application has specified > > a different metering mode. > > > > In case of Request underrun however, a non-initialized FrameContext > > gets provided to the algorithm, causing an (unhandled) exception when > > accessing the meteringModes_ array. > > > > Fix this by intializing the AGC metering mode to a supported value > > coming from the ActiveState in IPAFrameContext::init(). > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/agc.cpp | 2 ++ > > src/ipa/rkisp1/ipa_context.cpp | 5 +++++ > > 2 files changed, 7 insertions(+) > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > index 17d074d9c03e..dd7e9468741e 100644 > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first); > > context.activeState.agc.exposureMode = > > static_cast<controls::AeExposureModeEnum>(exposureModeHelpers().begin()->first); > > + > > + /* Use the metering matrix mode by default. */ > > context.activeState.agc.meteringMode = > > static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > > index 2dad42b3154f..e88609137345 100644 > > --- a/src/ipa/rkisp1/ipa_context.cpp > > +++ b/src/ipa/rkisp1/ipa_context.cpp > > @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum, > > const ActiveState &activeState) > > { > > FrameContext::init(frameNum, activeState); > > + > > + const IPAActiveState *rkisp1ActiveState = > > + reinterpret_cast<const IPAActiveState *>(&activeState); > > + > > + agc.meteringMode = rkisp1ActiveState->agc.meteringMode; > > I don't particularly like that the IPAFrameContext needs that knowledge > which should be internal to the agc algorithm. We could add a You know, my feeling was the opposite. Algorithms should make less assumptions about their internals implementation details but rely on a more globally shared state, kept in the FrameContext and the ActiveState. This would make easy to "swap" algoroithms. After all, this already shows the AGC algo depends on a precise ordering of operations which if not respected results in a run-time exception. > initContext() function to the algorithm base class. But then we need to > loop over all algorithms here, which doesn't feel efficient for a single > case. But on the other hand if we assume that request underruns become I considered that but it seemed rather non-efficient. > more common, then the other algorithms need to be able to continue with > current data without a prior queueRequest() call. While writing this, Isn't the "current data" exactly the active state ? > yes I think we should forward that to every algorithm. Otherwise we > might end up with strange effects because the algorithms use the data > from the frame context that was reused without proper initialization. I might be missing how a FrameContext could be "reused" and "without proper initialization". I understand a FrameContext might not initialized, or not correctly re-initalized but if that happens is an implementation issue, isn't it ? > > I hope it is somehow understandable. Thank you! j > > Cheers, > Stefan > > > } > > > > /** > > -- > > 2.47.0 > >
Hi Jacopo, On Mon, Oct 28, 2024 at 06:14:42PM +0100, Jacopo Mondi wrote: > Hi Stefan > > On Mon, Oct 28, 2024 at 05:47:28PM +0100, Stefan Klug wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote: > > > The RkISP1 AGC algorithms assumes the metering mode to be > > > "MeteringMatrix" and pre-fill an array of weights associated > > > with it stored in meteringModes_[MeteringMatrix] when intializing > > > the algorithm in parseMeteringModes(). > > > > > > It laters fetches the weights when computing parameters using the > > > FrameContext.agc.meteringMode as index of the meteringModes_ array. > > > > > > When a Request gets queued to the algorithm, the > > > FrameContext.agc.meteringMode index is populated from the algorithm's > > > active state, and optionally updated if the application has specified > > > a different metering mode. > > > > > > In case of Request underrun however, a non-initialized FrameContext > > > gets provided to the algorithm, causing an (unhandled) exception when > > > accessing the meteringModes_ array. > > > > > > Fix this by intializing the AGC metering mode to a supported value > > > coming from the ActiveState in IPAFrameContext::init(). > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > src/ipa/rkisp1/algorithms/agc.cpp | 2 ++ > > > src/ipa/rkisp1/ipa_context.cpp | 5 +++++ > > > 2 files changed, 7 insertions(+) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > index 17d074d9c03e..dd7e9468741e 100644 > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > > static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first); > > > context.activeState.agc.exposureMode = > > > static_cast<controls::AeExposureModeEnum>(exposureModeHelpers().begin()->first); > > > + > > > + /* Use the metering matrix mode by default. */ > > > context.activeState.agc.meteringMode = > > > static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > > > index 2dad42b3154f..e88609137345 100644 > > > --- a/src/ipa/rkisp1/ipa_context.cpp > > > +++ b/src/ipa/rkisp1/ipa_context.cpp > > > @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum, > > > const ActiveState &activeState) > > > { > > > FrameContext::init(frameNum, activeState); > > > + > > > + const IPAActiveState *rkisp1ActiveState = > > > + reinterpret_cast<const IPAActiveState *>(&activeState); > > > + > > > + agc.meteringMode = rkisp1ActiveState->agc.meteringMode; > > > > I don't particularly like that the IPAFrameContext needs that knowledge > > which should be internal to the agc algorithm. We could add a > > You know, my feeling was the opposite. Algorithms should make less > assumptions about their internals implementation details but rely on a > more globally shared state, kept in the FrameContext and the > ActiveState. This would make easy to "swap" algoroithms. > > After all, this already shows the AGC algo depends on a precise > ordering of operations which if not respected results in a run-time > exception. > > > initContext() function to the algorithm base class. But then we need to > > loop over all algorithms here, which doesn't feel efficient for a single > > case. But on the other hand if we assume that request underruns become > > I considered that but it seemed rather non-efficient. Agreed, but I'm not sure if we can completely avoid it. Maybe with some more restructuring... > > > more common, then the other algorithms need to be able to continue with > > current data without a prior queueRequest() call. While writing this, > > Isn't the "current data" exactly the active state ? The active state is the current measuered/stats data. But the current manual (user supplied) data is stored in the frame context. > > > yes I think we should forward that to every algorithm. Otherwise we > > might end up with strange effects because the algorithms use the data > > from the frame context that was reused without proper initialization. > > I might be missing how a FrameContext could be "reused" and "without > proper initialization". I understand a FrameContext might not > initialized, or not correctly re-initalized but if that happens is an > implementation issue, isn't it ? Let's see if I can get this straight. For reference I take the awb algo. Let's assume the following gets queued: Frame 1: awb.autoEnable = false, gain.blue = 4 Frame 2: awb.autoEnable = true ... no more requests get queued ... Let's further assume the FCQueue has a length of 2. Now no more requests get queued. But process() still get's called because the internal machinery is now ticked by the sensor producing images. That results in a cyclic reuse of the two frame comtexts. Now the blue gain gets constantly toggled between 4 and the automatic value. As the stats calculation depends on the gains there will be unexpected effects. I expect there are more cases like this and I'm not sure if we should handle them inside FrameContext::init() Maybe I'm missing something obvious... Cheers, Stefan > > > > > I hope it is somehow understandable. > > Thank you! > j > > > > > Cheers, > > Stefan > > > > > } > > > > > > /** > > > -- > > > 2.47.0 > > >
Hi Stefan On Mon, Oct 28, 2024 at 07:12:57PM +0100, Stefan Klug wrote: > Hi Jacopo, > > On Mon, Oct 28, 2024 at 06:14:42PM +0100, Jacopo Mondi wrote: > > Hi Stefan > > > > On Mon, Oct 28, 2024 at 05:47:28PM +0100, Stefan Klug wrote: > > > Hi Jacopo, > > > > > > Thank you for the patch. > > > > > > On Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote: > > > > The RkISP1 AGC algorithms assumes the metering mode to be > > > > "MeteringMatrix" and pre-fill an array of weights associated > > > > with it stored in meteringModes_[MeteringMatrix] when intializing > > > > the algorithm in parseMeteringModes(). > > > > > > > > It laters fetches the weights when computing parameters using the > > > > FrameContext.agc.meteringMode as index of the meteringModes_ array. > > > > > > > > When a Request gets queued to the algorithm, the > > > > FrameContext.agc.meteringMode index is populated from the algorithm's > > > > active state, and optionally updated if the application has specified > > > > a different metering mode. > > > > > > > > In case of Request underrun however, a non-initialized FrameContext > > > > gets provided to the algorithm, causing an (unhandled) exception when > > > > accessing the meteringModes_ array. > > > > > > > > Fix this by intializing the AGC metering mode to a supported value > > > > coming from the ActiveState in IPAFrameContext::init(). > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > src/ipa/rkisp1/algorithms/agc.cpp | 2 ++ > > > > src/ipa/rkisp1/ipa_context.cpp | 5 +++++ > > > > 2 files changed, 7 insertions(+) > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > > index 17d074d9c03e..dd7e9468741e 100644 > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > > @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > > > static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first); > > > > context.activeState.agc.exposureMode = > > > > static_cast<controls::AeExposureModeEnum>(exposureModeHelpers().begin()->first); > > > > + > > > > + /* Use the metering matrix mode by default. */ > > > > context.activeState.agc.meteringMode = > > > > static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); > > > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > > > > index 2dad42b3154f..e88609137345 100644 > > > > --- a/src/ipa/rkisp1/ipa_context.cpp > > > > +++ b/src/ipa/rkisp1/ipa_context.cpp > > > > @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum, > > > > const ActiveState &activeState) > > > > { > > > > FrameContext::init(frameNum, activeState); > > > > + > > > > + const IPAActiveState *rkisp1ActiveState = > > > > + reinterpret_cast<const IPAActiveState *>(&activeState); > > > > + > > > > + agc.meteringMode = rkisp1ActiveState->agc.meteringMode; > > > > > > I don't particularly like that the IPAFrameContext needs that knowledge > > > which should be internal to the agc algorithm. We could add a > > > > You know, my feeling was the opposite. Algorithms should make less > > assumptions about their internals implementation details but rely on a > > more globally shared state, kept in the FrameContext and the > > ActiveState. This would make easy to "swap" algoroithms. > > > > After all, this already shows the AGC algo depends on a precise > > ordering of operations which if not respected results in a run-time > > exception. > > > > > initContext() function to the algorithm base class. But then we need to > > > loop over all algorithms here, which doesn't feel efficient for a single > > > case. But on the other hand if we assume that request underruns become > > > > I considered that but it seemed rather non-efficient. > > Agreed, but I'm not sure if we can completely avoid it. Maybe with some > more restructuring... > > > > > > more common, then the other algorithms need to be able to continue with > > > current data without a prior queueRequest() call. While writing this, > > > > Isn't the "current data" exactly the active state ? > > The active state is the current measuered/stats data. But the current manual > (user supplied) data is stored in the frame context. > As the name suggests, per-frame settings are indeed stored in the frame context, but the active state keeps track of the algorithm's state. In example, the AGC algo for RkISP1 stores in the active state the auto/manual mode and the associated exposure time and gain. As I see it, and might be my partial understanding of algos, the FrameContext can be initialized using the active state values, then: - when alloc()-ed during a queueRequest values coming from the application ControlList will override both the FrameContext and the ActiveState as it happens today - when get() without an alloc (Request underrun) the values it has been initialized to will continue to be in use and the algorithms can keep operating without a user Request What I'm not sure about is, in case of alloc(), if we need the initialization or not, as the newly alloc()-ed FrameContext will be passed through Algo::queueRequest(), so values copied from the ActiveState will be overwritten anyway. > > > > > yes I think we should forward that to every algorithm. Otherwise we > > > might end up with strange effects because the algorithms use the data > > > from the frame context that was reused without proper initialization. > > > > I might be missing how a FrameContext could be "reused" and "without > > proper initialization". I understand a FrameContext might not > > initialized, or not correctly re-initalized but if that happens is an > > implementation issue, isn't it ? > > Let's see if I can get this straight. For reference I take the awb algo. > Let's assume the following gets queued: > > Frame 1: awb.autoEnable = false, gain.blue = 4 > Frame 2: awb.autoEnable = true > ... no more requests get queued ... > > Let's further assume the FCQueue has a length of 2. Now no more requests > get queued. But process() still get's called because the internal > machinery is now ticked by the sensor producing images. That results in > a cyclic reuse of the two frame comtexts. Now the blue gain gets That shouldn't happen. What have I missed ? In my understanding, following your example, we have FCQ: frame#0 frame#1 +--------+--------+ |auto = f|auto = t| |blue = 4| | +--------+--------+ They both get alloc()-ed and passed through Awb::queueRequest() in IPA::queueRequest(0) and IPA::queueRequest(1) The call to Awb::queueRequest(1) will void Awb::queueRequest(IPAContext &context, [[maybe_unused]] const uint32_t frame, IPAFrameContext &frameContext, const ControlList &controls) { auto &awb = context.activeState.awb; const auto &awbEnable = controls.get(controls::AwbEnable); if (awbEnable && *awbEnable != awb.autoEnabled) { awb.autoEnabled = *awbEnable; LOG(RkISP1Awb, Debug) << (*awbEnable ? "Enabling" : "Disabling") << " AWB"; } ... } So now ActiveContext contains awb.autoEnabled = true Now a new frame gets produced, and no Request has been queued, so we get(2) without a preceding alloc(2). We call get(2) and hence we target FCQ[0] which has already been alloc() to store frame#0 settings FrameContext &get(uint32_t frame, const ActiveState &activeState) { FrameContext &frameContext = contexts_[frame % contexts_.size()]; if (frame < frameContext.frame) /* * We don't get here, frame = 2, * frameContext.frame = 0 */ if (frame == 0 && !frameContext.initialised) /* We don't get here, frame = 2 */ if (frame == frameContext.frame) /* * We don't get here, frame = 2, * frameContext.frame = 0 */ /* * So we get here, where we WARN and re-initialize the * frame context */ LOG(FCQueue, Warning) << "Obtained an uninitialised FrameContext for " << frame; frameContext.init(frame, activeState); return frameContext; } The frameContext is re-initialized with the current activeState values so with autoEnabled=true and passed to process(). Now, how do we recover from a Request underrun is what we have to design, but assuming no futher Request gets queued, I don't see the algo oscillating between auto and manual.gain.blue = 4. What am I missing ? > constantly toggled between 4 and the automatic value. As the stats > calculation depends on the gains there will be unexpected effects. > > I expect there are more cases like this and I'm not sure if we should > handle them inside FrameContext::init() > > Maybe I'm missing something obvious... Maybe I am. The above is my interpretation of how thing should work. If we agree but you see a different behaviour, then we might have a bug somewhere (which I'm not seeing). > > Cheers, > Stefan Thanks! > > > > > > > > > I hope it is somehow understandable. > > > > Thank you! > > j > > > > > > > > Cheers, > > > Stefan > > > > > > > } > > > > > > > > /** > > > > -- > > > > 2.47.0 > > > >
Hi Jacopo, On Tue, Oct 29, 2024 at 08:49:26AM +0100, Jacopo Mondi wrote: > Hi Stefan > > On Mon, Oct 28, 2024 at 07:12:57PM +0100, Stefan Klug wrote: > > Hi Jacopo, > > > > On Mon, Oct 28, 2024 at 06:14:42PM +0100, Jacopo Mondi wrote: > > > Hi Stefan > > > > > > On Mon, Oct 28, 2024 at 05:47:28PM +0100, Stefan Klug wrote: > > > > Hi Jacopo, > > > > > > > > Thank you for the patch. > > > > > > > > On Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote: > > > > > The RkISP1 AGC algorithms assumes the metering mode to be > > > > > "MeteringMatrix" and pre-fill an array of weights associated > > > > > with it stored in meteringModes_[MeteringMatrix] when intializing > > > > > the algorithm in parseMeteringModes(). > > > > > > > > > > It laters fetches the weights when computing parameters using the > > > > > FrameContext.agc.meteringMode as index of the meteringModes_ array. > > > > > > > > > > When a Request gets queued to the algorithm, the > > > > > FrameContext.agc.meteringMode index is populated from the algorithm's > > > > > active state, and optionally updated if the application has specified > > > > > a different metering mode. > > > > > > > > > > In case of Request underrun however, a non-initialized FrameContext > > > > > gets provided to the algorithm, causing an (unhandled) exception when > > > > > accessing the meteringModes_ array. > > > > > > > > > > Fix this by intializing the AGC metering mode to a supported value > > > > > coming from the ActiveState in IPAFrameContext::init(). > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > --- > > > > > src/ipa/rkisp1/algorithms/agc.cpp | 2 ++ > > > > > src/ipa/rkisp1/ipa_context.cpp | 5 +++++ > > > > > 2 files changed, 7 insertions(+) > > > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > > > index 17d074d9c03e..dd7e9468741e 100644 > > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > > > @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > > > > static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first); > > > > > context.activeState.agc.exposureMode = > > > > > static_cast<controls::AeExposureModeEnum>(exposureModeHelpers().begin()->first); > > > > > + > > > > > + /* Use the metering matrix mode by default. */ > > > > > context.activeState.agc.meteringMode = > > > > > static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); > > > > > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > > > > > index 2dad42b3154f..e88609137345 100644 > > > > > --- a/src/ipa/rkisp1/ipa_context.cpp > > > > > +++ b/src/ipa/rkisp1/ipa_context.cpp > > > > > @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum, > > > > > const ActiveState &activeState) > > > > > { > > > > > FrameContext::init(frameNum, activeState); > > > > > + > > > > > + const IPAActiveState *rkisp1ActiveState = > > > > > + reinterpret_cast<const IPAActiveState *>(&activeState); > > > > > + > > > > > + agc.meteringMode = rkisp1ActiveState->agc.meteringMode; > > > > > > > > I don't particularly like that the IPAFrameContext needs that knowledge > > > > which should be internal to the agc algorithm. We could add a > > > > > > You know, my feeling was the opposite. Algorithms should make less > > > assumptions about their internals implementation details but rely on a > > > more globally shared state, kept in the FrameContext and the > > > ActiveState. This would make easy to "swap" algoroithms. > > > > > > After all, this already shows the AGC algo depends on a precise > > > ordering of operations which if not respected results in a run-time > > > exception. > > > > > > > initContext() function to the algorithm base class. But then we need to > > > > loop over all algorithms here, which doesn't feel efficient for a single > > > > case. But on the other hand if we assume that request underruns become > > > > > > I considered that but it seemed rather non-efficient. > > > > Agreed, but I'm not sure if we can completely avoid it. Maybe with some > > more restructuring... > > > > > > > > > more common, then the other algorithms need to be able to continue with > > > > current data without a prior queueRequest() call. While writing this, > > > > > > Isn't the "current data" exactly the active state ? > > > > The active state is the current measuered/stats data. But the current manual > > (user supplied) data is stored in the frame context. > > > > As the name suggests, per-frame settings are indeed stored in the > frame context, but the active state keeps track of the algorithm's > state. In example, the AGC algo for RkISP1 stores in the active state > the auto/manual mode and the associated exposure time and gain. > > As I see it, and might be my partial understanding of algos, the > FrameContext can be initialized using the active state values, then: > > - when alloc()-ed during a queueRequest values coming from the application > ControlList will override both the FrameContext and the ActiveState as > it happens today > > - when get() without an alloc (Request underrun) the values it has > been initialized to will continue to be in use and the algorithms > can keep operating without a user Request > > What I'm not sure about is, in case of alloc(), if we need the > initialization or not, as the newly alloc()-ed FrameContext will be > passed through Algo::queueRequest(), so values copied from the > ActiveState will be overwritten anyway. I wrote the following paragraph last. Maybe you should read the rest first :-) What about renaming Algorithm::queueRequest to Algorithm::initFrameContext which gets called once in both code paths (Either after/in alloc() or after/in get() without prior alloc()) For the get() without alloc we just pass an empty ControlsList. > > > > > > > > yes I think we should forward that to every algorithm. Otherwise we > > > > might end up with strange effects because the algorithms use the data > > > > from the frame context that was reused without proper initialization. > > > > > > I might be missing how a FrameContext could be "reused" and "without > > > proper initialization". I understand a FrameContext might not > > > initialized, or not correctly re-initalized but if that happens is an > > > implementation issue, isn't it ? > > > > Let's see if I can get this straight. For reference I take the awb algo. > > Let's assume the following gets queued: > > > > Frame 1: awb.autoEnable = false, gain.blue = 4 > > Frame 2: awb.autoEnable = true > > ... no more requests get queued ... > > > > Let's further assume the FCQueue has a length of 2. Now no more requests > > get queued. But process() still get's called because the internal > > machinery is now ticked by the sensor producing images. That results in > > a cyclic reuse of the two frame comtexts. Now the blue gain gets > > That shouldn't happen. What have I missed ? > > In my understanding, following your example, we have > > FCQ: > frame#0 frame#1 > +--------+--------+ > |auto = f|auto = t| > |blue = 4| | > +--------+--------+ > > They both get alloc()-ed and passed through Awb::queueRequest() in > IPA::queueRequest(0) and IPA::queueRequest(1) > > The call to Awb::queueRequest(1) will > > void Awb::queueRequest(IPAContext &context, > [[maybe_unused]] const uint32_t frame, > IPAFrameContext &frameContext, > const ControlList &controls) > { > auto &awb = context.activeState.awb; > > const auto &awbEnable = controls.get(controls::AwbEnable); > if (awbEnable && *awbEnable != awb.autoEnabled) { > awb.autoEnabled = *awbEnable; > > LOG(RkISP1Awb, Debug) > << (*awbEnable ? "Enabling" : "Disabling") << " AWB"; > } > > ... > > } > > So now ActiveContext contains > > awb.autoEnabled = true > > Now a new frame gets produced, and no Request has been queued, so we get(2) > without a preceding alloc(2). > > We call get(2) and hence we target FCQ[0] which has already been alloc() > to store frame#0 settings > > FrameContext &get(uint32_t frame, const ActiveState &activeState) > { > FrameContext &frameContext = contexts_[frame % contexts_.size()]; > > if (frame < frameContext.frame) > /* > * We don't get here, frame = 2, > * frameContext.frame = 0 > */ > > > if (frame == 0 && !frameContext.initialised) > /* We don't get here, frame = 2 */ > > if (frame == frameContext.frame) > /* > * We don't get here, frame = 2, > * frameContext.frame = 0 > */ > > /* > * So we get here, where we WARN and re-initialize the > * frame context > */ > LOG(FCQueue, Warning) > << "Obtained an uninitialised FrameContext for " << frame; > > frameContext.init(frame, activeState); > > return frameContext; > } > > The frameContext is re-initialized with the current activeState values > so with autoEnabled=true and passed to process(). Here is the catch (or the piece I can't see). Who does the assignment frameContext.awb.autoEnable = activeState.awb.autoEnable? It has to be inside the frameContext.init() function. Isn't that the place where we started off with the discussion? That we could either hardcode it inside IPAFrameContext::init() if the frame context has enough information on what to initialize from activeState or forward that to the corresponding algorithm and implement a Algorithm::initFrameContext(fc, activeState). > > Now, how do we recover from a Request underrun is what we have to > design, but assuming no futher Request gets queued, I don't see the > algo oscillating between auto and manual.gain.blue = 4. > > What am I missing ? I think we are on the same page. The only difference I see is that I tend to move things into the agorithms to keep all that logic in one place and I believe you see the algorithms more as a pluggable formula which operate on the state that is kept (and known) in frame context and therefore frame context is able to initialize it. > > > constantly toggled between 4 and the automatic value. As the stats > > calculation depends on the gains there will be unexpected effects. > > > > I expect there are more cases like this and I'm not sure if we should > > handle them inside FrameContext::init() > > > > Maybe I'm missing something obvious... > > Maybe I am. > > The above is my interpretation of how thing should work. If we agree > but you see a different behaviour, then we might have a bug somewhere > (which I'm not seeing). I think we agree in the overall picture. Maybe one approach to solve this is to further fill the FrameContext::init() to see how much algorithm specific knowledge we need there. This boils down to copying all assignments to frameContext from the Algorihm::queueRequest() functions into the FrameContext::init() or try to rename Algorithm::queueRequest as proposed above. Best regards, Stefan > > > > > Cheers, > > Stefan > > Thanks! > > > > > > > > > > > > > > I hope it is somehow understandable. > > > > > > Thank you! > > > j > > > > > > > > > > > Cheers, > > > > Stefan > > > > > > > > > } > > > > > > > > > > /** > > > > > -- > > > > > 2.47.0 > > > > >
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 17d074d9c03e..dd7e9468741e 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first); context.activeState.agc.exposureMode = static_cast<controls::AeExposureModeEnum>(exposureModeHelpers().begin()->first); + + /* Use the metering matrix mode by default. */ context.activeState.agc.meteringMode = static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp index 2dad42b3154f..e88609137345 100644 --- a/src/ipa/rkisp1/ipa_context.cpp +++ b/src/ipa/rkisp1/ipa_context.cpp @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum, const ActiveState &activeState) { FrameContext::init(frameNum, activeState); + + const IPAActiveState *rkisp1ActiveState = + reinterpret_cast<const IPAActiveState *>(&activeState); + + agc.meteringMode = rkisp1ActiveState->agc.meteringMode; } /**
The RkISP1 AGC algorithms assumes the metering mode to be "MeteringMatrix" and pre-fill an array of weights associated with it stored in meteringModes_[MeteringMatrix] when intializing the algorithm in parseMeteringModes(). It laters fetches the weights when computing parameters using the FrameContext.agc.meteringMode as index of the meteringModes_ array. When a Request gets queued to the algorithm, the FrameContext.agc.meteringMode index is populated from the algorithm's active state, and optionally updated if the application has specified a different metering mode. In case of Request underrun however, a non-initialized FrameContext gets provided to the algorithm, causing an (unhandled) exception when accessing the meteringModes_ array. Fix this by intializing the AGC metering mode to a supported value coming from the ActiveState in IPAFrameContext::init(). Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- src/ipa/rkisp1/algorithms/agc.cpp | 2 ++ src/ipa/rkisp1/ipa_context.cpp | 5 +++++ 2 files changed, 7 insertions(+)