[libcamera-devel] [PATCH v3 5/5] libcamera: v4l2_videodevice: Empty the V4L2 buffer cache on streamOff()
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Mar 21 15:01:22 CET 2022
Quoting Naushir Patuck (2022-03-21 13:37:11)
> On Mon, 21 Mar 2022 at 13:34, Kieran Bingham <
> kieran.bingham at ideasonboard.com> wrote:
>
> > 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:
> >
>
> Hmmm... How can this be possible?
I haven't looked close enough yet, but on the capture_async case, I
think the (application/test) code is requeing buffers unconditionally
when they come back - without caring if they are cancelled - so it's
never considering that we are shutting down.
We might want to make sure that when we call streamOff we set some flag
in V4L2VideoDevice that then prevents queueing buffers... as long as
that's expected...
It seems the V4L2 M2M test case is doing the same - unconditionally
requeueing buffers as long as it receives them.
While an application shouldn't do this - it 'can' so I think it
highlights that we should be more defensive in the library and make sure
those calls to queueBuffer fail with an error.
As soon as we call streamOff we should enter a Stopping state and fail
to accept any new calls to queueBuffer I believe.
--
Kieran
> Presumably on streamOff(), all buffers must have been dequeued out of the
> device
> driver, and the cache has to be empty. Is that an invalid assumption?
>
> Naush
>
>
> >
> > 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
> > >
> >
More information about the libcamera-devel
mailing list