<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>On Wed, 2 Jun 2021 at 12:10, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
Thanks for the reply!<br>
<br>
On Wed, 2 Jun 2021 at 11:10, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Hi David,<br>
><br>
> Thank you for your work.<br>
><br>
><br>
> On Thu, 27 May 2021 at 09:45, David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>> wrote:<br>
>><br>
>> Initialise it to the usual value of 1, until such time as we have a<br>
>> mechanism that allows us to discover the correct value.<br>
>><br>
>> The CamHelper class also gets a method to return this sensitivity<br>
>> value. This method is virtual so that it can be overridden for<br>
>> specific sensors. Once the correct value is obtainable elsewhere, this<br>
>> can be removed.<br>
>><br>
>> Signed-off-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
>> ---<br>
>>  src/ipa/raspberrypi/cam_helper.cpp           |  5 +++++<br>
>>  src/ipa/raspberrypi/cam_helper.hpp           |  3 +++<br>
>>  src/ipa/raspberrypi/controller/camera_mode.h |  2 ++<br>
>>  src/ipa/raspberrypi/raspberrypi.cpp          | 12 ++++++++++++<br>
>>  4 files changed, 22 insertions(+)<br>
>><br>
>> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp<br>
>> index 09917f3c..71ea21ff 100644<br>
>> --- a/src/ipa/raspberrypi/cam_helper.cpp<br>
>> +++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
>> @@ -128,6 +128,11 @@ bool CamHelper::SensorEmbeddedDataPresent() const<br>
>>         return false;<br>
>>  }<br>
>><br>
>> +double CamHelper::Sensitivity() const<br>
>> +{<br>
>> +       return mode_.sensitivity;<br>
>> +}<br>
>> +<br>
>>  unsigned int CamHelper::HideFramesStartup() const<br>
>>  {<br>
>>         /*<br>
>> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp<br>
>> index a52f3f0b..14c4714c 100644<br>
>> --- a/src/ipa/raspberrypi/cam_helper.hpp<br>
>> +++ b/src/ipa/raspberrypi/cam_helper.hpp<br>
>> @@ -39,6 +39,8 @@ namespace RPiController {<br>
>>  //<br>
>>  // A method to query if the sensor outputs embedded data that can be parsed.<br>
>>  //<br>
>> +// A method to return the sensitivity of the current camera mode.<br>
>> +//<br>
>>  // A parser to parse the embedded data buffers provided by some sensors (for<br>
>>  // example, the imx219 does; the ov5647 doesn't). This allows us to know for<br>
>>  // sure the exposure and gain of the frame we're looking at. CamHelper<br>
>> @@ -81,6 +83,7 @@ public:<br>
>>         virtual void GetDelays(int &exposure_delay, int &gain_delay,<br>
>>                                int &vblank_delay) const;<br>
>>         virtual bool SensorEmbeddedDataPresent() const;<br>
>> +       virtual double Sensitivity() const;<br>
>>         virtual unsigned int HideFramesStartup() const;<br>
>>         virtual unsigned int HideFramesModeSwitch() const;<br>
>>         virtual unsigned int MistrustFramesStartup() const;<br>
>> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h<br>
>> index 256438c9..326f4f20 100644<br>
>> --- a/src/ipa/raspberrypi/controller/camera_mode.h<br>
>> +++ b/src/ipa/raspberrypi/controller/camera_mode.h<br>
>> @@ -39,6 +39,8 @@ struct CameraMode {<br>
>>         libcamera::Transform transform;<br>
>>         // minimum and maximum fame lengths in units of lines<br>
>>         uint32_t min_frame_length, max_frame_length;<br>
>> +       // sensitivity of this mode<br>
>> +       double sensitivity;<br>
>>  };<br>
>><br>
>>  #ifdef __cplusplus<br>
>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
>> index e5bb8159..da42d279 100644<br>
>> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
>> @@ -321,6 +321,12 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)<br>
>>          */<br>
>>         mode_.min_frame_length = sensorInfo.minFrameLength;<br>
>>         mode_.max_frame_length = sensorInfo.maxFrameLength;<br>
>> +<br>
>> +       /*<br>
>> +        * For now, initialise the mode sensitivity to the usual value of 1.<br>
>> +        * \todo Obtain the correct sensitivity number automatically.<br>
>> +        */<br>
>> +       mode_.sensitivity = 1.0;<br>
>>  }<br>
>><br>
>>  int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,<br>
>> @@ -379,6 +385,12 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,<br>
>>         /* Pass the camera mode to the CamHelper to setup algorithms. */<br>
>>         helper_->SetCameraMode(mode_);<br>
>><br>
>> +       /*<br>
>> +        * For now, the CamHelper knows the sensitivity correctly.<br>
>> +        * \todo This can be removed once the sensitivity is initialised properly.<br>
>> +        */<br>
>> +       mode_.sensitivity = helper_->Sensitivity();<br>
>> +<br>
><br>
><br>
> I'm afraid I don't quite follow the logic here.  setMode sets the sensitivity in mode_,<br>
> then this gets updated via the helper to the same value.  Obviously some of this is<br>
> placeholder code right now, but could you explain how this may be expected to work<br>
> eventually?<br>
<br>
So the missing piece of the puzzle is that my under-development-camera<br>
overrides the default Sensitivity method with<br>
<br>
double CamHelperXXX::Sensitivity() const<br>
{<br>
    return isBinned() ? 2.0 : 1.0;<br>
}</blockquote><div><br></div><div>Right, I get it now.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
There's actually a slightly awkward chicken-and-egg thing going on<br>
here. So where I initially set "mode_.sensitivity = 1.0", I'd rather<br>
have<br>
"mode_.sensitivity = helper_->Sensitivity()".<br>
Only the helper doesn't know the mode yet.<br>
<br>
Perhaps having a method which allowed us to write<br>
"helper_->SetSensitivity(mode_)" instead would be less weird? It would<br>
look like:<br>
<br>
void CamHelperXXX:SetSensitivity(CameraMode &mode) const<br>
{<br>
    mode.sensitivity = isBinned() ? 2.0 : 1.0;<br>
}<br>
<br>
What do you think?<br></blockquote><div><br></div><div>Yes, I see the awkwardness here.  You can get around the chicken-and-egg problem</div><div>by perhaps not storing the sensitivity in the mode structure, but having an API in</div><div>AgcAlgorithm to set sensitivity directly?  Not sure I mind any approach.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> Another related question, do we need to involve the CamHelper at all?  Could we do<br>
> everything in the setMode function?<br>
<br>
Wasn't quite sure what you meant here... the CamHelper has to be<br>
involved somewhere, it's the only thing that knows that the binned<br>
mode for this sensor is "different", right?<br></blockquote><div><br></div><div>Ignore this comment, with your above explanation this does not make sense :-)</div><div><br></div><div>Thanks,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks again<br>
David<br>
<br>
><br>
> Thanks,<br>
> Naush<br>
><br>
>><br>
>>         if (firstStart_) {<br>
>>                 /* Supply initial values for frame durations. */<br>
>>                 applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);<br>
>><br>
>> --<br>
>> 2.20.1<br>
>><br>
</blockquote></div></div>