[libcamera-devel] [PATCH v3 1/1] ipa: raspberrypi: Use CamHelpers to generalise sensor embedded data parsing
David Plowman
david.plowman at raspberrypi.com
Fri May 7 11:21:32 CEST 2021
Hi Marvin
Thanks for the feedback!
On Thu, 6 May 2021 at 18:18, Marvin Schmidt
<marvin.schmidt1987 at gmail.com> wrote:
>
> 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 ;-)
Yes, thanks, that's nicer!
>
> 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
Sure, I think that makes sense too.
>
> > + 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 `= {}`
I tend to put "struct" in to remind myself that something is a vanilla
C data type. But I agree, I can just as easily take it out. I'm
slightly disinclined to zero-initialise things when it's not necessary
- if you do it all the time then it can cost actual CPU cycles. But I
don't particularly mind if there's a general preference for
zero-initialising stuff...
>
> > +
> > + 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
Thanks, I hadn't read that. I guess it doesn't formally tell us about
the merits of passing a Span by reference vs. passing it by value, but
it's tiny so hardly matters. Looking through libcamera, I would say
they're mostly passed by value so I can copy that pattern too.
Anyway, thanks for all those suggestions. I'll make those various
changes and submit another version.
Best regards
David
>
> 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