[libcamera-devel] [PATCH v3 14/14] libcamera: pipeline: Cast to derived pipeline handler with helpers

[Kieran Bingham]

On 12/08/2021 00:26, Laurent Pinchart wrote:
> Replace manual static casts from the PipelineHandler pointer to a
> derived class pointer with helper functions in the camera data classes.
> This simplifies code accessing the pipeline from the camera data.

And helps reduce the number of places someone might do casting wrong, so
I think that's a good idea, and cleaner code.


> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 ++++++++------
>  src/libcamera/pipeline/simple/simple.cpp | 16 +++++++++-------

I assume the other pipelines are not affected by this ...



>  2 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 3c37a8ccad8f..2dbd6304acce 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -87,6 +87,7 @@ public:
>  	{
>  	}
>  
> +	PipelineHandlerRkISP1 *pipe();
>  	int loadIPA(unsigned int hwRevision);
>  
>  	Stream mainPathStream_;
> @@ -304,6 +305,11 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request)
>  	return nullptr;
>  }
>  
> +PipelineHandlerRkISP1 *RkISP1CameraData::pipe()
> +{
> +	return static_cast<PipelineHandlerRkISP1 *>(Camera::Private::pipe());
> +}
> +
>  int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>  {
>  	ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe(), 1, 1);
> @@ -332,8 +338,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,
>  		break;
>  	}
>  	case ipa::rkisp1::ActionParamFilled: {
> -		PipelineHandlerRkISP1 *pipe =
> -			static_cast<PipelineHandlerRkISP1 *>(this->pipe());
> +		PipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();


Why the RkISP1CameraData:: prefix?

Isn't this just pipe(); here?

We're in RkISP1CameraData::queueFrameAction() right ?

With that resolved,

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


>  		RkISP1FrameInfo *info = frameInfo_.find(frame);
>  		if (!info)
>  			break;
> @@ -360,9 +365,6 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,
>  
>  void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)
>  {
> -	PipelineHandlerRkISP1 *pipe =
> -		static_cast<PipelineHandlerRkISP1 *>(this->pipe());
> -
>  	RkISP1FrameInfo *info = frameInfo_.find(frame);
>  	if (!info)
>  		return;
> @@ -370,7 +372,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta
>  	info->request->metadata().merge(metadata);
>  	info->metadataProcessed = true;
>  
> -	pipe->tryCompleteRequest(info->request);
> +	pipe()->tryCompleteRequest(info->request);
>  }
>  
>  RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index ef7687eaf502..8ff954722fed 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -155,6 +155,7 @@ public:
>  			 MediaEntity *sensor);
>  
>  	bool isValid() const { return sensor_ != nullptr; }
> +	SimplePipelineHandler *pipe();
>  
>  	int init();
>  	int setupLinks();
> @@ -352,11 +353,14 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
>  			       [](const Entity &e) { return e.entity->name(); });
>  }
>  
> +SimplePipelineHandler *SimpleCameraData::pipe()
> +{
> +	return static_cast<SimplePipelineHandler *>(Camera::Private::pipe());
> +}
> +
>  int SimpleCameraData::init()
>  {
> -	SimplePipelineHandler *pipe =
> -		static_cast<SimplePipelineHandler *>(this->pipe());
> -	SimpleConverter *converter = pipe->converter();
> +	SimpleConverter *converter = pipe()->converter();
>  	int ret;
>  
>  	/*
> @@ -480,8 +484,7 @@ int SimpleCameraData::setupLinks()
>  int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
>  				   V4L2Subdevice::Whence whence)
>  {
> -	SimplePipelineHandler *pipe =
> -		static_cast<SimplePipelineHandler *>(this->pipe());
> +	SimplePipelineHandler *pipe = SimpleCameraData::pipe();
>  	int ret;
>  
>  	/*
> @@ -582,8 +585,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  	}
>  
>  	/* Adjust the requested streams. */
> -	SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(data_->pipe());
> -	SimpleConverter *converter = pipe->converter();
> +	SimpleConverter *converter = data_->pipe()->converter();
>  
>  	/*
>  	 * Enable usage of the converter when producing multiple streams, as
>