[PATCH] libcamera: software_isp: Clean up pending requests on stop

[Kieran Bingham]

Quoting Robert Mader (2024-10-08 17:19:51)
> Hey, thanks a lot for the patch!
> 
> I gave it a quick try on my Pixel 3a and while the revision here avoids 
> the assertion most of the time and makes things much more stable, I 
> still managed to trigger it with enough trying (4 buffers, 
> starting/stopping/switching cameras in Gnome Snapshot). I haven't tried 
> the suggestion from Kieran yet, but to me it looks like Stop() is 
> currently still somehow racy - at least the way it's used by Pipewire.

This current version of the patch 'completes' the buffers/requests - so I am
weary pipewire probably tries to immediately resend them back into
libcamera! (Though I think we should reject any incoming requests as
soon as we hit stop())

But I think it's important to make sure they are 'cancelled'.

--
Kieran


> 
> Regards
> 
> On 08.10.24 17:09, Milan Zamazal wrote:
> > PipelineHandler::stop() calls stopDevice() method to perform pipeline
> > specific cleanup and then completes waiting requests.  If any queued
> > requests remain, an assertion error is raised.
> >
> > Software ISP stores request buffers in
> > SimpleCameraData::conversionQueue_ and queues them as V4L2 signals
> > bufferReady.  stopDevice() cleanup forgets to clean up the buffers and
> > their requests from conversionQueue_, possibly resulting in the
> > assertion error.  This patch fixes the omission.
> >
> > The problem wasn't very visible when
> > SimplePipelineHandler::kNumInternalBuffers (the number of buffers
> > allocated in V4L2) was equal to the number of buffers exported from
> > software ISP.  But when the number of the exported buffers was increased
> > by one in commit abe2ec64f9e4e97bbdfe3a50372611bd7b5315c2, the assertion
> > error started pop up in some environments.  Increasing the number of the
> > buffers much more, e.g. to 9, makes the problem very reproducible.
> >
> > Each pipeline uses its own mechanism to track the requests to clean up
> > and it can't be excluded that similar omissions are present in other
> > places.  But there is no obvious way to make a common cleanup for all
> > the pipelines (except for doing it instead of raising the assertion
> > error, which is probably undesirable, in order not to hide incomplete
> > pipeline specific cleanups).
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=234
> > Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> > ---
> >   src/libcamera/pipeline/simple/simple.cpp | 15 +++++++++++++++
> >   1 file changed, 15 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 3ddce71d3..4e8504922 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -284,6 +284,7 @@ public:
> >       std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;
> >       std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_;
> >       bool useConversion_;
> > +     void clearIncompleteRequests();
> >   
> >       std::unique_ptr<Converter> converter_;
> >       std::unique_ptr<SoftwareIsp> swIsp_;
> > @@ -897,6 +898,19 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)
> >               pipe->completeRequest(request);
> >   }
> >   
> > +void SimpleCameraData::clearIncompleteRequests()
> > +{
> > +     while (!conversionQueue_.empty()) {
> > +             for (auto &item : conversionQueue_.front()) {
> > +                     FrameBuffer *outputBuffer = item.second;
> > +                     Request *request = outputBuffer->request();
> > +                     pipe()->completeBuffer(request, outputBuffer);
> > +                     pipe()->completeRequest(request);
> > +             }
> > +             conversionQueue_.pop();
> > +     }
> > +}
> > +
> >   void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> >   {
> >       swIsp_->processStats(frame, bufferId,
> > @@ -1407,6 +1421,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
> >       video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
> >   
> >       data->conversionBuffers_.clear();
> > +     data->clearIncompleteRequests();
> >   
> >       releasePipeline(data);
> >   }
> 
> -- 
> Robert Mader
> Consultant Software Developer
> 
> Collabora Ltd.
> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
> Registered in England & Wales, no. 5513718
>