[{"id":11296,"web_url":"https://patchwork.libcamera.org/comment/11296/","msgid":"<20200709184042.GC5868@pendragon.ideasonboard.com>","date":"2020-07-09T18:40:42","subject":"Re: [libcamera-devel] [PATCH v5 16/23] libcamera: simple: Fill\n\tstride and frameSize at config validation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Thu, Jul 09, 2020 at 10:28:28PM +0900, Paul Elder wrote:\n> Fill the stride and frameSize fields of the StreamConfiguration at\n> configuration validation time instead of at camera configuration time.\n> This allows applications to get the stride when trying a configuration\n> without modifying the active configuration of the camera.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Laurent Pinchart \n> Reviewed-by: Jacopo Mondi \n> \n> ---\n> Cosmetic change in v5\n> \n> Changes in v4:\n> - fix converter's stride and frameSize (get it via tryFormat instead of\n> calculation)\n> - merge converter's stride and frameSize to one function\n> - return error if tryFormat fails\n> - mention motivation in commit message\n> \n> New in v3\n> ---\n> src/libcamera/pipeline/simple/converter.cpp | 19 +++++++++++++++\n> src/libcamera/pipeline/simple/converter.h | 4 ++++\n> src/libcamera/pipeline/simple/simple.cpp | 26 +++++++++++++++++++--\n> 3 files changed, 47 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp\n> index e5e2f0f..cef1503 100644\n> --- a/src/libcamera/pipeline/simple/converter.cpp\n> +++ b/src/libcamera/pipeline/simple/converter.cpp\n> @@ -261,4 +261,23 @@ void SimpleConverter::outputBufferReady(FrameBuffer *buffer)\n> \t}\n> }\n> \n> +int SimpleConverter::strideAndFrameSize(const Size &size,\n> +\t\t\t\t\tconst PixelFormat &pixelFormat,\n> +\t\t\t\t\tunsigned int *strideOut,\n> +\t\t\t\t\tunsigned int *frameSizeOut)\n> +{\n> +\tV4L2DeviceFormat format = {};\n> +\tformat.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat);\n> +\tformat.size = size;\n> +\n> +\tint ret = m2m_->capture()->tryFormat(&format);\n> +\tif (ret < 0)\n> +\t\treturn -1;\n\nreturn ret ?\n\n> +\n> +\t*strideOut = format.planes[0].bpl;\n> +\t*frameSizeOut = format.planes[0].size;\n> +\n> +\treturn 0;\n> +}\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h\n> index ef18cf7..3e46988 100644\n> --- a/src/libcamera/pipeline/simple/converter.h\n> +++ b/src/libcamera/pipeline/simple/converter.h\n> @@ -46,6 +46,10 @@ public:\n> \n> \tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n> \n> +\tint strideAndFrameSize(const Size &size, const PixelFormat &pixelFormat,\n> +\t\t\t unsigned int *strideOut,\n> +\t\t\t unsigned int *frameSizeOut);\n> +\n> \tSignal bufferReady;\n> \n> private:\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 1ec8d0f..2ca57d0 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -457,6 +457,30 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> \n> \tcfg.bufferCount = 3;\n> \n> +\t/* Set the stride and frameSize. */\n> +\tif (!needConversion_) {\n> +\t\tV4L2DeviceFormat format = {};\n> +\t\tformat.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);\n> +\t\tformat.size = cfg.size;\n> +\n> +\t\tint ret = data_->video_->tryFormat(&format);\n> +\t\tif (ret < 0)\n> +\t\t\treturn Invalid;\n> +\n> +\t\tcfg.stride = format.planes[0].bpl;\n> +\t\tcfg.frameSize = format.planes[0].size;\n> +\n> +\t\treturn status;\n> +\t}\n> +\n> +\tSimplePipelineHandler *pipe = static_cast(data_->pipe_);\n> +\tSimpleConverter *converter = pipe->converter();\n> +\n> +\tint ret = converter->strideAndFrameSize(cfg.size, cfg.pixelFormat,\n> +\t\t\t\t\t\t&cfg.stride, &cfg.frameSize);\n> +\tif (ret < 0)\n> +\t\treturn Invalid;\n> +\n> \treturn status;\n> }\n> \n> @@ -557,8 +581,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tcfg.stride = captureFormat.planes[0].bpl;\n> -\n> \t/* Configure the converter if required. */\n> \tuseConverter_ = config->needConversion();\n> \n\nI've tried this, based on an earlier comment from Jacopo. There are pros\nand cons, I like how it avoids output parameters, but on the other hand\nthe return parameters are not named, which can make the code more\nconfusing. I'll let you decide what you like better, with or without\nthis,\n\nReviewed-by: Laurent Pinchart \n\ndiff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp\nindex cef150336691..dc7c046329f1 100644\n--- a/src/libcamera/pipeline/simple/converter.cpp\n+++ b/src/libcamera/pipeline/simple/converter.cpp\n@@ -261,10 +261,9 @@ void SimpleConverter::outputBufferReady(FrameBuffer *buffer)\n \t}\n }\n\n-int SimpleConverter::strideAndFrameSize(const Size &size,\n-\t\t\t\t\tconst PixelFormat &pixelFormat,\n-\t\t\t\t\tunsigned int *strideOut,\n-\t\t\t\t\tunsigned int *frameSizeOut)\n+std::tuple\n+SimpleConverter::strideAndFrameSize(const Size &size,\n+\t\t\t\t const PixelFormat &pixelFormat)\n {\n \tV4L2DeviceFormat format = {};\n \tformat.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat);\n@@ -272,12 +271,9 @@ int SimpleConverter::strideAndFrameSize(const Size &size,\n\n \tint ret = m2m_->capture()->tryFormat(&format);\n \tif (ret < 0)\n-\t\treturn -1;\n+\t\treturn { 0, 0 };\n\n-\t*strideOut = format.planes[0].bpl;\n-\t*frameSizeOut = format.planes[0].size;\n-\n-\treturn 0;\n+\treturn { format.planes[0].bpl, format.planes[0].size };\n }\n\n } /* namespace libcamera */\ndiff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h\nindex 3e469888fecf..8ca88912b4be 100644\n--- a/src/libcamera/pipeline/simple/converter.h\n+++ b/src/libcamera/pipeline/simple/converter.h\n@@ -10,6 +10,7 @@\n\n #include \n #include \n+#include \n #include \n\n #include \n@@ -46,9 +47,8 @@ public:\n\n \tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n\n-\tint strideAndFrameSize(const Size &size, const PixelFormat &pixelFormat,\n-\t\t\t unsigned int *strideOut,\n-\t\t\t unsigned int *frameSizeOut);\n+\tstd::tuple\n+\tstrideAndFrameSize(const Size &size, const PixelFormat &pixelFormat);\n\n \tSignal bufferReady;\n\ndiff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\nindex 2ca57d05a174..28d367883323 100644\n--- a/src/libcamera/pipeline/simple/simple.cpp\n+++ b/src/libcamera/pipeline/simple/simple.cpp\n@@ -476,9 +476,9 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n \tSimplePipelineHandler *pipe = static_cast(data_->pipe_);\n \tSimpleConverter *converter = pipe->converter();\n\n-\tint ret = converter->strideAndFrameSize(cfg.size, cfg.pixelFormat,\n-\t\t\t\t\t\t&cfg.stride, &cfg.frameSize);\n-\tif (ret < 0)\n+\tstd::tie(cfg.stride, cfg.frameSize) =\n+\t\tconverter->strideAndFrameSize(cfg.size, cfg.pixelFormat);\n+\tif (cfg.stride == 0)\n \t\treturn Invalid;\n\n \treturn status;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BA706BD792\n\tfor ;\n\tThu, 9 Jul 2020 18:40:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5806661356;\n\tThu, 9 Jul 2020 20:40:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 12B3E6123A\n\tfor ;\n\tThu, 9 Jul 2020 20:40:49 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8A99356E;\n\tThu, 9 Jul 2020 20:40:48 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"u5/ZOgWD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594320048;\n\tbh=YYARi8QPetzAVYXyXuE37syENuMKMb4jSnDUJQP/XlI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=u5/ZOgWDhf/1OTq5y8hbd8V6IzgKef4RO7gEK2fX0No89eKxhEpkITtpYAlhd1jsM\n\tUxUCGlW4YwJat9IjCL+/4STSXcQ3Zfka9YjEdNPBE74SVOw3GhBqOAtnpB9rplBZUX\n\tknofjb98lbhcRePk9CffP91Z6C17cXpSpA+7zDQI=","Date":"Thu, 9 Jul 2020 21:40:42 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20200709184042.GC5868@pendragon.ideasonboard.com>","References":"<20200709132835.112593-1-paul.elder@ideasonboard.com>\n\t<20200709132835.112593-17-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200709132835.112593-17-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 16/23] libcamera: simple: Fill\n\tstride and frameSize at config validation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]