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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Feb 22, 2021 at 10:01:08AM +0100, Jacopo Mondi wrote:
> On Sun, Feb 21, 2021 at 08:39:52PM +0200, Laurent Pinchart wrote:
> > 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.

Correct, so I'm fine with this patch, but we'll need to immediately
improve it on top :-)

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

In theory that's possible, but given that the pipeline handler and IPA
are supposed to be developed in sync, that's something the developer
should be able to control. They could either make sure not conflict
occurs, or implement a more elaborate merge method. I would expect the
former to be the common case, hence the proposal for
ControlList::merge().

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

No, we can merge this patch first, that's not an issue. The fact that
Niklas and I have pointed out issues doesn't mean they all have to be
fixed as a prerequisite, they can also go on top where it makes sense
:-)

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