[libcamera-devel] [PATCH v5 1/6] pipeline: raspberrypi: Refactor stream configuration routine

[Laurent Pinchart]

On Thu, Feb 04, 2021 at 11:18:27PM +0200, Laurent Pinchart wrote:
> Hi Naush,
> 
> Thank you for the patch.
> 
> On Fri, Jan 29, 2021 at 03:11:49PM +0000, Naushir Patuck wrote:
> > Refactor the high/low resolution stream format and output selection
> > routine. This change is in preparation of adding 1/4 resolution output
> > for fast colour denoise.
> > 
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 57 ++++++-------------
> >  1 file changed, 16 insertions(+), 41 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 524cc960dd37..d9895c779725 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -624,52 +624,27 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >  			continue;
> >  		}
> >  
> > -		if (i == maxIndex) {
> > -			/* ISP main output format. */
> > -			V4L2VideoDevice *dev = data->isp_[Isp::Output0].dev();
> > -			V4L2PixelFormat fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> > -			format.size = cfg.size;
> > -			format.fourcc = fourcc;
> > -
> > -			ret = dev->setFormat(&format);
> > -			if (ret)
> > -				return -EINVAL;
> > -
> > -			if (format.size != cfg.size || format.fourcc != fourcc) {
> > -				LOG(RPI, Error)
> > -					<< "Failed to set format on ISP capture0 device: "
> > -					<< format.toString();
> > -				return -EINVAL;
> > -			}
> > -
> > -			cfg.setStream(&data->isp_[Isp::Output0]);
> > -			data->isp_[Isp::Output0].setExternal(true);
> > -		}
> > +		/* The largest resolution gets routed to the ISP Output 0 node. */
> > +		RPi::Stream *stream = (i == maxIndex) ? &data->isp_[Isp::Output0]
> 
> No need for the outer parentheses.
> 
> > +						      : &data->isp_[Isp::Output1];
> >  
> > -		/*
> > -		 * ISP second output format. This fallthrough means that if a
> > -		 * second output stream has not been configured, we simply use
> > -		 * the Output0 configuration.
> > -		 */
> > -		V4L2VideoDevice *dev = data->isp_[Isp::Output1].dev();
> > -		format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> > +		V4L2PixelFormat fourcc = stream->dev()->toV4L2PixelFormat(cfg.pixelFormat);
> >  		format.size = cfg.size;
> > +		format.fourcc = fourcc;
> >  
> > -		ret = dev->setFormat(&format);
> > -		if (ret) {
> > +		ret = stream->dev()->setFormat(&format);
> > +		if (ret)
> > +			return -EINVAL;
> > +
> > +		if (format.size != cfg.size || format.fourcc != fourcc) {
> >  			LOG(RPI, Error)
> > -				<< "Failed to set format on ISP capture1 device: "
> > -				<< format.toString();
> > -			return ret;
> > -		}
> > -		/*
> > -		 * If we have not yet provided a stream for this config, it
> > -		 * means this is to be routed from Output1.
> > -		 */
> > -		if (!cfg.stream()) {
> > -			cfg.setStream(&data->isp_[Isp::Output1]);
> > -			data->isp_[Isp::Output1].setExternal(true);
> > +				<< "Failed get requested format on " << stream->name()
> 
> "get" or "set" ?

And maybe "Failed to set" ?

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> I can fix these small issues when applying.
> 
> > +				<< ", returned " << format.toString();
> > +			return -EINVAL;
> >  		}
> > +
> > +		cfg.setStream(stream);
> > +		stream->setExternal(true);
> >  	}
> >  
> >  	/* ISP statistics output format. */

-- 
Regards,

Laurent Pinchart