[libcamera-devel] [PATCH] pipeline: vimc: Force complete of request on cancelled buffers

[Laurent Pinchart]

On Mon, Aug 16, 2021 at 04:15:02PM +0300, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Mon, Aug 16, 2021 at 06:33:37PM +0530, Umang Jain wrote:
> > When the stream is stopped, the V4L2VideoDevice sends back all
> > the queued buffers with FrameMetadata::FrameCancelled status.
> > It is the responsibility of the pipeline handler to handle
> > these buffers with FrameMetadata::FrameCancelled. VIMC is
> > currently missing this handling path.
> > 
> > As the FrameMetadata::FrameCancelled is set when the stream is
> > stopped, we can be sure that no more queued and re-use of request
> > shall happen.  Hence, cancel all the requests' buffers force a
> > complete with completeBuffer().
> > 
> > The issue is caught by the gstreamer_single_stream_test.cpp running
> > with vimc. During the check with meson built-in option
> > 	'-Db_sanitize=address,undefined'
> > it was observed:
> > 
> > ==118003==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e000037108 at pc 0x7f225160c9ac bp 0x7f224a47b620 sp 0x7f224a47b618
> > READ of size 4 at 0x60e000037108 thread T1
> >     #0 0x7f225160c9ab in libcamera::Request::sequence() const ../include/libcamera/request.h:55
> >     #1 0x7f22518297aa in libcamera::VimcCameraData::bufferReady(libcamera::FrameBuffer*) ../src/libcamera/pipeline/vimc/vimc.cpp:577
> >     #2 0x7f225183b1ef in libcamera::BoundMethodMember<libcamera::VimcCameraData, void, libcamera::FrameBuffer*>::activate(libcamera::FrameBuffer*, bool) ../include/libcamera/base/bound_method.h:194
> >     #3 0x7f22515cc91f in libcamera::Signal<libcamera::FrameBuffer*>::emit(libcamera::FrameBuffer*) ../include/libcamera/base/signal.h:126
> >     #4 0x7f22515c3305 in libcamera::V4L2VideoDevice::streamOff() ../src/libcamera/v4l2_videodevice.cpp:1605
> >     #5 0x7f225181f345 in libcamera::PipelineHandlerVimc::stop(libcamera::Camera*) ../src/libcamera/pipeline/vimc/vimc.cpp:365
> > 
> > The VimcCameraData::bufferReady seems to emit even after the stream
> > is stopped. It's primarily due to vimc's lack of handling
> > FrameMetadata::FrameCancelled in its pipeline handler.
> > 
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/vimc/vimc.cpp | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index 92b30f2e..e98adab6 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -571,6 +571,18 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)
> >  	request->metadata().set(controls::SensorTimestamp,
> >  				buffer->metadata().timestamp);
> >  
> > +	/* If the buffer is cancelled force a complete of the whole request. */
> > +	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> > +		for (auto it : request->buffers()) {
> > +			FrameBuffer *b = it.second;
> > +			b->cancel();
> > +			pipe_->completeBuffer(request, b);
> > +		}
> > +
> > +		pipe_->completeRequest(request);
> > +		return;
> > +	}
> > +
> 
> I'd move this before setting the SensorTimestamp, as timestamps are not
> relevant for cancelled requests. Apart from this,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Tested-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> >  	pipe_->completeBuffer(request, buffer);
> >  	pipe_->completeRequest(request);
> >  

-- 
Regards,

Laurent Pinchart