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

[Kieran Bingham]

Quoting Kieran Bingham (2023-07-06 12:20:58)
> 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;

[84/123] Compiling C++ object src/ipa/rpi/cam_helper/librpi_ipa_cam_helper.a.p/cam_helper_imx708.cpp.o
FAILED: src/ipa/rpi/cam_helper/librpi_ipa_cam_helper.a.p/cam_helper_imx708.cpp.o 
clang++-11 -Isrc/ipa/rpi/cam_helper/librpi_ipa_cam_helper.a.p -Isrc/ipa/rpi/cam_helper -I../../../src/libcamera/src/ipa/rpi/cam_helper -Isrc/ipa/rpi -I../../../src/libcamera/src/ipa/rpi -Iinclude -I../../../src/libcamera/include -Iinclude/libcamera/ipa -Iinclude/libcamera -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -O0 -g -Wextra-semi -Wthread-safety -Wshadow -include /home/kbingham/iob/libcamera/ci/integrator/builds/build-matrix/clang-11/config.h -Wno-c99-designator -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/ipa/rpi/cam_helper/librpi_ipa_cam_helper.a.p/cam_helper_imx708.cpp.o -MF src/ipa/rpi/cam_helper/librpi_ipa_cam_helper.a.p/cam_helper_imx708.cpp.o.d -o src/ipa/rpi/cam_helper/librpi_ipa_cam_helper.a.p/cam_helper_imx708.cpp.o -c ../../../src/libcamera/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
../../../src/libcamera/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp:62:15: error: 'hideFramesStartup' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
        unsigned int hideFramesStartup() const;
                     ^
../../../src/libcamera/src/ipa/rpi/cam_helper/cam_helper.h:98:23: note: overridden virtual function is here
        virtual unsigned int hideFramesStartup() const;
                             ^
1 error generated.
[85/123] Compiling C++ object src/android/libcamera-hal.so.p/jpeg_post_processor_jpeg.cpp.o


I can fix this up just adding the override after const.

--
Kieran



> >  
> >  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.
> 
> 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
> >  {
> > -- 
> > 2.34.1
> >