<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 21 Mar 2022 at 14:01, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Naushir Patuck (2022-03-21 13:37:11)<br>
> On Mon, 21 Mar 2022 at 13:34, Kieran Bingham <<br>
> <a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br>
> <br>
> > Quoting Naushir Patuck (2022-03-21 10:27:02)<br>
> > > When streamOff() is called, ensure the cache entires for the remaining<br>
> > queued<br>
> > > buffers are freed since this will not happen via the dequeueBuffer()<br>
> > mechanism.<br>
> > ><br>
> > > Additionally, add a V4L2BufferCache::isEmpty() function and assert that<br>
> > the<br>
> > > cache is empty at the end of the streamOff() call.<br>
> > ><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> ><br>
> > Ohh I love an assert addition. It catches things. I wonder if these are<br>
> > good things or bad things to catch:<br>
> ><br>
> <br>
> Hmmm... How can this be possible?<br>
<br>
I haven't looked close enough yet, but on the capture_async case, I<br>
think the (application/test) code is requeing buffers unconditionally<br>
when they come back - without caring if they are cancelled - so it's<br>
never considering that we are shutting down.<br>
<br>
We might want to make sure that when we call streamOff we set some flag<br>
in V4L2VideoDevice that then prevents queueing buffers... as long as<br>
that's expected...<br>
<br>
It seems the V4L2 M2M test case is doing the same - unconditionally<br>
requeueing buffers as long as it receives them.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
While an application shouldn't do this - it 'can' so I think it<br>
highlights that we should be more defensive in the library and make sure<br>
those calls to queueBuffer fail with an error.<br>
<br>
As soon as we call streamOff we should enter a Stopping state and fail<br>
to accept any new calls to queueBuffer I believe.<br></blockquote><div><br></div><div>Presumably in V4L2 you are allowed to queue buffers if the device is in a</div><div>"stream off" state? If so, I can't catch this in V4L2VideoDevice::queueBuffer()</div><div>with a simple test of if (!streaming_) ...</div><div><br></div><div>Hmmm, perhaps the ASSERT() test is not actually valid then?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
--<br>
Kieran<br>
<br>
> Presumably on streamOff(), all buffers must have been dequeued out of the<br>
> device<br>
> driver, and the cache has to be empty. Is that an invalid assumption?<br>
> <br>
> Naush<br>
> <br>
> <br>
> ><br>
> > Processed 31 frames<br>
> > Buffer received<br>
> > Buffer received<br>
> > Buffer received<br>
> > Buffer received<br>
> > Buffer received<br>
> > Buffer received<br>
> > Buffer received<br>
> > Buffer received<br>
> > stderr:<br>
> > [339:43:53.887319781] [3693294] WARN CameraSensorProperties<br>
> > camera_sensor_properties.cpp:148 No static properties available for 'Sensor<br>
> > A'<br>
> > [339:43:53.887388240] [3693294] WARN CameraSensorProperties<br>
> > camera_sensor_properties.cpp:150 Please consider updating the camera sensor<br>
> > properties database<br>
> > [339:43:53.887416152] [3693294] WARN CameraSensor camera_sensor.cpp:411<br>
> > 'Sensor A': Failed to retrieve the camera location<br>
> > [339:43:54.491916393] [3693294] FATAL default v4l2_videodevice.cpp:1854<br>
> > /dev/video6[7:cap]: assertion "cache_->isEmpty()" failed in streamOff()<br>
> > Backtrace:<br>
> > libcamera::V4L2VideoDevice::streamOff()+0x14c2<br>
> > (../../src/libcamera/src/libcamera/v4l2_videodevice.cpp:1854)<br>
> > CaptureAsyncTest::run()+0x2dbb<br>
> > (../../src/libcamera/test/v4l2_videodevice/capture_async.cpp:82)<br>
> > Test::execute()+0x472 (../../src/libcamera/test/libtest/test.cpp:33)<br>
> > main+0x2d2 (../../src/libcamera/test/v4l2_videodevice/capture_async.cpp:93)<br>
> > __libc_start_call_main+0x80 (../sysdeps/nptl/libc_start_call_main.h:58)<br>
> > __libc_start_main@@GLIBC_2.34+0x7d (../csu/libc-start.c:128)<br>
> > _start+0x25<br>
> > (/home/kbingham/iob/libcamera/ci/integrator/builds/unit-tests/test/v4l2_videodevice/capture_async<br>
> > [0x000055a86d271fb5])<br>
> ><br>
> > ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――<br>
> ><br>
> > 38/67 libcamera:v4l2_videodevice / buffer_sharing OK<br>
> > 6.31s<br>
> > 39/67 libcamera:v4l2_videodevice / v4l2_m2mdevice FAIL<br>
> > 1.58s killed by signal 6 SIGABRT<br>
> > >>> MALLOC_PERTURB_=171<br>
> > /home/kbingham/iob/libcamera/ci/integrator/builds/unit-tests/test/v4l2_videodevice/v4l2_m2mdevice<br>
> > ――――――――――――――――――――――――――――――――――――― ✀<br>
> > ―――――――――――――――――――――――――――――――――――――<br>
> > stdout:<br>
> ><br>
> ><br>
> ><br>
> > Received capture buffer<br>
> > Received capture buffer<br>
> > Received capture buffer<br>
> > Received capture buffer<br>
> > stderr:<br>
> > Output 31 frames<br>
> > Captured 31 frames<br>
> > [339:44:02.406962689] [3693463] FATAL default v4l2_videodevice.cpp:1854<br>
> > /dev/video9[9:cap]: assertion "cache_->isEmpty()" failed in streamOff()<br>
> > Backtrace:<br>
> > libcamera::V4L2VideoDevice::streamOff()+0x14c2<br>
> > (../../src/libcamera/src/libcamera/v4l2_videodevice.cpp:1854)<br>
> > V4L2M2MDeviceTest::run()+0x523d<br>
> > (../../src/libcamera/test/v4l2_videodevice/v4l2_m2mdevice.cpp:173)<br>
> > Test::execute()+0x472 (../../src/libcamera/test/libtest/test.cpp:33)<br>
> > main+0x2bb<br>
> > (../../src/libcamera/test/v4l2_videodevice/v4l2_m2mdevice.cpp:205)<br>
> > __libc_start_call_main+0x80 (../sysdeps/nptl/libc_start_call_main.h:58)<br>
> > __libc_start_main@@GLIBC_2.34+0x7d (../csu/libc-start.c:128)<br>
> > _start+0x25<br>
> > (/home/kbingham/iob/libcamera/ci/integrator/builds/unit-tests/test/v4l2_videodevice/v4l2_m2mdevice<br>
> > [0x000055b106567015])<br>
> ><br>
> > ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――<br>
> ><br>
> > --<br>
> > Kieran<br>
> ><br>
> ><br>
> ><br>
> > > ---<br>
> > > include/libcamera/internal/v4l2_videodevice.h | 1 +<br>
> > > src/libcamera/v4l2_videodevice.cpp | 16 ++++++++++++++++<br>
> > > 2 files changed, 17 insertions(+)<br>
> > ><br>
> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h<br>
> > b/include/libcamera/internal/v4l2_videodevice.h<br>
> > > index 2d2ccc477c91..37747c0b2f27 100644<br>
> > > --- a/include/libcamera/internal/v4l2_videodevice.h<br>
> > > +++ b/include/libcamera/internal/v4l2_videodevice.h<br>
> > > @@ -126,6 +126,7 @@ public:<br>
> > ><br>
> > > int get(const FrameBuffer &buffer);<br>
> > > void put(unsigned int index);<br>
> > > + bool isEmpty() const;<br>
> > ><br>
> > > private:<br>
> > > class Entry<br>
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp<br>
> > b/src/libcamera/v4l2_videodevice.cpp<br>
> > > index 5f36ee20710d..9da82697e7f0 100644<br>
> > > --- a/src/libcamera/v4l2_videodevice.cpp<br>
> > > +++ b/src/libcamera/v4l2_videodevice.cpp<br>
> > > @@ -262,6 +262,19 @@ void V4L2BufferCache::put(unsigned int index)<br>
> > > cache_[index].free_ = true;<br>
> > > }<br>
> > ><br>
> > > +/**<br>
> > > + * \brief Check if all the entries in the cache are unused<br>
> > > + */<br>
> > > +bool V4L2BufferCache::isEmpty() const<br>
> > > +{<br>
> > > + for (auto const &entry : cache_) {<br>
> > > + if (!entry.free_)<br>
> > > + return false;<br>
> > > + }<br>
> > > +<br>
> > > + return true;<br>
> > > +}<br>
> > > +<br>
> > > V4L2BufferCache::Entry::Entry()<br>
> > > : free_(true), lastUsed_(0)<br>
> > > {<br>
> > > @@ -1832,10 +1845,13 @@ int V4L2VideoDevice::streamOff()<br>
> > > for (auto it : queuedBuffers_) {<br>
> > > FrameBuffer *buffer = it.second;<br>
> > ><br>
> > > + cache_->put(it.first);<br>
> > > buffer->metadata_.status = FrameMetadata::FrameCancelled;<br>
> > > bufferReady.emit(buffer);<br>
> > > }<br>
> > ><br>
> > > + ASSERT(cache_->isEmpty());<br>
> > > +<br>
> > > queuedBuffers_.clear();<br>
> > > fdBufferNotifier_->setEnabled(false);<br>
> > > streaming_ = false;<br>
> > > --<br>
> > > 2.25.1<br>
> > ><br>
> ><br>
</blockquote></div></div>