| Message ID | 20220321102702.1753800-6-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Mon, Mar 21, 2022 at 10:27:02AM +0000, Naushir Patuck via libcamera-devel wrote: > When streamOff() is called, ensure the cache entires for the remaining queued s/entires/entries/ > 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@raspberrypi.com> > Tested-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > 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; I'd move this before get() and put(), as we usually declare the const query functions first. Same in the .cpp file. This can be handled when applying. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > 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;
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@raspberrypi.com> > Tested-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Kieran Bingham <kieran.bingham@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 >
On Mon, 21 Mar 2022 at 13:34, Kieran Bingham < kieran.bingham@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@raspberrypi.com> > > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@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? 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 > > >
On Mon, Mar 21, 2022 at 01:37:11PM +0000, Naushir Patuck via libcamera-devel wrote: > On Mon, 21 Mar 2022 at 13:34, Kieran Bingham 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@raspberrypi.com> > > > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > > Reviewed-by: Kieran Bingham <kieran.bingham@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? > 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? It doesn't sound invalid, but there's clearly a problem :-) You can reproduce this by running unit tests with "ninja test" (the assertion fails when running tests on an x86 machine with vimc, vim2m and vidid). And while at that, I forgot to note during review that the buffer cache unit test (test/v4l2_videodevice/buffer_cache.cpp) should be extended to test the new isEmpty() function. > > 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]) > > > > ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― > > > > > --- > > > 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;
Quoting Naushir Patuck (2022-03-21 13:37:11) > On Mon, 21 Mar 2022 at 13:34, Kieran Bingham < > kieran.bingham@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@raspberrypi.com> > > > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > > Reviewed-by: Kieran Bingham <kieran.bingham@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 > > > > >
On Mon, 21 Mar 2022 at 14:01, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck (2022-03-21 13:37:11) > > On Mon, 21 Mar 2022 at 13:34, Kieran Bingham < > > kieran.bingham@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@raspberrypi.com> > > > > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@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 > > > > > > > >
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;