[libcamera-devel] [PATCH v3 5/6] libcamera: pipeline: ipu3: Do not mark metadata complete early

[Kieran Bingham]

Hi Laurent,


On 24/03/2021 15:42, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wed, Mar 24, 2021 at 03:01:24PM +0000, Kieran Bingham wrote:
>> When the imguOutputBufferReady() detects a cancelled frame, it is
>> reporting that the metadata has been processed in order to be able to
>> complete the cancelled request.
>>
>> This causes the FrameInfo to be completed and deleted early, but then an
>> active buffer on the IMGU can complete and be unable to find the
>> FrameInfo for it to complete correctly.
> 
> This is because IPU3Frames::tryComplete() will erroneously consider that
> the frame is complete, right ? I'm a bit puzzled, that function checks
> if there's any pending buffer, wouldn't that condition be true until all
> the buffers are returned ? What am I missing ?

It waits until all application provided buffers are returned. It doesn't
track internal buffers. (assuming you mean the
request->hasPendingBuffers check)

This is why I previously raised the idea that internal buffers should
also be tracked against a request directly, but that should be handled
through a private implementation, as we can't associate them with
streams that the application may interrogate.


tryComplete requires 3 things.

  1) !request->hasPendingBuffers()
  2) info->metadataProcessed
  3) info->paramDequeued


info->metadataProcessed is set:

 A) IPU3CameraData::statBufferReady()
    If the stat buffer is cancelled, and not sent to the IPA.

 B) when ActionMetadataReady is received from the IPA
    I.e., after successfully parsing the stats in IPA.

 C) here in IPU3CameraData::imguOutputBufferReady()
    This is incorrect, as it's essentially completing the internal stats
    buffer while it can be potentially still on a V4L2 device, and just
    about to complete all by itself.
    imguOutputBufferReady completes the user facing buffer, and that's
     all it should do.

A, and B are perfectly valid times to complete the stats buffer. C
(fixed by this patch) is not.


>> Do not mark metadataProcessed early on the event that a single buffer is
>> detected as cancelled. The stopping of the V4L2 devices will ensure
>> that all queued buffers are returned to us and we can follow the normal
>> and expected shutdown sequence.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index 34ee600340b1..a8edf906220b 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -1232,9 +1232,6 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
>>  		cropRegion_ = request->controls().get(controls::ScalerCrop);
>>  	request->metadata().set(controls::ScalerCrop, cropRegion_);
>>  
>> -	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
>> -		info->metadataProcessed = true;
>> -
>>  	if (frameInfos_.tryComplete(info))
>>  		pipe_->completeRequest(request);
>>  }
> 

-- 
Regards
--
Kieran