[libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers to generalise sensor metadata parsing
Naushir Patuck
naush at raspberrypi.com
Fri Mar 26 10:57:10 CET 2021
Hi David,
On Fri, 26 Mar 2021 at 09:32, David Plowman <david.plowman at raspberrypi.com>
wrote:
> Hi Naush
>
> Thanks for the feedback!
>
> On Thu, 25 Mar 2021 at 16:08, Naushir Patuck <naush at raspberrypi.com>
> wrote:
> >
> > Hi David,
> >
> > Thank you for your work.
> >
> > On Mon, 22 Mar 2021 at 16:01, 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, and to supply exposure/gain values from the controls when the
> >> CamHelper does not.
> >>
> >> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> >> ---
> >> src/ipa/raspberrypi/cam_helper.cpp | 44 +++++++++++++++
> >> src/ipa/raspberrypi/cam_helper.hpp | 11 +++-
> >> src/ipa/raspberrypi/raspberrypi.cpp | 83 ++++++++++-------------------
> >> 3 files changed, 83 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> >> index 0ae0baa0..ce6482ba 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)
> >> +}
> >
> >
> > I don't think this namespace scope is needed. Seems to compile fine
> without.
>
> Funny one, this. It builds like that for me too but when I run it (for
> example, qcam), it bombs out with
>
> build/src/qcam/qcam: symbol lookup error:
> /home/pi/libcamera/build/src/ipa/raspberrypi/ipa_rpi.so: undefined
> symbol: _Z17logCategoryIPARPIv
>
> It seems to be looking for logCategory::IPARPI when I think it should
> be libcamera::logCategory::IPARPI. I guess I'm not clear why the
> linker doesn't flag that up... anyone know?
>
> >
> >>
> >>
> >> 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);
> >> +}
> >
> >
> >
> > Would it be useful to pass in the ControlList that contains the sensor
> setup
> > (from DelayedControls) into this function? Would the CamHelper ever want
> > to know these things?
> >
> > If the answer is yes, you could also pull in the case that deals with
> non-embedded
> > data where we pull values from the ControlList. So something like:
> >
> > void CamHelper::Prepare(const Span<uint8_t> &buffer,
> > Metadata &metadata, ControlList &SensorControls)
> > {
> > bool success = false;
> >
> > if (buffer.size())
> > success = parseEmbeddedData(buffer, metadata);
> >
> > if (!success) {
> > struct DeviceStatus deviceStatus = {};
> > int32_t exposureLines =
> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > int32_t gainCode =
> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> >
> > deviceStatus.shutter_speed = Exposure(exposureLines);
> > deviceStatus.analogue_gain = Gain(gainCode);
> > metadata.Set("device.status", deviceStatus);
> > }
> > }
> >
> > What do you think? Might be a bit neater to have deviecStatus set in one
> place only.
>
> Interesting point, and in fact, my previous version of this patch
> (labelled as "RFC") did exactly this. Obviously I back-tracked on
> that, for the following reasons.
>
> The version you've outlined above is nice because it makes the IPA
> file (raspberrypi.cpp) a bit tidier, functions like
> CamHelper::Exposure and CamHelper::Gain only need to be called once.
> The DeviceStatus only gets set in one place.
>
> The version I went for in the end has the advantage that it makes
> CamHelper::Prepare responsible only for parsing sensor metadata.
> There's nothing it has to remember to do if, for example, some
> particular metadata isn't there.
>
> This means that the CamHelper::Prepare for my mysterious new sensor
> doesn't have to do anything extra. It just parses the sensor's
> statistics buffer and nothing else. As this kind of code - CamHelpers
> - is likely to get duplicated more in future, potentially by 3rd
> parties, I was keener to avoid little "gotchas" in there, and slight
> uglification of raspberrypi.cpp seemed reasonable in return.
>
> But it's a close call and I'm definitely swayable...
>
Perhaps it's a bit premature to think about this, but what if some stuff
running in
prepare() (e.g. fancy phase detect AF) needs the exposure and gain values
used
for the sensor, and we don't have embedded data to give us these values.
You could
possibly run this fancy AF in process() where you do get the values, but
then you add
a frame's worth of latency.
It also possibly makes the code a bit cleaner for an up-coming change for
imx477 ultra
long exposures where I have to disable parsing when the exposure factor is
set. But
again, I have not written code for this yet to tell for sure.
I'll leave it to you to choose :-)
Naush
>
> > With or without this change and the fixup for the namespace scoping
> above:
> >
> > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
>
> Thanks!
>
> David
>
> >
> >>
> >> +
> >> +void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,
> >> + [[maybe_unused]] Metadata &metadata)
> >> +{
> >> +}
> >> +
> >> uint32_t CamHelper::ExposureLines(double exposure_us) const
> >> {
> >> assert(initialized_);
> >> @@ -139,6 +155,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch()
> const
> >> return 0;
> >> }
> >>
> >> +void CamHelper::parseEmbeddedData(const Span<uint8_t> &buffer,
> >> + Metadata &metadata)
> >>
> >> +{
> >> + if (buffer.size()) {
> >> + struct DeviceStatus deviceStatus = {};
> >> + 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;
> >> +
> >> + if (success) {
> >> + 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);
> >> + } 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 7904225a..e08b47c0 100644
> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >> @@ -103,8 +103,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,
> >> + void fillDeviceStatus(const ControlList &sensorControls,
> >> struct DeviceStatus &deviceStatus);
> >> void processStats(unsigned int bufferId);
> >> void applyFrameDurations(double minFrameDuration, double
> maxFrameDuration);
> >> @@ -913,35 +912,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;
> >> +
> >> + 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);
> >> + 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 add the DeviceStatus to the metadata, and depending
> on the
> >> + * sensor, may do additional custom processing.
> >> + */
> >> + helper_->Prepare(embeddedBuffer, rpiMetadata_);
> >> +
> >> + /* Add DeviceStatus metadata if helper_->Prepare() didn't. */
> >> + DeviceStatus deviceStatus = {};
> >> + if (rpiMetadata_.Get("device.status", deviceStatus))
> >> + fillDeviceStatus(data.controls, deviceStatus);
> >> +
> >> + /* 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. */
> >> @@ -991,41 +992,12 @@ 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,
> >> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls,
> >> struct DeviceStatus &deviceStatus)
> >> {
> >> + int32_t exposureLines =
> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >> + int32_t gainCode =
> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> >> +
> >> deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> >> deviceStatus.analogue_gain = helper_->Gain(gainCode);
> >>
> >> @@ -1033,6 +1005,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)
> >> @@ -1046,6 +1020,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/e3d64a23/attachment-0001.htm>
More information about the libcamera-devel
mailing list