[libcamera-devel,RFCv2,4/7] libcamera: pipeline: rkisp1: Do not try to process cancelled frames

3339 diff mbox series

Their is no need to try and process cancelled frames.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 +++++++++
 1 file changed, 9 insertions(+)

Hi Niklas,

Thank you for the patch.

On Thu, Mar 26, 2020 at 05:08:16PM +0100, Niklas Söderlund wrote:
> Their is no need to try and process cancelled frames.

s/Their/There/

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index f64807984519779b..f49b3a7f0945ac92 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1011,6 +1011,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
>  
>  void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>  {
> +	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> +		return;
> +
>  	ASSERT(activeCamera_);
>  	RkISP1CameraData *data = cameraData(activeCamera_);
>  	Request *request = buffer->request();

For the capture buffer, you actually need to process it in order to
complete the request. You'll need a special case there, as you should
not wait for metadata in that case (we're stopping, metadata may never
arrive).

> @@ -1026,6 +1029,9 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>  
>  void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
>  {
> +	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> +		return;
> +
>  	ASSERT(activeCamera_);
>  	RkISP1CameraData *data = cameraData(activeCamera_);

For the params buffer, I think it should not take part in request
completion at all, so the logic in this function has to be reworked.

>  
> @@ -1037,6 +1043,9 @@ void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
>  
>  void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>  {
> +	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> +		return;
> +
>  	ASSERT(activeCamera_);
>  	RkISP1CameraData *data = cameraData(activeCamera_);
>

Hi Laurent,

Thanks for your feedback.

On 2020-03-26 18:52:59 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thu, Mar 26, 2020 at 05:08:16PM +0100, Niklas Söderlund wrote:
> > Their is no need to try and process cancelled frames.
> 
> s/Their/There/
> 
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index f64807984519779b..f49b3a7f0945ac92 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1011,6 +1011,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
> >  
> >  void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> >  {
> > +	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> > +		return;
> > +
> >  	ASSERT(activeCamera_);
> >  	RkISP1CameraData *data = cameraData(activeCamera_);
> >  	Request *request = buffer->request();
> 
> For the capture buffer, you actually need to process it in order to
> complete the request. You'll need a special case there, as you should
> not wait for metadata in that case (we're stopping, metadata may never
> arrive).

True and then we have a problem :-(

In stop() we stops all video devices and set activeCamera_ to nullptr 
and we need the camera to complete the request. We would then need a way 
to wait for all requests to finish before that don't we ? I will dig 
some and see what is what.

> 
> > @@ -1026,6 +1029,9 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> >  
> >  void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
> >  {
> > +	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> > +		return;
> > +
> >  	ASSERT(activeCamera_);
> >  	RkISP1CameraData *data = cameraData(activeCamera_);
> 
> For the params buffer, I think it should not take part in request
> completion at all, so the logic in this function has to be reworked.
> 
> >  
> > @@ -1037,6 +1043,9 @@ void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
> >  
> >  void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> >  {
> > +	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> > +		return;
> > +
> >  	ASSERT(activeCamera_);
> >  	RkISP1CameraData *data = cameraData(activeCamera_);
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart