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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2020-03-20 01:50:35 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> 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.

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

-- 
Regards,
Niklas Söderlund