[libcamera-devel] [PATCH v2 15/25] libcamera: pipeline: rkisp1: Destroy frame information before completing request

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Dec 30, 2019 at 01:05:00PM +0100, Niklas Söderlund wrote:
> The FrameBuffer interface will allow reuse of FrameBuffers form the

s/FrameBuffers/FrameBuffer instances/
s/form/from/

> request completion handler. For this reason the pipeline must destroy
> its cached information freeing the statistics and parameters buffer used
> to allow them to be reused directly.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 46df871a51105ee4..9cd0ab3ad88b35cc 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -989,9 +989,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
>  	if (!info->paramDequeued)
>  		return;
>  
> -	completeRequest(activeCamera_, request);
> -
>  	data->frameInfo_.destroy(info->frame);
> +
> +	completeRequest(activeCamera_, request);

The change itself seems fine, but I don't see how it relates the commit
message. As far as I can tell, we can already queue a Buffer (albeit a
new one) from the request completion handler, and the only thing that
the pipeline handler does with the buffer is to store it in a newly
allocated RkISP1FrameInfo that is stored in the frameInfo_ map in a new
entry. I don't see what resource is now being reused that wasn't being
reused before and that requires the RkISP1FrameInfo to be destroyed
first.

Feel free to keep the change, but please update the commit message
(possibly to explain why I'm wrong :-)).

>  }
>  
>  void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)

-- 
Regards,

Laurent Pinchart