[libcamera-devel] [PATCH 08/10] libcamera: pipelines: Align where to call base class queueRequest()

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Oct 28, 2019 at 03:22:22AM +0100, Niklas Söderlund wrote:
> The PipelineHandler base class queueRequest() where called at different

s/where called/is called/ ?

> points in different pipeline handlers. Align where they are called to
> make it easier to read different pipeline implementations.

s/pipeline/pipeline handler/

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 5 +++--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +---
>  src/libcamera/pipeline/uvcvideo.cpp      | 4 +---
>  src/libcamera/pipeline/vimc.cpp          | 4 +---
>  4 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 8d3ad568d16e9f8f..06ee6ccac641eb41 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -768,9 +768,10 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
>  			error = ret;
>  	}
>  
> -	PipelineHandler::queueRequest(camera, request);
> +	if (error)
> +		return error;

Should we instead return an error immediately in the above loop instead
of continuing ?

>  
> -	return error;
> +	return PipelineHandler::queueRequest(camera, request);
>  }
>  
>  bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 7a28b03b8d38c8b9..a99df4f8b846d8b7 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -814,8 +814,6 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)
>  	RkISP1CameraData *data = cameraData(camera);
>  	Stream *stream = &data->stream_;
>  
> -	PipelineHandler::queueRequest(camera, request);
> -
>  	RkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,
>  							stream);
>  	if (!info)
> @@ -833,7 +831,7 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)
>  
>  	data->frame_++;
>  
> -	return 0;
> +	return PipelineHandler::queueRequest(camera, request);

What if the request completes before PipelineHandler::queueRequest() is
called ? This isn't really a problem today, but may become one later
when we'll use threads in pipeline handlers. The queueRequest() method
could be called from a thread different than the one running the
pipeline handler event loop. I suppose there will be more issues to fix
at that point, so maybe this change is OK for now.

>  }
>  
>  /* -----------------------------------------------------------------------------
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index fae0ffc4de30e1b7..dc17b456af6987e6 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -281,9 +281,7 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
>  	if (ret < 0)
>  		return ret;
>  
> -	PipelineHandler::queueRequest(camera, request);
> -
> -	return 0;
> +	return PipelineHandler::queueRequest(camera, request);

I'm slightly annoyed by this. It doesn't break anything, and forwards
the error code from PipelineHandler::queueRequest(), which is good
overall. However, PipelineHandler::queueRequest() never fails in its
current implementation (which is why the code isn't currently broken),
and if that were to change, we would need to handle the error here to
undo the previous actions (which can't really be done, as a buffer
queued to a V4L2 device can't be "unqueued"). The code is thus a bit
fragile. I suppose this change doesn't make the situation worse, so we
can address the issue later when it arises.

I'm not opposed to this patch, if other developers think it should be
merged, in its current form, let's do it.

>  }
>  
>  bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index c16ae4cb76b57e1c..5f83ae2b85997828 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -341,9 +341,7 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)
>  	if (ret < 0)
>  		return ret;
>  
> -	PipelineHandler::queueRequest(camera, request);
> -
> -	return 0;
> +	return PipelineHandler::queueRequest(camera, request);
>  }
>  
>  bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)

-- 
Regards,

Laurent Pinchart