[PATCH v1 5/8] ipa: rkisp1: Add debug metadata support to the rkisp1

[Stefan Klug]

Hi Laurent,

Thank you for the review. 

On Thu, Oct 03, 2024 at 08:05:58PM +0300, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Wed, Oct 02, 2024 at 06:19:23PM +0200, Stefan Klug wrote:
> > Add a DebugMetadata helper to the context and add the corresponding
> > plumbing.  This is all that is needed to support debug metadata in an
> > IPA.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/ipa_context.h | 5 +++++
> >  src/ipa/rkisp1/rkisp1.cpp    | 4 ++++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index d52e73ad2503..7b93a9e9461d 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -17,8 +17,11 @@
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/controls.h>
> >  #include <libcamera/geometry.h>
> > +
> >  #include <libcamera/ipa/core_ipa_interface.h>
> >  
> > +#include "libcamera/internal/debug_controls.h"
> > +
> >  #include <libipa/camera_sensor_helper.h>
> >  #include <libipa/fc_queue.h>
> >  #include <libipa/matrix.h>
> > @@ -194,6 +197,8 @@ struct IPAContext {
> >  
> >  	ControlInfoMap::Map ctrlMap;
> >  
> > +	DebugMetadata debugMetadata;
> > +
> >  	/* Interface to the Camera Helper */
> >  	std::unique_ptr<CameraSensorHelper> camHelper;
> >  };
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index a579f21de56f..5b1ef0c372c6 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -117,6 +117,7 @@ const IPAHwSettings ipaHwSettingsV12{
> >  const ControlInfoMap::Map rkisp1Controls{
> >  	{ &controls::AwbEnable, ControlInfo(false, true) },
> >  	{ &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
> > +	{ &controls::DebugMetadataEnable, ControlInfo(false, true, false) },
> >  	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
> >  	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> >  };
> > @@ -326,6 +327,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
> >  void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)
> >  {
> >  	IPAFrameContext &frameContext = context_.frameContexts.alloc(frame);
> > +	context_.debugMetadata.checkForEnable(controls);
> >  
> >  	for (auto const &a : algorithms()) {
> >  		Algorithm *algo = static_cast<Algorithm *>(a.get());
> > @@ -368,6 +370,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> >  		context_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> >  
> >  	ControlList metadata(controls::controls);
> > +	context_.debugMetadata.assignControlList(&metadata);
> 
> What's the advantage of doing this here, instead of...
> 
> >  
> >  	for (auto const &a : algorithms()) {
> >  		Algorithm *algo = static_cast<Algorithm *>(a.get());
> > @@ -377,6 +380,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> >  	}
> >  
> >  	setControls(frame);
> > +	context_.debugMetadata.assignControlList(nullptr);
> 
> ... extracting the control list from debugMetadata here and merging it
> into metadata ?

Oh you are right. I had that initial thought of attaching a ControlList
to a DebugMetadata object for longer than the current function call. But
in practice that case never happens as the Metadata list is short lived.
I'll drop the whole assignControlList part in the next version.

Regards,
Stefan

> 
> >  	metadataReady.emit(frame, metadata);
> >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart