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

[Laurent Pinchart]

Hi Kieran,

On Mon, Aug 16, 2021 at 01:30:36PM +0100, Kieran Bingham wrote:
> On 16/08/2021 13:26, Laurent Pinchart wrote:
> >>> @@ -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?
> > 
> > Because an unqualified 'pipe' refers to the local variable, so
> > 
> > 		PipelineHandlerRkISP1 *pipe = pipe();
> > 
> > makes the compiler complain that 'PipelineHandlerRkISP1 *' has no
> > operator() defined. It doesn't resolve 'pipe' to the function, but the
> > local variable.
> 
> But is the local variable needed?
> 
> You could change the two lines :
> 
>  		pipe->param_->queueBuffer(info->paramBuffer);
>  		pipe->stat_->queueBuffer(info->statBuffer);
> 
> to
>  		pipe()->param_->queueBuffer(info->paramBuffer);
>  		pipe()->stat_->queueBuffer(info->statBuffer);
> 
> 
> Which would match the usage everywhere else...

Yes indeed. I overall tried to use a local variable when pipe() would be
called multiple times, but got lazy in a few places I suppose. I haven't
removed existing variables, and was actually thinking about adding more
in the few places where I haven't yet :-)

> Anyway, it's a valid construct as you have it - it just looked out of place.
> 
> Tag still holds however you decide to do this.
> 
> >> Isn't this just pipe(); here?
> >>
> >> We're in RkISP1CameraData::queueFrameAction() right ?
> >>
> >> With that resolved,
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

-- 
Regards,

Laurent Pinchart