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

[Niklas Söderlund]

Hi Jacopo,

On 2021-04-22 09:06:16 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Thu, Apr 22, 2021 at 12:03:42AM +0200, Niklas Söderlund 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?
> >
> 
> Dunno, I think blindly overwriting metadata is wrong, but if the tag
> makes you unconfortable I'll drop it.

I agree that overwriting stuff is bad, but the commit in question does 
not do that, before it there was no metadata at all :-)

> 
> Tbh I don't even know why we use Fixes, since we have nowhere to
> backport too

I like the Fixes tags. When you bisect issues and hit a bad commit that 
have a Fixes tag you get way more context on what's going on. That's why 
I think in this particular case the Fixes tag is wrong as if I hit this 
commit when bisecting the Fixes tag would send me on a wild goose chase 
as this commit does not fix an issue that did not exist in 
0eb65e14e18d~1 but does in 0eb65e14e18d.

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

-- 
Regards,
Niklas Söderlund