[libcamera-devel] [PATCH v2 1/1] ipa: raspberrypi: Use CamHelpers to generalise sensor metadata parsing
Naushir Patuck
naush at raspberrypi.com
Fri Mar 26 14:43:10 CET 2021
Hi David,
On Fri, 26 Mar 2021 at 12:43, David Plowman <david.plowman at raspberrypi.com>
wrote:
> CamHelpers get virtual Prepare() and Process() methods, running just
> before and just after the ISP, just like Raspberry Pi Algorithms.
>
> The Prepare() method is able to parse the register dumps in embedded
> data buffers, and can be specialised to perform custom processing when
> necessary.
>
> The IPA itself only needs to call the new Prepare() and Process()
> methods. It must fill in the DeviceStatus from the controls first, in
> case the Prepare() method does not supply its own values.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
> src/ipa/raspberrypi/cam_helper.cpp | 51 ++++++++++++++++++
> src/ipa/raspberrypi/cam_helper.hpp | 11 +++-
> src/ipa/raspberrypi/raspberrypi.cpp | 80 ++++++++++-------------------
> 3 files changed, 87 insertions(+), 55 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> index 0ae0baa0..5af73c9c 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -17,6 +17,11 @@
> #include "md_parser.hpp"
>
> using namespace RPiController;
> +using namespace libcamera;
> +
> +namespace libcamera {
> +LOG_DECLARE_CATEGORY(IPARPI)
> +}
>
> static std::map<std::string, CamHelperCreateFunc> cam_helpers;
>
> @@ -45,6 +50,17 @@ CamHelper::~CamHelper()
> delete parser_;
> }
>
> +void CamHelper::Prepare(const Span<uint8_t> &buffer,
> + Metadata &metadata)
> +{
> + parseEmbeddedData(buffer, metadata);
> +}
> +
> +void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,
> + [[maybe_unused]] Metadata &metadata)
> +{
> +}
> +
> uint32_t CamHelper::ExposureLines(double exposure_us) const
> {
> assert(initialized_);
> @@ -139,6 +155,41 @@ unsigned int CamHelper::MistrustFramesModeSwitch()
> const
> return 0;
> }
>
> +void CamHelper::parseEmbeddedData(const Span<uint8_t> &buffer,
> + Metadata &metadata)
> +{
> + if (buffer.size()) {
> + bool success = false;
> + uint32_t exposureLines, gainCode;
> +
> + parser_->SetBufferSize(buffer.size());
> + success = parser_->Parse(buffer.data()) ==
> MdParser::Status::OK &&
> + parser_->GetExposureLines(exposureLines) ==
> MdParser::Status::OK &&
> + parser_->GetGainCode(gainCode) ==
> MdParser::Status::OK;
> +
> + /*
> + * Overwrite the exposure/gain values in the DeviceStatus,
> as
> + * we know better. Fetch it first in case any other fields
> were
> + * set meaningfully.
> + */
> + struct DeviceStatus deviceStatus;
> +
> + if (success &&
> + metadata.Get("device.status", deviceStatus) == 0) {
>
I'm trying to work out if the second clause is needed? Shouldn't we write
device.status
unconditionally, whether it is present in the metadata or not? I suppose
it does not matter,
it is guaranteed to be there from the IPA.
Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> + deviceStatus.shutter_speed =
> Exposure(exposureLines);
> + deviceStatus.analogue_gain = Gain(gainCode);
> +
> + LOG(IPARPI, Debug) << "Metadata updated - Exposure
> : "
> + << deviceStatus.shutter_speed
> + << " Gain : "
> + << deviceStatus.analogue_gain;
> +
> + metadata.Set("device.status", deviceStatus);
> + } else
> + LOG(IPARPI, Error) << "Embedded buffer parsing
> failed";
> + }
> +}
> +
> RegisterCamHelper::RegisterCamHelper(char const *cam_name,
> CamHelperCreateFunc create_func)
> {
> diff --git a/src/ipa/raspberrypi/cam_helper.hpp
> b/src/ipa/raspberrypi/cam_helper.hpp
> index 1b2d6eec..930ea39a 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -8,7 +8,11 @@
>
> #include <string>
>
> +#include <libcamera/span.h>
> +
> #include "camera_mode.h"
> +#include "controller/controller.hpp"
> +#include "controller/metadata.hpp"
> #include "md_parser.hpp"
>
> #include "libcamera/internal/v4l2_videodevice.h"
> @@ -65,7 +69,9 @@ public:
> CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);
> virtual ~CamHelper();
> void SetCameraMode(const CameraMode &mode);
> - MdParser &Parser() const { return *parser_; }
> + virtual void Prepare(const libcamera::Span<uint8_t> &buffer,
> + Metadata &metadata);
> + virtual void Process(StatisticsPtr &stats, Metadata &metadata);
> uint32_t ExposureLines(double exposure_us) const;
> double Exposure(uint32_t exposure_lines) const; // in us
> virtual uint32_t GetVBlanking(double &exposure_us, double
> minFrameDuration,
> @@ -81,6 +87,9 @@ public:
> virtual unsigned int MistrustFramesModeSwitch() const;
>
> protected:
> + void parseEmbeddedData(const libcamera::Span<uint8_t> &buffer,
> + Metadata &metadata);
> +
> MdParser *parser_;
> CameraMode mode_;
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 1c928b72..32ecbdcd 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -101,9 +101,7 @@ private:
> void returnEmbeddedBuffer(unsigned int bufferId);
> void prepareISP(const ipa::RPi::ISPConfig &data);
> void reportMetadata();
> - bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus
> &deviceStatus);
> - void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
> - struct DeviceStatus &deviceStatus);
> + void fillDeviceStatus(const ControlList &sensorControls);
> void processStats(unsigned int bufferId);
> void applyFrameDurations(double minFrameDuration, double
> maxFrameDuration);
> void applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls);
> @@ -895,35 +893,34 @@ void IPARPi::returnEmbeddedBuffer(unsigned int
> bufferId)
>
> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
> {
> - struct DeviceStatus deviceStatus = {};
> - bool success = false;
> + Span<uint8_t> embeddedBuffer;
> +
> + rpiMetadata_.Clear();
> +
> + fillDeviceStatus(data.controls);
>
> if (data.embeddedBufferPresent) {
> /*
> * Pipeline handler has supplied us with an embedded data
> buffer,
> - * so parse it and extract the exposure and gain.
> + * we must pass it to the CamHelper for parsing.
> */
> - success = parseEmbeddedData(data.embeddedBufferId,
> deviceStatus);
> -
> - /* Done with embedded data now, return to pipeline handler
> asap. */
> - returnEmbeddedBuffer(data.embeddedBufferId);
> + auto it = buffers_.find(data.embeddedBufferId);
> + ASSERT(it != buffers_.end());
> + embeddedBuffer = it->second.maps()[0];
> }
>
> - if (!success) {
> - /*
> - * Pipeline handler has not supplied an embedded data
> buffer,
> - * or embedded data buffer parsing has failed for some
> reason,
> - * so pull the exposure and gain values from the control
> list.
> - */
> - int32_t exposureLines =
> data.controls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> - int32_t gainCode =
> data.controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> - fillDeviceStatus(exposureLines, gainCode, deviceStatus);
> - }
> + /*
> + * This may overwrite the DeviceStatus using values from the sensor
> + * metadata, and may also do additional custom processing.
> + */
> + helper_->Prepare(embeddedBuffer, rpiMetadata_);
> +
> + /* Done with embedded data now, return to pipeline handler asap. */
> + if (data.embeddedBufferPresent)
> + returnEmbeddedBuffer(data.embeddedBufferId);
>
> ControlList ctrls(ispCtrls_);
>
> - rpiMetadata_.Clear();
> - rpiMetadata_.Set("device.status", deviceStatus);
> controller_.Prepare(&rpiMetadata_);
>
> /* Lock the metadata buffer to avoid constant locks/unlocks. */
> @@ -973,41 +970,13 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig
> &data)
> setIspControls.emit(ctrls);
> }
>
> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus
> &deviceStatus)
> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls)
> {
> - auto it = buffers_.find(bufferId);
> - if (it == buffers_.end()) {
> - LOG(IPARPI, Error) << "Could not find embedded buffer!";
> - return false;
> - }
> -
> - Span<uint8_t> mem = it->second.maps()[0];
> - helper_->Parser().SetBufferSize(mem.size());
> - RPiController::MdParser::Status status =
> helper_->Parser().Parse(mem.data());
> - if (status != RPiController::MdParser::Status::OK) {
> - LOG(IPARPI, Error) << "Embedded Buffer parsing failed,
> error " << status;
> - return false;
> - } else {
> - uint32_t exposureLines, gainCode;
> - if (helper_->Parser().GetExposureLines(exposureLines) !=
> RPiController::MdParser::Status::OK) {
> - LOG(IPARPI, Error) << "Exposure time failed";
> - return false;
> - }
> -
> - if (helper_->Parser().GetGainCode(gainCode) !=
> RPiController::MdParser::Status::OK) {
> - LOG(IPARPI, Error) << "Gain failed";
> - return false;
> - }
> -
> - fillDeviceStatus(exposureLines, gainCode, deviceStatus);
> - }
> + struct DeviceStatus deviceStatus = {};
>
> - return true;
> -}
> + int32_t exposureLines =
> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> + int32_t gainCode =
> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>
> -void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
> - struct DeviceStatus &deviceStatus)
> -{
> deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> deviceStatus.analogue_gain = helper_->Gain(gainCode);
>
> @@ -1015,6 +984,8 @@ void IPARPi::fillDeviceStatus(uint32_t exposureLines,
> uint32_t gainCode,
> << deviceStatus.shutter_speed
> << " Gain : "
> << deviceStatus.analogue_gain;
> +
> + rpiMetadata_.Set("device.status", deviceStatus);
> }
>
> void IPARPi::processStats(unsigned int bufferId)
> @@ -1028,6 +999,7 @@ void IPARPi::processStats(unsigned int bufferId)
> Span<uint8_t> mem = it->second.maps()[0];
> bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats
> *>(mem.data());
> RPiController::StatisticsPtr statistics =
> std::make_shared<bcm2835_isp_stats>(*stats);
> + helper_->Process(statistics, rpiMetadata_);
> controller_.Process(statistics, &rpiMetadata_);
>
> struct AgcStatus agcStatus;
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210326/d68103e5/attachment-0001.htm>
More information about the libcamera-devel
mailing list