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

[Hirokazu Honda]

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?

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

-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