| Message ID | 20200319234401.3522424-1-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
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@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) > 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 ? > 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());
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@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
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@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());
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; 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); 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());
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@ragnatech.se> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)