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

[Jacopo Mondi]

Hi Hiro, Niklas,

On Thu, Apr 22, 2021 at 01:53:45PM +0900, Hirokazu Honda wrote:
> Hi Jacopo, thank you for the patch,
>
> On Thu, Apr 22, 2021 at 7:03 AM Niklas Söderlund
> <niklas.soderlund at ragnatech.se> wrote:
> >
> > Hi again Jacopo,
> >
> > 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?

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

Anyway a merge() version which takes a const ControlList, and is
implemented by copying values might be desirable.

Thanks
   j

> -Hiro
>
> > >       pipe->tryCompleteRequest(info->request);
> > > --
> > > 2.31.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel