| Message ID | 20221003083934.31629-3-naush@raspberrypi.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Mon, Oct 03, 2022 at 09:39:28AM +0100, Naushir Patuck via libcamera-devel wrote: > Add fields for minimum and maximum line length (in units of pixels) to the I think the unit is seconds, not pixels. > CameraMode structure. This replaces the existing lineLength field. > > Any use of the existing lineLength field is replaced by the new minLineLength > field, as logically we always want to use the fastest sensor readout by default. I wonder if that's always the case, but I think it's fine here. > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/cam_helper.cpp | 8 ++++---- > src/ipa/raspberrypi/controller/camera_mode.h | 2 +- > src/ipa/raspberrypi/raspberrypi.cpp | 13 +++++++------ > 3 files changed, 12 insertions(+), 11 deletions(-) > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp > index cac8f39ee763..42251ba29682 100644 > --- a/src/ipa/raspberrypi/cam_helper.cpp > +++ b/src/ipa/raspberrypi/cam_helper.cpp > @@ -64,13 +64,13 @@ void CamHelper::process([[maybe_unused]] StatisticsPtr &stats, > uint32_t CamHelper::exposureLines(const Duration exposure) const > { > assert(initialized_); > - return exposure / mode_.lineLength; > + return exposure / mode_.minLineLength; > } > > Duration CamHelper::exposure(uint32_t exposureLines) const > { > assert(initialized_); > - return exposureLines * mode_.lineLength; > + return exposureLines * mode_.minLineLength; > } > > uint32_t CamHelper::getVBlanking(Duration &exposure, > @@ -86,8 +86,8 @@ uint32_t CamHelper::getVBlanking(Duration &exposure, > * minFrameDuration and maxFrameDuration are clamped by the caller > * based on the limits for the active sensor mode. > */ > - frameLengthMin = minFrameDuration / mode_.lineLength; > - frameLengthMax = maxFrameDuration / mode_.lineLength; > + frameLengthMin = minFrameDuration / mode_.minLineLength; > + frameLengthMax = maxFrameDuration / mode_.minLineLength; > > /* > * Limit the exposure to the maximum frame duration requested, and > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h > index a6ccf8c1c600..b7e73aae4698 100644 > --- a/src/ipa/raspberrypi/controller/camera_mode.h > +++ b/src/ipa/raspberrypi/controller/camera_mode.h > @@ -32,7 +32,7 @@ struct CameraMode { > /* scaling of the noise compared to the native sensor mode */ > double noiseFactor; > /* line time */ > - libcamera::utils::Duration lineLength; > + libcamera::utils::Duration minLineLength, maxLineLength; libcamera::utils::Duration minLineLength; libcamera::utils::Duration maxLineLength; would be more readable. > /* any camera transform *not* reflected already in the camera tuning */ > libcamera::Transform transform; > /* minimum and maximum fame lengths in units of lines */ > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 358a119da222..67326bcf4a14 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -314,7 +314,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig) > } > > startConfig->dropFrameCount = dropFrameCount_; > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength; > + const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength; > startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get<std::milli>(); > > firstStart_ = false; > @@ -356,7 +356,8 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo) > * Calculate the line length as the ratio between the line length in > * pixels and the pixel rate. > */ > - mode_.lineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate); > + mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate); > + mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate); > > /* > * Set the frame length limits for the mode to ensure exposure and > @@ -458,8 +459,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, > * based on the current sensor mode. > */ > ControlInfoMap::Map ctrlMap = ipaControls; > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.lineLength; > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength; > + const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength; > + const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength; > ctrlMap[&controls::FrameDurationLimits] = > ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()), > static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>())); > @@ -1150,8 +1151,8 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) > > void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration) > { > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.lineLength; > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength; > + const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength; > + const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength; > > /* > * This will only be applied once AGC recalculations occur.
Hi Laurent, Thank you for your feedback. On Tue, 4 Oct 2022 at 17:43, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Mon, Oct 03, 2022 at 09:39:28AM +0100, Naushir Patuck via > libcamera-devel wrote: > > Add fields for minimum and maximum line length (in units of pixels) to > the > > I think the unit is seconds, not pixels. > > > CameraMode structure. This replaces the existing lineLength field. > > > > Any use of the existing lineLength field is replaced by the new > minLineLength > > field, as logically we always want to use the fastest sensor readout by > default. > > I wonder if that's always the case, but I think it's fine here. > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/cam_helper.cpp | 8 ++++---- > > src/ipa/raspberrypi/controller/camera_mode.h | 2 +- > > src/ipa/raspberrypi/raspberrypi.cpp | 13 +++++++------ > > 3 files changed, 12 insertions(+), 11 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp > b/src/ipa/raspberrypi/cam_helper.cpp > > index cac8f39ee763..42251ba29682 100644 > > --- a/src/ipa/raspberrypi/cam_helper.cpp > > +++ b/src/ipa/raspberrypi/cam_helper.cpp > > @@ -64,13 +64,13 @@ void CamHelper::process([[maybe_unused]] > StatisticsPtr &stats, > > uint32_t CamHelper::exposureLines(const Duration exposure) const > > { > > assert(initialized_); > > - return exposure / mode_.lineLength; > > + return exposure / mode_.minLineLength; > > } > > > > Duration CamHelper::exposure(uint32_t exposureLines) const > > { > > assert(initialized_); > > - return exposureLines * mode_.lineLength; > > + return exposureLines * mode_.minLineLength; > > } > > > > uint32_t CamHelper::getVBlanking(Duration &exposure, > > @@ -86,8 +86,8 @@ uint32_t CamHelper::getVBlanking(Duration &exposure, > > * minFrameDuration and maxFrameDuration are clamped by the caller > > * based on the limits for the active sensor mode. > > */ > > - frameLengthMin = minFrameDuration / mode_.lineLength; > > - frameLengthMax = maxFrameDuration / mode_.lineLength; > > + frameLengthMin = minFrameDuration / mode_.minLineLength; > > + frameLengthMax = maxFrameDuration / mode_.minLineLength; > > > > /* > > * Limit the exposure to the maximum frame duration requested, and > > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h > b/src/ipa/raspberrypi/controller/camera_mode.h > > index a6ccf8c1c600..b7e73aae4698 100644 > > --- a/src/ipa/raspberrypi/controller/camera_mode.h > > +++ b/src/ipa/raspberrypi/controller/camera_mode.h > > @@ -32,7 +32,7 @@ struct CameraMode { > > /* scaling of the noise compared to the native sensor mode */ > > double noiseFactor; > > /* line time */ > > - libcamera::utils::Duration lineLength; > > + libcamera::utils::Duration minLineLength, maxLineLength; > > libcamera::utils::Duration minLineLength; > libcamera::utils::Duration maxLineLength; > > would be more readable. > I did the single line statement to be consistent with the rest of the struct definition, but I can amend this change so that all fields are on a single line. Regards, Naush > > > /* any camera transform *not* reflected already in the camera > tuning */ > > libcamera::Transform transform; > > /* minimum and maximum fame lengths in units of lines */ > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index 358a119da222..67326bcf4a14 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -314,7 +314,7 @@ void IPARPi::start(const ControlList &controls, > StartConfig *startConfig) > > } > > > > startConfig->dropFrameCount = dropFrameCount_; > > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * > mode_.lineLength; > > + const Duration maxSensorFrameDuration = mode_.maxFrameLength * > mode_.minLineLength; > > startConfig->maxSensorFrameLengthMs = > maxSensorFrameDuration.get<std::milli>(); > > > > firstStart_ = false; > > @@ -356,7 +356,8 @@ void IPARPi::setMode(const IPACameraSensorInfo > &sensorInfo) > > * Calculate the line length as the ratio between the line length > in > > * pixels and the pixel rate. > > */ > > - mode_.lineLength = sensorInfo.minLineLength * (1.0s / > sensorInfo.pixelRate); > > + mode_.minLineLength = sensorInfo.minLineLength * (1.0s / > sensorInfo.pixelRate); > > + mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / > sensorInfo.pixelRate); > > > > /* > > * Set the frame length limits for the mode to ensure exposure and > > @@ -458,8 +459,8 @@ int IPARPi::configure(const IPACameraSensorInfo > &sensorInfo, > > * based on the current sensor mode. > > */ > > ControlInfoMap::Map ctrlMap = ipaControls; > > - const Duration minSensorFrameDuration = mode_.minFrameLength * > mode_.lineLength; > > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * > mode_.lineLength; > > + const Duration minSensorFrameDuration = mode_.minFrameLength * > mode_.minLineLength; > > + const Duration maxSensorFrameDuration = mode_.maxFrameLength * > mode_.minLineLength; > > ctrlMap[&controls::FrameDurationLimits] = > > > ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()), > > > static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>())); > > @@ -1150,8 +1151,8 @@ void IPARPi::applyAWB(const struct AwbStatus > *awbStatus, ControlList &ctrls) > > > > void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration > maxFrameDuration) > > { > > - const Duration minSensorFrameDuration = mode_.minFrameLength * > mode_.lineLength; > > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * > mode_.lineLength; > > + const Duration minSensorFrameDuration = mode_.minFrameLength * > mode_.minLineLength; > > + const Duration maxSensorFrameDuration = mode_.maxFrameLength * > mode_.minLineLength; > > > > /* > > * This will only be applied once AGC recalculations occur. > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp index cac8f39ee763..42251ba29682 100644 --- a/src/ipa/raspberrypi/cam_helper.cpp +++ b/src/ipa/raspberrypi/cam_helper.cpp @@ -64,13 +64,13 @@ void CamHelper::process([[maybe_unused]] StatisticsPtr &stats, uint32_t CamHelper::exposureLines(const Duration exposure) const { assert(initialized_); - return exposure / mode_.lineLength; + return exposure / mode_.minLineLength; } Duration CamHelper::exposure(uint32_t exposureLines) const { assert(initialized_); - return exposureLines * mode_.lineLength; + return exposureLines * mode_.minLineLength; } uint32_t CamHelper::getVBlanking(Duration &exposure, @@ -86,8 +86,8 @@ uint32_t CamHelper::getVBlanking(Duration &exposure, * minFrameDuration and maxFrameDuration are clamped by the caller * based on the limits for the active sensor mode. */ - frameLengthMin = minFrameDuration / mode_.lineLength; - frameLengthMax = maxFrameDuration / mode_.lineLength; + frameLengthMin = minFrameDuration / mode_.minLineLength; + frameLengthMax = maxFrameDuration / mode_.minLineLength; /* * Limit the exposure to the maximum frame duration requested, and diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h index a6ccf8c1c600..b7e73aae4698 100644 --- a/src/ipa/raspberrypi/controller/camera_mode.h +++ b/src/ipa/raspberrypi/controller/camera_mode.h @@ -32,7 +32,7 @@ struct CameraMode { /* scaling of the noise compared to the native sensor mode */ double noiseFactor; /* line time */ - libcamera::utils::Duration lineLength; + libcamera::utils::Duration minLineLength, maxLineLength; /* any camera transform *not* reflected already in the camera tuning */ libcamera::Transform transform; /* minimum and maximum fame lengths in units of lines */ diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 358a119da222..67326bcf4a14 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -314,7 +314,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig) } startConfig->dropFrameCount = dropFrameCount_; - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength; + const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength; startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get<std::milli>(); firstStart_ = false; @@ -356,7 +356,8 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo) * Calculate the line length as the ratio between the line length in * pixels and the pixel rate. */ - mode_.lineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate); + mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate); + mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate); /* * Set the frame length limits for the mode to ensure exposure and @@ -458,8 +459,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, * based on the current sensor mode. */ ControlInfoMap::Map ctrlMap = ipaControls; - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.lineLength; - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength; + const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength; + const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength; ctrlMap[&controls::FrameDurationLimits] = ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()), static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>())); @@ -1150,8 +1151,8 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration) { - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.lineLength; - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength; + const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength; + const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength; /* * This will only be applied once AGC recalculations occur.
Add fields for minimum and maximum line length (in units of pixels) to the CameraMode structure. This replaces the existing lineLength field. Any use of the existing lineLength field is replaced by the new minLineLength field, as logically we always want to use the fastest sensor readout by default. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/raspberrypi/cam_helper.cpp | 8 ++++---- src/ipa/raspberrypi/controller/camera_mode.h | 2 +- src/ipa/raspberrypi/raspberrypi.cpp | 13 +++++++------ 3 files changed, 12 insertions(+), 11 deletions(-)