| Message ID | 20241014154747.2295253-2-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 IPA calculates and reports the FrameDurationLimits to applications > by configuring the ControlInfo accordingly during > IPARkISP1::updateControls() > > We later need to know these limits during Agc::configure() for > initialising the ActiveState of the AGC implementation with the limits. > > Store the FrameDurationLimits ControlInfo in the ControlInfoMap which is > now present in the IPAContext so that it is commonly available for the > AGC algorithm, removing the 'todo' accordingly. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 9 +++------ > src/ipa/rkisp1/rkisp1.cpp | 5 ++--- > 2 files changed, 5 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 17d074d9c03e..33f902862c4a 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -178,12 +178,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > context.activeState.agc.meteringMode = > static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); > > - /* > - * \todo This should probably come from FrameDurationLimits instead, > - * except it's computed in the IPA and not here so we'd have to > - * recompute it. > - */ > - context.activeState.agc.maxFrameDuration = context.configuration.sensor.maxShutterSpeed; > + /* Limit the frame duration to match current initialisation */ s/initialisation/initialization./ Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > + ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits]; > + context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>()); > > /* > * Define the measurement window for AGC as a centered rectangle > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 9e161cabdea4..47777ece783f 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -432,9 +432,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); > } > > - ctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0], > - frameDurations[1], > - frameDurations[2]); > + context_.ctrlMap[&controls::FrameDurationLimits] = > + ControlInfo(frameDurations[0], frameDurations[1], frameDurations[2]); > > ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end()); > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
Hi Kieran On 14/10/2024 16:47, Kieran Bingham wrote: > The IPA calculates and reports the FrameDurationLimits to applications > by configuring the ControlInfo accordingly during > IPARkISP1::updateControls() > > We later need to know these limits during Agc::configure() for > initialising the ActiveState of the AGC implementation with the limits. > > Store the FrameDurationLimits ControlInfo in the ControlInfoMap which is > now present in the IPAContext so that it is commonly available for the > AGC algorithm, removing the 'todo' accordingly. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 9 +++------ > src/ipa/rkisp1/rkisp1.cpp | 5 ++--- > 2 files changed, 5 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 17d074d9c03e..33f902862c4a 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -178,12 +178,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > context.activeState.agc.meteringMode = > static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); > > - /* > - * \todo This should probably come from FrameDurationLimits instead, > - * except it's computed in the IPA and not here so we'd have to > - * recompute it. > - */ > - context.activeState.agc.maxFrameDuration = context.configuration.sensor.maxShutterSpeed; > + /* Limit the frame duration to match current initialisation */ > + ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits]; > + context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>()); > > /* > * Define the measurement window for AGC as a centered rectangle > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 9e161cabdea4..47777ece783f 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -432,9 +432,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); > } > > - ctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0], > - frameDurations[1], > - frameDurations[2]); > + context_.ctrlMap[&controls::FrameDurationLimits] = > + ControlInfo(frameDurations[0], frameDurations[1], frameDurations[2]); > > ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end()); > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
Hi Kieran On Mon, Oct 14, 2024 at 04:47:45PM +0100, Kieran Bingham wrote: > The IPA calculates and reports the FrameDurationLimits to applications > by configuring the ControlInfo accordingly during > IPARkISP1::updateControls() > > We later need to know these limits during Agc::configure() for > initialising the ActiveState of the AGC implementation with the limits. > > Store the FrameDurationLimits ControlInfo in the ControlInfoMap which is > now present in the IPAContext so that it is commonly available for the > AGC algorithm, removing the 'todo' accordingly. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > src/ipa/rkisp1/algorithms/agc.cpp | 9 +++------ > src/ipa/rkisp1/rkisp1.cpp | 5 ++--- > 2 files changed, 5 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 17d074d9c03e..33f902862c4a 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -178,12 +178,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > context.activeState.agc.meteringMode = > static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); > > - /* > - * \todo This should probably come from FrameDurationLimits instead, > - * except it's computed in the IPA and not here so we'd have to > - * recompute it. > - */ > - context.activeState.agc.maxFrameDuration = context.configuration.sensor.maxShutterSpeed; > + /* Limit the frame duration to match current initialisation */ > + ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits]; > + context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>()); > > /* > * Define the measurement window for AGC as a centered rectangle > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 9e161cabdea4..47777ece783f 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -432,9 +432,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); > } > > - ctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0], > - frameDurations[1], > - frameDurations[2]); > + context_.ctrlMap[&controls::FrameDurationLimits] = > + ControlInfo(frameDurations[0], frameDurations[1], frameDurations[2]); > > ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end()); > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); > -- > 2.34.1 >
On Mon, Oct 14, 2024 at 04:47:45PM +0100, Kieran Bingham wrote: > The IPA calculates and reports the FrameDurationLimits to applications > by configuring the ControlInfo accordingly during > IPARkISP1::updateControls() > > We later need to know these limits during Agc::configure() for > initialising the ActiveState of the AGC implementation with the limits. > > Store the FrameDurationLimits ControlInfo in the ControlInfoMap which is > now present in the IPAContext so that it is commonly available for the > AGC algorithm, removing the 'todo' accordingly. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 9 +++------ > src/ipa/rkisp1/rkisp1.cpp | 5 ++--- > 2 files changed, 5 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 17d074d9c03e..33f902862c4a 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -178,12 +178,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > context.activeState.agc.meteringMode = > static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); > > - /* > - * \todo This should probably come from FrameDurationLimits instead, > - * except it's computed in the IPA and not here so we'd have to > - * recompute it. > - */ > - context.activeState.agc.maxFrameDuration = context.configuration.sensor.maxShutterSpeed; > + /* Limit the frame duration to match current initialisation */ > + ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits]; const > + context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>()); That's long, maybe context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>()); or int64_t maxFrameDuration = frameDurationLimits.max().get<int64_t>(); context.activeState.agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration); > > /* > * Define the measurement window for AGC as a centered rectangle > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 9e161cabdea4..47777ece783f 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -432,9 +432,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); > } > > - ctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0], > - frameDurations[1], > - frameDurations[2]); > + context_.ctrlMap[&controls::FrameDurationLimits] = > + ControlInfo(frameDurations[0], frameDurations[1], frameDurations[2]); I wonder if all this should be moved to the AGC algorithm. There are cross-dependencies between the IPARkISP1 class and the AGC algorithm that make the code fragile. Cleanups would be nice. > > ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end()); > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
On Mon, Oct 14, 2024 at 04:47:45PM +0100, Kieran Bingham wrote: > The IPA calculates and reports the FrameDurationLimits to applications > by configuring the ControlInfo accordingly during > IPARkISP1::updateControls() > > We later need to know these limits during Agc::configure() for > initialising the ActiveState of the AGC implementation with the limits. > > Store the FrameDurationLimits ControlInfo in the ControlInfoMap which is > now present in the IPAContext so that it is commonly available for the > AGC algorithm, removing the 'todo' accordingly. \o/ > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 9 +++------ > src/ipa/rkisp1/rkisp1.cpp | 5 ++--- > 2 files changed, 5 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 17d074d9c03e..33f902862c4a 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -178,12 +178,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > context.activeState.agc.meteringMode = > static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); > > - /* > - * \todo This should probably come from FrameDurationLimits instead, > - * except it's computed in the IPA and not here so we'd have to > - * recompute it. > - */ > - context.activeState.agc.maxFrameDuration = context.configuration.sensor.maxShutterSpeed; > + /* Limit the frame duration to match current initialisation */ > + ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits]; > + context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>()); > > /* > * Define the measurement window for AGC as a centered rectangle > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 9e161cabdea4..47777ece783f 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -432,9 +432,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); > } > > - ctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0], > - frameDurations[1], > - frameDurations[2]); > + context_.ctrlMap[&controls::FrameDurationLimits] = > + ControlInfo(frameDurations[0], frameDurations[1], frameDurations[2]); > > ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end()); > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); > -- > 2.34.1 >
On Sun, Oct 27, 2024 at 08:54:02PM +0200, Laurent Pinchart wrote: > On Mon, Oct 14, 2024 at 04:47:45PM +0100, Kieran Bingham wrote: > > The IPA calculates and reports the FrameDurationLimits to applications > > by configuring the ControlInfo accordingly during > > IPARkISP1::updateControls() > > > > We later need to know these limits during Agc::configure() for > > initialising the ActiveState of the AGC implementation with the limits. > > > > Store the FrameDurationLimits ControlInfo in the ControlInfoMap which is > > now present in the IPAContext so that it is commonly available for the > > AGC algorithm, removing the 'todo' accordingly. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/agc.cpp | 9 +++------ > > src/ipa/rkisp1/rkisp1.cpp | 5 ++--- > > 2 files changed, 5 insertions(+), 9 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > index 17d074d9c03e..33f902862c4a 100644 > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > @@ -178,12 +178,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > context.activeState.agc.meteringMode = > > static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); > > > > - /* > > - * \todo This should probably come from FrameDurationLimits instead, > > - * except it's computed in the IPA and not here so we'd have to > > - * recompute it. > > - */ > > - context.activeState.agc.maxFrameDuration = context.configuration.sensor.maxShutterSpeed; > > + /* Limit the frame duration to match current initialisation */ > > + ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits]; > > const > > > + context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>()); > > That's long, maybe > > context.activeState.agc.maxFrameDuration = > std::chrono::microseconds(frameDurationLimits.max().get<int64_t>()); > > or > > int64_t maxFrameDuration = frameDurationLimits.max().get<int64_t>(); > context.activeState.agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration); > > > > /* > > * Define the measurement window for AGC as a centered rectangle > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index 9e161cabdea4..47777ece783f 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -432,9 +432,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > > frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); > > } > > > > - ctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0], > > - frameDurations[1], > > - frameDurations[2]); > > + context_.ctrlMap[&controls::FrameDurationLimits] = > > + ControlInfo(frameDurations[0], frameDurations[1], frameDurations[2]); > > I wonder if all this should be moved to the AGC algorithm. There are > cross-dependencies between the IPARkISP1 class and the AGC algorithm > that make the code fragile. Cleanups would be nice. Probably. It'll need some change in the rkisp1 algorithm interface because we'd have to pass sensor controls to the algorithms at both init and configure time. I'll add a todo at least. Paul > > > > > ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end()); > > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 17d074d9c03e..33f902862c4a 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -178,12 +178,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) context.activeState.agc.meteringMode = static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first); - /* - * \todo This should probably come from FrameDurationLimits instead, - * except it's computed in the IPA and not here so we'd have to - * recompute it. - */ - context.activeState.agc.maxFrameDuration = context.configuration.sensor.maxShutterSpeed; + /* Limit the frame duration to match current initialisation */ + ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits]; + context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>()); /* * Define the measurement window for AGC as a centered rectangle diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 9e161cabdea4..47777ece783f 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -432,9 +432,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); } - ctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0], - frameDurations[1], - frameDurations[2]); + context_.ctrlMap[&controls::FrameDurationLimits] = + ControlInfo(frameDurations[0], frameDurations[1], frameDurations[2]); ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end()); *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
The IPA calculates and reports the FrameDurationLimits to applications by configuring the ControlInfo accordingly during IPARkISP1::updateControls() We later need to know these limits during Agc::configure() for initialising the ActiveState of the AGC implementation with the limits. Store the FrameDurationLimits ControlInfo in the ControlInfoMap which is now present in the IPAContext so that it is commonly available for the AGC algorithm, removing the 'todo' accordingly. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/ipa/rkisp1/algorithms/agc.cpp | 9 +++------ src/ipa/rkisp1/rkisp1.cpp | 5 ++--- 2 files changed, 5 insertions(+), 9 deletions(-)