[libcamera-devel,v4,17/21] libcamera: simple: Fill stride and frameSize at config validation
diff mbox series

Message ID 20200708134417.67747-18-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • Clean up formats in v4l2-compat and pipeline handlers
Related show

Commit Message

Paul Elder July 8, 2020, 1:44 p.m. UTC 
Fill the stride and frameSize fields of the StreamConfiguration at
configuration validation time instead of at camera configuration time.
This allows applications to get the stride when trying a configuration
without modifying the active configuration of the camera.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v4:
- fix converter's stride and frameSize (get it via tryFormat instead of
  calculation)
- merge converter's stride and frameSize to one function
- return error if tryFormat fails
- mention motivation in commit message

New in v3
---
 src/libcamera/pipeline/simple/converter.cpp | 19 +++++++++++++++
 src/libcamera/pipeline/simple/converter.h   |  4 +++
 src/libcamera/pipeline/simple/simple.cpp    | 27 +++++++++++++++++++--
 3 files changed, 48 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart July 8, 2020, 3:25 p.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Wed, Jul 08, 2020 at 10:44:13PM +0900, Paul Elder wrote:
> Fill the stride and frameSize fields of the StreamConfiguration at
> configuration validation time instead of at camera configuration time.
> This allows applications to get the stride when trying a configuration
> without modifying the active configuration of the camera.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v4:
> - fix converter's stride and frameSize (get it via tryFormat instead of
>   calculation)
> - merge converter's stride and frameSize to one function
> - return error if tryFormat fails
> - mention motivation in commit message
> 
> New in v3
> ---
>  src/libcamera/pipeline/simple/converter.cpp | 19 +++++++++++++++
>  src/libcamera/pipeline/simple/converter.h   |  4 +++
>  src/libcamera/pipeline/simple/simple.cpp    | 27 +++++++++++++++++++--
>  3 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> index e5e2f0f..cef1503 100644
> --- a/src/libcamera/pipeline/simple/converter.cpp
> +++ b/src/libcamera/pipeline/simple/converter.cpp
> @@ -261,4 +261,23 @@ void SimpleConverter::outputBufferReady(FrameBuffer *buffer)
>  	}
>  }
>  
> +int SimpleConverter::strideAndFrameSize(const Size &size,
> +					const PixelFormat &pixelFormat,
> +					unsigned int *strideOut,
> +					unsigned int *frameSizeOut)
> +{
> +	V4L2DeviceFormat format = {};
> +	format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat);
> +	format.size = size;
> +
> +	int ret = m2m_->capture()->tryFormat(&format);
> +	if (ret < 0)
> +		return -1;
> +
> +	*strideOut = format.planes[0].bpl;
> +	*frameSizeOut = format.planes[0].size;
> +
> +	return 0;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
> index ef18cf7..3e46988 100644
> --- a/src/libcamera/pipeline/simple/converter.h
> +++ b/src/libcamera/pipeline/simple/converter.h
> @@ -46,6 +46,10 @@ public:
>  
>  	int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>  
> +	int strideAndFrameSize(const Size &size, const PixelFormat &pixelFormat,
> +			       unsigned int *strideOut,
> +			       unsigned int *frameSizeOut);
> +
>  	Signal<FrameBuffer *, FrameBuffer *> bufferReady;
>  
>  private:
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 1ec8d0f..0c52b47 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -457,6 +457,31 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  
>  	cfg.bufferCount = 3;
>  
> +

One blank line is enough.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	/* Set the stride and frameSize. */
> +	if (!needConversion_) {
> +		V4L2DeviceFormat format = {};
> +		format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);
> +		format.size = cfg.size;
> +
> +		int ret = data_->video_->tryFormat(&format);
> +		if (ret < 0)
> +			return Invalid;
> +
> +		cfg.stride = format.planes[0].bpl;
> +		cfg.frameSize = format.planes[0].size;
> +
> +		return status;
> +	}
> +
> +	SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(data_->pipe_);
> +	SimpleConverter *converter = pipe->converter();
> +
> +	int ret = converter->strideAndFrameSize(cfg.size, cfg.pixelFormat,
> +						&cfg.stride, &cfg.frameSize);
> +	if (ret < 0)
> +		return Invalid;
> +
>  	return status;
>  }
>  
> @@ -557,8 +582,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  		return -EINVAL;
>  	}
>  
> -	cfg.stride = captureFormat.planes[0].bpl;
> -
>  	/* Configure the converter if required. */
>  	useConverter_ = config->needConversion();
>
Jacopo Mondi July 8, 2020, 9:40 p.m. UTC | #2 
On Wed, Jul 08, 2020 at 10:44:13PM +0900, Paul Elder wrote:
> Fill the stride and frameSize fields of the StreamConfiguration at
> configuration validation time instead of at camera configuration time.
> This allows applications to get the stride when trying a configuration
> without modifying the active configuration of the camera.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v4:
> - fix converter's stride and frameSize (get it via tryFormat instead of
>   calculation)
> - merge converter's stride and frameSize to one function
> - return error if tryFormat fails
> - mention motivation in commit message
>
> New in v3
> ---
>  src/libcamera/pipeline/simple/converter.cpp | 19 +++++++++++++++
>  src/libcamera/pipeline/simple/converter.h   |  4 +++
>  src/libcamera/pipeline/simple/simple.cpp    | 27 +++++++++++++++++++--
>  3 files changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> index e5e2f0f..cef1503 100644
> --- a/src/libcamera/pipeline/simple/converter.cpp
> +++ b/src/libcamera/pipeline/simple/converter.cpp
> @@ -261,4 +261,23 @@ void SimpleConverter::outputBufferReady(FrameBuffer *buffer)
>  	}
>  }
>
> +int SimpleConverter::strideAndFrameSize(const Size &size,
> +					const PixelFormat &pixelFormat,
> +					unsigned int *strideOut,
> +					unsigned int *frameSizeOut)
> +{
> +	V4L2DeviceFormat format = {};
> +	format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat);
> +	format.size = size;
> +
> +	int ret = m2m_->capture()->tryFormat(&format);
> +	if (ret < 0)
> +		return -1;
> +
> +	*strideOut = format.planes[0].bpl;
> +	*frameSizeOut = format.planes[0].size;
> +
> +	return 0;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
> index ef18cf7..3e46988 100644
> --- a/src/libcamera/pipeline/simple/converter.h
> +++ b/src/libcamera/pipeline/simple/converter.h
> @@ -46,6 +46,10 @@ public:
>
>  	int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>
> +	int strideAndFrameSize(const Size &size, const PixelFormat &pixelFormat,
> +			       unsigned int *strideOut,
> +			       unsigned int *frameSizeOut);
> +
>  	Signal<FrameBuffer *, FrameBuffer *> bufferReady;
>
>  private:
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 1ec8d0f..0c52b47 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -457,6 +457,31 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>
>  	cfg.bufferCount = 3;
>
> +
> +	/* Set the stride and frameSize. */
> +	if (!needConversion_) {
> +		V4L2DeviceFormat format = {};
> +		format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);
> +		format.size = cfg.size;
> +
> +		int ret = data_->video_->tryFormat(&format);
> +		if (ret < 0)
> +			return Invalid;
> +
> +		cfg.stride = format.planes[0].bpl;
> +		cfg.frameSize = format.planes[0].size;
> +
> +		return status;
> +	}
> +
> +	SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(data_->pipe_);
> +	SimpleConverter *converter = pipe->converter();
> +
> +	int ret = converter->strideAndFrameSize(cfg.size, cfg.pixelFormat,
> +						&cfg.stride, &cfg.frameSize);

If you want to feel adventurous you can return a tuple and use
std::tie instead of two output parameters

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> +	if (ret < 0)
> +		return Invalid;
> +
>  	return status;
>  }
>
> @@ -557,8 +582,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  		return -EINVAL;
>  	}
>
> -	cfg.stride = captureFormat.planes[0].bpl;
> -
>  	/* Configure the converter if required. */
>  	useConverter_ = config->needConversion();
>
> --
> 2.27.0
>
> _______________________________________________
> 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/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
index e5e2f0f..cef1503 100644
--- a/src/libcamera/pipeline/simple/converter.cpp
+++ b/src/libcamera/pipeline/simple/converter.cpp
@@ -261,4 +261,23 @@  void SimpleConverter::outputBufferReady(FrameBuffer *buffer)
 	}
 }
 
+int SimpleConverter::strideAndFrameSize(const Size &size,
+					const PixelFormat &pixelFormat,
+					unsigned int *strideOut,
+					unsigned int *frameSizeOut)
+{
+	V4L2DeviceFormat format = {};
+	format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat);
+	format.size = size;
+
+	int ret = m2m_->capture()->tryFormat(&format);
+	if (ret < 0)
+		return -1;
+
+	*strideOut = format.planes[0].bpl;
+	*frameSizeOut = format.planes[0].size;
+
+	return 0;
+}
+
 } /* namespace libcamera */
diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
index ef18cf7..3e46988 100644
--- a/src/libcamera/pipeline/simple/converter.h
+++ b/src/libcamera/pipeline/simple/converter.h
@@ -46,6 +46,10 @@  public:
 
 	int queueBuffers(FrameBuffer *input, FrameBuffer *output);
 
+	int strideAndFrameSize(const Size &size, const PixelFormat &pixelFormat,
+			       unsigned int *strideOut,
+			       unsigned int *frameSizeOut);
+
 	Signal<FrameBuffer *, FrameBuffer *> bufferReady;
 
 private:
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 1ec8d0f..0c52b47 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -457,6 +457,31 @@  CameraConfiguration::Status SimpleCameraConfiguration::validate()
 
 	cfg.bufferCount = 3;
 
+
+	/* Set the stride and frameSize. */
+	if (!needConversion_) {
+		V4L2DeviceFormat format = {};
+		format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);
+		format.size = cfg.size;
+
+		int ret = data_->video_->tryFormat(&format);
+		if (ret < 0)
+			return Invalid;
+
+		cfg.stride = format.planes[0].bpl;
+		cfg.frameSize = format.planes[0].size;
+
+		return status;
+	}
+
+	SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(data_->pipe_);
+	SimpleConverter *converter = pipe->converter();
+
+	int ret = converter->strideAndFrameSize(cfg.size, cfg.pixelFormat,
+						&cfg.stride, &cfg.frameSize);
+	if (ret < 0)
+		return Invalid;
+
 	return status;
 }
 
@@ -557,8 +582,6 @@  int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
 		return -EINVAL;
 	}
 
-	cfg.stride = captureFormat.planes[0].bpl;
-
 	/* Configure the converter if required. */
 	useConverter_ = config->needConversion();