[libcamera-devel] [PATCH 13/20] libcamera: pipeline: simple: Rename Configuration::pixelFormat

[Laurent Pinchart]

Hi Kieran,

On Mon, Mar 01, 2021 at 08:28:36PM +0000, Kieran Bingham wrote:
> On 31/01/2021 22:46, Laurent Pinchart wrote:
> > The Configuration::pixelFormat field stores the pixel format at the
> > output of the capture part of the pipeline. Rename it to captureFormat,
> > to match the related captureSize field.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 7c5a56a2f395..72a18cc0b97f 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -157,7 +157,7 @@ public:
> >  
> >  	struct Configuration {
> >  		uint32_t code;
> > -		PixelFormat pixelFormat;
> > +		PixelFormat captureFormat;
> >  		Size captureSize;
> >  		SizeRange outputSizes;
> >  	};
> > @@ -379,7 +379,7 @@ int SimpleCameraData::init()
> >  
> >  			Configuration config;
> >  			config.code = code;
> > -			config.pixelFormat = pixelFormat;
> > +			config.captureFormat = pixelFormat;
> >  			config.captureSize = format.size;
> >  
> >  			if (!converter) {
> > @@ -551,7 +551,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
> >  		status = Adjusted;
> >  	}
> >  
> > -	needConversion_ = cfg.pixelFormat != pipeConfig.pixelFormat
> > +	needConversion_ = cfg.pixelFormat != pipeConfig.captureFormat
> >  			|| cfg.size != pipeConfig.captureSize;
> 
> Should a 'format' and a 'size' be a wrapped type, to be able to express:
>  	needsConversion_ = cfg.ImageFormat != pipeConfig.ImageFormat?

Absolutely, I've been thinking about this a few times already, in
various places. I'll add a todo comment to remember.

> Probably not, but seeing captureFormat and captureSize, along with a
> batched comparison makes me wonder if this would be a common grouping.
> 
> Overkill for now unless it's used frequently, or needs 'multiple' stored
> equivalent groupings.
> 
> 
> Anyway, that's not required here.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> >  
> >  	cfg.bufferCount = 3;
> > @@ -656,7 +656,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> >  		return ret;
> >  
> >  	/* Configure the video node. */
> > -	V4L2PixelFormat videoFormat = video->toV4L2PixelFormat(pipeConfig.pixelFormat);
> > +	V4L2PixelFormat videoFormat = video->toV4L2PixelFormat(pipeConfig.captureFormat);
> >  
> >  	V4L2DeviceFormat captureFormat;
> >  	captureFormat.fourcc = videoFormat;
> > @@ -686,7 +686,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> >  
> >  	if (useConverter_) {
> >  		StreamConfiguration inputCfg;
> > -		inputCfg.pixelFormat = pipeConfig.pixelFormat;
> > +		inputCfg.pixelFormat = pipeConfig.captureFormat;
> >  		inputCfg.size = pipeConfig.captureSize;
> >  		inputCfg.stride = captureFormat.planes[0].bpl;
> >  		inputCfg.bufferCount = cfg.bufferCount;

-- 
Regards,

Laurent Pinchart