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

[Niklas Söderlund]

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.

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