[libcamera-devel] [PATCH v3 5/5] libcamera: v4l2_videodevice: Empty the V4L2 buffer cache on streamOff()
Naushir Patuck
naush at raspberrypi.com
Mon Mar 21 17:05:46 CET 2022
On Mon, 21 Mar 2022 at 14:01, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> 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.
>
Presumably in V4L2 you are allowed to queue buffers if the device is in a
"stream off" state? If so, I can't catch this in
V4L2VideoDevice::queueBuffer()
with a simple test of if (!streaming_) ...
Hmmm, perhaps the ASSERT() test is not actually valid then?
>
> --
> 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
> > > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220321/26c13989/attachment-0001.htm>
More information about the libcamera-devel
mailing list