[libcamera-devel] [RFC PATCH 1/1] ipa: raspberrypi: Use CamHelpers to generalise embedded data parsing
Naushir Patuck
naush at raspberrypi.com
Tue Mar 16 11:15:15 CET 2021
Hi David,
Thank you for your work.
On Wed, 10 Mar 2021 at 17:23, 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 no longer performs this register parsing, it just has
> to call the new Prepare() and Process() methods.
> ---
> src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++
> src/ipa/raspberrypi/cam_helper.hpp | 14 ++++-
> src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++---------------------
> 3 files changed, 84 insertions(+), 67 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> index 0ae0baa0..fc69f4cb 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)
> +}
>
Is this namespace enclosure needed for some reason?
Seems to compile well enough without it. I wonder if we should add a
category for cam_helper based logging?
>
> static std::map<std::string, CamHelperCreateFunc> cam_helpers;
>
> @@ -45,6 +50,17 @@ CamHelper::~CamHelper()
> delete parser_;
> }
>
> +void CamHelper::Prepare(const Span<uint8_t> &buffer,
> + const ControlList &controls, Metadata &metadata)
> +{
> + parseRegisters(buffer, controls, 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,39 @@ unsigned int CamHelper::MistrustFramesModeSwitch()
> const
> return 0;
> }
>
> +void CamHelper::parseRegisters(const Span<uint8_t> &buffer,
> + const ControlList &controls, Metadata
> &metadata)
> +{
>
Bit of a nit-pick (sorry) , but could you consider a few renames:
s/parseRegisters/parseEmbeddedData/
s/controls/sensorControls/
s/metadata/controllerMetadata/
Took me a bit of inspection to remind myself what those variables
represented.
Not too fussed either way though.
+ struct DeviceStatus deviceStatus = {};
> + bool success = false;
> + uint32_t exposureLines, gainCode;
> +
> + if (buffer.size()) {
> + parser_->SetBufferSize(buffer.size());
> + success = parser_->Parse(buffer.data()) ==
> MdParser::Status::OK &&
> + parser_->GetExposureLines(exposureLines) ==
> MdParser::Status::OK &&
> + parser_->GetGainCode(gainCode) ==
> MdParser::Status::OK;
> +
> + if (!success)
> + LOG(IPARPI, Error) << "Embedded buffer parsing
> failed";
> + }
> +
> + if (!success) {
> + exposureLines =
> controls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> + gainCode =
> controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> + }
> +
> + deviceStatus.shutter_speed = Exposure(exposureLines);
> + deviceStatus.analogue_gain = Gain(gainCode);
> +
> + LOG(IPARPI, Debug) << "Metadata - Exposure : "
> + << deviceStatus.shutter_speed
> + << " Gain : "
> + << deviceStatus.analogue_gain;
> +
> + metadata.Set("device.status", deviceStatus);
> +}
> +
> 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..ce5b82d2 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -8,8 +8,13 @@
>
> #include <string>
>
> +#include <libcamera/controls.h>
> +#include <libcamera/span.h>
> +
> #include "camera_mode.h"
> +#include "controller/controller.hpp"
> #include "md_parser.hpp"
> +#include "controller/metadata.hpp"
>
This will need to be ordered alphabetically.
>
> #include "libcamera/internal/v4l2_videodevice.h"
>
> @@ -65,7 +70,10 @@ 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,
> + const libcamera::ControlList &controls,
> + 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 +89,10 @@ public:
> virtual unsigned int MistrustFramesModeSwitch() const;
>
> protected:
> + void parseRegisters(const libcamera::Span<uint8_t> &buffer,
> + const libcamera::ControlList &controls,
> + Metadata &metadata);
> +
> MdParser *parser_;
> CameraMode mode_;
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 7904225a..d699540a 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -103,9 +103,6 @@ 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 processStats(unsigned int bufferId);
> void applyFrameDurations(double minFrameDuration, double
> maxFrameDuration);
> void applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls);
> @@ -913,35 +910,37 @@ void IPARPi::returnEmbeddedBuffer(unsigned int
> bufferId)
>
> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
> {
> - struct DeviceStatus deviceStatus = {};
> - bool success = false;
> + Span<uint8_t> embeddedBuffer;
> + bool returnEmbedded = false;
> +
> + rpiMetadata_.Clear();
>
> 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);
> + if (it == buffers_.end())
> + LOG(IPARPI, Error) << "Could not find embedded
> buffer!";
>
This really should never happen, perhaps an assert in its place?
> + else {
> + embeddedBuffer = it->second.maps()[0];
> + returnEmbedded = true;
> + }
> }
>
> - 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 will add the DeviceStatus to the metadata, and depending
> on the
> + * sensor, may do additional custom processing.
> + */
> + helper_->Prepare(embeddedBuffer, data.controls, rpiMetadata_);
> +
> + /* Done with embedded data now, return to pipeline handler asap. */
> + if (returnEmbedded)
> + returnEmbeddedBuffer(data.embeddedBufferId);
>
I think this should happen unconditionally - but as before, returnEmbedded
should never
be false. If the earlier code was changed to an assert, you could
remove returnEmbedded.
Regards,
Naush
> ControlList ctrls(ispCtrls_);
>
> - rpiMetadata_.Clear();
> - rpiMetadata_.Set("device.status", deviceStatus);
> controller_.Prepare(&rpiMetadata_);
>
> /* Lock the metadata buffer to avoid constant locks/unlocks. */
> @@ -991,50 +990,6 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig
> &data)
> setIspControls.emit(ctrls);
> }
>
> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus
> &deviceStatus)
> -{
> - 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);
> - }
> -
> - return true;
> -}
> -
> -void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
> - struct DeviceStatus &deviceStatus)
> -{
> - deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> - deviceStatus.analogue_gain = helper_->Gain(gainCode);
> -
> - LOG(IPARPI, Debug) << "Metadata - Exposure : "
> - << deviceStatus.shutter_speed
> - << " Gain : "
> - << deviceStatus.analogue_gain;
> -}
> -
> void IPARPi::processStats(unsigned int bufferId)
> {
> auto it = buffers_.find(bufferId);
> @@ -1046,6 +1001,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/20210316/37147d1a/attachment-0001.htm>
More information about the libcamera-devel
mailing list