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

[paul.elder at ideasonboard.com]

Hi Laurent and Kieran,

On Tue, Mar 02, 2021 at 01:00:01AM +0200, Laurent Pinchart wrote:
> 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.

I remember thinking about this too.

> > 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>

Reviewed-by: Paul Elder <paul.elder 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel