[libcamera-devel] [PATCH v3 1/1] ipa: raspberrypi: Use CamHelpers to generalise sensor embedded data parsing
Marvin Schmidt
marvin.schmidt1987 at gmail.com
Thu May 6 19:17:56 CEST 2021
Hi David,
Thanks for your work on this
Am Mi., 5. Mai 2021 um 16:04 Uhr schrieb David Plowman
<david.plowman at raspberrypi.com>:
>
> 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>
> Reviewed-by: Naushir Patuck <naush 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()) {
You can use `Span::empty()` to check if the span is... empty ;-)
Since the function can't do much without some data, I would return
immediately in that case
and save a level of indentation:
if (buffer.empty())
return;
// do real work
> + 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;
> +
I would probably check `success` right here and return early if it
wasn't successful
> + /*
> + * 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;
The `struct` isn't needed. And just for good measure, I would
zero-initialize it with `= {}`
> +
> + if (success &&
> + metadata.Get("device.status", deviceStatus) == 0) {
> + 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";
If we check `success` right after the parsing step and bail out if it
wasn't successful, this could be written
like:
if (metadata.Get("device.status", deviceStatus) != 0) {
LOG(IPARPI, Error) << "Could not get device status from metadata";
return;
}
deviceStatus.shutter_speed = Exposure(exposureLines);
deviceStatus.analogue_gain = Gain(gainCode);
[...]
I guess it's good to have a different error message for the
`metadata.Get("device.status", deviceStatus) != 0` case
anyway, since it's not really related to the parsing of the embedded data?
> + }
> +}
> +
> 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 c3ed5362..6ee5051c 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);
The CppCoreGuidelines suggest passing spans by value:
Note: A span<T> object does not own its elements and is so small
that it can be passed by value.
Passing a span object as an argument is exactly as efficient as
passing a pair of pointer arguments
or passing a pointer and an integer count.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f24-use-a-spant-or-a-span_pt-to-designate-a-half-open-sequence
So this could be `libcamera::Span<const uint8_t> buffer` since the
function just needs read-only access to the
buffer data. This would require changing the signature of
`MdParser::Parse()` though. Either to `const void *` or
better yet `libcamera::Span<const uint8_t>` as well. Which would allow
removing the `static_cast<uint8_t *>(data)`
inside the `MdParser::Parse()` implementations.
> + 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 dad6395f..b0f61d35 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);
> @@ -894,35 +892,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;
This can be `Span<const uint8_t>`
> +
> + 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. */
> @@ -972,41 +969,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 = {};
`struct` can be removed here as well
>
> - 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);
>
> @@ -1014,6 +983,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)
> @@ -1027,6 +998,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
More information about the libcamera-devel
mailing list