| Message ID | 20200709132835.112593-17-paul.elder@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Jacopo Mondi <jacopo@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@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;
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..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();