| Message ID | 20210602102326.106549-4-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Wed, Jun 02, 2021 at 03:53:25PM +0530, Umang Jain wrote: > IPACameraSensorInfo members will be needed at various places in the > IPAIPU3 class, in subsequent commits. Hence, it seems trivial to copy > this structure for wider availability throughout the class. > > This commit does not introduce any functional changes. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/ipa/ipu3/ipu3.cpp | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index 2496b0a0..97ddb863 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -63,6 +63,8 @@ private: > > ControlInfoMap ctrls_; > > + IPACameraSensorInfo sensorInfo_; This is OK for now, but in the not too distant future I think we should store fields individually as we should probably convert them. For instance, the line duration should be converted to a time unit when we receive it, and then used as such, instead of converting between pixels and time in multiple places. Jean-Michel, how about moving IPU3Agc to handling exposure time in time units, and doing the conversion in ipu3.cpp ? Is this something you have considered, or maybe you're already planning it ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > /* Camera sensor controls. */ > uint32_t exposure_; > uint32_t minExposure_; > @@ -144,6 +146,8 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo) > if (configInfo.entityControls.empty()) > return; > > + sensorInfo_ = configInfo.sensorInfo; > + > ctrls_ = configInfo.entityControls.at(0); > > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); > @@ -174,7 +178,7 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo) > awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_); > > agcAlgo_ = std::make_unique<IPU3Agc>(); > - agcAlgo_->initialise(bdsGrid_, configInfo.sensorInfo); > + agcAlgo_->initialise(bdsGrid_, sensorInfo_); > } > > void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
Hi Laurent, Umang, On 08/06/2021 01:00, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Wed, Jun 02, 2021 at 03:53:25PM +0530, Umang Jain wrote: >> IPACameraSensorInfo members will be needed at various places in the >> IPAIPU3 class, in subsequent commits. Hence, it seems trivial to copy >> this structure for wider availability throughout the class. >> >> This commit does not introduce any functional changes. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/ipa/ipu3/ipu3.cpp | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >> index 2496b0a0..97ddb863 100644 >> --- a/src/ipa/ipu3/ipu3.cpp >> +++ b/src/ipa/ipu3/ipu3.cpp >> @@ -63,6 +63,8 @@ private: >> >> ControlInfoMap ctrls_; >> >> + IPACameraSensorInfo sensorInfo_; > > This is OK for now, but in the not too distant future I think we should > store fields individually as we should probably convert them. For > instance, the line duration should be converted to a time unit when we > receive it, and then used as such, instead of converting between pixels > and time in multiple places. > > Jean-Michel, how about moving IPU3Agc to handling exposure time in time > units, and doing the conversion in ipu3.cpp ? Is this something you have > considered, or maybe you're already planning it ? > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I indeed have a local branch where exposure is a time unit and the line duration is calculated in the agcAlgo_->initialise() call. Those ctrls disappear from ipu3.cpp except for setting those in setControls(). There is no need for a local variable in IPAIPU3 as IPU3Agc receives the configInfo reference directly. Would you prefer that new patch instead ? Or we can go with this one for the moment and I will fix it when I will post the new agc algorithm series ? >> + >> /* Camera sensor controls. */ >> uint32_t exposure_; >> uint32_t minExposure_; >> @@ -144,6 +146,8 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo) >> if (configInfo.entityControls.empty()) >> return; >> >> + sensorInfo_ = configInfo.sensorInfo; >> + >> ctrls_ = configInfo.entityControls.at(0); >> >> const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); >> @@ -174,7 +178,7 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo) >> awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_); >> >> agcAlgo_ = std::make_unique<IPU3Agc>(); >> - agcAlgo_->initialise(bdsGrid_, configInfo.sensorInfo); >> + agcAlgo_->initialise(bdsGrid_, sensorInfo_); >> } >> >> void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers) >
Hi JM. On 6/8/21 11:01 AM, Jean-Michel Hautbois wrote: > Hi Laurent, Umang, > > On 08/06/2021 01:00, Laurent Pinchart wrote: >> Hi Umang, >> >> Thank you for the patch. >> >> On Wed, Jun 02, 2021 at 03:53:25PM +0530, Umang Jain wrote: >>> IPACameraSensorInfo members will be needed at various places in the >>> IPAIPU3 class, in subsequent commits. Hence, it seems trivial to copy >>> this structure for wider availability throughout the class. >>> >>> This commit does not introduce any functional changes. >>> >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>> --- >>> src/ipa/ipu3/ipu3.cpp | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>> index 2496b0a0..97ddb863 100644 >>> --- a/src/ipa/ipu3/ipu3.cpp >>> +++ b/src/ipa/ipu3/ipu3.cpp >>> @@ -63,6 +63,8 @@ private: >>> >>> ControlInfoMap ctrls_; >>> >>> + IPACameraSensorInfo sensorInfo_; >> This is OK for now, but in the not too distant future I think we should >> store fields individually as we should probably convert them. For >> instance, the line duration should be converted to a time unit when we >> receive it, and then used as such, instead of converting between pixels >> and time in multiple places. >> >> Jean-Michel, how about moving IPU3Agc to handling exposure time in time >> units, and doing the conversion in ipu3.cpp ? Is this something you have >> considered, or maybe you're already planning it ? >> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > I indeed have a local branch where exposure is a time unit and the line > duration is calculated in the agcAlgo_->initialise() call. > Those ctrls disappear from ipu3.cpp except for setting those in > setControls(). > There is no need for a local variable in IPAIPU3 as IPU3Agc receives the > configInfo reference directly. > Would you prefer that new patch instead ? Or we can go with this one for > the moment and I will fix it when I will post the new agc algorithm series ? I plan to keep this as is for now assuming that the context of the rework, will be better when you post your agc algorithm series. > >>> + >>> /* Camera sensor controls. */ >>> uint32_t exposure_; >>> uint32_t minExposure_; >>> @@ -144,6 +146,8 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo) >>> if (configInfo.entityControls.empty()) >>> return; >>> >>> + sensorInfo_ = configInfo.sensorInfo; >>> + >>> ctrls_ = configInfo.entityControls.at(0); >>> >>> const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); >>> @@ -174,7 +178,7 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo) >>> awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_); >>> >>> agcAlgo_ = std::make_unique<IPU3Agc>(); >>> - agcAlgo_->initialise(bdsGrid_, configInfo.sensorInfo); >>> + agcAlgo_->initialise(bdsGrid_, sensorInfo_); >>> } >>> >>> void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index 2496b0a0..97ddb863 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -63,6 +63,8 @@ private: ControlInfoMap ctrls_; + IPACameraSensorInfo sensorInfo_; + /* Camera sensor controls. */ uint32_t exposure_; uint32_t minExposure_; @@ -144,6 +146,8 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo) if (configInfo.entityControls.empty()) return; + sensorInfo_ = configInfo.sensorInfo; + ctrls_ = configInfo.entityControls.at(0); const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); @@ -174,7 +178,7 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo) awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_); agcAlgo_ = std::make_unique<IPU3Agc>(); - agcAlgo_->initialise(bdsGrid_, configInfo.sensorInfo); + agcAlgo_->initialise(bdsGrid_, sensorInfo_); } void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
IPACameraSensorInfo members will be needed at various places in the IPAIPU3 class, in subsequent commits. Hence, it seems trivial to copy this structure for wider availability throughout the class. This commit does not introduce any functional changes. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/ipa/ipu3/ipu3.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)