| Message ID | 20211110195901.85597-13-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jean-Michel Hautbois (2021-11-10 19:58:59) > We use the line duration several times in the IPAIPU3. Instead of > recalculating it each time, cache the value as a utils::Duration. When > the value is needed in double, call the intern count() function to get > it. caching it I like ... using the internal count() ... sounds scary... Oh ... in fact, it does look like the 'correct' way to do this. But I think that's a limitation on our implementation of utils::Duration. I think we can do: maxShutterSpeed = maxExposure * lineDuration.get<std::nano>(); But that essentially just casts a std::nano to a std::nano and then calls .count() on it ... so that's a bit redundant compared to what you're already doing... Perhaps we should have our Duration class implement a default .get() which always returns nanoseconds.... (and simply calls .count() or an explicit operator double() ... ?) Anyway, that isn't a fault of this patch as far as I can tell - so lets consider that on top. If you prefer the explicit named cast with .get<std::nano>() I think it's ok to use that as the compiler will probably optimise it ... but either way is up to you I think... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/ipu3.cpp | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index eb8c0dc4..68a97e8e 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -174,6 +174,8 @@ private: > uint32_t minGain_; > uint32_t maxGain_; > > + utils::Duration lineDuration_; > + > /* Interface to the Camera Helper */ > std::unique_ptr<CameraSensorHelper> camHelper_; > > @@ -195,8 +197,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo, > int32_t minExposure = v4l2Exposure.min().get<int32_t>(); > int32_t maxExposure = v4l2Exposure.max().get<int32_t>(); > > - utils::Duration lineDuration = sensorInfo.lineLength * 1.0s > - / sensorInfo.pixelRate; > + lineDuration_ = sensorInfo.lineLength * 1.0s > + / sensorInfo.pixelRate; > > const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; > int32_t minGain = v4l2Gain.min().get<int32_t>(); > @@ -209,8 +211,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo, > * > * \todo take VBLANK into account for maximum shutter speed > */ > - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration; > - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration; > + context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_; > + context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_; > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); > } > @@ -239,11 +241,10 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, > * exposure min, max and default and convert it from lines to > * microseconds. > */ > - double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6); I like that we get rid of this 1e6 though ;-) > const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; > - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; > - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; > - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; > + int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration_.count(); > + int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration_.count(); > + int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration_.count(); > controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, > defExposure); > > -- > 2.32.0 >
Quoting Kieran Bingham (2021-11-10 23:10:49) > Quoting Jean-Michel Hautbois (2021-11-10 19:58:59) > > We use the line duration several times in the IPAIPU3. Instead of > > recalculating it each time, cache the value as a utils::Duration. When > > the value is needed in double, call the intern count() function to get > > it. > > caching it I like ... using the internal count() ... sounds scary... > > Oh ... in fact, it does look like the 'correct' way to do this. > But I think that's a limitation on our implementation of > utils::Duration. > > I think we can do: > maxShutterSpeed = maxExposure * lineDuration.get<std::nano>(); > > But that essentially just casts a std::nano to a std::nano and then > calls .count() on it ... so that's a bit redundant compared to what > you're already doing... > > Perhaps we should have our Duration class implement a default .get() > which always returns nanoseconds.... (and simply calls .count() or an > explicit operator double() ... ?) > > Anyway, that isn't a fault of this patch as far as I can tell - so lets > consider that on top. > > If you prefer the explicit named cast with .get<std::nano>() I think > it's ok to use that as the compiler will probably optimise it ... but > either way is up to you I think... > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Hrm... perhaps except for one more thing to check first ... > > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > --- > > src/ipa/ipu3/ipu3.cpp | 17 +++++++++-------- > > 1 file changed, 9 insertions(+), 8 deletions(-) > > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > index eb8c0dc4..68a97e8e 100644 > > --- a/src/ipa/ipu3/ipu3.cpp > > +++ b/src/ipa/ipu3/ipu3.cpp > > @@ -174,6 +174,8 @@ private: > > uint32_t minGain_; > > uint32_t maxGain_; > > > > + utils::Duration lineDuration_; > > + > > /* Interface to the Camera Helper */ > > std::unique_ptr<CameraSensorHelper> camHelper_; > > > > @@ -195,8 +197,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo, > > int32_t minExposure = v4l2Exposure.min().get<int32_t>(); > > int32_t maxExposure = v4l2Exposure.max().get<int32_t>(); > > > > - utils::Duration lineDuration = sensorInfo.lineLength * 1.0s > > - / sensorInfo.pixelRate; > > + lineDuration_ = sensorInfo.lineLength * 1.0s > > + / sensorInfo.pixelRate; > > > > const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; > > int32_t minGain = v4l2Gain.min().get<int32_t>(); > > @@ -209,8 +211,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo, > > * > > * \todo take VBLANK into account for maximum shutter speed > > */ > > - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration; > > - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration; > > + context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_; > > + context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_; > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); > > } > > @@ -239,11 +241,10 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, > > * exposure min, max and default and convert it from lines to > > * microseconds. > > */ > > - double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6); > > I like that we get rid of this 1e6 though ;-) It 'might' still be right - but needs a correct/thorough checking (or use the explicit .get<std::nano>() / .get<std::micro>() .... The 1e6 is I assume taking us to micro's?... but... I believe the Duration.count() is going to be returning nano seconds... can you validate/check that this calculation is using the correct units as expected please? I think this makes me favour the explicit units / period in the get<>() call to make it clear what the calculation does.. > > > const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; > > - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; > > - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; > > - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; > > + int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration_.count(); > > + int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration_.count(); > > + int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration_.count(); > > controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, > > defExposure); > > > > -- > > 2.32.0 > >
Hi Kieran, On 11/11/2021 00:45, Kieran Bingham wrote: > Quoting Kieran Bingham (2021-11-10 23:10:49) >> Quoting Jean-Michel Hautbois (2021-11-10 19:58:59) >>> We use the line duration several times in the IPAIPU3. Instead of >>> recalculating it each time, cache the value as a utils::Duration. When >>> the value is needed in double, call the intern count() function to get >>> it. >> >> caching it I like ... using the internal count() ... sounds scary... >> >> Oh ... in fact, it does look like the 'correct' way to do this. >> But I think that's a limitation on our implementation of >> utils::Duration. >> >> I think we can do: >> maxShutterSpeed = maxExposure * lineDuration.get<std::nano>(); >> >> But that essentially just casts a std::nano to a std::nano and then >> calls .count() on it ... so that's a bit redundant compared to what >> you're already doing... >> >> Perhaps we should have our Duration class implement a default .get() >> which always returns nanoseconds.... (and simply calls .count() or an >> explicit operator double() ... ?) >> >> Anyway, that isn't a fault of this patch as far as I can tell - so lets >> consider that on top. >> >> If you prefer the explicit named cast with .get<std::nano>() I think >> it's ok to use that as the compiler will probably optimise it ... but >> either way is up to you I think... >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Hrm... perhaps except for one more thing to check first ... > > >> >>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>> --- >>> src/ipa/ipu3/ipu3.cpp | 17 +++++++++-------- >>> 1 file changed, 9 insertions(+), 8 deletions(-) >>> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>> index eb8c0dc4..68a97e8e 100644 >>> --- a/src/ipa/ipu3/ipu3.cpp >>> +++ b/src/ipa/ipu3/ipu3.cpp >>> @@ -174,6 +174,8 @@ private: >>> uint32_t minGain_; >>> uint32_t maxGain_; >>> >>> + utils::Duration lineDuration_; >>> + >>> /* Interface to the Camera Helper */ >>> std::unique_ptr<CameraSensorHelper> camHelper_; >>> >>> @@ -195,8 +197,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo, >>> int32_t minExposure = v4l2Exposure.min().get<int32_t>(); >>> int32_t maxExposure = v4l2Exposure.max().get<int32_t>(); >>> >>> - utils::Duration lineDuration = sensorInfo.lineLength * 1.0s >>> - / sensorInfo.pixelRate; >>> + lineDuration_ = sensorInfo.lineLength * 1.0s >>> + / sensorInfo.pixelRate; I just see this is bad, it should not be set here, but in configure() before calling updateControls... >>> >>> const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; >>> int32_t minGain = v4l2Gain.min().get<int32_t>(); >>> @@ -209,8 +211,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo, >>> * >>> * \todo take VBLANK into account for maximum shutter speed >>> */ >>> - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration; >>> - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration; >>> + context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_; >>> + context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_; >>> context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); >>> context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); >>> } >>> @@ -239,11 +241,10 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, >>> * exposure min, max and default and convert it from lines to >>> * microseconds. >>> */ >>> - double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6); >> >> I like that we get rid of this 1e6 though ;-) > > It 'might' still be right - but needs a correct/thorough checking (or > use the explicit .get<std::nano>() / .get<std::micro>() .... > > The 1e6 is I assume taking us to micro's?... but... > > I believe the Duration.count() is going to be returning nano seconds... > can you validate/check that this calculation is using the correct units > as expected please? > > I think this makes me favour the explicit units / period in the get<>() > call to make it clear what the calculation does.. > > > >> >>> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; >>> - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; >>> - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; >>> - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; >>> + int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration_.count(); >>> + int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration_.count(); >>> + int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration_.count(); >>> controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, >>> defExposure); >>> >>> -- >>> 2.32.0 >>>
Hi Kieran, On 11/11/2021 00:45, Kieran Bingham wrote: > Quoting Kieran Bingham (2021-11-10 23:10:49) >> Quoting Jean-Michel Hautbois (2021-11-10 19:58:59) >>> We use the line duration several times in the IPAIPU3. Instead of >>> recalculating it each time, cache the value as a utils::Duration. When >>> the value is needed in double, call the intern count() function to get >>> it. >> >> caching it I like ... using the internal count() ... sounds scary... >> >> Oh ... in fact, it does look like the 'correct' way to do this. >> But I think that's a limitation on our implementation of >> utils::Duration. >> >> I think we can do: >> maxShutterSpeed = maxExposure * lineDuration.get<std::nano>(); >> >> But that essentially just casts a std::nano to a std::nano and then >> calls .count() on it ... so that's a bit redundant compared to what >> you're already doing... >> >> Perhaps we should have our Duration class implement a default .get() >> which always returns nanoseconds.... (and simply calls .count() or an >> explicit operator double() ... ?) >> >> Anyway, that isn't a fault of this patch as far as I can tell - so lets >> consider that on top. >> >> If you prefer the explicit named cast with .get<std::nano>() I think >> it's ok to use that as the compiler will probably optimise it ... but >> either way is up to you I think... >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Hrm... perhaps except for one more thing to check first ... > > >> >>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>> --- >>> src/ipa/ipu3/ipu3.cpp | 17 +++++++++-------- >>> 1 file changed, 9 insertions(+), 8 deletions(-) >>> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>> index eb8c0dc4..68a97e8e 100644 >>> --- a/src/ipa/ipu3/ipu3.cpp >>> +++ b/src/ipa/ipu3/ipu3.cpp >>> @@ -174,6 +174,8 @@ private: >>> uint32_t minGain_; >>> uint32_t maxGain_; >>> >>> + utils::Duration lineDuration_; >>> + >>> /* Interface to the Camera Helper */ >>> std::unique_ptr<CameraSensorHelper> camHelper_; >>> >>> @@ -195,8 +197,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo, >>> int32_t minExposure = v4l2Exposure.min().get<int32_t>(); >>> int32_t maxExposure = v4l2Exposure.max().get<int32_t>(); >>> >>> - utils::Duration lineDuration = sensorInfo.lineLength * 1.0s >>> - / sensorInfo.pixelRate; >>> + lineDuration_ = sensorInfo.lineLength * 1.0s >>> + / sensorInfo.pixelRate; >>> >>> const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; >>> int32_t minGain = v4l2Gain.min().get<int32_t>(); >>> @@ -209,8 +211,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo, >>> * >>> * \todo take VBLANK into account for maximum shutter speed >>> */ >>> - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration; >>> - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration; >>> + context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_; >>> + context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_; >>> context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); >>> context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); >>> } >>> @@ -239,11 +241,10 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, >>> * exposure min, max and default and convert it from lines to >>> * microseconds. >>> */ >>> - double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6); >> >> I like that we get rid of this 1e6 though ;-) > > It 'might' still be right - but needs a correct/thorough checking (or > use the explicit .get<std::nano>() / .get<std::micro>() .... > > The 1e6 is I assume taking us to micro's?... but... > > I believe the Duration.count() is going to be returning nano seconds... > can you validate/check that this calculation is using the correct units > as expected please? It returns the number of ticks for this duration in a number of ns indeed. In our case: lineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate; LOG(IPAIPU3, Debug) << "Line duration: " << lineDuration_ << " is " << lineDuration_.count() << " ticks" << " and " << lineDuration.get<std::micro>() << " microseconds"; => output is: Line duration: 16.80us is 16800 ticks and 16.8 microseconds So, I guess you are right ;-), we should use .get<std::micro>(). > > I think this makes me favour the explicit units / period in the get<>() > call to make it clear what the calculation does.. > > > >> >>> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; >>> - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; >>> - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; >>> - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; >>> + int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration_.count(); >>> + int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration_.count(); >>> + int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration_.count(); >>> controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, >>> defExposure); >>> >>> -- >>> 2.32.0 >>>
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index eb8c0dc4..68a97e8e 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -174,6 +174,8 @@ private: uint32_t minGain_; uint32_t maxGain_; + utils::Duration lineDuration_; + /* Interface to the Camera Helper */ std::unique_ptr<CameraSensorHelper> camHelper_; @@ -195,8 +197,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo, int32_t minExposure = v4l2Exposure.min().get<int32_t>(); int32_t maxExposure = v4l2Exposure.max().get<int32_t>(); - utils::Duration lineDuration = sensorInfo.lineLength * 1.0s - / sensorInfo.pixelRate; + lineDuration_ = sensorInfo.lineLength * 1.0s + / sensorInfo.pixelRate; const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; int32_t minGain = v4l2Gain.min().get<int32_t>(); @@ -209,8 +211,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo, * * \todo take VBLANK into account for maximum shutter speed */ - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration; - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration; + context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_; + context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_; context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); } @@ -239,11 +241,10 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, * exposure min, max and default and convert it from lines to * microseconds. */ - double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6); const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; + int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration_.count(); + int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration_.count(); + int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration_.count(); controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, defExposure);
We use the line duration several times in the IPAIPU3. Instead of recalculating it each time, cache the value as a utils::Duration. When the value is needed in double, call the intern count() function to get it. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/ipu3.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)