[libcamera-devel] [PATCH v3 5/5] libcamera: v4l2_videodevice: Empty the V4L2 buffer cache on streamOff()

[Kieran Bingham]

Quoting Naushir Patuck (2022-03-21 10:27:02)
> When streamOff() is called, ensure the cache entires for the remaining queued
> buffers are freed since this will not happen via the dequeueBuffer() mechanism.
> 
> Additionally, add a V4L2BufferCache::isEmpty() function and assert that the
> cache is empty at the end of the streamOff() call.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Ohh I love an assert addition. It catches things. I wonder if these are
good things or bad things to catch:

Processed 31 frames
Buffer received
Buffer received
Buffer received
Buffer received
Buffer received
Buffer received
Buffer received
Buffer received
stderr:
[339:43:53.887319781] [3693294]  WARN CameraSensorProperties camera_sensor_properties.cpp:148 No static properties available for 'Sensor A'
[339:43:53.887388240] [3693294]  WARN CameraSensorProperties camera_sensor_properties.cpp:150 Please consider updating the camera sensor properties database
[339:43:53.887416152] [3693294]  WARN CameraSensor camera_sensor.cpp:411 'Sensor A': Failed to retrieve the camera location
[339:43:54.491916393] [3693294] FATAL default v4l2_videodevice.cpp:1854 /dev/video6[7:cap]: assertion "cache_->isEmpty()" failed in streamOff()
Backtrace:
libcamera::V4L2VideoDevice::streamOff()+0x14c2 (../../src/libcamera/src/libcamera/v4l2_videodevice.cpp:1854)
CaptureAsyncTest::run()+0x2dbb (../../src/libcamera/test/v4l2_videodevice/capture_async.cpp:82)
Test::execute()+0x472 (../../src/libcamera/test/libtest/test.cpp:33)
main+0x2d2 (../../src/libcamera/test/v4l2_videodevice/capture_async.cpp:93)
__libc_start_call_main+0x80 (../sysdeps/nptl/libc_start_call_main.h:58)
__libc_start_main@@GLIBC_2.34+0x7d (../csu/libc-start.c:128)
_start+0x25 (/home/kbingham/iob/libcamera/ci/integrator/builds/unit-tests/test/v4l2_videodevice/capture_async [0x000055a86d271fb5])
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

38/67 libcamera:v4l2_videodevice / buffer_sharing                OK              6.31s
39/67 libcamera:v4l2_videodevice / v4l2_m2mdevice                FAIL            1.58s   killed by signal 6 SIGABRT
>>> MALLOC_PERTURB_=171 /home/kbingham/iob/libcamera/ci/integrator/builds/unit-tests/test/v4l2_videodevice/v4l2_m2mdevice
――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
stdout:



Received capture buffer
Received capture buffer
Received capture buffer
Received capture buffer
stderr:
Output 31 frames
Captured 31 frames
[339:44:02.406962689] [3693463] FATAL default v4l2_videodevice.cpp:1854 /dev/video9[9:cap]: assertion "cache_->isEmpty()" failed in streamOff()
Backtrace:
libcamera::V4L2VideoDevice::streamOff()+0x14c2 (../../src/libcamera/src/libcamera/v4l2_videodevice.cpp:1854)
V4L2M2MDeviceTest::run()+0x523d (../../src/libcamera/test/v4l2_videodevice/v4l2_m2mdevice.cpp:173)
Test::execute()+0x472 (../../src/libcamera/test/libtest/test.cpp:33)
main+0x2bb (../../src/libcamera/test/v4l2_videodevice/v4l2_m2mdevice.cpp:205)
__libc_start_call_main+0x80 (../sysdeps/nptl/libc_start_call_main.h:58)
__libc_start_main@@GLIBC_2.34+0x7d (../csu/libc-start.c:128)
_start+0x25 (/home/kbingham/iob/libcamera/ci/integrator/builds/unit-tests/test/v4l2_videodevice/v4l2_m2mdevice [0x000055b106567015])
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

--
Kieran



> ---
>  include/libcamera/internal/v4l2_videodevice.h |  1 +
>  src/libcamera/v4l2_videodevice.cpp            | 16 ++++++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 2d2ccc477c91..37747c0b2f27 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -126,6 +126,7 @@ public:
>  
>         int get(const FrameBuffer &buffer);
>         void put(unsigned int index);
> +       bool isEmpty() const;
>  
>  private:
>         class Entry
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 5f36ee20710d..9da82697e7f0 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -262,6 +262,19 @@ void V4L2BufferCache::put(unsigned int index)
>         cache_[index].free_ = true;
>  }
>  
> +/**
> + * \brief Check if all the entries in the cache are unused
> + */
> +bool V4L2BufferCache::isEmpty() const
> +{
> +       for (auto const &entry : cache_) {
> +               if (!entry.free_)
> +                       return false;
> +       }
> +
> +       return true;
> +}
> +
>  V4L2BufferCache::Entry::Entry()
>         : free_(true), lastUsed_(0)
>  {
> @@ -1832,10 +1845,13 @@ int V4L2VideoDevice::streamOff()
>         for (auto it : queuedBuffers_) {
>                 FrameBuffer *buffer = it.second;
>  
> +               cache_->put(it.first);
>                 buffer->metadata_.status = FrameMetadata::FrameCancelled;
>                 bufferReady.emit(buffer);
>         }
>  
> +       ASSERT(cache_->isEmpty());
> +
>         queuedBuffers_.clear();
>         fdBufferNotifier_->setEnabled(false);
>         streaming_ = false;
> -- 
> 2.25.1
>