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

[Jacopo Mondi]

Hello Niklas, Laurent,

On Sun, Feb 21, 2021 at 08:39:52PM +0200, Laurent Pinchart wrote:
> 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.
>

At the moment the IPA does not report any metadata if I'm not
mistaken, and this change breaks 80+ CTS tests as it overwrites the actual
metadata with an empty list.

I would also question that the pipeline handler should blindly merge
the two as over-writes might happen, but I might be mistaken.

Anyway, I'm fine having more CTS tests broken waiting for the IPA to
actually set something there if that's the desired direction. I would
prefer the other way around as this hinders CTS progresses but I would
go with what the group prefers.

> > 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