[libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write metadata

[Laurent Pinchart]

Hello,

On Fri, Feb 19, 2021 at 03:28:34PM +0100, Niklas Söderlund wrote:
> On 2021-02-19 12:22:57 +0100, Jacopo Mondi wrote:
> > When a Request is completed upon receiving the IPA produced metadata,
> > the metadata associated with the Request are over-written, deleting
> > the information set, in example, at ImgU output buffer completion time.
> > 
> > If any additional Request metadata should be registered by inspecting
> > the IPA produced metadata it has to be done without deleting the already
> > registered entries.
> > 
> > Fix this by replacing the metadata over-write with a todo entry.
> > 
> > This change fixes CTS which was broken due to missing metadata in
> > the completed requests.
> > 
> > Fixes: 9708f49fecf2 ("libcamera: ipu3: Share parameter and statistic buffers with IPA")
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 2aed826a892a..9e867ab2e98a 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1101,8 +1101,11 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
> >  		if (!info)
> >  			break;
> > 
> > +		/*
> > +		 * \todo Parse the value of the controls returned by the IPA
> > +		 * in action.controls to register additional request metadata.
> > +		 */
> >  		Request *request = info->request;
> > -		request->metadata() = action.controls;
> 
> I understand the intent is to keep metadata set by the pipeline but this 
> change simply moves the problem. With this change any metadata reported 
> by the IPA is discarded. I thought the idea was that all metadata would 
> be generated by the IPA? If not I think we need to merge the two here.

Merging the two seems like the right solution. The ControlList class
doesn't support this at the moment, but it shouldn't be difficult to
extend it. A merge() function, along the lines of
https://en.cppreference.com/w/cpp/container/unordered_map/merge, should
do.

> If we wish to take this in the direction of this patch I think the IPA 
> interface and IPA should also be updated to not report any metadata.
> 
> >  		info->metadataProcessed = true;
> >  		if (frameInfos_.tryComplete(info))
> >  			pipe_->completeRequest(request);

-- 
Regards,

Laurent Pinchart