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

Message ID 20191230120510.938333-16-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: Rework buffer API
Related show

Commit Message

Niklas Söderlund Dec. 30, 2019, 12:05 p.m. UTC
The FrameBuffer interface will allow reuse of FrameBuffers form the
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@ragnatech.se>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart Jan. 8, 2020, 1:34 a.m. UTC | #1
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@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)

Patch

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);
 }
 
 void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)