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

[Laurent Pinchart]

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>

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

-- 
Regards,

Laurent Pinchart