[libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not over-write metadata

[Laurent Pinchart]

Hi Jacopo,

On Thu, Apr 22, 2021 at 09:04:44AM +0200, Jacopo Mondi wrote:
> On Thu, Apr 22, 2021 at 01:53:45PM +0900, Hirokazu Honda wrote:
> > On Thu, Apr 22, 2021 at 7:03 AM Niklas Söderlund wrote:
> > > On 2021-04-21 18:03:10 +0200, 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 at output buffer completion, such as the
> > > > SensorTimestamp.
> > > >
> > > > This commit applies to the RkISP1 pipeline handler the same change
> > > > applied to IPU3 in commit 13a7ed7b1f1f ("libcamera: ipu3: Do not
> > > > over-write metadata") but compared to that commit it uses the newly
> > > > introduced ControlList::merge() function.
> > > >
> > > > Fixes: 0eb65e14e18d ("libcamera: pipeline: rkisp1: Attach to an IPA")
> > >
> > > This fixes tag is incorrect. Testing on master the only metadata
> > > recorded in the request is the AeLock. Applying this series the only
> > > metadata reported is the AeLock and SensorTimestamp, and the timestamp
> > > is added by the next patch in this series. There is no regression here
> > > to fix, only preparation for additional work, or am I missing something?
> > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index 549f4a4e61a8..c3d390f775f2 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta
> > > >       if (!info)
> > > >               return;
> > > >
> > > > -     info->request->metadata() = metadata;
> > > > +     info->request->metadata().merge(const_cast<ControlList &>(metadata));
> > > >       info->metadataProcessed = true;
> > >
> > > I like the change but this cast makes me think twice of the merge()
> > > signature. Could we make the argument a const reference?

Ouch. const_cast<> is very often a sign of a design issue.

> We currently can't as ControlList::merge() is implemented by wrapping
> std::unordered_map::merge() as Hiro explained
> 
> > I think there are two solutions,
> > 1.) drop const of action in queueFrameAction.
> > 2.) merge doesn't change a given ControlList.
> >
> > We currently use std::unordered_map::merge() for efficiency, which
> > doesn't copy nodes among maps.
> > I expect the copy of ControlValue is basically cheap, but this copy
> > might be expensive depending on the number of nodes to be copied.
> 
> The number of nodes and their content, as we can transport Span<> of
> values
> 
> > I don't loot at the serialization code during IPA communication, but
> > perhaps, the cost between cost reference and value are the same?
> > Then, (1) is definitely better in terms of the efficiency.
> > I would like to hear others' opinions.
> 
> I see your concerns about performaces, and in this specific case I
> think we can safely drop the const from the queueFrameAction
> ControlList parameter. Even better, if after merge there are elements
> in the list passed as argument we can catch a development issue
> earlier (an attempt to overwrite a metadata which has been set already).

I also think this is better, and I like the idea of being able to check
if metadata is empty after the merge. You'll run into an issue though,
as the IPA IPC code generator inserts the const automatically. To avoid
delay in this patch series, I'd recommend modifying ControlList::merge()
to take a const reference and copy elements, with a \todo to mention we
should optimize it.

> Anyway a merge() version which takes a const ControlList, and is
> implemented by copying values might be desirable.
> 
> > > >       pipe->tryCompleteRequest(info->request);

-- 
Regards,

Laurent Pinchart