<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your work.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 22 Mar 2021 at 16:01, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">CamHelpers get virtual Prepare() and Process() methods, running just<br>
before and just after the ISP, just like Raspberry Pi Algorithms.<br>
<br>
The Prepare() method is able to parse the register dumps in embedded<br>
data buffers, and can be specialised to perform custom processing when<br>
necessary.<br>
<br>
The IPA itself only needs to call the new Prepare() and Process()<br>
methods, and to supply exposure/gain values from the controls when the<br>
CamHelper does not.<br>
<br>
Signed-off-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
---<br>
src/ipa/raspberrypi/cam_helper.cpp | 44 +++++++++++++++<br>
src/ipa/raspberrypi/cam_helper.hpp | 11 +++-<br>
src/ipa/raspberrypi/raspberrypi.cpp | 83 ++++++++++-------------------<br>
3 files changed, 83 insertions(+), 55 deletions(-)<br>
<br>
diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp<br>
index 0ae0baa0..ce6482ba 100644<br>
--- a/src/ipa/raspberrypi/cam_helper.cpp<br>
+++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
@@ -17,6 +17,11 @@<br>
#include "md_parser.hpp"<br>
<br>
using namespace RPiController;<br>
+using namespace libcamera;<br>
+<br>
+namespace libcamera {<br>
+LOG_DECLARE_CATEGORY(IPARPI)<br>
+}<br></blockquote><div><br></div><div>I don't think this namespace scope is needed. Seems to compile fine without.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
static std::map<std::string, CamHelperCreateFunc> cam_helpers;<br>
<br>
@@ -45,6 +50,17 @@ CamHelper::~CamHelper()<br>
delete parser_;<br>
}<br>
<br>
+void CamHelper::Prepare(const Span<uint8_t> &buffer,<br>
+ Metadata &metadata)<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+{<br>
+ parseEmbeddedData(buffer, metadata);<br>
+}<br>
</blockquote><div><br></div><div><div><br></div><div>Would it be useful to pass in the ControlList that contains the sensor setup</div><div>(from DelayedControls) into this function? Would the CamHelper ever want</div><div>to know these things?</div><div><br></div><div>If the answer is yes, you could also pull in the case that deals with non-embedded</div><div>data where we pull values from the ControlList. So something like:</div><div><font style="background-color:rgb(255,255,255)" color="#000000"><br></font></div><div><div style="line-height:18px;white-space:pre"><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif">void CamHelper::Prepare(const Span<uint8_t> &buffer,</font></div><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"> Metadata &metadata,</font><span style="color:rgb(0,0,0);font-family:arial,sans-serif"> ControlList </span>&SensorControls<span style="color:rgb(0,0,0);font-family:arial,sans-serif">)</span></div><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif">{</font></div><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"> bool success = false;</font></div><font style="font-size:13px;background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"><br></font><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"> if (buffer.size())</font></div><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"> success = parseEmbeddedData(buffer, metadata);</font></div><font style="font-size:13px;background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"><br></font><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"> if (!success) {</font></div><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"> struct DeviceStatus deviceStatus = {};</font></div><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"> int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();</font></div><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"> int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();</font></div><font style="font-size:13px;background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"><br></font><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"> deviceStatus.shutter_speed = Exposure(exposureLines);</font></div><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"> deviceStatus.analogue_gain = Gain(gainCode);</font></div><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"> metadata.Set("device.status", deviceStatus);</font></div><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"> }</font></div><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif">}</font></div><div style="font-size:13px"><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"><br></font></div><div style=""><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif">What do you think? Might be a bit neater to have deviecStatus set in one place only.</font></div><div style=""><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif">With or without this change and the fixup for the namespace scoping above:</font></div><div style=""><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif"><br></font></div><div style=""><font style="background-color:rgb(255,255,255)" color="#000000" face="arial, sans-serif">Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.com</a>></font></div></div></div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+<br>
+void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,<br>
+ [[maybe_unused]] Metadata &metadata)<br>
+{<br>
+}<br>
+<br>
uint32_t CamHelper::ExposureLines(double exposure_us) const<br>
{<br>
assert(initialized_);<br>
@@ -139,6 +155,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const<br>
return 0;<br>
}<br>
<br>
+void CamHelper::parseEmbeddedData(const Span<uint8_t> &buffer,<br>
+ Metadata &metadata)<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+{<br>
+ if (buffer.size()) {<br>
+ struct DeviceStatus deviceStatus = {};<br>
+ bool success = false;<br>
+ uint32_t exposureLines, gainCode;<br>
+<br>
+ parser_->SetBufferSize(buffer.size());<br>
+ success = parser_->Parse(buffer.data()) == MdParser::Status::OK &&<br>
+ parser_->GetExposureLines(exposureLines) == MdParser::Status::OK &&<br>
+ parser_->GetGainCode(gainCode) == MdParser::Status::OK;<br>
+<br>
+ if (success) {<br>
+ deviceStatus.shutter_speed = Exposure(exposureLines);<br>
+ deviceStatus.analogue_gain = Gain(gainCode);<br>
+<br>
+ LOG(IPARPI, Debug) << "Metadata - Exposure : "<br>
+ << deviceStatus.shutter_speed<br>
+ << " Gain : "<br>
+ << deviceStatus.analogue_gain;<br>
+<br>
+ metadata.Set("device.status", deviceStatus);<br>
+ } else<br>
+ LOG(IPARPI, Error) << "Embedded buffer parsing failed";<br>
+ }<br>
+}<br>
+<br>
RegisterCamHelper::RegisterCamHelper(char const *cam_name,<br>
CamHelperCreateFunc create_func)<br>
{<br>
diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp<br>
index 1b2d6eec..930ea39a 100644<br>
--- a/src/ipa/raspberrypi/cam_helper.hpp<br>
+++ b/src/ipa/raspberrypi/cam_helper.hpp<br>
@@ -8,7 +8,11 @@<br>
<br>
#include <string><br>
<br>
+#include <libcamera/span.h><br>
+<br>
#include "camera_mode.h"<br>
+#include "controller/controller.hpp"<br>
+#include "controller/metadata.hpp"<br>
#include "md_parser.hpp"<br>
<br>
#include "libcamera/internal/v4l2_videodevice.h"<br>
@@ -65,7 +69,9 @@ public:<br>
CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);<br>
virtual ~CamHelper();<br>
void SetCameraMode(const CameraMode &mode);<br>
- MdParser &Parser() const { return *parser_; }<br>
+ virtual void Prepare(const libcamera::Span<uint8_t> &buffer,<br>
+ Metadata &metadata);<br>
+ virtual void Process(StatisticsPtr &stats, Metadata &metadata);<br>
uint32_t ExposureLines(double exposure_us) const;<br>
double Exposure(uint32_t exposure_lines) const; // in us<br>
virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,<br>
@@ -81,6 +87,9 @@ public:<br>
virtual unsigned int MistrustFramesModeSwitch() const;<br>
<br>
protected:<br>
+ void parseEmbeddedData(const libcamera::Span<uint8_t> &buffer,<br>
+ Metadata &metadata);<br>
+<br>
MdParser *parser_;<br>
CameraMode mode_;<br>
<br>
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
index 7904225a..e08b47c0 100644<br>
--- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
+++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
@@ -103,8 +103,7 @@ private:<br>
void returnEmbeddedBuffer(unsigned int bufferId);<br>
void prepareISP(const ipa::RPi::ISPConfig &data);<br>
void reportMetadata();<br>
- bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);<br>
- void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,<br>
+ void fillDeviceStatus(const ControlList &sensorControls,<br>
struct DeviceStatus &deviceStatus);<br>
void processStats(unsigned int bufferId);<br>
void applyFrameDurations(double minFrameDuration, double maxFrameDuration);<br>
@@ -913,35 +912,37 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)<br>
<br>
void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)<br>
{<br>
- struct DeviceStatus deviceStatus = {};<br>
- bool success = false;<br>
+ Span<uint8_t> embeddedBuffer;<br>
+<br>
+ rpiMetadata_.Clear();<br>
<br>
if (data.embeddedBufferPresent) {<br>
/*<br>
* Pipeline handler has supplied us with an embedded data buffer,<br>
- * so parse it and extract the exposure and gain.<br>
+ * we must pass it to the CamHelper for parsing.<br>
*/<br>
- success = parseEmbeddedData(data.embeddedBufferId, deviceStatus);<br>
-<br>
- /* Done with embedded data now, return to pipeline handler asap. */<br>
- returnEmbeddedBuffer(data.embeddedBufferId);<br>
+ auto it = buffers_.find(data.embeddedBufferId);<br>
+ ASSERT(it != buffers_.end());<br>
+ embeddedBuffer = it->second.maps()[0];<br>
}<br>
<br>
- if (!success) {<br>
- /*<br>
- * Pipeline handler has not supplied an embedded data buffer,<br>
- * or embedded data buffer parsing has failed for some reason,<br>
- * so pull the exposure and gain values from the control list.<br>
- */<br>
- int32_t exposureLines = data.controls.get(V4L2_CID_EXPOSURE).get<int32_t>();<br>
- int32_t gainCode = data.controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();<br>
- fillDeviceStatus(exposureLines, gainCode, deviceStatus);<br>
- }<br>
+ /*<br>
+ * This may add the DeviceStatus to the metadata, and depending on the<br>
+ * sensor, may do additional custom processing.<br>
+ */<br>
+ helper_->Prepare(embeddedBuffer, rpiMetadata_);<br>
+<br>
+ /* Add DeviceStatus metadata if helper_->Prepare() didn't. */<br>
+ DeviceStatus deviceStatus = {};<br>
+ if (rpiMetadata_.Get("device.status", deviceStatus))<br>
+ fillDeviceStatus(data.controls, deviceStatus);<br>
+<br>
+ /* Done with embedded data now, return to pipeline handler asap. */<br>
+ if (data.embeddedBufferPresent)<br>
+ returnEmbeddedBuffer(data.embeddedBufferId);<br>
<br>
ControlList ctrls(ispCtrls_);<br>
<br>
- rpiMetadata_.Clear();<br>
- rpiMetadata_.Set("device.status", deviceStatus);<br>
controller_.Prepare(&rpiMetadata_);<br>
<br>
/* Lock the metadata buffer to avoid constant locks/unlocks. */<br>
@@ -991,41 +992,12 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)<br>
setIspControls.emit(ctrls);<br>
}<br>
<br>
-bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus)<br>
-{<br>
- auto it = buffers_.find(bufferId);<br>
- if (it == buffers_.end()) {<br>
- LOG(IPARPI, Error) << "Could not find embedded buffer!";<br>
- return false;<br>
- }<br>
-<br>
- Span<uint8_t> mem = it->second.maps()[0];<br>
- helper_->Parser().SetBufferSize(mem.size());<br>
- RPiController::MdParser::Status status = helper_->Parser().Parse(mem.data());<br>
- if (status != RPiController::MdParser::Status::OK) {<br>
- LOG(IPARPI, Error) << "Embedded Buffer parsing failed, error " << status;<br>
- return false;<br>
- } else {<br>
- uint32_t exposureLines, gainCode;<br>
- if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {<br>
- LOG(IPARPI, Error) << "Exposure time failed";<br>
- return false;<br>
- }<br>
-<br>
- if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {<br>
- LOG(IPARPI, Error) << "Gain failed";<br>
- return false;<br>
- }<br>
-<br>
- fillDeviceStatus(exposureLines, gainCode, deviceStatus);<br>
- }<br>
-<br>
- return true;<br>
-}<br>
-<br>
-void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,<br>
+void IPARPi::fillDeviceStatus(const ControlList &sensorControls,<br>
struct DeviceStatus &deviceStatus)<br>
{<br>
+ int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();<br>
+ int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();<br>
+<br>
deviceStatus.shutter_speed = helper_->Exposure(exposureLines);<br>
deviceStatus.analogue_gain = helper_->Gain(gainCode);<br>
<br>
@@ -1033,6 +1005,8 @@ void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,<br>
<< deviceStatus.shutter_speed<br>
<< " Gain : "<br>
<< deviceStatus.analogue_gain;<br>
+<br>
+ rpiMetadata_.Set("device.status", deviceStatus);<br>
}<br>
<br>
void IPARPi::processStats(unsigned int bufferId)<br>
@@ -1046,6 +1020,7 @@ void IPARPi::processStats(unsigned int bufferId)<br>
Span<uint8_t> mem = it->second.maps()[0];<br>
bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats *>(mem.data());<br>
RPiController::StatisticsPtr statistics = std::make_shared<bcm2835_isp_stats>(*stats);<br>
+ helper_->Process(statistics, rpiMetadata_);<br>
controller_.Process(statistics, &rpiMetadata_);<br>
<br>
struct AgcStatus agcStatus;<br>
-- <br>
2.20.1<br>
<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>