[libcamera-devel] [PATCH v3 05/22] libcamera: pipeline: rkisp1: Prepare buffer ready handlers for multiple streams

[Jacopo Mondi]

Hi Niklas,

On Fri, Sep 25, 2020 at 06:18:13PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2020-09-25 16:20:18 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Fri, Sep 25, 2020 at 03:41:50AM +0200, Niklas Söderlund wrote:
> > > The buffer ready handlers are designed for a single application facing
> > > stream from the main path. To prepare for multiple application facing
> > > streams from main and/or self path the handlers need to be prepared.
> > >
> > > The data keeping tasks of frame number and advancing the timeline can be
> >
> > Keepin 'track'
> >
> > the frame number
> > or
> > frame numbers
> >
> >
> > > moved from the application facing buffer ready handler to the statistics
> > > handler. For each request processed there will always be a statistic
> >
> > be one statistic
> >
> > > buffer and as the ISP is inline and is the source of both main, self and
> > > statistic paths there is no lose in precision from this.
> >
> > not sure but 'lose in precision' sounds weird
> >
> > >
> > > The application facing handler no longer needs a special case for
> > > cancelled frames and can be made simpler. With this change the handlers
> > > are ready to deal with any combinations of application facing streams.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++------------
> > >  1 file changed, 5 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 8bb4582eeb688a4c..d6b5073b2084a9f4 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -1070,20 +1070,8 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
> > >  void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> > >  {
> > >  	ASSERT(activeCamera_);
> > > -	RkISP1CameraData *data = cameraData(activeCamera_);
> > >  	Request *request = buffer->request();
> > >
> > > -	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> > > -		completeBuffer(activeCamera_, request, buffer);
> > > -		completeRequest(activeCamera_, request);
> > > -		return;
> > > -	}
> > > -
> > > -	data->timeline_.bufferReady(buffer);
> > > -
> > > -	if (data->frame_ <= buffer->metadata().sequence)
> > > -		data->frame_ = buffer->metadata().sequence + 1;
> > > -
> > >  	completeBuffer(activeCamera_, request, buffer);
> > >  	tryCompleteRequest(request);
> > >  }
> > > @@ -1114,6 +1102,11 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> > >  	if (!info)
> > >  		return;
> > >
> > > +	data->timeline_.bufferReady(buffer);
> > > +
> > > +	if (data->frame_ <= buffer->metadata().sequence)
> >
> > How can frame_ be larger ?
>
> It can't :-) But if it's smaller the hardware have dropped frames and we
> need to account for that.
>

If it can't be larger why the check then ?
And if it's smaller it seems to me it just gets advanced to
metadata().sequence + 1 unconditionally.

What have I missed ?


> >
> > Please keep my tag
> >
> > Thanks
> >   j
> >
> > > +		data->frame_ = buffer->metadata().sequence + 1;
> > > +
> > >  	IPAOperationData op;
> > >  	op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;
> > >  	op.data = { info->frame, info->statBuffer->cookie() };
> > > --
> > > 2.28.0
> > >
>
> --
> Regards,
> Niklas Söderlund