<div dir="ltr"><div dir="ltr">Hi David,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 26 Mar 2021 at 11:07, 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">Hi Naush<br>
<br>
On Fri, 26 Mar 2021 at 09:57, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Hi David,<br>
><br>
> On Fri, 26 Mar 2021 at 09:32, David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>> wrote:<br>
>><br>
>> Hi Naush<br>
>><br>
>> Thanks for the feedback!<br>
>><br>
>> On Thu, 25 Mar 2021 at 16:08, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
>> ><br>
>> > Hi David,<br>
>> ><br>
>> > Thank you for your work.<br>
>> ><br>
>> > On Mon, 22 Mar 2021 at 16:01, David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>> wrote:<br>
>> >><br>
>> >> 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>
>> ><br>
>> ><br>
>> > I don't think this namespace scope is needed. Seems to compile fine without.<br>
>><br>
>> Funny one, this. It builds like that for me too but when I run it (for<br>
>> example, qcam), it bombs out with<br>
>><br>
>> build/src/qcam/qcam: symbol lookup error:<br>
>> /home/pi/libcamera/build/src/ipa/raspberrypi/ipa_rpi.so: undefined<br>
>> symbol: _Z17logCategoryIPARPIv<br>
>><br>
>> It seems to be looking for logCategory::IPARPI when I think it should<br>
>> be libcamera::logCategory::IPARPI. I guess I'm not clear why the<br>
>> linker doesn't flag that up... anyone know?<br>
>><br>
>> ><br>
>> >><br>
>> >><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>
>> >><br>
>> >> +{<br>
>> >> + parseEmbeddedData(buffer, metadata);<br>
>> >> +}<br>
>> ><br>
>> ><br>
>> ><br>
>> > Would it be useful to pass in the ControlList that contains the sensor setup<br>
>> > (from DelayedControls) into this function? Would the CamHelper ever want<br>
>> > to know these things?<br>
>> ><br>
>> > If the answer is yes, you could also pull in the case that deals with non-embedded<br>
>> > data where we pull values from the ControlList. So something like:<br>
>> ><br>
>> > void CamHelper::Prepare(const Span<uint8_t> &buffer,<br>
>> > Metadata &metadata, ControlList &SensorControls)<br>
>> > {<br>
>> > bool success = false;<br>
>> ><br>
>> > if (buffer.size())<br>
>> > success = parseEmbeddedData(buffer, metadata);<br>
>> ><br>
>> > if (!success) {<br>
>> > struct DeviceStatus deviceStatus = {};<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 = Exposure(exposureLines);<br>
>> > deviceStatus.analogue_gain = Gain(gainCode);<br>
>> > metadata.Set("device.status", deviceStatus);<br>
>> > }<br>
>> > }<br>
>> ><br>
>> > What do you think? Might be a bit neater to have deviecStatus set in one place only.<br>
>><br>
>> Interesting point, and in fact, my previous version of this patch<br>
>> (labelled as "RFC") did exactly this. Obviously I back-tracked on<br>
>> that, for the following reasons.<br>
>><br>
>> The version you've outlined above is nice because it makes the IPA<br>
>> file (raspberrypi.cpp) a bit tidier, functions like<br>
>> CamHelper::Exposure and CamHelper::Gain only need to be called once.<br>
>> The DeviceStatus only gets set in one place.<br>
>><br>
>> The version I went for in the end has the advantage that it makes<br>
>> CamHelper::Prepare responsible only for parsing sensor metadata.<br>
>> There's nothing it has to remember to do if, for example, some<br>
>> particular metadata isn't there.<br>
>><br>
>> This means that the CamHelper::Prepare for my mysterious new sensor<br>
>> doesn't have to do anything extra. It just parses the sensor's<br>
>> statistics buffer and nothing else. As this kind of code - CamHelpers<br>
>> - is likely to get duplicated more in future, potentially by 3rd<br>
>> parties, I was keener to avoid little "gotchas" in there, and slight<br>
>> uglification of raspberrypi.cpp seemed reasonable in return.<br>
>><br>
>> But it's a close call and I'm definitely swayable...<br>
><br>
><br>
> Perhaps it's a bit premature to think about this, but what if some stuff running in<br>
> prepare() (e.g. fancy phase detect AF) needs the exposure and gain values used<br>
> for the sensor, and we don't have embedded data to give us these values. You could<br>
> possibly run this fancy AF in process() where you do get the values, but then you add<br>
> a frame's worth of latency.<br>
><br>
> It also possibly makes the code a bit cleaner for an up-coming change for imx477 ultra<br>
> long exposures where I have to disable parsing when the exposure factor is set. But<br>
> again, I have not written code for this yet to tell for sure.<br>
<br>
That's an interesting thought. Generally I'm quite strongly opposed to<br>
"algorithm" code creeping into the helpers, the helpers should be<br>
restricted to the minimum "parsing" duties. To take the AF example,<br>
CamHelper::Prepare would put the PDAF information into some kind of<br>
"pdaf" entry in our RPi metadata. Hopefully many sensors would be able<br>
to re-use this "pdaf" metadata. Then there'd be a bona fide "pdaf"<br>
algorithm, loaded as usual via the json file, that would interpret it.<br></blockquote><div><br></div><div>Yes I agree that algorithm stuff should end up in the Controller, so maybe</div><div>this is not something we deal with.</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>
On the subject of long exposures, how can you tell whether you're in<br>
the long exposure case - are the values enough?</blockquote><div><br></div><div>Unfortunately not. We have to use the fact that the helper (specifically</div><div>GetVBlanking) will setup the long exposure factor based on the requested</div><div>exposure time. Once this is done, we store state in the CamHelper to</div><div>abort parsing until long exposure is turned off.</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">I'm thinking about<br>
always reading the exposure/gain from the controls and filling in the<br>
DeviceStatus *before* we call CamHelper::Prepare. That way you have<br>
the values, and you can overwrite them if you know better. Would that<br>
be helpful to the imx477?<br></blockquote><div><br></div><div>Probably won't make much difference to imx477, but this might be a bit neater</div><div>than we have it now. I think we should make this change!</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></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>
Thanks!<br>
David<br>
<br>
><br>
> I'll leave it to you to choose :-)<br>
><br>
> Naush<br>
><br>
>><br>
>><br>
>> > With or without this change and the fixup for the namespace scoping above:<br>
>> ><br>
>> > Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
>><br>
>> Thanks!<br>
>><br>
>> David<br>
>><br>
>> ><br>
>> >><br>
>> >> +<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>
>> >><br>
>> >> +{<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>