Message ID | 20210702150940.226941-5-naush@raspberrypi.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Naush Thanks for this update - I can see that this change has become more troublesome than initially expected! On Fri, 2 Jul 2021 at 16:09, Naushir Patuck <naush@raspberrypi.com> wrote: > > Update the IPA to fill frame length into the DeviceStatus struct from the > VBLANK Control value passed from DelayedControls. > > Update imx477 and imx219 CamHelper classes to extract the frame length from the > embedded data buffer. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +++++- > src/ipa/raspberrypi/cam_helper_imx477.cpp | 6 +++++- > src/ipa/raspberrypi/controller/device_status.h | 5 ++++- > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ > 4 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp > index 4d68e01fce71..a3caab714602 100644 > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp > @@ -30,7 +30,10 @@ using namespace RPiController; > constexpr uint32_t gainReg = 0x157; > constexpr uint32_t expHiReg = 0x15a; > constexpr uint32_t expLoReg = 0x15b; > -constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] = { expHiReg, expLoReg, gainReg }; > +constexpr uint32_t frameLengthHiReg = 0x160; > +constexpr uint32_t frameLengthLoReg = 0x161; > +constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] > + = { expHiReg, expLoReg, gainReg, frameLengthHiReg, frameLengthLoReg }; Didn't we have a thing about these registers having to be in numerical order? So gainReg would have to go first. Though I'm assuming that we still aren't using metadata for the imx219 so it doesn't actually matter - but we should probably get it the right way round. (Should probably have spotted that on a previous review - oh well, sorry!) > > class CamHelperImx219 : public CamHelper > { > @@ -93,6 +96,7 @@ void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap ®isters, > > deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg)); > deviceStatus.analogue_gain = Gain(registers.at(gainReg)); > + deviceStatus.frame_length = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg); > > metadata.Set("device.status", deviceStatus); > } > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp > index 4098fde6f322..91d05d9226ff 100644 > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp > @@ -23,7 +23,10 @@ constexpr uint32_t expHiReg = 0x0202; > constexpr uint32_t expLoReg = 0x0203; > constexpr uint32_t gainHiReg = 0x0204; > constexpr uint32_t gainLoReg = 0x0205; > -constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] = { expHiReg, expLoReg, gainHiReg, gainLoReg }; > +constexpr uint32_t frameLengthHiReg = 0x0340; > +constexpr uint32_t frameLengthLoReg = 0x0341; > +constexpr std::initializer_list<uint32_t> registerList = > + { expHiReg, expLoReg, gainHiReg, gainLoReg, frameLengthHiReg, frameLengthLoReg }; > > class CamHelperImx477 : public CamHelper > { > @@ -81,6 +84,7 @@ void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap ®isters, > > deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg)); > deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg)); > + deviceStatus.frame_length = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg); > > metadata.Set("device.status", deviceStatus); > } > diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h > index e2511d19b96d..916471ab258b 100644 > --- a/src/ipa/raspberrypi/controller/device_status.h > +++ b/src/ipa/raspberrypi/controller/device_status.h > @@ -17,7 +17,7 @@ > > struct DeviceStatus { > DeviceStatus() > - : shutter_speed(std::chrono::seconds(0)), analogue_gain(0.0), > + : shutter_speed(std::chrono::seconds(0)), frame_length(0), analogue_gain(0.0), > lens_position(0.0), aperture(0.0), flash_intensity(0.0) > { > } > @@ -28,6 +28,7 @@ struct DeviceStatus { > > out << "Exposure: " << d.shutter_speed > << " Gain: " << d.analogue_gain > + << " Frame Length: " << d.frame_length > << " Aperture: " << d.aperture > << " Lens: " << d.lens_position > << " Flash: " << d.flash_intensity; > @@ -37,6 +38,8 @@ struct DeviceStatus { > > /* time shutter is open */ > libcamera::utils::Duration shutter_speed; > + // frame length given in number of lines Didn't I see a patch go by where you carefully replaced C++ comments by C-style ones? :) Or perhaps that was a later patch... I can't even remember! With those little things addressed: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > + uint32_t frame_length; > double analogue_gain; > /* 1.0/distance-in-metres, or 0 if unknown */ > double lens_position; > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index f51c970befb5..db103a885b7a 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -1015,9 +1015,11 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls) > > int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); > + int32_t vblank = sensorControls.get(V4L2_CID_VBLANK).get<int32_t>(); > > deviceStatus.shutter_speed = helper_->Exposure(exposureLines); > deviceStatus.analogue_gain = helper_->Gain(gainCode); > + deviceStatus.frame_length = mode_.height + vblank; > > LOG(IPARPI, Debug) << "Metadata - " << deviceStatus; > > -- > 2.25.1 >
Hi David, Thank you for your feedback. On Fri, 2 Jul 2021 at 17:09, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for this update - I can see that this change has become more > troublesome than initially expected! > Indeed it has :( But frame length is probably important enough that we should record it down. > On Fri, 2 Jul 2021 at 16:09, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > Update the IPA to fill frame length into the DeviceStatus struct from the > > VBLANK Control value passed from DelayedControls. > > > > Update imx477 and imx219 CamHelper classes to extract the frame length > from the > > embedded data buffer. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +++++- > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 6 +++++- > > src/ipa/raspberrypi/controller/device_status.h | 5 ++++- > > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ > > 4 files changed, 16 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp > b/src/ipa/raspberrypi/cam_helper_imx219.cpp > > index 4d68e01fce71..a3caab714602 100644 > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp > > @@ -30,7 +30,10 @@ using namespace RPiController; > > constexpr uint32_t gainReg = 0x157; > > constexpr uint32_t expHiReg = 0x15a; > > constexpr uint32_t expLoReg = 0x15b; > > -constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] > = { expHiReg, expLoReg, gainReg }; > > +constexpr uint32_t frameLengthHiReg = 0x160; > > +constexpr uint32_t frameLengthLoReg = 0x161; > > +constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] > > + = { expHiReg, expLoReg, gainReg, frameLengthHiReg, > frameLengthLoReg }; > > Didn't we have a thing about these registers having to be in numerical > order? So gainReg would have to go first. Though I'm assuming that we > still aren't using metadata for the imx219 so it doesn't actually > matter - but we should probably get it the right way round. > With my last set of updates (i.e. C++ifying and generalising the SMIA parser), these registers can be listed in any order. > > (Should probably have spotted that on a previous review - oh well, sorry!) > > > > > class CamHelperImx219 : public CamHelper > > { > > @@ -93,6 +96,7 @@ void CamHelperImx219::PopulateMetadata(const > MdParser::RegisterMap ®isters, > > > > deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * > 256 + registers.at(expLoReg)); > > deviceStatus.analogue_gain = Gain(registers.at(gainReg)); > > + deviceStatus.frame_length = registers.at(frameLengthHiReg) * > 256 + registers.at(frameLengthLoReg); > > > > metadata.Set("device.status", deviceStatus); > > } > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp > b/src/ipa/raspberrypi/cam_helper_imx477.cpp > > index 4098fde6f322..91d05d9226ff 100644 > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp > > @@ -23,7 +23,10 @@ constexpr uint32_t expHiReg = 0x0202; > > constexpr uint32_t expLoReg = 0x0203; > > constexpr uint32_t gainHiReg = 0x0204; > > constexpr uint32_t gainLoReg = 0x0205; > > -constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] > = { expHiReg, expLoReg, gainHiReg, gainLoReg }; > > +constexpr uint32_t frameLengthHiReg = 0x0340; > > +constexpr uint32_t frameLengthLoReg = 0x0341; > > +constexpr std::initializer_list<uint32_t> registerList = > > + { expHiReg, expLoReg, gainHiReg, gainLoReg, frameLengthHiReg, > frameLengthLoReg }; > > > > class CamHelperImx477 : public CamHelper > > { > > @@ -81,6 +84,7 @@ void CamHelperImx477::PopulateMetadata(const > MdParser::RegisterMap ®isters, > > > > deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * > 256 + registers.at(expLoReg)); > > deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 > + registers.at(gainLoReg)); > > + deviceStatus.frame_length = registers.at(frameLengthHiReg) * > 256 + registers.at(frameLengthLoReg); > > > > metadata.Set("device.status", deviceStatus); > > } > > diff --git a/src/ipa/raspberrypi/controller/device_status.h > b/src/ipa/raspberrypi/controller/device_status.h > > index e2511d19b96d..916471ab258b 100644 > > --- a/src/ipa/raspberrypi/controller/device_status.h > > +++ b/src/ipa/raspberrypi/controller/device_status.h > > @@ -17,7 +17,7 @@ > > > > struct DeviceStatus { > > DeviceStatus() > > - : shutter_speed(std::chrono::seconds(0)), > analogue_gain(0.0), > > + : shutter_speed(std::chrono::seconds(0)), > frame_length(0), analogue_gain(0.0), > > lens_position(0.0), aperture(0.0), flash_intensity(0.0) > > { > > } > > @@ -28,6 +28,7 @@ struct DeviceStatus { > > > > out << "Exposure: " << d.shutter_speed > > << " Gain: " << d.analogue_gain > > + << " Frame Length: " << d.frame_length > > << " Aperture: " << d.aperture > > << " Lens: " << d.lens_position > > << " Flash: " << d.flash_intensity; > > @@ -37,6 +38,8 @@ struct DeviceStatus { > > > > /* time shutter is open */ > > libcamera::utils::Duration shutter_speed; > > + // frame length given in number of lines > > Didn't I see a patch go by where you carefully replaced C++ comments > by C-style ones? :) > Or perhaps that was a later patch... I can't even remember! > Groan, yes, the first patch did this, and I went and used C++ comments in this one. Will fix it in the next revision. Regards, Naush > > With those little things addressed: > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks! > David > > > + uint32_t frame_length; > > double analogue_gain; > > /* 1.0/distance-in-metres, or 0 if unknown */ > > double lens_position; > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index f51c970befb5..db103a885b7a 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -1015,9 +1015,11 @@ void IPARPi::fillDeviceStatus(const ControlList > &sensorControls) > > > > int32_t exposureLines = > sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > > int32_t gainCode = > sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); > > + int32_t vblank = > sensorControls.get(V4L2_CID_VBLANK).get<int32_t>(); > > > > deviceStatus.shutter_speed = helper_->Exposure(exposureLines); > > deviceStatus.analogue_gain = helper_->Gain(gainCode); > > + deviceStatus.frame_length = mode_.height + vblank; > > > > LOG(IPARPI, Debug) << "Metadata - " << deviceStatus; > > > > -- > > 2.25.1 > > >
diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp index 4d68e01fce71..a3caab714602 100644 --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp @@ -30,7 +30,10 @@ using namespace RPiController; constexpr uint32_t gainReg = 0x157; constexpr uint32_t expHiReg = 0x15a; constexpr uint32_t expLoReg = 0x15b; -constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] = { expHiReg, expLoReg, gainReg }; +constexpr uint32_t frameLengthHiReg = 0x160; +constexpr uint32_t frameLengthLoReg = 0x161; +constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] + = { expHiReg, expLoReg, gainReg, frameLengthHiReg, frameLengthLoReg }; class CamHelperImx219 : public CamHelper { @@ -93,6 +96,7 @@ void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap ®isters, deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg)); deviceStatus.analogue_gain = Gain(registers.at(gainReg)); + deviceStatus.frame_length = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg); metadata.Set("device.status", deviceStatus); } diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp index 4098fde6f322..91d05d9226ff 100644 --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp @@ -23,7 +23,10 @@ constexpr uint32_t expHiReg = 0x0202; constexpr uint32_t expLoReg = 0x0203; constexpr uint32_t gainHiReg = 0x0204; constexpr uint32_t gainLoReg = 0x0205; -constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] = { expHiReg, expLoReg, gainHiReg, gainLoReg }; +constexpr uint32_t frameLengthHiReg = 0x0340; +constexpr uint32_t frameLengthLoReg = 0x0341; +constexpr std::initializer_list<uint32_t> registerList = + { expHiReg, expLoReg, gainHiReg, gainLoReg, frameLengthHiReg, frameLengthLoReg }; class CamHelperImx477 : public CamHelper { @@ -81,6 +84,7 @@ void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap ®isters, deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg)); deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg)); + deviceStatus.frame_length = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg); metadata.Set("device.status", deviceStatus); } diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h index e2511d19b96d..916471ab258b 100644 --- a/src/ipa/raspberrypi/controller/device_status.h +++ b/src/ipa/raspberrypi/controller/device_status.h @@ -17,7 +17,7 @@ struct DeviceStatus { DeviceStatus() - : shutter_speed(std::chrono::seconds(0)), analogue_gain(0.0), + : shutter_speed(std::chrono::seconds(0)), frame_length(0), analogue_gain(0.0), lens_position(0.0), aperture(0.0), flash_intensity(0.0) { } @@ -28,6 +28,7 @@ struct DeviceStatus { out << "Exposure: " << d.shutter_speed << " Gain: " << d.analogue_gain + << " Frame Length: " << d.frame_length << " Aperture: " << d.aperture << " Lens: " << d.lens_position << " Flash: " << d.flash_intensity; @@ -37,6 +38,8 @@ struct DeviceStatus { /* time shutter is open */ libcamera::utils::Duration shutter_speed; + // frame length given in number of lines + uint32_t frame_length; double analogue_gain; /* 1.0/distance-in-metres, or 0 if unknown */ double lens_position; diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index f51c970befb5..db103a885b7a 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -1015,9 +1015,11 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls) int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); + int32_t vblank = sensorControls.get(V4L2_CID_VBLANK).get<int32_t>(); deviceStatus.shutter_speed = helper_->Exposure(exposureLines); deviceStatus.analogue_gain = helper_->Gain(gainCode); + deviceStatus.frame_length = mode_.height + vblank; LOG(IPARPI, Debug) << "Metadata - " << deviceStatus;
Update the IPA to fill frame length into the DeviceStatus struct from the VBLANK Control value passed from DelayedControls. Update imx477 and imx219 CamHelper classes to extract the frame length from the embedded data buffer. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +++++- src/ipa/raspberrypi/cam_helper_imx477.cpp | 6 +++++- src/ipa/raspberrypi/controller/device_status.h | 5 ++++- src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ 4 files changed, 16 insertions(+), 3 deletions(-)