[libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Use maxBuffers instead of count when importing buffers

[Laurent Pinchart]

Hi Niklas,

On Fri, Mar 20, 2020 at 01:05:41AM +0100, Niklas Söderlund wrote:
> On 2020-03-20 01:50:35 +0200, Laurent Pinchart wrote:
> > On Fri, Mar 20, 2020 at 12:44:01AM +0100, Niklas Söderlund wrote:
> > > When folding buffer management with start/stop the wrong variable was
> > > passed to importBuffers() resulting in only one buffer being imported
> > > for the video node making capture impossible. Fix this by using the
> > > right variable, maxBuffers.
> > > 
> > > Rename the misleadingly named count variable to avoid confusion in the
> > > future.
> > > 
> > > Fixes: 33fedea818e2b6a9 ("libcamera: pipeline_handler: Fold buffer management with start/stop")
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 97bb4f72cde5423e..d9a221d2df3e4b4c 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -668,14 +668,14 @@ int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream,
> > >  int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> > >  {
> > >  	RkISP1CameraData *data = cameraData(camera);
> > > -	unsigned int count = 1;
> > > +	unsigned int ipaBufferIDCounter = 1;
> > 
> > That's a bit of a long name. How about bufferId ? Or ipaBufferId if you
> > really insist ? :-) (ID should be spelled Id in any case)
> 
> ipaBufferId it is :-)
> 
> > 
> > >  	int ret;
> > >  
> > >  	unsigned int maxBuffers = 0;
> > >  	for (const Stream *s : camera->streams())
> > >  		maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
> > >  
> > > -	ret = video_->importBuffers(count);
> > > +	ret = video_->importBuffers(maxBuffers);
> > 
> > So before the faulty patch, we were importing, for each stream, the
> > number of buffers specified for that stream. Now we're importing the
> > maximum number of buffers. As the pipeline handler supports a single
> > stream this is equivalent, but is it the right thing to do when we'll
> > have multiple streams ?
> 
> I'm not sure, for parameters and statistics I think it is. For the video 
> node probably not. How about something like this (making count the right 
> name again ;-P)
> 
>         unsigned int maxBuffers = 0; 
>         for (const Stream *s : camera->streams()) {
>                 unsigned int count = s->configuration().bufferCount;
> 
>                 if (s == &data->stream_) {
>                         ret = video_->importBuffers(count);
>                         if (ret < 0) 
>                                 goto error;
>                 }    
> 
>                 maxBuffers = std::max(maxBuffers, count);
>         }
> 
> The check for 's == data->stream_' seems a bit silly but makes it 
> extreamly clear where the checks needs to be added when we add more 
> streams.

That's over-complicated for something that supports a single stream in
practice. Also, looping over camera->streams() is broken, you should
loop over all active streams. I think you should just remove the loop
and use the number of buffers for the single stream we have. Easy and
hassle-free :-)

> > 
> > >  	if (ret < 0)
> > >  		goto error;
> > >  
> > > @@ -688,14 +688,14 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> > >  		goto error;
> > >  
> > >  	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
> > > -		buffer->setCookie(count++);
> > > +		buffer->setCookie(ipaBufferIDCounter++);
> > >  		data->ipaBuffers_.push_back({ .id = buffer->cookie(),
> > >  					      .planes = buffer->planes() });
> > >  		availableParamBuffers_.push(buffer.get());
> > >  	}
> > >  
> > >  	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_) {
> > > -		buffer->setCookie(count++);
> > > +		buffer->setCookie(ipaBufferIDCounter++);
> > >  		data->ipaBuffers_.push_back({ .id = buffer->cookie(),
> > >  					      .planes = buffer->planes() });
> > >  		availableStatBuffers_.push(buffer.get());

-- 
Regards,

Laurent Pinchart