| Message ID | 20230627130553.26940-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@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. Reviewed-by: Kieran Bingham <kieran.bingham@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 ®isters, > Metadata &metadata) const > { > -- > 2.34.1 >
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@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@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@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 ®isters, > > Metadata &metadata) const > > { > > -- > > 2.34.1 > >
On Thu, 6 Jul 2023 at 12:39, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > 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@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@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. Yes please - thank you! Naush > > -- > 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@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 ®isters, > > > Metadata &metadata) const > > > { > > > -- > > > 2.34.1 > > >
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@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@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 ? > Reviewed-by: Kieran Bingham <kieran.bingham@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 ®isters, > > Metadata &metadata) const > > {
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@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@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?" > > Reviewed-by: Kieran Bingham <kieran.bingham@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 ®isters, > > > Metadata &metadata) const > > > { > > -- > Regards, > > Laurent Pinchart
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@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@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@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 ®isters, > > > > Metadata &metadata) const > > > > {
On Thu, 6 Jul 2023 at 13:53, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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@raspberrypi.com> > > > > > Reviewed-by: David Plowman <david.plowman@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. IMO sensor HDR would need to become part of the V4L2 API. This might mean HDR is signalled as either a new pixel format (not very neat), or a control value (mostly like today, also not very neat). In any hypothetical rework of the kernel sensor API, HDR modes ought to become a part of the format/mode information, possibly as a decoupled/modifier field? Regards, Naush > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@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 ®isters, > > > > > Metadata &metadata) const > > > > > { > > -- > Regards, > > Laurent Pinchart
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? David On Thu, 6 Jul 2023 at 13:53, Laurent Pinchart via libcamera-devel <libcamera-devel@lists.libcamera.org> 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@raspberrypi.com> > > > > > Reviewed-by: David Plowman <david.plowman@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@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 ®isters, > > > > > Metadata &metadata) const > > > > > { > > -- > Regards, > > 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@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@raspberrypi.com> > > > > > > Reviewed-by: David Plowman <david.plowman@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@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 ®isters, > > > > > > Metadata &metadata) const > > > > > > {
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. + */ + 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 ®isters, Metadata &metadata) const {