[{"id":14870,"web_url":"https://patchwork.libcamera.org/comment/14870/","msgid":"","date":"2021-02-01T06:50:22","subject":"Re: [libcamera-devel] [PATCH v3 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":"Laurent, can you review this patch and merge if there is no issue?\n\nThanks so much in advance,\n\nBest Regards,\n-Hiro\n\nOn Fri, Jan 29, 2021 at 7:42 AM Hirokazu Honda wrote:\n>\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 | 123 +++++++++++++++++++\n> src/android/libyuv/post_processor_libyuv.h | 44 +++++++\n> src/android/meson.build | 1 +\n> 3 files changed, 168 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..81f237e0\n> --- /dev/null\n> +++ b/src/android/libyuv/post_processor_libyuv.cpp\n> @@ -0,0 +1,123 @@\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> +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> + << \" (from \" << inCfg.toString()\n> + << \" to \" << outCfg.toString() << \")\";\n> + return -EINVAL;\n> + }\n> +\n> + if (inCfg.size < outCfg.size) {\n> + LOG(LIBYUV, Error) << \"Up-scaling is not supported\"\n> + << \" (from \" << inCfg.toString()\n> + << \" to \" << outCfg.toString() << \")\";\n> + return -EINVAL;\n> + }\n> +\n> + if (inCfg.pixelFormat == formats::NV12) {\n> + LOG(LIBYUV, Error) << \"Unsupported format \" << inCfg.pixelFormat\n> + << \" (only NV12 is supported)\";\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> + [[maybe_unused]] const CameraMetadata &requestMetadata,\n> + [[maybe_unused]] CameraMetadata *metadata)\n> +{\n> + if (!isValidNV12Buffers(source, *destination))\n> + return -EINVAL;\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> + 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> +\n> +// static\n> +void PostProcessorLibyuv::getNV12LengthAndStride(Size size,\n> + unsigned int length[2],\n> + unsigned int stride[2])\n> +{\n> + const PixelFormatInfo &nv12Info = PixelFormatInfo::info(formats::NV12);\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> 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..a65a16b3\n> --- /dev/null\n> +++ b/src/android/libyuv/post_processor_libyuv.h\n> @@ -0,0 +1,44 @@\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 \n> +\n> +#include \"../post_processor.h\"\n> +\n> +class CameraDevice;\n> +\n> +class PostProcessorLibyuv : public PostProcessor\n> +{\n> +public:\n> + PostProcessorLibyuv() = default;\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> + CameraMetadata *metadata) override;\n> +\n> +private:\n> + bool isValidNV12Buffers(const libcamera::FrameBuffer &source,\n> + const libcamera::MappedBuffer &destination) const;\n> +\n> + static void getNV12LengthAndStride(libcamera::Size size,\n> + unsigned int length[2],\n> + unsigned int stride[2]);\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 e1533d7c..cbe09200 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -44,6 +44,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> 2.30.0.365.g02bc693789-goog","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 99BBEBD808\n\tfor ;\n\tMon, 1 Feb 2021 06:50:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1429560107;\n\tMon, 1 Feb 2021 07:50:35 +0100 (CET)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 449CC60107\n\tfor ;\n\tMon, 1 Feb 2021 07:50:33 +0100 (CET)","by mail-ed1-x536.google.com with SMTP id y18so254591edw.13\n\tfor ;\n\tSun, 31 Jan 2021 22:50: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=\"Cgaae9ne\"; 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\tbh=m8dnxxc4PboHoJUyWlfp32WZQGBKQ22YZPJh8Db/6DU=;\n\tb=Cgaae9neob1zmY11rvHNE6ofSYUS2Zkhp6k68cl9EU8+IOjuB4ih6vCOeWOsHzrGEw\n\tufDT5vWREykstBlWeBRbrqOVI/FJtGu6S1OfGzKniz8toL7TzXM1ROM/ZS8Ki782qEnj\n\trMQ8/HTLtU74lyfrqINW4KyjOkohOhq22q4sY=","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;\n\tbh=m8dnxxc4PboHoJUyWlfp32WZQGBKQ22YZPJh8Db/6DU=;\n\tb=X98qjx/6AW/QBH/dbUhGI8ops4J5G3fu6kWBXoAYq0riq2zzd0vTqvq5U2kiYzIGiq\n\t/4u9oYo+76prbkccR39RibXRbOwEoRW2i23sPe2jkzCJjs4DHnJqBTiWagPRenZcDTxF\n\tATfufdU977niMST4w6avER1YEEOa2AjwssTEvm6OD+In+34bff7xCRt0b3NBeOIJ9D62\n\tLGF+naYg8xQXKGhLxHiFV2d5elnvDsvldHySTCvGWskx8wkhZaxguRkY30GiG06QjiUl\n\tueUum/pNilB9c6Eey5j6mn6VaCjRjWnU0P/jUunjXksr9kRggR1YaimsboeWuY+Z62PR\n\t8dxg==","X-Gm-Message-State":"AOAM530hoGVqDVAwz88W5+8P/6zbg89ZPPb/0K98Yk3uZ6/9iWTqh2kF\n\tI4uVSQUSHUyzvh0Gt54mpuyF6RxZd+cXfBSyReWMY18vq1Q=","X-Google-Smtp-Source":"ABdhPJyMRkFAbdrnni//sYMqJR4MPjcHisTi2F0Abz7OjAFQiFPjF7+Lhqgk252YwR5cua513k80OOt5C4pactpBheQ=","X-Received":"by 2002:a05:6402:40c4:: with SMTP id\n\tz4mr17208319edb.233.1612162232499; \n\tSun, 31 Jan 2021 22:50:32 -0800 (PST)","MIME-Version":"1.0","References":"<20210128224217.2919790-1-hiroh@chromium.org>\n\t<20210128224217.2919790-3-hiroh@chromium.org>","In-Reply-To":"<20210128224217.2919790-3-hiroh@chromium.org>","From":"Hirokazu Honda ","Date":"Mon, 1 Feb 2021 15:50:22 +0900","Message-ID":"","To":"libcamera devel , \n\tLaurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14880,"web_url":"https://patchwork.libcamera.org/comment/14880/","msgid":"<20210201115345.ct2kod5phhsefddj@uno.localdomain>","date":"2021-02-01T11:53:45","subject":"Re: [libcamera-devel] [PATCH v3 2/2] android: libyuv: Introduce\n\tPostProcessorLibyuv","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Thu, Jan 28, 2021 at 10:42:16PM +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 | 123 +++++++++++++++++++\n> src/android/libyuv/post_processor_libyuv.h | 44 +++++++\n\nwe have\n src/android/jpeg\nwhich used libjpeg, but that's not reflected in the directory name\n\nwould\n src/android/yuv/\nbe a better match ?\n\nsame question for the class and the associated filename, would\nPostProcessorYuv be better ?\n\n> src/android/meson.build | 1 +\n> 3 files changed, 168 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..81f237e0\n> --- /dev/null\n> +++ b/src/android/libyuv/post_processor_libyuv.cpp\n> @@ -0,0 +1,123 @@\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\nSame question as the usage of 'lib' in class and file name.\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<< \" (from \" << inCfg.toString()\n> +\t\t\t<< \" to \" << outCfg.toString() << \")\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (inCfg.size < outCfg.size) {\n> +\t\tLOG(LIBYUV, Error) << \"Up-scaling is not supported\"\n> +\t\t\t\t << \" (from \" << inCfg.toString()\n> +\t\t\t\t << \" to \" << outCfg.toString() << \")\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (inCfg.pixelFormat == formats::NV12) {\n> +\t\tLOG(LIBYUV, Error) << \"Unsupported format \" << inCfg.pixelFormat\n> +\t\t\t\t << \" (only NV12 is supported)\";\n\nShould the condition check for != insted of == ?\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 [[maybe_unused]] const CameraMetadata &requestMetadata,\n> +\t\t\t\t [[maybe_unused]] CameraMetadata *metadata)\n> +{\n> +\tif (!isValidNV12Buffers(source, *destination))\n> +\t\treturn -EINVAL;\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\nNice API libyuv! :/\n\n> +}\n> +\n> +bool PostProcessorLibyuv::isValidNV12Buffers(\n\nPlease drop NV12 from name.\nThis could be validateBuffers()\n\n> +\tconst FrameBuffer &source,\n\nFit on the previous line ?\n\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\nHere and above\n \"Invalid number of planes: \" << destination.maps().size();\n\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\nI would also print the actual size and the expected one to make the\nerror more useful for debug.\n\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> +\n> +// static\n\nC++ comment, and not of much value. Why does checkstyle not complain ?\n\n> +void PostProcessorLibyuv::getNV12LengthAndStride(Size size,\n> +\t\t\t\t\t\t unsigned int length[2],\n> +\t\t\t\t\t\t unsigned int stride[2])\n\nPlease drop NV12 from the function name, it will work for other\nformats too.\n\nThe lenghts and strides are class members this functions has access\nto, see the below suggestion on how to avoid passing them in.\n\n> +{\n> +\tconst PixelFormatInfo &nv12Info = PixelFormatInfo::info(formats::NV12);\n\nPass in the whole StreamConfig and use the pixelformat there contained\nto retrieve the info, do not assume NV12 even if it's the only\nsupported one, as as soon NV16 is added this will have to be changed.\n\nAll in all, I would make this\n\nvoid PostProcessorLibyuv::calculateLengths(const StreamConfiguration &inCfg,\n const StreamConfiguration &outCfg)\n{\n}\n\nand initialize both [source|destination][Length_|Stride_] in one go.\nAfter all you call the function two times one after the other. This\nway you won't have to pass paramters in.\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\nA comment inside a function call is rather unusual in the library code\nbase.\n\n> +\t\t\t ((size.height + vertSubSample - 1) / vertSubSample);\n\nThis might be a good occasion to create a PixelFormatInfo::planeSize()\nas we already have frameSize that performs the same calculation on all\nplanes. Not required for this patch though.\n\n> +\t\tstride[i] = nv12Info.stride(size.width, i, 1);\n> +\t}\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..a65a16b3\n> --- /dev/null\n> +++ b/src/android/libyuv/post_processor_libyuv.h\n> @@ -0,0 +1,44 @@\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 \n> +\n> +#include \"../post_processor.h\"\n> +\n> +class CameraDevice;\n> +\n> +class PostProcessorLibyuv : public PostProcessor\n> +{\n> +public:\n> +\tPostProcessorLibyuv() = default;\n\nThis looks weird, but I recall you and Laurent discussed this lenght, so I\npresume it's ok.\n\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\nwhy is requestMetadata commented ?\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> +\tstatic void getNV12LengthAndStride(libcamera::Size size,\n> +\t\t\t\t\t unsigned int length[2],\n> +\t\t\t\t\t unsigned int stride[2]);\n\nWhy static if it operates on class members ?\n\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\nClass member initialization is something we don't often do in\nlibcamera. Not sure why, it's a C++11 feature, so we should have\nsupported it from the very beginning. Not sure, just pointing it out.\n\nThanks\n j\n\n> +};\n> +\n> +#endif /* __ANDROID_POST_PROCESSOR_LIBYUV_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index e1533d7c..cbe09200 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -44,6 +44,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> 2.30.0.365.g02bc693789-goog\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 584EEBD808\n\tfor ;\n\tMon, 1 Feb 2021 11:53:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E261D68400;\n\tMon, 1 Feb 2021 12:53:25 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2CC5168374\n\tfor ;\n\tMon, 1 Feb 2021 12:53:24 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 78A2D100004;\n\tMon, 1 Feb 2021 11:53:23 +0000 (UTC)"],"Date":"Mon, 1 Feb 2021 12:53:45 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210201115345.ct2kod5phhsefddj@uno.localdomain>","References":"<20210128224217.2919790-1-hiroh@chromium.org>\n\t<20210128224217.2919790-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210128224217.2919790-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 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":14901,"web_url":"https://patchwork.libcamera.org/comment/14901/","msgid":"","date":"2021-02-02T07:26:54","subject":"Re: [libcamera-devel] [PATCH v3 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 Jacopo,\n\nThanks for reviewing!\n\nOn Mon, Feb 1, 2021 at 8:53 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Thu, Jan 28, 2021 at 10:42:16PM +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 | 123 +++++++++++++++++++\n> > src/android/libyuv/post_processor_libyuv.h | 44 +++++++\n>\n> we have\n> src/android/jpeg\n> which used libjpeg, but that's not reflected in the directory name\n>\n> would\n> src/android/yuv/\n> be a better match ?\n>\n> same question for the class and the associated filename, would\n> PostProcessorYuv be better ?\n>\n> > src/android/meson.build | 1 +\n> > 3 files changed, 168 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..81f237e0\n> > --- /dev/null\n> > +++ b/src/android/libyuv/post_processor_libyuv.cpp\n> > @@ -0,0 +1,123 @@\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> Same question as the usage of 'lib' in class and file name.\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> > + << \" (from \" << inCfg.toString()\n> > + << \" to \" << outCfg.toString() << \")\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + if (inCfg.size < outCfg.size) {\n> > + LOG(LIBYUV, Error) << \"Up-scaling is not supported\"\n> > + << \" (from \" << inCfg.toString()\n> > + << \" to \" << outCfg.toString() << \")\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + if (inCfg.pixelFormat == formats::NV12) {\n> > + LOG(LIBYUV, Error) << \"Unsupported format \" << inCfg.pixelFormat\n> > + << \" (only NV12 is supported)\";\n>\n> Should the condition check for != insted of == ?\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> > + [[maybe_unused]] const CameraMetadata &requestMetadata,\n> > + [[maybe_unused]] CameraMetadata *metadata)\n> > +{\n> > + if (!isValidNV12Buffers(source, *destination))\n> > + return -EINVAL;\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> Nice API libyuv! :/\n>\n> > +}\n> > +\n> > +bool PostProcessorLibyuv::isValidNV12Buffers(\n>\n> Please drop NV12 from name.\n> This could be validateBuffers()\n>\n> > + const FrameBuffer &source,\n>\n> Fit on the previous line ?\n>\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>\n> Here and above\n> \"Invalid number of planes: \" << destination.maps().size();\n>\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>\n> I would also print the actual size and the expected one to make the\n> error more useful for debug.\n>\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> > +\n> > +// static\n>\n> C++ comment, and not of much value. Why does checkstyle not complain ?\n>\n> > +void PostProcessorLibyuv::getNV12LengthAndStride(Size size,\n> > + unsigned int length[2],\n> > + unsigned int stride[2])\n>\n> Please drop NV12 from the function name, it will work for other\n> formats too.\n>\n> The lenghts and strides are class members this functions has access\n> to, see the below suggestion on how to avoid passing them in.\n>\n> > +{\n> > + const PixelFormatInfo &nv12Info = PixelFormatInfo::info(formats::NV12);\n>\n> Pass in the whole StreamConfig and use the pixelformat there contained\n> to retrieve the info, do not assume NV12 even if it's the only\n> supported one, as as soon NV16 is added this will have to be changed.\n>\n> All in all, I would make this\n>\n> void PostProcessorLibyuv::calculateLengths(const StreamConfiguration &inCfg,\n> const StreamConfiguration &outCfg)\n> {\n> }\n>\n> and initialize both [source|destination][Length_|Stride_] in one go.\n> After all you call the function two times one after the other. This\n> way you won't have to pass paramters in.\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>\n> A comment inside a function call is rather unusual in the library code\n> base.\n>\n> > + ((size.height + vertSubSample - 1) / vertSubSample);\n>\n> This might be a good occasion to create a PixelFormatInfo::planeSize()\n> as we already have frameSize that performs the same calculation on all\n> planes. Not required for this patch though.\n>\n> > + stride[i] = nv12Info.stride(size.width, i, 1);\n> > + }\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..a65a16b3\n> > --- /dev/null\n> > +++ b/src/android/libyuv/post_processor_libyuv.h\n> > @@ -0,0 +1,44 @@\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 \n> > +\n> > +#include \"../post_processor.h\"\n> > +\n> > +class CameraDevice;\n> > +\n> > +class PostProcessorLibyuv : public PostProcessor\n> > +{\n> > +public:\n> > + PostProcessorLibyuv() = default;\n>\n> This looks weird, but I recall you and Laurent discussed this lenght, so I\n> presume it's ok.\n>\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> why is requestMetadata commented ?\n>\n> > + CameraMetadata *metadata) override;\n> > +\n> > +private:\n> > + bool isValidNV12Buffers(const libcamera::FrameBuffer &source,\n> > + const libcamera::MappedBuffer &destination) const;\n> > +\n> > + static void getNV12LengthAndStride(libcamera::Size size,\n> > + unsigned int length[2],\n> > + unsigned int stride[2]);\n>\n> Why static if it operates on class members ?\n>\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> Class member initialization is something we don't often do in\n> libcamera. Not sure why, it's a C++11 feature, so we should have\n> supported it from the very beginning. Not sure, just pointing it out.\n>\n> Thanks\n> j\n>\n\nI addressed your comments locally. I wait for more comments from\nothers and will upload the patches tomorrow's evening in JST.\n\nBest Regards,\n-Hiro\n> > +};\n> > +\n> > +#endif /* __ANDROID_POST_PROCESSOR_LIBYUV_H__ */\n> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > index e1533d7c..cbe09200 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -44,6 +44,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> > 2.30.0.365.g02bc693789-goog\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 7EF87BD160\n\tfor ;\n\tTue, 2 Feb 2021 07:27:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8C1668421;\n\tTue, 2 Feb 2021 08:27:05 +0100 (CET)","from mail-ed1-x529.google.com (mail-ed1-x529.google.com\n\t[IPv6:2a00:1450:4864:20::529])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C86F60106\n\tfor ;\n\tTue, 2 Feb 2021 08:27:04 +0100 (CET)","by mail-ed1-x529.google.com with SMTP id q2so7018664edi.4\n\tfor ;\n\tMon, 01 Feb 2021 23:27:04 -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=\"X9JEmhEc\"; 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=kq8fJux3kLqCzJLxKcyrZ/n3UVE3fdSVE8QT3i/7EmI=;\n\tb=X9JEmhEctHeLbmyAvuQNBbQHVa+/ooMPoR7ZldHDwpuLeLuRq9l9QJ5Wl1UYfe9SGn\n\t45A+tRCDJGxjpl9f2qpT3grve949abFI1wYLoHNkBSI2mTy1TwCRg44ppDegckmVox/n\n\t2eP8xnfOIhGWUHFfikHP9gODnoP+XOYxXN4KQ=","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=kq8fJux3kLqCzJLxKcyrZ/n3UVE3fdSVE8QT3i/7EmI=;\n\tb=rARQgq6DS7sgIOmK6MiycxIIA8ale/l4Cmg/6dQHoOH7VIETkLVRtNMoxwEdtMGVcO\n\tCQeuXDREfedwppKmNtvLwU9o1a6rDwTVc1DL3ZJkIJDesVmLToVHeQtmBqrQbv1kN+Jc\n\t8Q79dm1J7SK/u5+XZxd5uzppe7qB7qDETI7clHStRRZZ5Jha2thz6Cu4puWNHtETof+A\n\teLZDjIJX87cT6G0E1fpAgzfMc/mAwnLc85LHrMX+u6U/tvXXJBTnxejfLk/0CMwC1F9V\n\t92jf/WNENcPrubcDxROFh2htoK45/6m/ZH8hPirCaI7YqPE7lr8Bq9e7mZQaODqp+arf\n\t4B8g==","X-Gm-Message-State":"AOAM531JpH1ScehaTqHhWzUH7AIUtiAOUIcb9HSRZggvZ/n80c2OlI7N\n\tS8DfJGCHGAEOf6RsjWnHTD9j5bWfqShcBysRh/9u0LFh9Uo=","X-Google-Smtp-Source":"ABdhPJyvJxA7mcd8MgOytLwakjDqWmMTaq1QStw4m4pYBuXGXXHj7D3Xky52giIvPQ0DRGgg0WS5FuU+qJBL8OQE9C0=","X-Received":"by 2002:a05:6402:40c4:: with SMTP id\n\tz4mr22264363edb.233.1612250823821; \n\tMon, 01 Feb 2021 23:27:03 -0800 (PST)","MIME-Version":"1.0","References":"<20210128224217.2919790-1-hiroh@chromium.org>\n\t<20210128224217.2919790-3-hiroh@chromium.org>\n\t<20210201115345.ct2kod5phhsefddj@uno.localdomain>","In-Reply-To":"<20210201115345.ct2kod5phhsefddj@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 2 Feb 2021 16:26:54 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 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\" "}}]