[libcamera-devel,13/20] libcamera: pipeline: simple: Rename Configuration::pixelFormat
diff mbox series

Message ID 20210131224702.8838-14-laurent.pinchart@ideasonboard.com
State Accepted
Delegated to: Laurent Pinchart
Headers show
Series
  • [libcamera-devel,01/20] libcamera: pipeline: simple: Manage converter with std::unique_ptr<>
Related show

Commit Message

Laurent Pinchart Jan. 31, 2021, 10:46 p.m. UTC 
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@ideasonboard.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Kieran Bingham March 1, 2021, 8:28 p.m. UTC | #1 
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@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?

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@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;
>
Laurent Pinchart March 1, 2021, 11 p.m. UTC | #2 
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@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@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;
Paul Elder March 2, 2021, 1:21 a.m. UTC | #3 
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@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@ideasonboard.com>

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

Patch
diff mbox series 

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