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