[libcamera-devel] [PATCH v2] ipa: rpi: imx708: Fix mode switch drop frame count

[Laurent Pinchart]

On Thu, Jul 06, 2023 at 02:07:40PM +0100, David Plowman wrote:
> It would be nice if V4L2 could tell us this kind of mode-specific
> information, so that it might perhaps feature in the camera mode?

If my understanding is correct, the plan is to retrieve the information
from the camera sensor configuration and make it available to the IPA
module, probably storing it in the mode_ member variable. That looks
good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> On Thu, 6 Jul 2023 at 13:53, Laurent Pinchart via libcamera-devel wrote:
> > On Thu, Jul 06, 2023 at 01:19:29PM +0100, Kieran Bingham wrote:
> > > Quoting Laurent Pinchart (2023-07-06 13:09:41)
> > > > On Thu, Jul 06, 2023 at 12:20:58PM +0100, Kieran Bingham via libcamera-devel wrote:
> > > > > Quoting Naushir Patuck via libcamera-devel (2023-06-27 14:05:53)
> > > > > > The imx708 must drop a single frame on startup - but only when in HDR
> > > > > > mode. Non-HDR modes do not need to drop frames. Fix the logic in
> > > > > > hideFramesModeSwitch() which currently unconditionally advertises to
> > > > > > drop one frame.
> > > > > >
> > > > > > Unfortunately there is no clear way to tell if the sensor is in the HDR
> > > > > > mode. So for now, look the resolution and framerate to deduce this.
> > > > > >
> > > > > > Additionally ensure we override hideFramesStartup() and return the same
> > > > > > number as hideFramesModeSwitch().
> > > > > >
> > > > > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/524
> > > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > > > > ---
> > > > > > Changes since v1:
> > > > > > - Fix a typo in the comparison statement.
> > > > > > ---
> > > > > >  src/ipa/rpi/cam_helper/cam_helper_imx708.cpp | 25 +++++++++++++++++++-
> > > > > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> > > > > > index 641ba18f4b9d..9bc0272dd4c1 100644
> > > > > > --- a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> > > > > > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> > > > > > @@ -21,6 +21,8 @@ using namespace RPiController;
> > > > > >  using namespace libcamera;
> > > > > >  using libcamera::utils::Duration;
> > > > > >
> > > > > > +using namespace std::literals::chrono_literals;
> > > > > > +
> > > > > >  namespace libcamera {
> > > > > >  LOG_DECLARE_CATEGORY(IPARPI)
> > > > > >  }
> > > > > > @@ -56,7 +58,8 @@ public:
> > > > > >                        int &vblankDelay, int &hblankDelay) const override;
> > > > > >         bool sensorEmbeddedDataPresent() const override;
> > > > > >         double getModeSensitivity(const CameraMode &mode) const override;
> > > > > > -       unsigned int hideFramesModeSwitch() const override { return 1; } // seems to be required for HDR
> > > > > > +       unsigned int hideFramesModeSwitch() const override;
> > > > > > +       unsigned int hideFramesStartup() const;
> > > > > >
> > > > > >  private:
> > > > > >         /*
> > > > > > @@ -225,6 +228,26 @@ double CamHelperImx708::getModeSensitivity(const CameraMode &mode) const
> > > > > >         return (mode.width > 2304) ? 1.0 : 2.0;
> > > > > >  }
> > > > > >
> > > > > > +unsigned int CamHelperImx708::hideFramesModeSwitch() const
> > > > > > +{
> > > > > > +       /*
> > > > > > +        * We need to drop the first startup frame in HDR mode.
> > > > > > +        * Unfortunately the only way to currently determine if the sensor is in
> > > > > > +        * the HDR mode is to match with the resolution and framerate - the HDR
> > > > > > +        * mode only runs upto 30fps.
> > > > >
> > > > > Perhaps we should add a
> > > > >
> > > > >        * \todo: Revisit this when the HDR mode is handled by
> > > > >        * libcamera.
> > > >
> > > > How will this issue be handled then ?
> > >
> > > I don't know, that would be the action of the todo - but are you saying
> > > "No don't ever bother revisiting this?"
> >
> > It was a question for Naush or David, I'd like to know if we have any
> > idea on how we could handle this in the future, or if it's completely
> > unknown.
> >
> > > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > >
> > > > > > +        */
> > > > > > +       if (mode_.width == 2304 && mode_.height == 1296 &&
> > > > > > +           mode_.minFrameDuration > 1.0s / 32)
> > > > > > +               return 1;
> > > > > > +       else
> > > > > > +               return 0;
> > > > > > +}
> > > > > > +
> > > > > > +unsigned int CamHelperImx708::hideFramesStartup() const
> > > > > > +{
> > > > > > +       return hideFramesModeSwitch();
> > > > > > +}
> > > > > > +
> > > > > >  void CamHelperImx708::populateMetadata(const MdParser::RegisterMap &registers,
> > > > > >                                        Metadata &metadata) const
> > > > > >  {

-- 
Regards,

Laurent Pinchart