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

[Laurent Pinchart]

Hi Kieran,

On Wed, Mar 24, 2021 at 04:09:35PM +0000, Kieran Bingham wrote:
> On 24/03/2021 15:42, Laurent Pinchart wrote:
> > 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)

Ah of course. That's what I was missing. Thanks.

> 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.

Thanks for putting up with me :-)

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

> >> 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,

Laurent Pinchart