[libcamera-devel] [PATCH v2 1/1] ipa: raspberrypi: Use CamHelpers to generalise sensor metadata parsing
David Plowman
david.plowman at raspberrypi.com
Tue May 4 10:20:11 CEST 2021
Hi again everyone
Can I give this one another little prod too, please?
Thanks!
David
On Wed, 21 Apr 2021 at 09:59, David Plowman
<david.plowman at raspberrypi.com> wrote:
>
> Hi everyone
>
> I was wondering if I could apply a little nudge to this one too as
> there are some other patches in the system relying on it. Does this
> need more thought, do we think?
>
> Thanks!
> David
>
> On Fri, 26 Mar 2021 at 13:43, Naushir Patuck <naush at raspberrypi.com> wrote:
> >
> > 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
More information about the libcamera-devel
mailing list