[{"id":14832,"web_url":"https://patchwork.libcamera.org/comment/14832/","msgid":"","date":"2021-01-27T11:38:53","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: libyuv: Introduce\n\tPostProcessorLibyuv","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Wed, Jan 27, 2021 at 04:44:25AM +0000, Hirokazu Honda wrote:\n> This adds PostProcessorLibyuv. It supports NV12 buffer scaling.\n> \n> Signed-off-by: Hirokazu Honda \n> \n> ---\n> src/android/libyuv/post_processor_libyuv.cpp | 126 +++++++++++++++++++\n> src/android/libyuv/post_processor_libyuv.h | 38 ++++++\n> src/android/meson.build | 1 +\n> 3 files changed, 165 insertions(+)\n> create mode 100644 src/android/libyuv/post_processor_libyuv.cpp\n> create mode 100644 src/android/libyuv/post_processor_libyuv.h\n> \n> diff --git a/src/android/libyuv/post_processor_libyuv.cpp b/src/android/libyuv/post_processor_libyuv.cpp\n> new file mode 100644\n> index 00000000..5f40fd25\n> --- /dev/null\n> +++ b/src/android/libyuv/post_processor_libyuv.cpp\n> @@ -0,0 +1,126 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * post_processor_libyuv.cpp - Post Processor using libyuv\n> + */\n> +\n> +#include \"post_processor_libyuv.h\"\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(LIBYUV)\n> +\n> +namespace {\n\nMissing blank line.\n\n> +void getNV12LengthAndStride(Size size, unsigned int length[2],\n> +\t\t\t unsigned int stride[2])\n> +{\n> +\tconst auto nv12Info = PixelFormatInfo::info(PixelFormat(formats::NV12));\n\nformats::NV12 is already a PixelFormat, so you can write\n\n\tconst auto nv12Info = PixelFormatInfo::info(formats::NV12);\n\nI'd write out the type explicitly though:\n\n\tconst PixelFormatInfo &nv12Info = PixelFormatInfo::info(formats::NV12);\n\n> +\tfor (unsigned int i = 0; i < 2; i++) {\n> +\t\tconst unsigned int vertSubSample =\n> +\t\t\tnv12Info.planes[i].verticalSubSampling;\n> +\t\tlength[i] = nv12Info.stride(size.width, i, /*align=*/1) *\n> +\t\t\t ((size.height + vertSubSample - 1) / vertSubSample);\n> +\t\tstride[i] = nv12Info.stride(size.width, i, 1);\n> +\t}\n> +}\n\nMissing blank line here too.\n\n> +} // namespace\n\nWe could also make this a private static member function. For classes\nexposed through the public API it could polute the public headers\nunnecessarily, but for internal classes, it's less of an issue.\n\n> +\n> +PostProcessorLibyuv::PostProcessorLibyuv() = default;\n\nIs this needed ? The compiler should generate a default constructor. You\ncould drop this line and the declaration of the constructor in the\nclass definition.\n\n> +\n> +int PostProcessorLibyuv::configure(const StreamConfiguration &inCfg,\n> +\t\t\t\t const StreamConfiguration &outCfg)\n> +{\n> +\tif (inCfg.pixelFormat != outCfg.pixelFormat) {\n> +\t\tLOG(LIBYUV, Error)\n> +\t\t\t<< \"Pixel format conversion is not supported\"\n> +\t\t\t<< \"-In: \" << inCfg.toString()\n> +\t\t\t<< \"-Out: \" << outCfg.toString();\n\nThis will print something like\n\n\t\"Pixel format conversion is not supported-In: NV12-Out: NV24\"\n\nHow about the following ?\n\n\t\tLOG(LIBYUV, Error)\n\t\t\t<< \"Pixel format conversion is not supported\"\n\t\t\t<< \" (from \" << inCfg.toString()\n\t\t\t<< \" to \" << outCfg.toString() << \")\";\n\n> +\t\treturn -EINVAL;\n> +\t}\n\nBlank line.\n\n> +\tif (inCfg.size < outCfg.size) {\n> +\t\tLOG(LIBYUV, Error) << \"Up-scaling is not supported\"\n> +\t\t\t\t << \", -In: \" << inCfg.toString()\n> +\t\t\t\t << \", -Out: \" << outCfg.toString();\n\nSame here for the message.\n\n> +\t\treturn -EINVAL;\n> +\t}\n\nHere too.\n\n> +\tif (inCfg.pixelFormat == formats::NV12) {\n> +\t\tLOG(LIBYUV, Error) << \"Only NV12 is supported\"\n> +\t\t\t\t << \", -In: \" << inCfg.toString()\n> +\t\t\t\t << \", -Out: \" << outCfg.toString();\n\n\t\tLOG(LIBYUV, Error)\n\t\t\t<< \"Unsupported format \" << inCfg.pixelFormat\n\t\t\t<< \" (only NV12 is supported)\";\n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tgetNV12LengthAndStride(inCfg.size, sourceLength_, sourceStride_);\n> +\tgetNV12LengthAndStride(outCfg.size, destinationLength_,\n> +\t\t\t destinationStride_);\n> +\treturn 0;\n> +}\n> +\n> +int PostProcessorLibyuv::process(const FrameBuffer &source,\n> +\t\t\t\t libcamera::MappedBuffer *destination,\n> +\t\t\t\t const CameraMetadata & /*requestMetadata*/,\n> +\t\t\t\t CameraMetadata * /*metadata*/)\n> +{\n> +\tif (!IsValidNV12Buffers(source, *destination)) {\n> +\t\tLOG(LIBYUV, Error) << \"Invalid source and destination\";\n\nI think you could drop this message as IsValidNV12Buffers() already logs\nerrors.\n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tconst MappedFrameBuffer sourceMapped(&source, PROT_READ);\n> +\tif (!sourceMapped.isValid()) {\n> +\t\tLOG(LIBYUV, Error) << \"Failed to mmap camera frame buffer\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0 == libyuv::NV12Scale(sourceMapped.maps()[0].data(),\n> +\t\t\t\t sourceStride_[0],\n> +\t\t\t\t sourceMapped.maps()[1].data(),\n> +\t\t\t\t sourceStride_[1],\n> +\t\t\t\t sourceSize_.width, sourceSize_.height,\n> +\t\t\t\t destination->maps()[0].data(),\n> +\t\t\t\t destinationStride_[0],\n> +\t\t\t\t destination->maps()[1].data(),\n> +\t\t\t\t destinationStride_[1],\n> +\t\t\t\t destinationSize_.width,\n> +\t\t\t\t destinationSize_.height,\n> +\t\t\t\t libyuv::FilterMode::kFilterBilinear);\n> +}\n> +\n> +bool PostProcessorLibyuv::IsValidNV12Buffers(\n\ns/IsValidNV12Buffers/isValidNV12Buffers/\n\n> +\tconst FrameBuffer &source,\n> +\tconst libcamera::MappedBuffer &destination) const\n> +{\n> +\tif (source.planes().size() != 2u) {\n> +\t\tLOG(LIBYUV, Error) << \"The number of source planes is not 2\";\n> +\t\treturn false;\n> +\t}\n> +\tif (destination.maps().size() != 2u) {\n> +\t\tLOG(LIBYUV, Error)\n> +\t\t\t<< \"The number of destination planes is not 2\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tif (source.planes()[0].length < sourceLength_[0] ||\n> +\t source.planes()[1].length < sourceLength_[1]) {\n> +\t\tLOG(LIBYUV, Error)\n> +\t\t\t<< \"The source planes lengths are too small\";\n> +\t\treturn false;\n> +\t}\n> +\tif (destination.maps()[0].size() < destinationLength_[0] ||\n> +\t destination.maps()[1].size() < destinationLength_[1]) {\n> +\t\tLOG(LIBYUV, Error)\n> +\t\t\t<< \"The destination planes lengths are too small\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> diff --git a/src/android/libyuv/post_processor_libyuv.h b/src/android/libyuv/post_processor_libyuv.h\n> new file mode 100644\n> index 00000000..40c635a1\n> --- /dev/null\n> +++ b/src/android/libyuv/post_processor_libyuv.h\n> @@ -0,0 +1,38 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * post_processor_libyuv.h - Post Processor using libyuv\n> + */\n> +#ifndef __ANDROID_POST_PROCESSOR_LIBYUV_H__\n> +#define __ANDROID_POST_PROCESSOR_LIBYUV_H__\n> +\n> +#include \"../post_processor.h\"\n> +\n> +class CameraDevice;\n> +\n> +class PostProcessorLibyuv : public PostProcessor\n> +{\n> +public:\n> +\tPostProcessorLibyuv();\n> +\n> +\tint configure(const libcamera::StreamConfiguration &incfg,\n> +\t\t const libcamera::StreamConfiguration &outcfg) override;\n> +\tint process(const libcamera::FrameBuffer &source,\n> +\t\t libcamera::MappedBuffer *destination,\n> +\t\t const CameraMetadata & /*requestMetadata*/,\n\nYou can use [[maybe_unused]].\n\n> +\t\t CameraMetadata *metadata) override;\n> +\n> +private:\n> +\tbool IsValidNV12Buffers(const libcamera::FrameBuffer &source,\n> +\t\t\t\tconst libcamera::MappedBuffer &destination) const;\n> +\n> +\tlibcamera::Size sourceSize_;\n> +\tlibcamera::Size destinationSize_;\n> +\tunsigned int sourceLength_[2] = {};\n> +\tunsigned int destinationLength_[2] = {};\n> +\tunsigned int sourceStride_[2] = {};\n> +\tunsigned int destinationStride_[2] = {};\n> +};\n> +\n> +#endif /* __ANDROID_POST_PROCESSOR_LIBYUV_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 3d4d3be4..ff067d12 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -26,6 +26,7 @@ android_hal_sources = files([\n> 'jpeg/exif.cpp',\n> 'jpeg/post_processor_jpeg.cpp',\n> 'jpeg/thumbnailer.cpp',\n> + 'libyuv/post_processor_libyuv.cpp'\n> ])\n> \n> android_camera_metadata_sources = files([","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 70F1ABD808\n\tfor ;\n\tWed, 27 Jan 2021 11:39:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93C5F6836F;\n\tWed, 27 Jan 2021 12:39:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AEDC760F1F\n\tfor ;\n\tWed, 27 Jan 2021 12:39:13 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2E31D240;\n\tWed, 27 Jan 2021 12:39:13 +0100 (CET)"],"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=\"sC3GRPZU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611747553;\n\tbh=+dx2IE5Nro9e27H/i37/Mz/JWXtrn6DdhzsL81FlwA4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sC3GRPZU9HhCTSyeYJWi1yqkB7Xn/7JCzwUj5NtvYPwcjRZ1AxTzN3i7BKSrrDz2M\n\tcbtQiLRjrWXEc/5X1EuJJs/P5bkkWkO+YbsweqaiSJhxRHnOxHFmhnl6EFqcdvtrvX\n\teDtnfcPd/AkS74DeWzj+xNw5wE/HC5slHpSSvG8I=","Date":"Wed, 27 Jan 2021 13:38:53 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210127044425.2193480-1-hiroh@chromium.org>\n\t<20210127044425.2193480-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210127044425.2193480-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: libyuv: Introduce\n\tPostProcessorLibyuv","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\" "}},{"id":14837,"web_url":"https://patchwork.libcamera.org/comment/14837/","msgid":"","date":"2021-01-28T00:52:46","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: libyuv: Introduce\n\tPostProcessorLibyuv","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nThanks for reviewing.\n\nOn Wed, Jan 27, 2021 at 8:39 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Wed, Jan 27, 2021 at 04:44:25AM +0000, Hirokazu Honda wrote:\n> > This adds PostProcessorLibyuv. It supports NV12 buffer scaling.\n> >\n> > Signed-off-by: Hirokazu Honda \n> >\n> > ---\n> > src/android/libyuv/post_processor_libyuv.cpp | 126 +++++++++++++++++++\n> > src/android/libyuv/post_processor_libyuv.h | 38 ++++++\n> > src/android/meson.build | 1 +\n> > 3 files changed, 165 insertions(+)\n> > create mode 100644 src/android/libyuv/post_processor_libyuv.cpp\n> > create mode 100644 src/android/libyuv/post_processor_libyuv.h\n> >\n> > diff --git a/src/android/libyuv/post_processor_libyuv.cpp b/src/android/libyuv/post_processor_libyuv.cpp\n> > new file mode 100644\n> > index 00000000..5f40fd25\n> > --- /dev/null\n> > +++ b/src/android/libyuv/post_processor_libyuv.cpp\n> > @@ -0,0 +1,126 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * post_processor_libyuv.cpp - Post Processor using libyuv\n> > + */\n> > +\n> > +#include \"post_processor_libyuv.h\"\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DEFINE_CATEGORY(LIBYUV)\n> > +\n> > +namespace {\n>\n> Missing blank line.\n>\n> > +void getNV12LengthAndStride(Size size, unsigned int length[2],\n> > + unsigned int stride[2])\n> > +{\n> > + const auto nv12Info = PixelFormatInfo::info(PixelFormat(formats::NV12));\n>\n> formats::NV12 is already a PixelFormat, so you can write\n>\n> const auto nv12Info = PixelFormatInfo::info(formats::NV12);\n>\n> I'd write out the type explicitly though:\n>\n> const PixelFormatInfo &nv12Info = PixelFormatInfo::info(formats::NV12);\n>\n> > + for (unsigned int i = 0; i < 2; i++) {\n> > + const unsigned int vertSubSample =\n> > + nv12Info.planes[i].verticalSubSampling;\n> > + length[i] = nv12Info.stride(size.width, i, /*align=*/1) *\n> > + ((size.height + vertSubSample - 1) / vertSubSample);\n> > + stride[i] = nv12Info.stride(size.width, i, 1);\n> > + }\n> > +}\n>\n> Missing blank line here too.\n>\n> > +} // namespace\n>\n> We could also make this a private static member function. For classes\n> exposed through the public API it could polute the public headers\n> unnecessarily, but for internal classes, it's less of an issue.\n>\n> > +\n> > +PostProcessorLibyuv::PostProcessorLibyuv() = default;\n>\n> Is this needed ? The compiler should generate a default constructor. You\n> could drop this line and the declaration of the constructor in the\n> class definition.\n>\n\nDone.\nThe answer is up to our code policy. I am used to obey this chromium\ncoding style rule.\n In this case, using default in the header file is okay as the class\nis sufficiently simple.\nhttps://www.chromium.org/developers/coding-style/chromium-style-checker-errors#TOC-Constructor-Destructor-Errors\n\nBest Regards,\n-Hiro\n\n\n> > +\n> > +int PostProcessorLibyuv::configure(const StreamConfiguration &inCfg,\n> > + const StreamConfiguration &outCfg)\n> > +{\n> > + if (inCfg.pixelFormat != outCfg.pixelFormat) {\n> > + LOG(LIBYUV, Error)\n> > + << \"Pixel format conversion is not supported\"\n> > + << \"-In: \" << inCfg.toString()\n> > + << \"-Out: \" << outCfg.toString();\n>\n> This will print something like\n>\n> \"Pixel format conversion is not supported-In: NV12-Out: NV24\"\n>\n> How about the following ?\n>\n> LOG(LIBYUV, Error)\n> << \"Pixel format conversion is not supported\"\n> << \" (from \" << inCfg.toString()\n> << \" to \" << outCfg.toString() << \")\";\n>\n> > + return -EINVAL;\n> > + }\n>\n> Blank line.\n>\n> > + if (inCfg.size < outCfg.size) {\n> > + LOG(LIBYUV, Error) << \"Up-scaling is not supported\"\n> > + << \", -In: \" << inCfg.toString()\n> > + << \", -Out: \" << outCfg.toString();\n>\n> Same here for the message.\n>\n> > + return -EINVAL;\n> > + }\n>\n> Here too.\n>\n> > + if (inCfg.pixelFormat == formats::NV12) {\n> > + LOG(LIBYUV, Error) << \"Only NV12 is supported\"\n> > + << \", -In: \" << inCfg.toString()\n> > + << \", -Out: \" << outCfg.toString();\n>\n> LOG(LIBYUV, Error)\n> << \"Unsupported format \" << inCfg.pixelFormat\n> << \" (only NV12 is supported)\";\n>\n> > + return -EINVAL;\n> > + }\n> > +\n> > + getNV12LengthAndStride(inCfg.size, sourceLength_, sourceStride_);\n> > + getNV12LengthAndStride(outCfg.size, destinationLength_,\n> > + destinationStride_);\n> > + return 0;\n> > +}\n> > +\n> > +int PostProcessorLibyuv::process(const FrameBuffer &source,\n> > + libcamera::MappedBuffer *destination,\n> > + const CameraMetadata & /*requestMetadata*/,\n> > + CameraMetadata * /*metadata*/)\n> > +{\n> > + if (!IsValidNV12Buffers(source, *destination)) {\n> > + LOG(LIBYUV, Error) << \"Invalid source and destination\";\n>\n> I think you could drop this message as IsValidNV12Buffers() already logs\n> errors.\n>\n> > + return -EINVAL;\n> > + }\n> > +\n> > + const MappedFrameBuffer sourceMapped(&source, PROT_READ);\n> > + if (!sourceMapped.isValid()) {\n> > + LOG(LIBYUV, Error) << \"Failed to mmap camera frame buffer\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + return 0 == libyuv::NV12Scale(sourceMapped.maps()[0].data(),\n> > + sourceStride_[0],\n> > + sourceMapped.maps()[1].data(),\n> > + sourceStride_[1],\n> > + sourceSize_.width, sourceSize_.height,\n> > + destination->maps()[0].data(),\n> > + destinationStride_[0],\n> > + destination->maps()[1].data(),\n> > + destinationStride_[1],\n> > + destinationSize_.width,\n> > + destinationSize_.height,\n> > + libyuv::FilterMode::kFilterBilinear);\n> > +}\n> > +\n> > +bool PostProcessorLibyuv::IsValidNV12Buffers(\n>\n> s/IsValidNV12Buffers/isValidNV12Buffers/\n>\n> > + const FrameBuffer &source,\n> > + const libcamera::MappedBuffer &destination) const\n> > +{\n> > + if (source.planes().size() != 2u) {\n> > + LOG(LIBYUV, Error) << \"The number of source planes is not 2\";\n> > + return false;\n> > + }\n> > + if (destination.maps().size() != 2u) {\n> > + LOG(LIBYUV, Error)\n> > + << \"The number of destination planes is not 2\";\n> > + return false;\n> > + }\n> > +\n> > + if (source.planes()[0].length < sourceLength_[0] ||\n> > + source.planes()[1].length < sourceLength_[1]) {\n> > + LOG(LIBYUV, Error)\n> > + << \"The source planes lengths are too small\";\n> > + return false;\n> > + }\n> > + if (destination.maps()[0].size() < destinationLength_[0] ||\n> > + destination.maps()[1].size() < destinationLength_[1]) {\n> > + LOG(LIBYUV, Error)\n> > + << \"The destination planes lengths are too small\";\n> > + return false;\n> > + }\n> > +\n> > + return true;\n> > +}\n> > diff --git a/src/android/libyuv/post_processor_libyuv.h b/src/android/libyuv/post_processor_libyuv.h\n> > new file mode 100644\n> > index 00000000..40c635a1\n> > --- /dev/null\n> > +++ b/src/android/libyuv/post_processor_libyuv.h\n> > @@ -0,0 +1,38 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * post_processor_libyuv.h - Post Processor using libyuv\n> > + */\n> > +#ifndef __ANDROID_POST_PROCESSOR_LIBYUV_H__\n> > +#define __ANDROID_POST_PROCESSOR_LIBYUV_H__\n> > +\n> > +#include \"../post_processor.h\"\n> > +\n> > +class CameraDevice;\n> > +\n> > +class PostProcessorLibyuv : public PostProcessor\n> > +{\n> > +public:\n> > + PostProcessorLibyuv();\n> > +\n> > + int configure(const libcamera::StreamConfiguration &incfg,\n> > + const libcamera::StreamConfiguration &outcfg) override;\n> > + int process(const libcamera::FrameBuffer &source,\n> > + libcamera::MappedBuffer *destination,\n> > + const CameraMetadata & /*requestMetadata*/,\n>\n> You can use [[maybe_unused]].\n>\n> > + CameraMetadata *metadata) override;\n> > +\n> > +private:\n> > + bool IsValidNV12Buffers(const libcamera::FrameBuffer &source,\n> > + const libcamera::MappedBuffer &destination) const;\n> > +\n> > + libcamera::Size sourceSize_;\n> > + libcamera::Size destinationSize_;\n> > + unsigned int sourceLength_[2] = {};\n> > + unsigned int destinationLength_[2] = {};\n> > + unsigned int sourceStride_[2] = {};\n> > + unsigned int destinationStride_[2] = {};\n> > +};\n> > +\n> > +#endif /* __ANDROID_POST_PROCESSOR_LIBYUV_H__ */\n> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > index 3d4d3be4..ff067d12 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -26,6 +26,7 @@ android_hal_sources = files([\n> > 'jpeg/exif.cpp',\n> > 'jpeg/post_processor_jpeg.cpp',\n> > 'jpeg/thumbnailer.cpp',\n> > + 'libyuv/post_processor_libyuv.cpp'\n> > ])\n> >\n> > android_camera_metadata_sources = files([\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 69553BD808\n\tfor ;\n\tThu, 28 Jan 2021 00:53:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9A516835F;\n\tThu, 28 Jan 2021 01:53:00 +0100 (CET)","from mail-ej1-x635.google.com (mail-ej1-x635.google.com\n\t[IPv6:2a00:1450:4864:20::635])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 30605682BC\n\tfor ;\n\tThu, 28 Jan 2021 01:52:58 +0100 (CET)","by mail-ej1-x635.google.com with SMTP id bl23so5315286ejb.5\n\tfor ;\n\tWed, 27 Jan 2021 16:52:58 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"GpL5iAaj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=O71ki7EvhzAI4Ma4eMjePZvETJ1SOVHo43WPHu+p6OA=;\n\tb=GpL5iAajkUBwCZ3Y4JL0jVLxVgEkmCa4idUPSs2zZ4U8Yb3sA5PdPyIhJ5VDOOO6za\n\toUD0EzsXz92VPa0ftwuBVTIAAUuIZBmnE1vPdtQLdpKMtMrK1rIGQXdspTs2C9oWZrOy\n\tjtRv7GJLtLIV9DyI8W67dYtcZfbtLLT3UHEyk=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=O71ki7EvhzAI4Ma4eMjePZvETJ1SOVHo43WPHu+p6OA=;\n\tb=NylZTWpXRpyS8ZiCv2RWfGfuDCgzlmwWGRD+dI9nkxpLp9SE7mB/7fzOsGojKieEpI\n\tj5jDlysKLVFfugzvIOrED5q0n+epGzENNK//C5MGgLsacX+8z6d5Sa53oYwiTIyrha51\n\tpB28MzlPrxX01loV+VUVw2yY5Zq86ncNmHkMcrplwg9/l2GbCBRQmScBwBVVLlXXvhBj\n\tyk85QNr2kOP+VX28teP5VdvaQFO4skp7RiT8bNXIzM+ez9k6NQ69KO4e6SyBezWvXTDA\n\tfFx7+vUMi95uv870EJkzoDNf5SOFU+QdZhqaPNaHs3vlGU0G2SEoC63TRzWT2NRyfOo8\n\tAKyQ==","X-Gm-Message-State":"AOAM532WByljvBRWCoZUcnKlefNnMmijb57wJjAstIY7L4DEYX3jwzn+\n\tHKHeuIP0WKf63Y5E4uW/iwxX9mHDPqT5MLF6e19ggw==","X-Google-Smtp-Source":"ABdhPJy1d44OY5i3cV1dRTj/PV5hCRCoa10Fzh7LkS/O/dtcEPEbcG/Lz/MAF+3xPisAQ5rmLC2vWHeG2pyJhWj3cD8=","X-Received":"by 2002:a17:906:804c:: with SMTP id\n\tx12mr8685854ejw.42.1611795177706; \n\tWed, 27 Jan 2021 16:52:57 -0800 (PST)","MIME-Version":"1.0","References":"<20210127044425.2193480-1-hiroh@chromium.org>\n\t<20210127044425.2193480-3-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 28 Jan 2021 09:52:46 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: libyuv: Introduce\n\tPostProcessorLibyuv","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14844,"web_url":"https://patchwork.libcamera.org/comment/14844/","msgid":"","date":"2021-01-28T15:50:58","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: libyuv: Introduce\n\tPostProcessorLibyuv","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Thu, Jan 28, 2021 at 09:52:46AM +0900, Hirokazu Honda wrote:\n> On Wed, Jan 27, 2021 at 8:39 PM Laurent Pinchart wrote:\n> > On Wed, Jan 27, 2021 at 04:44:25AM +0000, Hirokazu Honda wrote:\n> > > This adds PostProcessorLibyuv. It supports NV12 buffer scaling.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > >\n> > > ---\n> > > src/android/libyuv/post_processor_libyuv.cpp | 126 +++++++++++++++++++\n> > > src/android/libyuv/post_processor_libyuv.h | 38 ++++++\n> > > src/android/meson.build | 1 +\n> > > 3 files changed, 165 insertions(+)\n> > > create mode 100644 src/android/libyuv/post_processor_libyuv.cpp\n> > > create mode 100644 src/android/libyuv/post_processor_libyuv.h\n> > >\n> > > diff --git a/src/android/libyuv/post_processor_libyuv.cpp b/src/android/libyuv/post_processor_libyuv.cpp\n> > > new file mode 100644\n> > > index 00000000..5f40fd25\n> > > --- /dev/null\n> > > +++ b/src/android/libyuv/post_processor_libyuv.cpp\n> > > @@ -0,0 +1,126 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * post_processor_libyuv.cpp - Post Processor using libyuv\n> > > + */\n> > > +\n> > > +#include \"post_processor_libyuv.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DEFINE_CATEGORY(LIBYUV)\n> > > +\n> > > +namespace {\n> >\n> > Missing blank line.\n> >\n> > > +void getNV12LengthAndStride(Size size, unsigned int length[2],\n> > > + unsigned int stride[2])\n> > > +{\n> > > + const auto nv12Info = PixelFormatInfo::info(PixelFormat(formats::NV12));\n> >\n> > formats::NV12 is already a PixelFormat, so you can write\n> >\n> > const auto nv12Info = PixelFormatInfo::info(formats::NV12);\n> >\n> > I'd write out the type explicitly though:\n> >\n> > const PixelFormatInfo &nv12Info = PixelFormatInfo::info(formats::NV12);\n> >\n> > > + for (unsigned int i = 0; i < 2; i++) {\n> > > + const unsigned int vertSubSample =\n> > > + nv12Info.planes[i].verticalSubSampling;\n> > > + length[i] = nv12Info.stride(size.width, i, /*align=*/1) *\n> > > + ((size.height + vertSubSample - 1) / vertSubSample);\n> > > + stride[i] = nv12Info.stride(size.width, i, 1);\n> > > + }\n> > > +}\n> >\n> > Missing blank line here too.\n> >\n> > > +} // namespace\n> >\n> > We could also make this a private static member function. For classes\n> > exposed through the public API it could polute the public headers\n> > unnecessarily, but for internal classes, it's less of an issue.\n> >\n> > > +\n> > > +PostProcessorLibyuv::PostProcessorLibyuv() = default;\n> >\n> > Is this needed ? The compiler should generate a default constructor. You\n> > could drop this line and the declaration of the constructor in the\n> > class definition.\n> \n> Done.\n> The answer is up to our code policy. I am used to obey this chromium\n> coding style rule.\n> In this case, using default in the header file is okay as the class\n> is sufficiently simple.\n> https://www.chromium.org/developers/coding-style/chromium-style-checker-errors#TOC-Constructor-Destructor-Errors\n\nThanks for sharing the information, it's useful. Constructors (and\ndestructors) can definitely be large, even if a class looks simple (it's\none of the beauties of C++ :-)), and the fact that functions defined\ninline in the class definition are inline in C++, and thus duplicated in\ndifferent compilation units, even without explicit usage of the inline\nkeyword, can easily be overlooked and lead to code\nsize increase.\n\nI was however under the impression that defaulted constructors were not\nduplicated, but after reading the coding style guide, it occurred to me\nthat it could have been an incorrect assumption. I've written this very\nsimple test:\n\n---------- a.h --------\n#pragma once\n\nvoid a(const char *name);\n\n---------- b.h --------\n#pragma once\n\nvoid b(unsigned int value);\n\n---------- foo.h --------\n#pragma once\n\n#include \n\nclass Foo\n{\npublic:\n\tconst std::string &name() const;\n\tvoid setName(const std::string &name);\n\nprivate:\n\tstd::string name_;\n};\n\n---------- a.cpp --------\n#include \n\n#include \"foo.h\"\n\nvoid a(const char *name)\n{\n\tFoo f;\n\n\tf.setName(name);\n\tstd::cout << f.name() << std::endl;\n}\n\n---------- b.cpp --------\n#include \n\n#include \"foo.h\"\n\nvoid b(unsigned int value)\n{\n\tFoo f;\n\n\tf.setName(std::to_string(value));\n\tstd::cout << f.name() << std::endl;\n}\n\n---------- foo.cpp --------\n#include \"foo.h\"\n\nconst std::string &Foo::name() const\n{\n\treturn name_;\n}\n\nvoid Foo::setName(const std::string &name)\n{\n\tname_ = name;\n}\n\n---------- main.cpp --------\n#include \"a.h\"\n#include \"b.h\"\n\nint main()\n{\n\ta(\"bar\");\n\tb(42);\n\n\treturn 0;\n}\n\n---------- Makefile --------\n\nall: main\n\nmain: a.o b.o foo.o main.o\n\tg++ -W -Wall -o $@ $^\n%.o: %.cpp\n\tg++ -W -Wall -c -o $@ $<\n\n\nLooking at the output of objdump -d -C main, I see a single occurrence\nof Foo::Foo(), with two calls to the function, one in a() and one in\nb(). It thus looks like the compiler doesn't inline the implicitly\ndefined default constructor. Changing the Foo class definition to\n\nclass Foo\n{\npublic:\n\tFoo() = default;\n\n\tconst std::string &name() const;\n\tvoid setName(const std::string &name);\n\nprivate:\n\tstd::string name_;\n};\n\ndoesn't change the situation, and quite interestingly, neither does\n\nclass Foo\n{\npublic:\n\tFoo()\n\t{\n\t}\n\n\tconst std::string &name() const;\n\tvoid setName(const std::string &name);\n\nprivate:\n\tstd::string name_;\n};\n\nI wonder what I'm missing, and if my test is incorrect :-)\n\n> > > +\n> > > +int PostProcessorLibyuv::configure(const StreamConfiguration &inCfg,\n> > > + const StreamConfiguration &outCfg)\n> > > +{\n> > > + if (inCfg.pixelFormat != outCfg.pixelFormat) {\n> > > + LOG(LIBYUV, Error)\n> > > + << \"Pixel format conversion is not supported\"\n> > > + << \"-In: \" << inCfg.toString()\n> > > + << \"-Out: \" << outCfg.toString();\n> >\n> > This will print something like\n> >\n> > \"Pixel format conversion is not supported-In: NV12-Out: NV24\"\n> >\n> > How about the following ?\n> >\n> > LOG(LIBYUV, Error)\n> > << \"Pixel format conversion is not supported\"\n> > << \" (from \" << inCfg.toString()\n> > << \" to \" << outCfg.toString() << \")\";\n> >\n> > > + return -EINVAL;\n> > > + }\n> >\n> > Blank line.\n> >\n> > > + if (inCfg.size < outCfg.size) {\n> > > + LOG(LIBYUV, Error) << \"Up-scaling is not supported\"\n> > > + << \", -In: \" << inCfg.toString()\n> > > + << \", -Out: \" << outCfg.toString();\n> >\n> > Same here for the message.\n> >\n> > > + return -EINVAL;\n> > > + }\n> >\n> > Here too.\n> >\n> > > + if (inCfg.pixelFormat == formats::NV12) {\n> > > + LOG(LIBYUV, Error) << \"Only NV12 is supported\"\n> > > + << \", -In: \" << inCfg.toString()\n> > > + << \", -Out: \" << outCfg.toString();\n> >\n> > LOG(LIBYUV, Error)\n> > << \"Unsupported format \" << inCfg.pixelFormat\n> > << \" (only NV12 is supported)\";\n> >\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + getNV12LengthAndStride(inCfg.size, sourceLength_, sourceStride_);\n> > > + getNV12LengthAndStride(outCfg.size, destinationLength_,\n> > > + destinationStride_);\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int PostProcessorLibyuv::process(const FrameBuffer &source,\n> > > + libcamera::MappedBuffer *destination,\n> > > + const CameraMetadata & /*requestMetadata*/,\n> > > + CameraMetadata * /*metadata*/)\n> > > +{\n> > > + if (!IsValidNV12Buffers(source, *destination)) {\n> > > + LOG(LIBYUV, Error) << \"Invalid source and destination\";\n> >\n> > I think you could drop this message as IsValidNV12Buffers() already logs\n> > errors.\n> >\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + const MappedFrameBuffer sourceMapped(&source, PROT_READ);\n> > > + if (!sourceMapped.isValid()) {\n> > > + LOG(LIBYUV, Error) << \"Failed to mmap camera frame buffer\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + return 0 == libyuv::NV12Scale(sourceMapped.maps()[0].data(),\n> > > + sourceStride_[0],\n> > > + sourceMapped.maps()[1].data(),\n> > > + sourceStride_[1],\n> > > + sourceSize_.width, sourceSize_.height,\n> > > + destination->maps()[0].data(),\n> > > + destinationStride_[0],\n> > > + destination->maps()[1].data(),\n> > > + destinationStride_[1],\n> > > + destinationSize_.width,\n> > > + destinationSize_.height,\n> > > + libyuv::FilterMode::kFilterBilinear);\n> > > +}\n> > > +\n> > > +bool PostProcessorLibyuv::IsValidNV12Buffers(\n> >\n> > s/IsValidNV12Buffers/isValidNV12Buffers/\n> >\n> > > + const FrameBuffer &source,\n> > > + const libcamera::MappedBuffer &destination) const\n> > > +{\n> > > + if (source.planes().size() != 2u) {\n> > > + LOG(LIBYUV, Error) << \"The number of source planes is not 2\";\n> > > + return false;\n> > > + }\n> > > + if (destination.maps().size() != 2u) {\n> > > + LOG(LIBYUV, Error)\n> > > + << \"The number of destination planes is not 2\";\n> > > + return false;\n> > > + }\n> > > +\n> > > + if (source.planes()[0].length < sourceLength_[0] ||\n> > > + source.planes()[1].length < sourceLength_[1]) {\n> > > + LOG(LIBYUV, Error)\n> > > + << \"The source planes lengths are too small\";\n> > > + return false;\n> > > + }\n> > > + if (destination.maps()[0].size() < destinationLength_[0] ||\n> > > + destination.maps()[1].size() < destinationLength_[1]) {\n> > > + LOG(LIBYUV, Error)\n> > > + << \"The destination planes lengths are too small\";\n> > > + return false;\n> > > + }\n> > > +\n> > > + return true;\n> > > +}\n> > > diff --git a/src/android/libyuv/post_processor_libyuv.h b/src/android/libyuv/post_processor_libyuv.h\n> > > new file mode 100644\n> > > index 00000000..40c635a1\n> > > --- /dev/null\n> > > +++ b/src/android/libyuv/post_processor_libyuv.h\n> > > @@ -0,0 +1,38 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * post_processor_libyuv.h - Post Processor using libyuv\n> > > + */\n> > > +#ifndef __ANDROID_POST_PROCESSOR_LIBYUV_H__\n> > > +#define __ANDROID_POST_PROCESSOR_LIBYUV_H__\n> > > +\n> > > +#include \"../post_processor.h\"\n> > > +\n> > > +class CameraDevice;\n> > > +\n> > > +class PostProcessorLibyuv : public PostProcessor\n> > > +{\n> > > +public:\n> > > + PostProcessorLibyuv();\n> > > +\n> > > + int configure(const libcamera::StreamConfiguration &incfg,\n> > > + const libcamera::StreamConfiguration &outcfg) override;\n> > > + int process(const libcamera::FrameBuffer &source,\n> > > + libcamera::MappedBuffer *destination,\n> > > + const CameraMetadata & /*requestMetadata*/,\n> >\n> > You can use [[maybe_unused]].\n> >\n> > > + CameraMetadata *metadata) override;\n> > > +\n> > > +private:\n> > > + bool IsValidNV12Buffers(const libcamera::FrameBuffer &source,\n> > > + const libcamera::MappedBuffer &destination) const;\n> > > +\n> > > + libcamera::Size sourceSize_;\n> > > + libcamera::Size destinationSize_;\n> > > + unsigned int sourceLength_[2] = {};\n> > > + unsigned int destinationLength_[2] = {};\n> > > + unsigned int sourceStride_[2] = {};\n> > > + unsigned int destinationStride_[2] = {};\n> > > +};\n> > > +\n> > > +#endif /* __ANDROID_POST_PROCESSOR_LIBYUV_H__ */\n> > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > index 3d4d3be4..ff067d12 100644\n> > > --- a/src/android/meson.build\n> > > +++ b/src/android/meson.build\n> > > @@ -26,6 +26,7 @@ android_hal_sources = files([\n> > > 'jpeg/exif.cpp',\n> > > 'jpeg/post_processor_jpeg.cpp',\n> > > 'jpeg/thumbnailer.cpp',\n> > > + 'libyuv/post_processor_libyuv.cpp'\n> > > ])\n> > >\n> > > android_camera_metadata_sources = files([","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 D92C4C33BB\n\tfor ;\n\tThu, 28 Jan 2021 15:51:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 293CF6838F;\n\tThu, 28 Jan 2021 16:51:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9CE266030A\n\tfor ;\n\tThu, 28 Jan 2021 16:51:18 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F2A1658E;\n\tThu, 28 Jan 2021 16:51:17 +0100 (CET)"],"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=\"M82lcEjQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611849078;\n\tbh=kbZJtZha919+t/ZLY9l/LZFR36Oh8gzWq9jCAi3HFi8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M82lcEjQVeNQV4IEVjBWLBWxNUrFZeYXKIWbE3v8Ds5U7G9ycpbBQaCC2a8+9JALF\n\t16uyXgByxC+RPIJyC5QPeq2DfnH82+t//vtElqkonwyETpjtyZtwfMAFZDe8gZD+7f\n\tRvrF4EJY3oQgGRIML4BTb4mH2T8jQ4HKkoaSD9kY=","Date":"Thu, 28 Jan 2021 17:50:58 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210127044425.2193480-1-hiroh@chromium.org>\n\t<20210127044425.2193480-3-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: libyuv: Introduce\n\tPostProcessorLibyuv","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14846,"web_url":"https://patchwork.libcamera.org/comment/14846/","msgid":"","date":"2021-01-28T22:38:22","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: libyuv: Introduce\n\tPostProcessorLibyuv","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Fri, Jan 29, 2021 at 12:51 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Thu, Jan 28, 2021 at 09:52:46AM +0900, Hirokazu Honda wrote:\n> > On Wed, Jan 27, 2021 at 8:39 PM Laurent Pinchart wrote:\n> > > On Wed, Jan 27, 2021 at 04:44:25AM +0000, Hirokazu Honda wrote:\n> > > > This adds PostProcessorLibyuv. It supports NV12 buffer scaling.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > >\n> > > > ---\n> > > > src/android/libyuv/post_processor_libyuv.cpp | 126 +++++++++++++++++++\n> > > > src/android/libyuv/post_processor_libyuv.h | 38 ++++++\n> > > > src/android/meson.build | 1 +\n> > > > 3 files changed, 165 insertions(+)\n> > > > create mode 100644 src/android/libyuv/post_processor_libyuv.cpp\n> > > > create mode 100644 src/android/libyuv/post_processor_libyuv.h\n> > > >\n> > > > diff --git a/src/android/libyuv/post_processor_libyuv.cpp b/src/android/libyuv/post_processor_libyuv.cpp\n> > > > new file mode 100644\n> > > > index 00000000..5f40fd25\n> > > > --- /dev/null\n> > > > +++ b/src/android/libyuv/post_processor_libyuv.cpp\n> > > > @@ -0,0 +1,126 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * post_processor_libyuv.cpp - Post Processor using libyuv\n> > > > + */\n> > > > +\n> > > > +#include \"post_processor_libyuv.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(LIBYUV)\n> > > > +\n> > > > +namespace {\n> > >\n> > > Missing blank line.\n> > >\n> > > > +void getNV12LengthAndStride(Size size, unsigned int length[2],\n> > > > + unsigned int stride[2])\n> > > > +{\n> > > > + const auto nv12Info = PixelFormatInfo::info(PixelFormat(formats::NV12));\n> > >\n> > > formats::NV12 is already a PixelFormat, so you can write\n> > >\n> > > const auto nv12Info = PixelFormatInfo::info(formats::NV12);\n> > >\n> > > I'd write out the type explicitly though:\n> > >\n> > > const PixelFormatInfo &nv12Info = PixelFormatInfo::info(formats::NV12);\n> > >\n> > > > + for (unsigned int i = 0; i < 2; i++) {\n> > > > + const unsigned int vertSubSample =\n> > > > + nv12Info.planes[i].verticalSubSampling;\n> > > > + length[i] = nv12Info.stride(size.width, i, /*align=*/1) *\n> > > > + ((size.height + vertSubSample - 1) / vertSubSample);\n> > > > + stride[i] = nv12Info.stride(size.width, i, 1);\n> > > > + }\n> > > > +}\n> > >\n> > > Missing blank line here too.\n> > >\n> > > > +} // namespace\n> > >\n> > > We could also make this a private static member function. For classes\n> > > exposed through the public API it could polute the public headers\n> > > unnecessarily, but for internal classes, it's less of an issue.\n> > >\n> > > > +\n> > > > +PostProcessorLibyuv::PostProcessorLibyuv() = default;\n> > >\n> > > Is this needed ? The compiler should generate a default constructor. You\n> > > could drop this line and the declaration of the constructor in the\n> > > class definition.\n> >\n> > Done.\n> > The answer is up to our code policy. I am used to obey this chromium\n> > coding style rule.\n> > In this case, using default in the header file is okay as the class\n> > is sufficiently simple.\n> > https://www.chromium.org/developers/coding-style/chromium-style-checker-errors#TOC-Constructor-Destructor-Errors\n>\n> Thanks for sharing the information, it's useful. Constructors (and\n> destructors) can definitely be large, even if a class looks simple (it's\n> one of the beauties of C++ :-)), and the fact that functions defined\n> inline in the class definition are inline in C++, and thus duplicated in\n> different compilation units, even without explicit usage of the inline\n> keyword, can easily be overlooked and lead to code\n> size increase.\n>\n> I was however under the impression that defaulted constructors were not\n> duplicated, but after reading the coding style guide, it occurred to me\n> that it could have been an incorrect assumption. I've written this very\n> simple test:\n>\n> ---------- a.h --------\n> #pragma once\n>\n> void a(const char *name);\n>\n> ---------- b.h --------\n> #pragma once\n>\n> void b(unsigned int value);\n>\n> ---------- foo.h --------\n> #pragma once\n>\n> #include \n>\n> class Foo\n> {\n> public:\n> const std::string &name() const;\n> void setName(const std::string &name);\n>\n> private:\n> std::string name_;\n> };\n>\n> ---------- a.cpp --------\n> #include \n>\n> #include \"foo.h\"\n>\n> void a(const char *name)\n> {\n> Foo f;\n>\n> f.setName(name);\n> std::cout << f.name() << std::endl;\n> }\n>\n> ---------- b.cpp --------\n> #include \n>\n> #include \"foo.h\"\n>\n> void b(unsigned int value)\n> {\n> Foo f;\n>\n> f.setName(std::to_string(value));\n> std::cout << f.name() << std::endl;\n> }\n>\n> ---------- foo.cpp --------\n> #include \"foo.h\"\n>\n> const std::string &Foo::name() const\n> {\n> return name_;\n> }\n>\n> void Foo::setName(const std::string &name)\n> {\n> name_ = name;\n> }\n>\n> ---------- main.cpp --------\n> #include \"a.h\"\n> #include \"b.h\"\n>\n> int main()\n> {\n> a(\"bar\");\n> b(42);\n>\n> return 0;\n> }\n>\n> ---------- Makefile --------\n>\n> all: main\n>\n> main: a.o b.o foo.o main.o\n> g++ -W -Wall -o $@ $^\n> %.o: %.cpp\n> g++ -W -Wall -c -o $@ $<\n>\n>\n> Looking at the output of objdump -d -C main, I see a single occurrence\n> of Foo::Foo(), with two calls to the function, one in a() and one in\n> b(). It thus looks like the compiler doesn't inline the implicitly\n> defined default constructor. Changing the Foo class definition to\n>\n> class Foo\n> {\n> public:\n> Foo() = default;\n>\n> const std::string &name() const;\n> void setName(const std::string &name);\n>\n> private:\n> std::string name_;\n> };\n>\n> doesn't change the situation, and quite interestingly, neither does\n>\n> class Foo\n> {\n> public:\n> Foo()\n> {\n> }\n>\n> const std::string &name() const;\n> void setName(const std::string &name);\n>\n> private:\n> std::string name_;\n> };\n>\n> I wonder what I'm missing, and if my test is incorrect :-)\n>\n\nThe \"inline\" keyword is basically a compiler hint.\nA function with \"inline\" is not necessarily inlined.\nAccording to https://stackoverflow.com/a/21322, putting a function\nwithin a class definition has the same effect as using \"inline\"\nkeyword.\n\nPerhaps, does the result change with -O options?\nI am happy to continue discussing here. Since this may not change the\ncode so much, let me upload the next patch series.\nThanks.\n\nBest Regards,\n-Hiro\n> > > > +\n> > > > +int PostProcessorLibyuv::configure(const StreamConfiguration &inCfg,\n> > > > + const StreamConfiguration &outCfg)\n> > > > +{\n> > > > + if (inCfg.pixelFormat != outCfg.pixelFormat) {\n> > > > + LOG(LIBYUV, Error)\n> > > > + << \"Pixel format conversion is not supported\"\n> > > > + << \"-In: \" << inCfg.toString()\n> > > > + << \"-Out: \" << outCfg.toString();\n> > >\n> > > This will print something like\n> > >\n> > > \"Pixel format conversion is not supported-In: NV12-Out: NV24\"\n> > >\n> > > How about the following ?\n> > >\n> > > LOG(LIBYUV, Error)\n> > > << \"Pixel format conversion is not supported\"\n> > > << \" (from \" << inCfg.toString()\n> > > << \" to \" << outCfg.toString() << \")\";\n> > >\n> > > > + return -EINVAL;\n> > > > + }\n> > >\n> > > Blank line.\n> > >\n> > > > + if (inCfg.size < outCfg.size) {\n> > > > + LOG(LIBYUV, Error) << \"Up-scaling is not supported\"\n> > > > + << \", -In: \" << inCfg.toString()\n> > > > + << \", -Out: \" << outCfg.toString();\n> > >\n> > > Same here for the message.\n> > >\n> > > > + return -EINVAL;\n> > > > + }\n> > >\n> > > Here too.\n> > >\n> > > > + if (inCfg.pixelFormat == formats::NV12) {\n> > > > + LOG(LIBYUV, Error) << \"Only NV12 is supported\"\n> > > > + << \", -In: \" << inCfg.toString()\n> > > > + << \", -Out: \" << outCfg.toString();\n> > >\n> > > LOG(LIBYUV, Error)\n> > > << \"Unsupported format \" << inCfg.pixelFormat\n> > > << \" (only NV12 is supported)\";\n> > >\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + getNV12LengthAndStride(inCfg.size, sourceLength_, sourceStride_);\n> > > > + getNV12LengthAndStride(outCfg.size, destinationLength_,\n> > > > + destinationStride_);\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int PostProcessorLibyuv::process(const FrameBuffer &source,\n> > > > + libcamera::MappedBuffer *destination,\n> > > > + const CameraMetadata & /*requestMetadata*/,\n> > > > + CameraMetadata * /*metadata*/)\n> > > > +{\n> > > > + if (!IsValidNV12Buffers(source, *destination)) {\n> > > > + LOG(LIBYUV, Error) << \"Invalid source and destination\";\n> > >\n> > > I think you could drop this message as IsValidNV12Buffers() already logs\n> > > errors.\n> > >\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + const MappedFrameBuffer sourceMapped(&source, PROT_READ);\n> > > > + if (!sourceMapped.isValid()) {\n> > > > + LOG(LIBYUV, Error) << \"Failed to mmap camera frame buffer\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + return 0 == libyuv::NV12Scale(sourceMapped.maps()[0].data(),\n> > > > + sourceStride_[0],\n> > > > + sourceMapped.maps()[1].data(),\n> > > > + sourceStride_[1],\n> > > > + sourceSize_.width, sourceSize_.height,\n> > > > + destination->maps()[0].data(),\n> > > > + destinationStride_[0],\n> > > > + destination->maps()[1].data(),\n> > > > + destinationStride_[1],\n> > > > + destinationSize_.width,\n> > > > + destinationSize_.height,\n> > > > + libyuv::FilterMode::kFilterBilinear);\n> > > > +}\n> > > > +\n> > > > +bool PostProcessorLibyuv::IsValidNV12Buffers(\n> > >\n> > > s/IsValidNV12Buffers/isValidNV12Buffers/\n> > >\n> > > > + const FrameBuffer &source,\n> > > > + const libcamera::MappedBuffer &destination) const\n> > > > +{\n> > > > + if (source.planes().size() != 2u) {\n> > > > + LOG(LIBYUV, Error) << \"The number of source planes is not 2\";\n> > > > + return false;\n> > > > + }\n> > > > + if (destination.maps().size() != 2u) {\n> > > > + LOG(LIBYUV, Error)\n> > > > + << \"The number of destination planes is not 2\";\n> > > > + return false;\n> > > > + }\n> > > > +\n> > > > + if (source.planes()[0].length < sourceLength_[0] ||\n> > > > + source.planes()[1].length < sourceLength_[1]) {\n> > > > + LOG(LIBYUV, Error)\n> > > > + << \"The source planes lengths are too small\";\n> > > > + return false;\n> > > > + }\n> > > > + if (destination.maps()[0].size() < destinationLength_[0] ||\n> > > > + destination.maps()[1].size() < destinationLength_[1]) {\n> > > > + LOG(LIBYUV, Error)\n> > > > + << \"The destination planes lengths are too small\";\n> > > > + return false;\n> > > > + }\n> > > > +\n> > > > + return true;\n> > > > +}\n> > > > diff --git a/src/android/libyuv/post_processor_libyuv.h b/src/android/libyuv/post_processor_libyuv.h\n> > > > new file mode 100644\n> > > > index 00000000..40c635a1\n> > > > --- /dev/null\n> > > > +++ b/src/android/libyuv/post_processor_libyuv.h\n> > > > @@ -0,0 +1,38 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * post_processor_libyuv.h - Post Processor using libyuv\n> > > > + */\n> > > > +#ifndef __ANDROID_POST_PROCESSOR_LIBYUV_H__\n> > > > +#define __ANDROID_POST_PROCESSOR_LIBYUV_H__\n> > > > +\n> > > > +#include \"../post_processor.h\"\n> > > > +\n> > > > +class CameraDevice;\n> > > > +\n> > > > +class PostProcessorLibyuv : public PostProcessor\n> > > > +{\n> > > > +public:\n> > > > + PostProcessorLibyuv();\n> > > > +\n> > > > + int configure(const libcamera::StreamConfiguration &incfg,\n> > > > + const libcamera::StreamConfiguration &outcfg) override;\n> > > > + int process(const libcamera::FrameBuffer &source,\n> > > > + libcamera::MappedBuffer *destination,\n> > > > + const CameraMetadata & /*requestMetadata*/,\n> > >\n> > > You can use [[maybe_unused]].\n> > >\n> > > > + CameraMetadata *metadata) override;\n> > > > +\n> > > > +private:\n> > > > + bool IsValidNV12Buffers(const libcamera::FrameBuffer &source,\n> > > > + const libcamera::MappedBuffer &destination) const;\n> > > > +\n> > > > + libcamera::Size sourceSize_;\n> > > > + libcamera::Size destinationSize_;\n> > > > + unsigned int sourceLength_[2] = {};\n> > > > + unsigned int destinationLength_[2] = {};\n> > > > + unsigned int sourceStride_[2] = {};\n> > > > + unsigned int destinationStride_[2] = {};\n> > > > +};\n> > > > +\n> > > > +#endif /* __ANDROID_POST_PROCESSOR_LIBYUV_H__ */\n> > > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > > index 3d4d3be4..ff067d12 100644\n> > > > --- a/src/android/meson.build\n> > > > +++ b/src/android/meson.build\n> > > > @@ -26,6 +26,7 @@ android_hal_sources = files([\n> > > > 'jpeg/exif.cpp',\n> > > > 'jpeg/post_processor_jpeg.cpp',\n> > > > 'jpeg/thumbnailer.cpp',\n> > > > + 'libyuv/post_processor_libyuv.cpp'\n> > > > ])\n> > > >\n> > > > android_camera_metadata_sources = files([\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 A1E91C33BB\n\tfor ;\n\tThu, 28 Jan 2021 22:38:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A9966838E;\n\tThu, 28 Jan 2021 23:38:34 +0100 (CET)","from mail-ed1-x530.google.com (mail-ed1-x530.google.com\n\t[IPv6:2a00:1450:4864:20::530])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19F096030A\n\tfor ;\n\tThu, 28 Jan 2021 23:38:33 +0100 (CET)","by mail-ed1-x530.google.com with SMTP id d2so8500832edz.3\n\tfor ;\n\tThu, 28 Jan 2021 14:38:33 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"ePyBIJEn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=L64qQHQG94fnYI4lDzdimRkBrShPXu8apAUq3qtooqQ=;\n\tb=ePyBIJEn6Xukd2lFWQNhlcJ5wGPOGAQLEmDunT5hDQ8JoVO2SYd5Fl9PnOj09JVido\n\tBwKWduWsIPU+v7ly6q3xFjOQUxBmRyuAeHvZ5+c5f8vNjRg1xomlP2v1JYZ3sFmzJvJ/\n\tXRnZP34gniC4nKU4vPxNuNyt7yopukWIFNiQM=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=L64qQHQG94fnYI4lDzdimRkBrShPXu8apAUq3qtooqQ=;\n\tb=JY3pN/93Gm6ahiuvv2qr8qqL7aNo0eHeJFGWmsXPB8jIsUAyk9bHv6OAzegQybbW6Y\n\tE4rlSLnpQoa3iwzwr5dZheyuDlV8Olz2imRYi3hgvlAb41+SA6k+TtN+bLsFdzbIjwtt\n\tPGYlprlhwwuusGkl872fde7fXh/Bx37bQQWfOkaG5ayWSL5loqMLyCSXhGwI/OvZmKTF\n\tVgBCsdAv+cbbkmx1anG06SPwwKFztg+aHpJVSWvlXK9X66p7G6EHQNgog0/4FEEB7Hzy\n\tIs991AtySGYAsDPwZVP5TEEW1OYlWd3X/NswFnkz68q1hpJSPz8pftYRCZQk3K/xzJrw\n\tOAbA==","X-Gm-Message-State":"AOAM5320n8cswfQOor9/+glo6y26WdbKHyifnQYW6pbllm6e2byBp5l2\n\tj5y62XZ359ZXB8PmktYQB6xRjPAjk52jyfA9Iq/ZZg==","X-Google-Smtp-Source":"ABdhPJzlUFdv8UKUeLM4p3HezGKqUaNIMIb452Qa8oTczq4S6T41oqxDS95kTvsavgjhJUmzcimrF41NdE/VtDZ+HnQ=","X-Received":"by 2002:aa7:c983:: with SMTP id c3mr2013034edt.327.1611873512553;\n\tThu, 28 Jan 2021 14:38:32 -0800 (PST)","MIME-Version":"1.0","References":"<20210127044425.2193480-1-hiroh@chromium.org>\n\t<20210127044425.2193480-3-hiroh@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 29 Jan 2021 07:38:22 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: libyuv: Introduce\n\tPostProcessorLibyuv","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]