Message ID | 20221109101642.4957-1-david.plowman@raspberrypi.com |
---|---|
State | Accepted |
Commit | 75e7befb1667f620410f0f15a10eccb32d7df66d |
Headers | show |
Series |
|
Related | show |
Hi David, Thank you for the patch! On Wed, 9 Nov 2022 at 10:16, David Plowman via libcamera-devel < libcamera-devel@lists.libcamera.org> wrote: > This reverts commit 30d704732badc675f72fe73d14749669cb645c23. > > It turns out that this commit causes some regressions and is in fact > unnecessary because the related commit "libcamera: v4l2_videodevice: > Guard against releasing unallocated buffers" > (a2bdff6d0b67475492ac7cf9318866b6d89a28fd) fixes the problem > completely (if the buffers were never allocated, the video device > avoids trying to free them even if the pipeline handler asks). > > The reason for the regressions is that in this new (broken) scheme we > would never call clearBuffers() on all the streams if the internal > buffers were never allocated (i.e. buffersAllocated_ is never > set). This causes the stream's bufferMap_ list to get longer and > longer if there are multiple back-to-back calls to configure, and > dev_->importBuffers() will ultimately to fail. > > So either we need to think more carefully about how to stop the > pipeline handler from freeing buffers that it doesn't own, or we just > leave it as the other commit resolves the problem on its own. In the > interim, simply reverting this commit certainly seems like the best > solution. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Probably worth adding a Fixes tag here. Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 1b599fcc..343f8cb2 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1506,9 +1506,6 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, > const RPi::BufferMap &buffer > > void RPiCameraData::freeBuffers() > { > - if (!buffersAllocated_) > - return; > - > if (ipa_) { > /* > * Copy the buffer ids from the unordered_set to a vector > to > -- > 2.30.2 > >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 1b599fcc..343f8cb2 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1506,9 +1506,6 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer void RPiCameraData::freeBuffers() { - if (!buffersAllocated_) - return; - if (ipa_) { /* * Copy the buffer ids from the unordered_set to a vector to
This reverts commit 30d704732badc675f72fe73d14749669cb645c23. It turns out that this commit causes some regressions and is in fact unnecessary because the related commit "libcamera: v4l2_videodevice: Guard against releasing unallocated buffers" (a2bdff6d0b67475492ac7cf9318866b6d89a28fd) fixes the problem completely (if the buffers were never allocated, the video device avoids trying to free them even if the pipeline handler asks). The reason for the regressions is that in this new (broken) scheme we would never call clearBuffers() on all the streams if the internal buffers were never allocated (i.e. buffersAllocated_ is never set). This causes the stream's bufferMap_ list to get longer and longer if there are multiple back-to-back calls to configure, and dev_->importBuffers() will ultimately to fail. So either we need to think more carefully about how to stop the pipeline handler from freeing buffers that it doesn't own, or we just leave it as the other commit resolves the problem on its own. In the interim, simply reverting this commit certainly seems like the best solution. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 --- 1 file changed, 3 deletions(-)