[libcamera-devel] [PATCH v5 16/23] libcamera: simple: Fill stride and frameSize at config validation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jul 9 20:40:42 CEST 2020
Hi Paul,
Thank you for the patch.
On Thu, Jul 09, 2020 at 10:28:28PM +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 at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> ---
> Cosmetic change in v5
>
> 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 | 26 +++++++++++++++++++--
> 3 files changed, 47 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;
return ret ?
> +
> + *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..2ca57d0 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -457,6 +457,30 @@ 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 +581,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> return -EINVAL;
> }
>
> - cfg.stride = captureFormat.planes[0].bpl;
> -
> /* Configure the converter if required. */
> useConverter_ = config->needConversion();
>
I've tried this, based on an earlier comment from Jacopo. There are pros
and cons, I like how it avoids output parameters, but on the other hand
the return parameters are not named, which can make the code more
confusing. I'll let you decide what you like better, with or without
this,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
index cef150336691..dc7c046329f1 100644
--- a/src/libcamera/pipeline/simple/converter.cpp
+++ b/src/libcamera/pipeline/simple/converter.cpp
@@ -261,10 +261,9 @@ void SimpleConverter::outputBufferReady(FrameBuffer *buffer)
}
}
-int SimpleConverter::strideAndFrameSize(const Size &size,
- const PixelFormat &pixelFormat,
- unsigned int *strideOut,
- unsigned int *frameSizeOut)
+std::tuple<unsigned int, unsigned int>
+SimpleConverter::strideAndFrameSize(const Size &size,
+ const PixelFormat &pixelFormat)
{
V4L2DeviceFormat format = {};
format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat);
@@ -272,12 +271,9 @@ int SimpleConverter::strideAndFrameSize(const Size &size,
int ret = m2m_->capture()->tryFormat(&format);
if (ret < 0)
- return -1;
+ return { 0, 0 };
- *strideOut = format.planes[0].bpl;
- *frameSizeOut = format.planes[0].size;
-
- return 0;
+ return { format.planes[0].bpl, format.planes[0].size };
}
} /* namespace libcamera */
diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
index 3e469888fecf..8ca88912b4be 100644
--- a/src/libcamera/pipeline/simple/converter.h
+++ b/src/libcamera/pipeline/simple/converter.h
@@ -10,6 +10,7 @@
#include <memory>
#include <queue>
+#include <tuple>
#include <vector>
#include <libcamera/pixel_format.h>
@@ -46,9 +47,8 @@ public:
int queueBuffers(FrameBuffer *input, FrameBuffer *output);
- int strideAndFrameSize(const Size &size, const PixelFormat &pixelFormat,
- unsigned int *strideOut,
- unsigned int *frameSizeOut);
+ std::tuple<unsigned int, unsigned int>
+ strideAndFrameSize(const Size &size, const PixelFormat &pixelFormat);
Signal<FrameBuffer *, FrameBuffer *> bufferReady;
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 2ca57d05a174..28d367883323 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -476,9 +476,9 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
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)
+ std::tie(cfg.stride, cfg.frameSize) =
+ converter->strideAndFrameSize(cfg.size, cfg.pixelFormat);
+ if (cfg.stride == 0)
return Invalid;
return status;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list