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

[Niklas Söderlund]

Hi Jacopo,

On 2020-09-28 10:16:55 +0200, Jacopo Mondi wrote:
> 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 ?

This is not the optimum location to increment the internal frame counter 
as it's only incremented once a buffer is complete and have been 
dequeued. This will lead to multiple requests queued in a short interval 
would have the same (pipeline internal) frame number.

Instead we increment the frame whenever a request is queued to the 
pipeline from the application. But we still need this check here to 
detect if the hardware have dropped any frames due to the pipeline being 
to slow (this have been observed before OpenGL rendering was added to 
qcam).

Going forward I think we need to rethink the Timeline used in this 
pipeline, possibly looking to how this is solved in the RPi pipeline.  
But that is a task for another series.

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

-- 
Regards,
Niklas Söderlund