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

[Kieran Bingham]

On 16/08/2021 15:09, Kieran Bingham wrote:
> Hi Umang,
> 
> On 16/08/2021 14:03, 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;
>> +	}
>> +
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 

My tests are passing again locally, so also:

Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> I see this is the same location as in the IPU3, so I don't mind if it's
> here or above the timestamp setting, as Laurent suggested.
> 
> If you make a v2 of this with the FrameCancelled check first, you might
> also want to make a patch to move the handling in the
> IPU3CameraData::cio2BufferReady() to make it consistent, and check the
> others too.
> 
> --
> Kieran
> 
> 
> 
> 
>>  	pipe_->completeBuffer(request, buffer);
>>  	pipe_->completeRequest(request);
>>  
>>

-- 
Regards
--
Kieran