[libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Use maxBuffers instead of count when importing buffers
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Mar 20 01:09:44 CET 2020
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
More information about the libcamera-devel
mailing list