[{"id":22799,"web_url":"https://patchwork.libcamera.org/comment/22799/","msgid":"","date":"2022-04-26T23:44:42","subject":"Re: [libcamera-devel] [PATCH v3 3/4] Rework JPEG encoder API and\n\tupdate PostProcessorJpeg and EncoderLibJpeg","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey,\n\nThank you for the patch.\n\nOn Tue, Apr 26, 2022 at 09:14:08AM +0000, Harvey Yang via libcamera-devel wrote:\n> To add JEA in the next CL, this CL reworks JPEG encoder API and move\n\nSame comment as in 1/4 for \"CL\" (I won't repeat it for the other patches\nin this series, please update them as applicable).\n\n> thumbnail encoding code into EncoderLibJpeg's implementation, as JEA\n> supports that as well.\n> \n> Signed-off-by: Harvey Yang \n\nThe e-mail address should use \"@\" instead of \" at \".\n\n> ---\n> src/android/jpeg/encoder.h | 9 ++-\n> src/android/jpeg/encoder_libjpeg.cpp | 70 +++++++++++++++++++\n> src/android/jpeg/encoder_libjpeg.h | 21 +++++-\n> .../jpeg/generic_post_processor_jpeg.cpp | 14 ++++\n> src/android/jpeg/meson.build | 9 +++\n> src/android/jpeg/post_processor_jpeg.cpp | 60 ++--------------\n> src/android/jpeg/post_processor_jpeg.h | 11 +--\n> src/android/meson.build | 5 +-\n\nThis is more reviewable than in the previous version, but there are\nstill quite a few changes bundled in the same patch. As a v4 will be\nneeded anyway, could you split this further as follows ? Feel free to\nchange the order as appropriate.\n\n- Add meson.build file in jpeg/ directory\n- Move thumbnail generate to Encoder class\n- Pass streamBuffer to encode() to prepare for JPEG encoders that need\n access to the HAL buffer handle\n- Add PostProcessorJpeg::SetEncoder()\n\n> 8 files changed, 128 insertions(+), 71 deletions(-)\n> create mode 100644 src/android/jpeg/generic_post_processor_jpeg.cpp\n> create mode 100644 src/android/jpeg/meson.build\n> \n> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> index b974d367..6d527d91 100644\n> --- a/src/android/jpeg/encoder.h\n> +++ b/src/android/jpeg/encoder.h\n> @@ -12,14 +12,19 @@\n> #include \n> #include \n> \n> +#include \"../camera_request.h\"\n> +\n> class Encoder\n> {\n> public:\n> \tvirtual ~Encoder() = default;\n> \n> \tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> -\tvirtual int encode(const libcamera::FrameBuffer &source,\n> -\t\t\t libcamera::Span destination,\n> +\tvirtual int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> \t\t\t libcamera::Span exifData,\n> \t\t\t unsigned int quality) = 0;\n> +\tvirtual int generateThumbnail(const libcamera::FrameBuffer &source,\n> +\t\t\t\t const libcamera::Size &targetSize,\n> +\t\t\t\t unsigned int quality,\n> +\t\t\t\t std::vector *thumbnail) = 0;\n> };\n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index 21a3b33d..b5591e33 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -24,6 +24,8 @@\n> #include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/mapped_framebuffer.h\"\n> \n> +#include \"../camera_buffer.h\"\n> +\n> using namespace libcamera;\n> \n> LOG_DECLARE_CATEGORY(JPEG)\n> @@ -82,8 +84,17 @@ EncoderLibJpeg::~EncoderLibJpeg()\n> }\n> \n> int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> +{\n> +\tthumbnailer_.configure(cfg.size, cfg.pixelFormat);\n> +\tcfg_ = cfg;\n\nI'd store the size and pixel format only, not the full\nStreamConfiguration, as they are all you need. internalConfigure()\nshould then take a size and pixel format, which will make it easier to\nuse in generateThumbnail().\n\n> +\n> +\treturn internalConfigure(cfg);\n> +}\n> +\n> +int EncoderLibJpeg::internalConfigure(const StreamConfiguration &cfg)\n\nLet's name the function configureEncoder().\n\n> {\n> \tconst struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);\n> +\n> \tif (info.colorSpace == JCS_UNKNOWN)\n> \t\treturn -ENOTSUP;\n> \n> @@ -178,6 +189,65 @@ void EncoderLibJpeg::compressNV(const std::vector> &planes)\n> \t}\n> }\n> \n> +int EncoderLibJpeg::encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> +\t\t\t libcamera::Span exifData,\n> +\t\t\t unsigned int quality)\n> +{\n> +\tinternalConfigure(cfg_);\n\ncfg_ is only used here, I suppose to reconfigure the libjpeg encoder for\nthe\n\n> +\treturn encode(*streamBuffer->srcBuffer, streamBuffer->dstBuffer.get()->plane(0), exifData, quality);\n\nLet's shorten the line a bit (we aim for 80 columns as a soft target)\n\n\treturn encode(*streamBuffer->srcBuffer, streamBuffer->dstBuffer.get()->plane(0),\n\t\t exifData, quality);\n\n> +}\n> +\n> +int EncoderLibJpeg::generateThumbnail(\n> +\tconst libcamera::FrameBuffer &source,\n> +\tconst libcamera::Size &targetSize,\n> +\tunsigned int quality,\n> +\tstd::vector *thumbnail)\n\nint EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer &source,\n\t\t\t\t const libcamera::Size &targetSize,\n\t\t\t\t unsigned int quality,\n\t\t\t\t std::vector *thumbnail)\n\nto match the coding style.\n\n> +{\n> +\t/* Stores the raw scaled-down thumbnail bytes. */\n> +\tstd::vector rawThumbnail;\n> +\n> +\tthumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n> +\n> +\tStreamConfiguration thCfg;\n> +\tthCfg.size = targetSize;\n> +\tthCfg.pixelFormat = thumbnailer_.pixelFormat();\n> +\tint ret = internalConfigure(thCfg);\n\nYou now use the same libjpeg encoder instance for both the main image\nand the thumbnail. This leads to EncoderLibJpeg::internalConfigure()\nbeing called twice for every frame. The function looks fairly cheap so\nit should be fine, but could you confirm that jpeg_set_defaults()\ndoesn't cause a large overhead ? Please mention this change in the\ncommit message of the appropriate patch, it's an important one.\n\n> +\n> +\tif (!rawThumbnail.empty() && !ret) {\n> +\t\t/*\n> +\t\t * \\todo Avoid value-initialization of all elements of the\n> +\t\t * vector.\n> +\t\t */\n> +\t\tthumbnail->resize(rawThumbnail.size());\n> +\n> +\t\t/*\n> +\t\t * Split planes manually as the encoder expects a vector of\n> +\t\t * planes.\n> +\t\t *\n> +\t\t * \\todo Pass a vector of planes directly to\n> +\t\t * Thumbnailer::createThumbnailer above and remove the manual\n> +\t\t * planes split from here.\n> +\t\t */\n> +\t\tstd::vector> thumbnailPlanes;\n> +\t\tconst PixelFormatInfo &formatNV12 = PixelFormatInfo::info(formats::NV12);\n> +\t\tsize_t yPlaneSize = formatNV12.planeSize(targetSize, 0);\n> +\t\tsize_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);\n> +\t\tthumbnailPlanes.push_back({ rawThumbnail.data(), yPlaneSize });\n> +\t\tthumbnailPlanes.push_back({ rawThumbnail.data() + yPlaneSize, uvPlaneSize });\n> +\n> +\t\tint jpeg_size = encode(thumbnailPlanes, *thumbnail, {}, quality);\n> +\t\tthumbnail->resize(jpeg_size);\n> +\n> +\t\tLOG(JPEG, Debug)\n> +\t\t\t<< \"Thumbnail compress returned \"\n> +\t\t\t<< jpeg_size << \" bytes\";\n> +\n> +\t\treturn jpeg_size;\n> +\t}\n> +\n> +\treturn -1;\n> +}\n> +\n> int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n> \t\t\t Span exifData, unsigned int quality)\n> {\n> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> index 1b3ac067..56b27bae 100644\n> --- a/src/android/jpeg/encoder_libjpeg.h\n> +++ b/src/android/jpeg/encoder_libjpeg.h\n> @@ -15,6 +15,8 @@\n> \n> #include \n> \n> +#include \"thumbnailer.h\"\n> +\n> class EncoderLibJpeg : public Encoder\n> {\n> public:\n> @@ -22,19 +24,32 @@ public:\n> \t~EncoderLibJpeg();\n> \n> \tint configure(const libcamera::StreamConfiguration &cfg) override;\n> +\tint encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> +\t\t libcamera::Span exifData,\n> +\t\t unsigned int quality) override;\n> +\tint generateThumbnail(\n> +\t\tconst libcamera::FrameBuffer &source,\n> +\t\tconst libcamera::Size &targetSize,\n> +\t\tunsigned int quality,\n> +\t\tstd::vector *thumbnail) override;\n\nSame here.\n\n> +\n> +private:\n> +\tint internalConfigure(const libcamera::StreamConfiguration &cfg);\n> +\n> \tint encode(const libcamera::FrameBuffer &source,\n> \t\t libcamera::Span destination,\n> \t\t libcamera::Span exifData,\n> -\t\t unsigned int quality) override;\n> +\t\t unsigned int quality);\n> \tint encode(const std::vector> &planes,\n> \t\t libcamera::Span destination,\n> \t\t libcamera::Span exifData,\n> \t\t unsigned int quality);\n> \n> -private:\n> \tvoid compressRGB(const std::vector> &planes);\n> \tvoid compressNV(const std::vector> &planes);\n> \n> +\tlibcamera::StreamConfiguration cfg_;\n> +\n> \tstruct jpeg_compress_struct compress_;\n> \tstruct jpeg_error_mgr jerr_;\n> \n> @@ -42,4 +57,6 @@ private:\n> \n> \tbool nv_;\n> \tbool nvSwap_;\n> +\n> +\tThumbnailer thumbnailer_;\n> };\n> diff --git a/src/android/jpeg/generic_post_processor_jpeg.cpp b/src/android/jpeg/generic_post_processor_jpeg.cpp\n> new file mode 100644\n> index 00000000..890f6972\n> --- /dev/null\n> +++ b/src/android/jpeg/generic_post_processor_jpeg.cpp\n> @@ -0,0 +1,14 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Google Inc.\n> + *\n> + * generic_post_processor_jpeg.cpp - Generic JPEG Post Processor\n> + */\n> +\n> +#include \"encoder_libjpeg.h\"\n> +#include \"post_processor_jpeg.h\"\n> +\n> +void PostProcessorJpeg::SetEncoder()\n\nWe use camelCase for function names, not CamelCase. Let's name the\nfunction createEncoder(), as it doesn't just set it but actually creates\nit.\n\n> +{\n> +\tencoder_ = std::make_unique();\n\nI'm tempted to simplify this by using conditional compilation (we have a\nOS_CHROMEOS macro), the function could then go to\npost_processor_jpeg.cpp.\n\n> +}\n> diff --git a/src/android/jpeg/meson.build b/src/android/jpeg/meson.build\n> new file mode 100644\n> index 00000000..94522dc0\n> --- /dev/null\n> +++ b/src/android/jpeg/meson.build\n> @@ -0,0 +1,9 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +android_hal_sources += files([\n> + 'exif.cpp',\n> + 'post_processor_jpeg.cpp'])\n\nThe '])' should be on the next line.\n\n> +\n> +android_hal_sources += files(['encoder_libjpeg.cpp',\n> + 'generic_post_processor_jpeg.cpp',\n> + 'thumbnailer.cpp'])\n\nSimilar comment here,\n\nandroid_hal_sources += files([\n 'encoder_libjpeg.cpp',\n 'generic_post_processor_jpeg.cpp',\n 'thumbnailer.cpp',\n])\n\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index d72ebc3c..7ceba60e 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -9,10 +9,10 @@\n> \n> #include \n> \n> +#include \"../android_framebuffer.h\"\n\nIs this needed ?\n\n> #include \"../camera_device.h\"\n> #include \"../camera_metadata.h\"\n> #include \"../camera_request.h\"\n> -#include \"encoder_libjpeg.h\"\n> #include \"exif.h\"\n> \n> #include \n> @@ -44,60 +44,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> \n> \tstreamSize_ = outCfg.size;\n> \n> -\tthumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> -\n> -\tencoder_ = std::make_unique();\n> +\tSetEncoder();\n> \n> \treturn encoder_->configure(inCfg);\n> }\n> \n> -void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> -\t\t\t\t\t const Size &targetSize,\n> -\t\t\t\t\t unsigned int quality,\n> -\t\t\t\t\t std::vector *thumbnail)\n> -{\n> -\t/* Stores the raw scaled-down thumbnail bytes. */\n> -\tstd::vector rawThumbnail;\n> -\n> -\tthumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n> -\n> -\tStreamConfiguration thCfg;\n> -\tthCfg.size = targetSize;\n> -\tthCfg.pixelFormat = thumbnailer_.pixelFormat();\n> -\tint ret = thumbnailEncoder_.configure(thCfg);\n> -\n> -\tif (!rawThumbnail.empty() && !ret) {\n> -\t\t/*\n> -\t\t * \\todo Avoid value-initialization of all elements of the\n> -\t\t * vector.\n> -\t\t */\n> -\t\tthumbnail->resize(rawThumbnail.size());\n> -\n> -\t\t/*\n> -\t\t * Split planes manually as the encoder expects a vector of\n> -\t\t * planes.\n> -\t\t *\n> -\t\t * \\todo Pass a vector of planes directly to\n> -\t\t * Thumbnailer::createThumbnailer above and remove the manual\n> -\t\t * planes split from here.\n> -\t\t */\n> -\t\tstd::vector> thumbnailPlanes;\n> -\t\tconst PixelFormatInfo &formatNV12 = PixelFormatInfo::info(formats::NV12);\n> -\t\tsize_t yPlaneSize = formatNV12.planeSize(targetSize, 0);\n> -\t\tsize_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);\n> -\t\tthumbnailPlanes.push_back({ rawThumbnail.data(), yPlaneSize });\n> -\t\tthumbnailPlanes.push_back({ rawThumbnail.data() + yPlaneSize, uvPlaneSize });\n> -\n> -\t\tint jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,\n> -\t\t\t\t\t\t\t *thumbnail, {}, quality);\n> -\t\tthumbnail->resize(jpeg_size);\n> -\n> -\t\tLOG(JPEG, Debug)\n> -\t\t\t<< \"Thumbnail compress returned \"\n> -\t\t\t<< jpeg_size << \" bytes\";\n> -\t}\n> -}\n> -\n> void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> {\n> \tASSERT(encoder_);\n> @@ -164,8 +115,8 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu\n> \n> \t\tif (thumbnailSize != Size(0, 0)) {\n> \t\t\tstd::vector thumbnail;\n> -\t\t\tgenerateThumbnail(source, thumbnailSize, quality, &thumbnail);\n> -\t\t\tif (!thumbnail.empty())\n> +\t\t\tret = encoder_->generateThumbnail(source, thumbnailSize, quality, &thumbnail);\n> +\t\t\tif (ret > 0 && !thumbnail.empty())\n> \t\t\t\texif.setThumbnail(thumbnail, Exif::Compression::JPEG);\n> \t\t}\n> \n> @@ -194,8 +145,7 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu\n> \tconst uint8_t quality = ret ? *entry.data.u8 : 95;\n> \tresultMetadata->addEntry(ANDROID_JPEG_QUALITY, quality);\n> \n> -\tint jpeg_size = encoder_->encode(source, destination->plane(0),\n> -\t\t\t\t\t exif.data(), quality);\n> +\tint jpeg_size = encoder_->encode(streamBuffer, exif.data(), quality);\n> \tif (jpeg_size < 0) {\n> \t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n> \t\tprocessComplete.emit(streamBuffer, PostProcessor::Status::Error);\n> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> index 98309b01..a09f8798 100644\n> --- a/src/android/jpeg/post_processor_jpeg.h\n> +++ b/src/android/jpeg/post_processor_jpeg.h\n> @@ -8,11 +8,11 @@\n> #pragma once\n> \n> #include \"../post_processor.h\"\n> -#include \"encoder_libjpeg.h\"\n> -#include \"thumbnailer.h\"\n> \n> #include \n> \n> +#include \"encoder.h\"\n> +\n> class CameraDevice;\n> \n> class PostProcessorJpeg : public PostProcessor\n> @@ -25,14 +25,9 @@ public:\n> \tvoid process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;\n> \n> private:\n> -\tvoid generateThumbnail(const libcamera::FrameBuffer &source,\n> -\t\t\t const libcamera::Size &targetSize,\n> -\t\t\t unsigned int quality,\n> -\t\t\t std::vector *thumbnail);\n> +\tvoid SetEncoder();\n> \n> \tCameraDevice *const cameraDevice_;\n> \tstd::unique_ptr encoder_;\n> \tlibcamera::Size streamSize_;\n> -\tEncoderLibJpeg thumbnailEncoder_;\n> -\tThumbnailer thumbnailer_;\n> };\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 27be27bb..026b8b3c 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -48,10 +48,6 @@ android_hal_sources = files([\n> 'camera_ops.cpp',\n> 'camera_request.cpp',\n> 'camera_stream.cpp',\n> - 'jpeg/encoder_libjpeg.cpp',\n> - 'jpeg/exif.cpp',\n> - 'jpeg/post_processor_jpeg.cpp',\n> - 'jpeg/thumbnailer.cpp',\n> 'yuv/post_processor_yuv.cpp'\n> ])\n> \n> @@ -59,6 +55,7 @@ android_cpp_args = []\n> \n> subdir('cros')\n> subdir('mm')\n> +subdir('jpeg')\n\nPlease keep these alphabetically sorted.\n\n> \n> android_camera_metadata_sources = files([\n> 'metadata/camera_metadata.c',","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 2D0F9C3256\n\tfor ;\n\tTue, 26 Apr 2022 23:44:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93AE965647;\n\tWed, 27 Apr 2022 01:44:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 48072604A9\n\tfor ;\n\tWed, 27 Apr 2022 01:44:43 +0200 (CEST)","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 C313830B;\n\tWed, 27 Apr 2022 01:44:42 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1651016684;\n\tbh=dUzyclcrlvUUjsPcFcvq6wnQ2HU3ssQu2WiUatB2aEY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=R9h4AvyBDvfq7XhGvwYOu6QPs5BG7ukPYisMky97DCcCVac9XmSvrQyoNzgi/booF\n\tWW3OJzGYMWhv4lb/xVtcwXQM9UCjEzgW8Hue2dDXws298Jdr8x8XFXAoJOnf/gze5b\n\t+R70bh4qhoMDc8qJ2guuOEvlU9ji1ZQaUYFAuemPPP3v/eNBKHR8urm5N6y+cXspN6\n\tCOcl1Qo+rvVisCffVuN3WnnQdgsg0K5McXYPwzGT4/FJ1bhyiS6Urlsh2nGb7PFT+K\n\tM4LJj7B0zDaUwHjnYLmnFzYbWz6hx2eS0BkIa3C79IWycfIiD3It7Oo4hAq7Tq2pVV\n\tNju4m3x1Ph7sw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1651016683;\n\tbh=dUzyclcrlvUUjsPcFcvq6wnQ2HU3ssQu2WiUatB2aEY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bjWz3qK4Pow1cwkoVsRpws/knIUc2PGdF+tLnOWvkXMGIi8L0T2p8siBkvBDW7gsG\n\tR5K6CUWSoDd+Gl98vpZwfoy+5GA5ufnzyYfkjHyUnw2glNXAVW9PtJ5iuQhxG5X7nS\n\tQ3leLrRn7wjjRQCpsyB1qpJQJXHctsul+ZGiYPfE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"bjWz3qK4\"; dkim-atps=neutral","Date":"Wed, 27 Apr 2022 02:44:42 +0300","To":"Harvey Yang ","Message-ID":"","References":"<20220426091409.1352047-1-chenghaoyang@chromium.org>\n\t<20220426091409.1352047-4-chenghaoyang@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220426091409.1352047-4-chenghaoyang@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 3/4] Rework JPEG encoder API and\n\tupdate PostProcessorJpeg and EncoderLibJpeg","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22821,"web_url":"https://patchwork.libcamera.org/comment/22821/","msgid":"","date":"2022-05-03T03:29:49","subject":"Re: [libcamera-devel] [PATCH v3 3/4] Rework JPEG encoder API and\n\tupdate PostProcessorJpeg and EncoderLibJpeg","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nSorry I forgot to send this reply before going on vacation...\n\n> The e-mail address should use \"@\" instead of \" at \".\nOh I see! I checked the libcamera archives for examples, and thought\nthat was intentional haha.\nUpdated.\n\n> cfg_ is only used here, I suppose to reconfigure the libjpeg encoder for\nthe\nIs this a deprecated comment?\n\n> > int configure(const libcamera::StreamConfiguration &cfg) override;\n> > + int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> > + libcamera::Span exifData,\n> > + unsigned int quality) override;\n> > + int generateThumbnail(\n> > + const libcamera::FrameBuffer &source,\n> > + const libcamera::Size &targetSize,\n> > + unsigned int quality,\n> > + std::vector *thumbnail) override;\n\n> Same here.\nI think I need your help on the indentation format. Any doc or rule I can\nstudy? Thanks!\n\n> I'm tempted to simplify this by using conditional compilation (we have a\nOS_CHROMEOS macro), the function could then go to post_processor_jpeg.cpp.\nOh I didn't notice there's such a macro to use. Thanks! I think we can get\nrid of the PostProcessorJpeg::createEncoder() then.\n\n> You now use the same libjpeg encoder instance for both the main image and\nthe thumbnail. This leads to EncoderLibJpeg::internalConfigure() being\ncalled twice for every frame. The function looks fairly cheap so it should\nbe fine, but could you confirm that jpeg_set_defaults() doesn't cause a\nlarge overhead ? Please mention this change in the commit message of the\nappropriate patch, it's an important one.\nOriginally for each frame we need at least one call to jpeg_set_defaults()\nto encode the thumbnail, and we need two now with patch v3, so I thought it\nwas fine in terms of the complexity, while you're right that I'm not sure\nabout the complexity of jpeg_set_defaults(), even looking into the\nimplementation [1]. I can also modify the code not to configure twice per\nframe. WDYT?\n\n[1]: https://github.com/kornelski/libjpeg/blob/master/jcparam.c#L268\n\n\nOn Wed, Apr 27, 2022 at 7:44 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Harvey,\n>\n> Thank you for the patch.\n>\n> On Tue, Apr 26, 2022 at 09:14:08AM +0000, Harvey Yang via libcamera-devel\n> wrote:\n> > To add JEA in the next CL, this CL reworks JPEG encoder API and move\n>\n> Same comment as in 1/4 for \"CL\" (I won't repeat it for the other patches\n> in this series, please update them as applicable).\n>\n> > thumbnail encoding code into EncoderLibJpeg's implementation, as JEA\n> > supports that as well.\n> >\n> > Signed-off-by: Harvey Yang \n>\n> The e-mail address should use \"@\" instead of \" at \".\n>\n> > ---\n> > src/android/jpeg/encoder.h | 9 ++-\n> > src/android/jpeg/encoder_libjpeg.cpp | 70 +++++++++++++++++++\n> > src/android/jpeg/encoder_libjpeg.h | 21 +++++-\n> > .../jpeg/generic_post_processor_jpeg.cpp | 14 ++++\n> > src/android/jpeg/meson.build | 9 +++\n> > src/android/jpeg/post_processor_jpeg.cpp | 60 ++--------------\n> > src/android/jpeg/post_processor_jpeg.h | 11 +--\n> > src/android/meson.build | 5 +-\n>\n> This is more reviewable than in the previous version, but there are\n> still quite a few changes bundled in the same patch. As a v4 will be\n> needed anyway, could you split this further as follows ? Feel free to\n> change the order as appropriate.\n>\n> - Add meson.build file in jpeg/ directory\n> - Move thumbnail generate to Encoder class\n> - Pass streamBuffer to encode() to prepare for JPEG encoders that need\n> access to the HAL buffer handle\n> - Add PostProcessorJpeg::SetEncoder()\n>\n> > 8 files changed, 128 insertions(+), 71 deletions(-)\n> > create mode 100644 src/android/jpeg/generic_post_processor_jpeg.cpp\n> > create mode 100644 src/android/jpeg/meson.build\n> >\n> > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> > index b974d367..6d527d91 100644\n> > --- a/src/android/jpeg/encoder.h\n> > +++ b/src/android/jpeg/encoder.h\n> > @@ -12,14 +12,19 @@\n> > #include \n> > #include \n> >\n> > +#include \"../camera_request.h\"\n> > +\n> > class Encoder\n> > {\n> > public:\n> > virtual ~Encoder() = default;\n> >\n> > virtual int configure(const libcamera::StreamConfiguration &cfg) =\n> 0;\n> > - virtual int encode(const libcamera::FrameBuffer &source,\n> > - libcamera::Span destination,\n> > + virtual int encode(Camera3RequestDescriptor::StreamBuffer\n> *streamBuffer,\n> > libcamera::Span exifData,\n> > unsigned int quality) = 0;\n> > + virtual int generateThumbnail(const libcamera::FrameBuffer &source,\n> > + const libcamera::Size &targetSize,\n> > + unsigned int quality,\n> > + std::vector\n> *thumbnail) = 0;\n> > };\n> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp\n> b/src/android/jpeg/encoder_libjpeg.cpp\n> > index 21a3b33d..b5591e33 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > @@ -24,6 +24,8 @@\n> > #include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/mapped_framebuffer.h\"\n> >\n> > +#include \"../camera_buffer.h\"\n> > +\n> > using namespace libcamera;\n> >\n> > LOG_DECLARE_CATEGORY(JPEG)\n> > @@ -82,8 +84,17 @@ EncoderLibJpeg::~EncoderLibJpeg()\n> > }\n> >\n> > int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> > +{\n> > + thumbnailer_.configure(cfg.size, cfg.pixelFormat);\n> > + cfg_ = cfg;\n>\n> I'd store the size and pixel format only, not the full\n> StreamConfiguration, as they are all you need. internalConfigure()\n> should then take a size and pixel format, which will make it easier to\n> use in generateThumbnail().\n>\n> > +\n> > + return internalConfigure(cfg);\n> > +}\n> > +\n> > +int EncoderLibJpeg::internalConfigure(const StreamConfiguration &cfg)\n>\n> Let's name the function configureEncoder().\n>\n> > {\n> > const struct JPEGPixelFormatInfo info =\n> findPixelInfo(cfg.pixelFormat);\n> > +\n> > if (info.colorSpace == JCS_UNKNOWN)\n> > return -ENOTSUP;\n> >\n> > @@ -178,6 +189,65 @@ void EncoderLibJpeg::compressNV(const\n> std::vector> &planes)\n> > }\n> > }\n> >\n> > +int EncoderLibJpeg::encode(Camera3RequestDescriptor::StreamBuffer\n> *streamBuffer,\n> > + libcamera::Span exifData,\n> > + unsigned int quality)\n> > +{\n> > + internalConfigure(cfg_);\n>\n> cfg_ is only used here, I suppose to reconfigure the libjpeg encoder for\n> the\n>\n> > + return encode(*streamBuffer->srcBuffer,\n> streamBuffer->dstBuffer.get()->plane(0), exifData, quality);\n>\n> Let's shorten the line a bit (we aim for 80 columns as a soft target)\n>\n> return encode(*streamBuffer->srcBuffer,\n> streamBuffer->dstBuffer.get()->plane(0),\n> exifData, quality);\n>\n> > +}\n> > +\n> > +int EncoderLibJpeg::generateThumbnail(\n> > + const libcamera::FrameBuffer &source,\n> > + const libcamera::Size &targetSize,\n> > + unsigned int quality,\n> > + std::vector *thumbnail)\n>\n> int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer &source,\n> const libcamera::Size &targetSize,\n> unsigned int quality,\n> std::vector\n> *thumbnail)\n>\n> to match the coding style.\n>\n> > +{\n> > + /* Stores the raw scaled-down thumbnail bytes. */\n> > + std::vector rawThumbnail;\n> > +\n> > + thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n> > +\n> > + StreamConfiguration thCfg;\n> > + thCfg.size = targetSize;\n> > + thCfg.pixelFormat = thumbnailer_.pixelFormat();\n> > + int ret = internalConfigure(thCfg);\n>\n> You now use the same libjpeg encoder instance for both the main image\n> and the thumbnail. This leads to EncoderLibJpeg::internalConfigure()\n> being called twice for every frame. The function looks fairly cheap so\n> it should be fine, but could you confirm that jpeg_set_defaults()\n> doesn't cause a large overhead ? Please mention this change in the\n> commit message of the appropriate patch, it's an important one.\n>\n> > +\n> > + if (!rawThumbnail.empty() && !ret) {\n> > + /*\n> > + * \\todo Avoid value-initialization of all elements of the\n> > + * vector.\n> > + */\n> > + thumbnail->resize(rawThumbnail.size());\n> > +\n> > + /*\n> > + * Split planes manually as the encoder expects a vector of\n> > + * planes.\n> > + *\n> > + * \\todo Pass a vector of planes directly to\n> > + * Thumbnailer::createThumbnailer above and remove the\n> manual\n> > + * planes split from here.\n> > + */\n> > + std::vector> thumbnailPlanes;\n> > + const PixelFormatInfo &formatNV12 =\n> PixelFormatInfo::info(formats::NV12);\n> > + size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);\n> > + size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);\n> > + thumbnailPlanes.push_back({ rawThumbnail.data(),\n> yPlaneSize });\n> > + thumbnailPlanes.push_back({ rawThumbnail.data() +\n> yPlaneSize, uvPlaneSize });\n> > +\n> > + int jpeg_size = encode(thumbnailPlanes, *thumbnail, {},\n> quality);\n> > + thumbnail->resize(jpeg_size);\n> > +\n> > + LOG(JPEG, Debug)\n> > + << \"Thumbnail compress returned \"\n> > + << jpeg_size << \" bytes\";\n> > +\n> > + return jpeg_size;\n> > + }\n> > +\n> > + return -1;\n> > +}\n> > +\n> > int EncoderLibJpeg::encode(const FrameBuffer &source, Span\n> dest,\n> > Span exifData, unsigned int\n> quality)\n> > {\n> > diff --git a/src/android/jpeg/encoder_libjpeg.h\n> b/src/android/jpeg/encoder_libjpeg.h\n> > index 1b3ac067..56b27bae 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.h\n> > +++ b/src/android/jpeg/encoder_libjpeg.h\n> > @@ -15,6 +15,8 @@\n> >\n> > #include \n> >\n> > +#include \"thumbnailer.h\"\n> > +\n> > class EncoderLibJpeg : public Encoder\n> > {\n> > public:\n> > @@ -22,19 +24,32 @@ public:\n> > ~EncoderLibJpeg();\n> >\n> > int configure(const libcamera::StreamConfiguration &cfg) override;\n> > + int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> > + libcamera::Span exifData,\n> > + unsigned int quality) override;\n> > + int generateThumbnail(\n> > + const libcamera::FrameBuffer &source,\n> > + const libcamera::Size &targetSize,\n> > + unsigned int quality,\n> > + std::vector *thumbnail) override;\n>\n> Same here.\n>\n> > +\n> > +private:\n> > + int internalConfigure(const libcamera::StreamConfiguration &cfg);\n> > +\n> > int encode(const libcamera::FrameBuffer &source,\n> > libcamera::Span destination,\n> > libcamera::Span exifData,\n> > - unsigned int quality) override;\n> > + unsigned int quality);\n> > int encode(const std::vector> &planes,\n> > libcamera::Span destination,\n> > libcamera::Span exifData,\n> > unsigned int quality);\n> >\n> > -private:\n> > void compressRGB(const std::vector>\n> &planes);\n> > void compressNV(const std::vector>\n> &planes);\n> >\n> > + libcamera::StreamConfiguration cfg_;\n> > +\n> > struct jpeg_compress_struct compress_;\n> > struct jpeg_error_mgr jerr_;\n> >\n> > @@ -42,4 +57,6 @@ private:\n> >\n> > bool nv_;\n> > bool nvSwap_;\n> > +\n> > + Thumbnailer thumbnailer_;\n> > };\n> > diff --git a/src/android/jpeg/generic_post_processor_jpeg.cpp\n> b/src/android/jpeg/generic_post_processor_jpeg.cpp\n> > new file mode 100644\n> > index 00000000..890f6972\n> > --- /dev/null\n> > +++ b/src/android/jpeg/generic_post_processor_jpeg.cpp\n> > @@ -0,0 +1,14 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Google Inc.\n> > + *\n> > + * generic_post_processor_jpeg.cpp - Generic JPEG Post Processor\n> > + */\n> > +\n> > +#include \"encoder_libjpeg.h\"\n> > +#include \"post_processor_jpeg.h\"\n> > +\n> > +void PostProcessorJpeg::SetEncoder()\n>\n> We use camelCase for function names, not CamelCase. Let's name the\n> function createEncoder(), as it doesn't just set it but actually creates\n> it.\n>\n> > +{\n> > + encoder_ = std::make_unique();\n>\n> I'm tempted to simplify this by using conditional compilation (we have a\n> OS_CHROMEOS macro), the function could then go to\n> post_processor_jpeg.cpp.\n>\n> > +}\n> > diff --git a/src/android/jpeg/meson.build b/src/android/jpeg/meson.build\n> > new file mode 100644\n> > index 00000000..94522dc0\n> > --- /dev/null\n> > +++ b/src/android/jpeg/meson.build\n> > @@ -0,0 +1,9 @@\n> > +# SPDX-License-Identifier: CC0-1.0\n> > +\n> > +android_hal_sources += files([\n> > + 'exif.cpp',\n> > + 'post_processor_jpeg.cpp'])\n>\n> The '])' should be on the next line.\n>\n> > +\n> > +android_hal_sources += files(['encoder_libjpeg.cpp',\n> > + 'generic_post_processor_jpeg.cpp',\n> > + 'thumbnailer.cpp'])\n>\n> Similar comment here,\n>\n> android_hal_sources += files([\n> 'encoder_libjpeg.cpp',\n> 'generic_post_processor_jpeg.cpp',\n> 'thumbnailer.cpp',\n> ])\n>\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp\n> b/src/android/jpeg/post_processor_jpeg.cpp\n> > index d72ebc3c..7ceba60e 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -9,10 +9,10 @@\n> >\n> > #include \n> >\n> > +#include \"../android_framebuffer.h\"\n>\n> Is this needed ?\n>\n> > #include \"../camera_device.h\"\n> > #include \"../camera_metadata.h\"\n> > #include \"../camera_request.h\"\n> > -#include \"encoder_libjpeg.h\"\n> > #include \"exif.h\"\n> >\n> > #include \n> > @@ -44,60 +44,11 @@ int PostProcessorJpeg::configure(const\n> StreamConfiguration &inCfg,\n> >\n> > streamSize_ = outCfg.size;\n> >\n> > - thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> > -\n> > - encoder_ = std::make_unique();\n> > + SetEncoder();\n> >\n> > return encoder_->configure(inCfg);\n> > }\n> >\n> > -void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> > - const Size &targetSize,\n> > - unsigned int quality,\n> > - std::vector\n> *thumbnail)\n> > -{\n> > - /* Stores the raw scaled-down thumbnail bytes. */\n> > - std::vector rawThumbnail;\n> > -\n> > - thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n> > -\n> > - StreamConfiguration thCfg;\n> > - thCfg.size = targetSize;\n> > - thCfg.pixelFormat = thumbnailer_.pixelFormat();\n> > - int ret = thumbnailEncoder_.configure(thCfg);\n> > -\n> > - if (!rawThumbnail.empty() && !ret) {\n> > - /*\n> > - * \\todo Avoid value-initialization of all elements of the\n> > - * vector.\n> > - */\n> > - thumbnail->resize(rawThumbnail.size());\n> > -\n> > - /*\n> > - * Split planes manually as the encoder expects a vector of\n> > - * planes.\n> > - *\n> > - * \\todo Pass a vector of planes directly to\n> > - * Thumbnailer::createThumbnailer above and remove the\n> manual\n> > - * planes split from here.\n> > - */\n> > - std::vector> thumbnailPlanes;\n> > - const PixelFormatInfo &formatNV12 =\n> PixelFormatInfo::info(formats::NV12);\n> > - size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);\n> > - size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);\n> > - thumbnailPlanes.push_back({ rawThumbnail.data(),\n> yPlaneSize });\n> > - thumbnailPlanes.push_back({ rawThumbnail.data() +\n> yPlaneSize, uvPlaneSize });\n> > -\n> > - int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,\n> > - *thumbnail, {},\n> quality);\n> > - thumbnail->resize(jpeg_size);\n> > -\n> > - LOG(JPEG, Debug)\n> > - << \"Thumbnail compress returned \"\n> > - << jpeg_size << \" bytes\";\n> > - }\n> > -}\n> > -\n> > void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer\n> *streamBuffer)\n> > {\n> > ASSERT(encoder_);\n> > @@ -164,8 +115,8 @@ void\n> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu\n> >\n> > if (thumbnailSize != Size(0, 0)) {\n> > std::vector thumbnail;\n> > - generateThumbnail(source, thumbnailSize, quality,\n> &thumbnail);\n> > - if (!thumbnail.empty())\n> > + ret = encoder_->generateThumbnail(source,\n> thumbnailSize, quality, &thumbnail);\n> > + if (ret > 0 && !thumbnail.empty())\n> > exif.setThumbnail(thumbnail,\n> Exif::Compression::JPEG);\n> > }\n> >\n> > @@ -194,8 +145,7 @@ void\n> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu\n> > const uint8_t quality = ret ? *entry.data.u8 : 95;\n> > resultMetadata->addEntry(ANDROID_JPEG_QUALITY, quality);\n> >\n> > - int jpeg_size = encoder_->encode(source, destination->plane(0),\n> > - exif.data(), quality);\n> > + int jpeg_size = encoder_->encode(streamBuffer, exif.data(),\n> quality);\n> > if (jpeg_size < 0) {\n> > LOG(JPEG, Error) << \"Failed to encode stream image\";\n> > processComplete.emit(streamBuffer,\n> PostProcessor::Status::Error);\n> > diff --git a/src/android/jpeg/post_processor_jpeg.h\n> b/src/android/jpeg/post_processor_jpeg.h\n> > index 98309b01..a09f8798 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.h\n> > +++ b/src/android/jpeg/post_processor_jpeg.h\n> > @@ -8,11 +8,11 @@\n> > #pragma once\n> >\n> > #include \"../post_processor.h\"\n> > -#include \"encoder_libjpeg.h\"\n> > -#include \"thumbnailer.h\"\n> >\n> > #include \n> >\n> > +#include \"encoder.h\"\n> > +\n> > class CameraDevice;\n> >\n> > class PostProcessorJpeg : public PostProcessor\n> > @@ -25,14 +25,9 @@ public:\n> > void process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> override;\n> >\n> > private:\n> > - void generateThumbnail(const libcamera::FrameBuffer &source,\n> > - const libcamera::Size &targetSize,\n> > - unsigned int quality,\n> > - std::vector *thumbnail);\n> > + void SetEncoder();\n> >\n> > CameraDevice *const cameraDevice_;\n> > std::unique_ptr encoder_;\n> > libcamera::Size streamSize_;\n> > - EncoderLibJpeg thumbnailEncoder_;\n> > - Thumbnailer thumbnailer_;\n> > };\n> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > index 27be27bb..026b8b3c 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -48,10 +48,6 @@ android_hal_sources = files([\n> > 'camera_ops.cpp',\n> > 'camera_request.cpp',\n> > 'camera_stream.cpp',\n> > - 'jpeg/encoder_libjpeg.cpp',\n> > - 'jpeg/exif.cpp',\n> > - 'jpeg/post_processor_jpeg.cpp',\n> > - 'jpeg/thumbnailer.cpp',\n> > 'yuv/post_processor_yuv.cpp'\n> > ])\n> >\n> > @@ -59,6 +55,7 @@ android_cpp_args = []\n> >\n> > subdir('cros')\n> > subdir('mm')\n> > +subdir('jpeg')\n>\n> Please keep these alphabetically sorted.\n>\n> >\n> > android_camera_metadata_sources = files([\n> > 'metadata/camera_metadata.c',\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","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 EA0F0C3256\n\tfor ;\n\tTue, 3 May 2022 03:30:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 96F6C6564A;\n\tTue, 3 May 2022 05:30:02 +0200 (CEST)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 15649604A1\n\tfor ;\n\tTue, 3 May 2022 05:30:01 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id i10so348869lfg.13\n\tfor ;\n\tMon, 02 May 2022 20:30:01 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1651548602;\n\tbh=/xIa1pFvOo8EcbhEl0e1VyLBs+5wvyINI2yZQPtvx4s=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=D7cXxdDpOLcud1BtcjN+lpykYF7u5zPdwXTZ/OgaE118pJPCdy/v718SWn4OLUAoJ\n\tEMlaZ3KofCvhsjmkyvRYdL6EWTCGMt6GeOP5Jnuhu2vGNKNDR99KtURMbDWw1y3S1I\n\t3tGTC873rizFi2Zc6C0z3J0wRvUo2NEqKSk1ro8tZsL/Z4mhKLi3OdkmD/DEQ6n0hw\n\tfRwuhDgaID55DUpk9TNe9JE6VJy1vrAPFyd/ZGTKgbHXZWn0EPMGAV/oHiWDKnKYc/\n\tnOFASlK1ryz0gYituuc+SM/9Hr9Zqe8c+0LFVhDweMiOOVAaX7ufVlU+MVKrR5P45A\n\t/6K/JjMKOyuNA==","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=ZAyHFJ0+UoVm/8/7Y7fyViYsXtcHglcI8jeEjW3Zcg0=;\n\tb=VWgmZKQgLV4TmIiVedpjEqTPI4WDRAKQ39qLo7rD5tks7xmWHz8fr+Jglh0CwkrLd8\n\t/ODhFFwT7ivG3o9RUS4W8STH7dzH/RDu/8VCMOe823RVIi6jd52kKi4suBZCQe/A7CQk\n\tCA2fPHgZyiiYovgrnQV466b/QbJbQpWSG8hgg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"VWgmZKQg\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=ZAyHFJ0+UoVm/8/7Y7fyViYsXtcHglcI8jeEjW3Zcg0=;\n\tb=AXnaTYZeVT9Q3ME1+AeF7uKqoDNmmzE/x2ZNzAyhvukBoZKmb6r2rjW6r3ZFzGvdgE\n\t7/DjR3rqtkpebK0jkJtqeatRxonnqEW5jCFrgv71dVn5xyHH1orGkjHVNjepzRKPLxq3\n\tXy/Gc7FHKiB296pm5PM75J9DzdkvPGNKntnKHLJht1iTDM5vxShAhIpGgHE/u70q1QUu\n\tYEghkSndIdjDV/WsOR6/5K3ArAJe6i4IIBhbtdkuTZwKuosdIc3nIn9HRUhMQ4b6H7gV\n\tRxBvtcoH7WI4HUDNeYZ7+sNW8dJfy8OAKlCljkLAbYRfWY2eIgl8URhz+O0sMe83o7Nf\n\tV5kA==","X-Gm-Message-State":"AOAM532mFPwXZLCa4FpSVtLV5Brgzouki7WzdXajFc+iMt/UeNJJp+O5\n\tpS2vUjfWP0cNoAU7HY1RoO2aGc2v1sziCPJ0VvDNE9ud6vE1yQ==","X-Google-Smtp-Source":"ABdhPJw6GrUyc6ibyRY3kq72guhhid1K0w3PvIyGYRx48dDUtfkUmAhPHsJXHNlVSCsmFwPeUFVbJV+g8kEZ1P7xDXc=","X-Received":"by 2002:a19:5208:0:b0:472:292a:6d9b with SMTP id\n\tm8-20020a195208000000b00472292a6d9bmr9914119lfb.360.1651548600386;\n\tMon, 02 May 2022 20:30:00 -0700 (PDT)","MIME-Version":"1.0","References":"<20220426091409.1352047-1-chenghaoyang@chromium.org>\n\t<20220426091409.1352047-4-chenghaoyang@chromium.org>\n\t","In-Reply-To":"","Date":"Tue, 3 May 2022 11:29:49 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000a7de8a05de131fc0\"","Subject":"Re: [libcamera-devel] [PATCH v3 3/4] Rework JPEG encoder API and\n\tupdate PostProcessorJpeg and EncoderLibJpeg","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","From":"Cheng-Hao Yang via libcamera-devel\n\t","Reply-To":"Cheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23525,"web_url":"https://patchwork.libcamera.org/comment/23525/","msgid":"","date":"2022-06-22T12:37:58","subject":"Re: [libcamera-devel] [PATCH v3 3/4] Rework JPEG encoder API and\n\tupdate PostProcessorJpeg and EncoderLibJpeg","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nUploaded PATCH v5 and fixed the following:\n\n> > > int configure(const libcamera::StreamConfiguration &cfg)\noverride;\n> > > + int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> > > + libcamera::Span exifData,\n> > > + unsigned int quality) override;\n> > > + int generateThumbnail(\n> > > + const libcamera::FrameBuffer &source,\n> > > + const libcamera::Size &targetSize,\n> > > + unsigned int quality,\n> > > + std::vector *thumbnail) override;\n\n> > Same here.\n> I think I need your help on the indentation format. Any doc or rule I can\nstudy? Thanks!\n\nI was using gmail to view your comment. Now I use `\nhttps://patchwork.libcamera.org/patch`, and the tabs are correctly aligned.\nDone.\n\n\n> > You now use the same libjpeg encoder instance for both the main image\nand the thumbnail. This leads to EncoderLibJpeg::internalConfigure() being\ncalled twice for every frame. The function looks fairly cheap so it should\nbe fine, but could you confirm that jpeg_set_defaults() doesn't cause a\nlarge overhead ? Please mention this change in the commit message of the\nappropriate patch, it's an important one.\n\n> Originally for each frame we need at least one call to\njpeg_set_defaults() to encode the thumbnail, and we need two now with patch\nv3, so I thought it was fine in terms of the complexity, while you're right\nthat I'm not sure about the complexity of jpeg_set_defaults(), even looking\ninto the implementation [1]. I can also modify the code not to configure\ntwice per frame. WDYT?\n\nDone with only one jpeg_set_defaults() being called per frame. Please check.\n\n---------\n\nThanks!\n\nOn Tue, May 3, 2022 at 11:29 AM Cheng-Hao Yang \nwrote:\n\n> Hi Laurent,\n>\n> Sorry I forgot to send this reply before going on vacation...\n>\n> > The e-mail address should use \"@\" instead of \" at \".\n> Oh I see! I checked the libcamera archives for examples, and thought\n> that was intentional haha.\n> Updated.\n>\n> > cfg_ is only used here, I suppose to reconfigure the libjpeg encoder for\n> the\n> Is this a deprecated comment?\n>\n> > > int configure(const libcamera::StreamConfiguration &cfg)\n> override;\n> > > + int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> > > + libcamera::Span exifData,\n> > > + unsigned int quality) override;\n> > > + int generateThumbnail(\n> > > + const libcamera::FrameBuffer &source,\n> > > + const libcamera::Size &targetSize,\n> > > + unsigned int quality,\n> > > + std::vector *thumbnail) override;\n>\n> > Same here.\n> I think I need your help on the indentation format. Any doc or rule I can\n> study? Thanks!\n>\n> > I'm tempted to simplify this by using conditional compilation (we have a\n> OS_CHROMEOS macro), the function could then go to post_processor_jpeg.cpp.\n> Oh I didn't notice there's such a macro to use. Thanks! I think we can get\n> rid of the PostProcessorJpeg::createEncoder() then.\n>\n> > You now use the same libjpeg encoder instance for both the main image\n> and the thumbnail. This leads to EncoderLibJpeg::internalConfigure() being\n> called twice for every frame. The function looks fairly cheap so it should\n> be fine, but could you confirm that jpeg_set_defaults() doesn't cause a\n> large overhead ? Please mention this change in the commit message of the\n> appropriate patch, it's an important one.\n> Originally for each frame we need at least one call to jpeg_set_defaults()\n> to encode the thumbnail, and we need two now with patch v3, so I thought it\n> was fine in terms of the complexity, while you're right that I'm not sure\n> about the complexity of jpeg_set_defaults(), even looking into the\n> implementation [1]. I can also modify the code not to configure twice per\n> frame. WDYT?\n>\n> [1]: https://github.com/kornelski/libjpeg/blob/master/jcparam.c#L268\n>\n>\n> On Wed, Apr 27, 2022 at 7:44 AM Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n>\n>> Hi Harvey,\n>>\n>> Thank you for the patch.\n>>\n>> On Tue, Apr 26, 2022 at 09:14:08AM +0000, Harvey Yang via libcamera-devel\n>> wrote:\n>> > To add JEA in the next CL, this CL reworks JPEG encoder API and move\n>>\n>> Same comment as in 1/4 for \"CL\" (I won't repeat it for the other patches\n>> in this series, please update them as applicable).\n>>\n>> > thumbnail encoding code into EncoderLibJpeg's implementation, as JEA\n>> > supports that as well.\n>> >\n>> > Signed-off-by: Harvey Yang \n>>\n>> The e-mail address should use \"@\" instead of \" at \".\n>>\n>> > ---\n>> > src/android/jpeg/encoder.h | 9 ++-\n>> > src/android/jpeg/encoder_libjpeg.cpp | 70 +++++++++++++++++++\n>> > src/android/jpeg/encoder_libjpeg.h | 21 +++++-\n>> > .../jpeg/generic_post_processor_jpeg.cpp | 14 ++++\n>> > src/android/jpeg/meson.build | 9 +++\n>> > src/android/jpeg/post_processor_jpeg.cpp | 60 ++--------------\n>> > src/android/jpeg/post_processor_jpeg.h | 11 +--\n>> > src/android/meson.build | 5 +-\n>>\n>> This is more reviewable than in the previous version, but there are\n>> still quite a few changes bundled in the same patch. As a v4 will be\n>> needed anyway, could you split this further as follows ? Feel free to\n>> change the order as appropriate.\n>>\n>> - Add meson.build file in jpeg/ directory\n>> - Move thumbnail generate to Encoder class\n>> - Pass streamBuffer to encode() to prepare for JPEG encoders that need\n>> access to the HAL buffer handle\n>> - Add PostProcessorJpeg::SetEncoder()\n>>\n>> > 8 files changed, 128 insertions(+), 71 deletions(-)\n>> > create mode 100644 src/android/jpeg/generic_post_processor_jpeg.cpp\n>> > create mode 100644 src/android/jpeg/meson.build\n>> >\n>> > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n>> > index b974d367..6d527d91 100644\n>> > --- a/src/android/jpeg/encoder.h\n>> > +++ b/src/android/jpeg/encoder.h\n>> > @@ -12,14 +12,19 @@\n>> > #include \n>> > #include \n>> >\n>> > +#include \"../camera_request.h\"\n>> > +\n>> > class Encoder\n>> > {\n>> > public:\n>> > virtual ~Encoder() = default;\n>> >\n>> > virtual int configure(const libcamera::StreamConfiguration &cfg)\n>> = 0;\n>> > - virtual int encode(const libcamera::FrameBuffer &source,\n>> > - libcamera::Span destination,\n>> > + virtual int encode(Camera3RequestDescriptor::StreamBuffer\n>> *streamBuffer,\n>> > libcamera::Span exifData,\n>> > unsigned int quality) = 0;\n>> > + virtual int generateThumbnail(const libcamera::FrameBuffer\n>> &source,\n>> > + const libcamera::Size &targetSize,\n>> > + unsigned int quality,\n>> > + std::vector\n>> *thumbnail) = 0;\n>> > };\n>> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp\n>> b/src/android/jpeg/encoder_libjpeg.cpp\n>> > index 21a3b33d..b5591e33 100644\n>> > --- a/src/android/jpeg/encoder_libjpeg.cpp\n>> > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n>> > @@ -24,6 +24,8 @@\n>> > #include \"libcamera/internal/formats.h\"\n>> > #include \"libcamera/internal/mapped_framebuffer.h\"\n>> >\n>> > +#include \"../camera_buffer.h\"\n>> > +\n>> > using namespace libcamera;\n>> >\n>> > LOG_DECLARE_CATEGORY(JPEG)\n>> > @@ -82,8 +84,17 @@ EncoderLibJpeg::~EncoderLibJpeg()\n>> > }\n>> >\n>> > int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n>> > +{\n>> > + thumbnailer_.configure(cfg.size, cfg.pixelFormat);\n>> > + cfg_ = cfg;\n>>\n>> I'd store the size and pixel format only, not the full\n>> StreamConfiguration, as they are all you need. internalConfigure()\n>> should then take a size and pixel format, which will make it easier to\n>> use in generateThumbnail().\n>>\n>> > +\n>> > + return internalConfigure(cfg);\n>> > +}\n>> > +\n>> > +int EncoderLibJpeg::internalConfigure(const StreamConfiguration &cfg)\n>>\n>> Let's name the function configureEncoder().\n>>\n>> > {\n>> > const struct JPEGPixelFormatInfo info =\n>> findPixelInfo(cfg.pixelFormat);\n>> > +\n>> > if (info.colorSpace == JCS_UNKNOWN)\n>> > return -ENOTSUP;\n>> >\n>> > @@ -178,6 +189,65 @@ void EncoderLibJpeg::compressNV(const\n>> std::vector> &planes)\n>> > }\n>> > }\n>> >\n>> > +int EncoderLibJpeg::encode(Camera3RequestDescriptor::StreamBuffer\n>> *streamBuffer,\n>> > + libcamera::Span exifData,\n>> > + unsigned int quality)\n>> > +{\n>> > + internalConfigure(cfg_);\n>>\n>> cfg_ is only used here, I suppose to reconfigure the libjpeg encoder for\n>> the\n>>\n>> > + return encode(*streamBuffer->srcBuffer,\n>> streamBuffer->dstBuffer.get()->plane(0), exifData, quality);\n>>\n>> Let's shorten the line a bit (we aim for 80 columns as a soft target)\n>>\n>> return encode(*streamBuffer->srcBuffer,\n>> streamBuffer->dstBuffer.get()->plane(0),\n>> exifData, quality);\n>>\n>> > +}\n>> > +\n>> > +int EncoderLibJpeg::generateThumbnail(\n>> > + const libcamera::FrameBuffer &source,\n>> > + const libcamera::Size &targetSize,\n>> > + unsigned int quality,\n>> > + std::vector *thumbnail)\n>>\n>> int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer\n>> &source,\n>> const libcamera::Size &targetSize,\n>> unsigned int quality,\n>> std::vector\n>> *thumbnail)\n>>\n>> to match the coding style.\n>>\n>> > +{\n>> > + /* Stores the raw scaled-down thumbnail bytes. */\n>> > + std::vector rawThumbnail;\n>> > +\n>> > + thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n>> > +\n>> > + StreamConfiguration thCfg;\n>> > + thCfg.size = targetSize;\n>> > + thCfg.pixelFormat = thumbnailer_.pixelFormat();\n>> > + int ret = internalConfigure(thCfg);\n>>\n>> You now use the same libjpeg encoder instance for both the main image\n>> and the thumbnail. This leads to EncoderLibJpeg::internalConfigure()\n>> being called twice for every frame. The function looks fairly cheap so\n>> it should be fine, but could you confirm that jpeg_set_defaults()\n>> doesn't cause a large overhead ? Please mention this change in the\n>> commit message of the appropriate patch, it's an important one.\n>>\n>> > +\n>> > + if (!rawThumbnail.empty() && !ret) {\n>> > + /*\n>> > + * \\todo Avoid value-initialization of all elements of the\n>> > + * vector.\n>> > + */\n>> > + thumbnail->resize(rawThumbnail.size());\n>> > +\n>> > + /*\n>> > + * Split planes manually as the encoder expects a vector\n>> of\n>> > + * planes.\n>> > + *\n>> > + * \\todo Pass a vector of planes directly to\n>> > + * Thumbnailer::createThumbnailer above and remove the\n>> manual\n>> > + * planes split from here.\n>> > + */\n>> > + std::vector> thumbnailPlanes;\n>> > + const PixelFormatInfo &formatNV12 =\n>> PixelFormatInfo::info(formats::NV12);\n>> > + size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);\n>> > + size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);\n>> > + thumbnailPlanes.push_back({ rawThumbnail.data(),\n>> yPlaneSize });\n>> > + thumbnailPlanes.push_back({ rawThumbnail.data() +\n>> yPlaneSize, uvPlaneSize });\n>> > +\n>> > + int jpeg_size = encode(thumbnailPlanes, *thumbnail, {},\n>> quality);\n>> > + thumbnail->resize(jpeg_size);\n>> > +\n>> > + LOG(JPEG, Debug)\n>> > + << \"Thumbnail compress returned \"\n>> > + << jpeg_size << \" bytes\";\n>> > +\n>> > + return jpeg_size;\n>> > + }\n>> > +\n>> > + return -1;\n>> > +}\n>> > +\n>> > int EncoderLibJpeg::encode(const FrameBuffer &source, Span\n>> dest,\n>> > Span exifData, unsigned int\n>> quality)\n>> > {\n>> > diff --git a/src/android/jpeg/encoder_libjpeg.h\n>> b/src/android/jpeg/encoder_libjpeg.h\n>> > index 1b3ac067..56b27bae 100644\n>> > --- a/src/android/jpeg/encoder_libjpeg.h\n>> > +++ b/src/android/jpeg/encoder_libjpeg.h\n>> > @@ -15,6 +15,8 @@\n>> >\n>> > #include \n>> >\n>> > +#include \"thumbnailer.h\"\n>> > +\n>> > class EncoderLibJpeg : public Encoder\n>> > {\n>> > public:\n>> > @@ -22,19 +24,32 @@ public:\n>> > ~EncoderLibJpeg();\n>> >\n>> > int configure(const libcamera::StreamConfiguration &cfg) override;\n>> > + int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n>> > + libcamera::Span exifData,\n>> > + unsigned int quality) override;\n>> > + int generateThumbnail(\n>> > + const libcamera::FrameBuffer &source,\n>> > + const libcamera::Size &targetSize,\n>> > + unsigned int quality,\n>> > + std::vector *thumbnail) override;\n>>\n>> Same here.\n>>\n>> > +\n>> > +private:\n>> > + int internalConfigure(const libcamera::StreamConfiguration &cfg);\n>> > +\n>> > int encode(const libcamera::FrameBuffer &source,\n>> > libcamera::Span destination,\n>> > libcamera::Span exifData,\n>> > - unsigned int quality) override;\n>> > + unsigned int quality);\n>> > int encode(const std::vector> &planes,\n>> > libcamera::Span destination,\n>> > libcamera::Span exifData,\n>> > unsigned int quality);\n>> >\n>> > -private:\n>> > void compressRGB(const std::vector>\n>> &planes);\n>> > void compressNV(const std::vector>\n>> &planes);\n>> >\n>> > + libcamera::StreamConfiguration cfg_;\n>> > +\n>> > struct jpeg_compress_struct compress_;\n>> > struct jpeg_error_mgr jerr_;\n>> >\n>> > @@ -42,4 +57,6 @@ private:\n>> >\n>> > bool nv_;\n>> > bool nvSwap_;\n>> > +\n>> > + Thumbnailer thumbnailer_;\n>> > };\n>> > diff --git a/src/android/jpeg/generic_post_processor_jpeg.cpp\n>> b/src/android/jpeg/generic_post_processor_jpeg.cpp\n>> > new file mode 100644\n>> > index 00000000..890f6972\n>> > --- /dev/null\n>> > +++ b/src/android/jpeg/generic_post_processor_jpeg.cpp\n>> > @@ -0,0 +1,14 @@\n>> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> > +/*\n>> > + * Copyright (C) 2022, Google Inc.\n>> > + *\n>> > + * generic_post_processor_jpeg.cpp - Generic JPEG Post Processor\n>> > + */\n>> > +\n>> > +#include \"encoder_libjpeg.h\"\n>> > +#include \"post_processor_jpeg.h\"\n>> > +\n>> > +void PostProcessorJpeg::SetEncoder()\n>>\n>> We use camelCase for function names, not CamelCase. Let's name the\n>> function createEncoder(), as it doesn't just set it but actually creates\n>> it.\n>>\n>> > +{\n>> > + encoder_ = std::make_unique();\n>>\n>> I'm tempted to simplify this by using conditional compilation (we have a\n>> OS_CHROMEOS macro), the function could then go to\n>> post_processor_jpeg.cpp.\n>>\n>> > +}\n>> > diff --git a/src/android/jpeg/meson.build b/src/android/jpeg/meson.build\n>> > new file mode 100644\n>> > index 00000000..94522dc0\n>> > --- /dev/null\n>> > +++ b/src/android/jpeg/meson.build\n>> > @@ -0,0 +1,9 @@\n>> > +# SPDX-License-Identifier: CC0-1.0\n>> > +\n>> > +android_hal_sources += files([\n>> > + 'exif.cpp',\n>> > + 'post_processor_jpeg.cpp'])\n>>\n>> The '])' should be on the next line.\n>>\n>> > +\n>> > +android_hal_sources += files(['encoder_libjpeg.cpp',\n>> > + 'generic_post_processor_jpeg.cpp',\n>> > + 'thumbnailer.cpp'])\n>>\n>> Similar comment here,\n>>\n>> android_hal_sources += files([\n>> 'encoder_libjpeg.cpp',\n>> 'generic_post_processor_jpeg.cpp',\n>> 'thumbnailer.cpp',\n>> ])\n>>\n>> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp\n>> b/src/android/jpeg/post_processor_jpeg.cpp\n>> > index d72ebc3c..7ceba60e 100644\n>> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n>> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>> > @@ -9,10 +9,10 @@\n>> >\n>> > #include \n>> >\n>> > +#include \"../android_framebuffer.h\"\n>>\n>> Is this needed ?\n>>\n>> > #include \"../camera_device.h\"\n>> > #include \"../camera_metadata.h\"\n>> > #include \"../camera_request.h\"\n>> > -#include \"encoder_libjpeg.h\"\n>> > #include \"exif.h\"\n>> >\n>> > #include \n>> > @@ -44,60 +44,11 @@ int PostProcessorJpeg::configure(const\n>> StreamConfiguration &inCfg,\n>> >\n>> > streamSize_ = outCfg.size;\n>> >\n>> > - thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n>> > -\n>> > - encoder_ = std::make_unique();\n>> > + SetEncoder();\n>> >\n>> > return encoder_->configure(inCfg);\n>> > }\n>> >\n>> > -void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n>> > - const Size &targetSize,\n>> > - unsigned int quality,\n>> > - std::vector\n>> *thumbnail)\n>> > -{\n>> > - /* Stores the raw scaled-down thumbnail bytes. */\n>> > - std::vector rawThumbnail;\n>> > -\n>> > - thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n>> > -\n>> > - StreamConfiguration thCfg;\n>> > - thCfg.size = targetSize;\n>> > - thCfg.pixelFormat = thumbnailer_.pixelFormat();\n>> > - int ret = thumbnailEncoder_.configure(thCfg);\n>> > -\n>> > - if (!rawThumbnail.empty() && !ret) {\n>> > - /*\n>> > - * \\todo Avoid value-initialization of all elements of the\n>> > - * vector.\n>> > - */\n>> > - thumbnail->resize(rawThumbnail.size());\n>> > -\n>> > - /*\n>> > - * Split planes manually as the encoder expects a vector\n>> of\n>> > - * planes.\n>> > - *\n>> > - * \\todo Pass a vector of planes directly to\n>> > - * Thumbnailer::createThumbnailer above and remove the\n>> manual\n>> > - * planes split from here.\n>> > - */\n>> > - std::vector> thumbnailPlanes;\n>> > - const PixelFormatInfo &formatNV12 =\n>> PixelFormatInfo::info(formats::NV12);\n>> > - size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);\n>> > - size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);\n>> > - thumbnailPlanes.push_back({ rawThumbnail.data(),\n>> yPlaneSize });\n>> > - thumbnailPlanes.push_back({ rawThumbnail.data() +\n>> yPlaneSize, uvPlaneSize });\n>> > -\n>> > - int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,\n>> > - *thumbnail, {},\n>> quality);\n>> > - thumbnail->resize(jpeg_size);\n>> > -\n>> > - LOG(JPEG, Debug)\n>> > - << \"Thumbnail compress returned \"\n>> > - << jpeg_size << \" bytes\";\n>> > - }\n>> > -}\n>> > -\n>> > void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer\n>> *streamBuffer)\n>> > {\n>> > ASSERT(encoder_);\n>> > @@ -164,8 +115,8 @@ void\n>> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu\n>> >\n>> > if (thumbnailSize != Size(0, 0)) {\n>> > std::vector thumbnail;\n>> > - generateThumbnail(source, thumbnailSize, quality,\n>> &thumbnail);\n>> > - if (!thumbnail.empty())\n>> > + ret = encoder_->generateThumbnail(source,\n>> thumbnailSize, quality, &thumbnail);\n>> > + if (ret > 0 && !thumbnail.empty())\n>> > exif.setThumbnail(thumbnail,\n>> Exif::Compression::JPEG);\n>> > }\n>> >\n>> > @@ -194,8 +145,7 @@ void\n>> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu\n>> > const uint8_t quality = ret ? *entry.data.u8 : 95;\n>> > resultMetadata->addEntry(ANDROID_JPEG_QUALITY, quality);\n>> >\n>> > - int jpeg_size = encoder_->encode(source, destination->plane(0),\n>> > - exif.data(), quality);\n>> > + int jpeg_size = encoder_->encode(streamBuffer, exif.data(),\n>> quality);\n>> > if (jpeg_size < 0) {\n>> > LOG(JPEG, Error) << \"Failed to encode stream image\";\n>> > processComplete.emit(streamBuffer,\n>> PostProcessor::Status::Error);\n>> > diff --git a/src/android/jpeg/post_processor_jpeg.h\n>> b/src/android/jpeg/post_processor_jpeg.h\n>> > index 98309b01..a09f8798 100644\n>> > --- a/src/android/jpeg/post_processor_jpeg.h\n>> > +++ b/src/android/jpeg/post_processor_jpeg.h\n>> > @@ -8,11 +8,11 @@\n>> > #pragma once\n>> >\n>> > #include \"../post_processor.h\"\n>> > -#include \"encoder_libjpeg.h\"\n>> > -#include \"thumbnailer.h\"\n>> >\n>> > #include \n>> >\n>> > +#include \"encoder.h\"\n>> > +\n>> > class CameraDevice;\n>> >\n>> > class PostProcessorJpeg : public PostProcessor\n>> > @@ -25,14 +25,9 @@ public:\n>> > void process(Camera3RequestDescriptor::StreamBuffer\n>> *streamBuffer) override;\n>> >\n>> > private:\n>> > - void generateThumbnail(const libcamera::FrameBuffer &source,\n>> > - const libcamera::Size &targetSize,\n>> > - unsigned int quality,\n>> > - std::vector *thumbnail);\n>> > + void SetEncoder();\n>> >\n>> > CameraDevice *const cameraDevice_;\n>> > std::unique_ptr encoder_;\n>> > libcamera::Size streamSize_;\n>> > - EncoderLibJpeg thumbnailEncoder_;\n>> > - Thumbnailer thumbnailer_;\n>> > };\n>> > diff --git a/src/android/meson.build b/src/android/meson.build\n>> > index 27be27bb..026b8b3c 100644\n>> > --- a/src/android/meson.build\n>> > +++ b/src/android/meson.build\n>> > @@ -48,10 +48,6 @@ android_hal_sources = files([\n>> > 'camera_ops.cpp',\n>> > 'camera_request.cpp',\n>> > 'camera_stream.cpp',\n>> > - 'jpeg/encoder_libjpeg.cpp',\n>> > - 'jpeg/exif.cpp',\n>> > - 'jpeg/post_processor_jpeg.cpp',\n>> > - 'jpeg/thumbnailer.cpp',\n>> > 'yuv/post_processor_yuv.cpp'\n>> > ])\n>> >\n>> > @@ -59,6 +55,7 @@ android_cpp_args = []\n>> >\n>> > subdir('cros')\n>> > subdir('mm')\n>> > +subdir('jpeg')\n>>\n>> Please keep these alphabetically sorted.\n>>\n>> >\n>> > android_camera_metadata_sources = files([\n>> > 'metadata/camera_metadata.c',\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart\n>>\n>","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 1EE87BE173\n\tfor ;\n\tWed, 22 Jun 2022 12:38:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C404465639;\n\tWed, 22 Jun 2022 14:38:12 +0200 (CEST)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A232261FB2\n\tfor ;\n\tWed, 22 Jun 2022 14:38:11 +0200 (CEST)","by mail-lj1-x232.google.com with SMTP id b23so10609498ljh.7\n\tfor ;\n\tWed, 22 Jun 2022 05:38:11 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655901492;\n\tbh=OJ9DIrX9NMneEQ8dUswRPn2GIlpqZ6CeuJi0yfuNo2k=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=BlrASgOSegzPt5OhWJP019rfRZVamYap5rCDndDRH2Pz8VlbkdbSXauJSwb8PXGqa\n\tyQFY2ruZHHjJr95B9CQ3dtfS3CjTRI4XVU29W3IkwvIwk8EKXkDYgxz8H7qk6YCeFZ\n\tWLuV9ccrvdJvn0flRUiS7GaBJBaHqd0QE+lMfhVqJ2M3QQ37NCrWQfUvrn11GsP87A\n\t08fjXbQJUFrzgxjbWlnNAqrGFKEximDMKnRVjICS6O65k0xGstkGMhwgyXPSIRYwxg\n\tEWBjoz6kb1/2uL5oXhAKrKo50BM9i0LLX3UhZ5iudbBmg8AjCc5jUGbfNaAAv4Ynee\n\tn4hQmx2/+mJIQ==","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=i3TpWJBjLEJK9UFl0eYFWVCitTOSOwXfR+A0Uc2mqFw=;\n\tb=UqPcFI+BOpuL6QoPUNC2OceFugzJ5cXZGFEuVbytPFhD0lXrOT+TShnWB8JqqeTtt3\n\tEHdiWfZ4Gk7TFxuqx3uy71ck9IIkFltivuqI1tXx3ulKgB/92Zb6KrqOuydqHxten5Z0\n\ttqS6RntcIKcX/EUVXpGVVV8AbVKWNMWSIprS0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"UqPcFI+B\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=i3TpWJBjLEJK9UFl0eYFWVCitTOSOwXfR+A0Uc2mqFw=;\n\tb=hDxUZa4UMuvsBhMMhNfbDN3zC+rOKxRsKLD+ucid411k/uSs0P6JUdPJUaTj26tiCy\n\t7bS2PC9r4idXEHRi/C1ZVCjF+ubkpFbiwniQXX0Ngg5E2HbTztAwmEn63jUbmjLd7shh\n\tSaNBp9ZvZJNz0XwjotKzKHGZeoGYydsjWNgdi6Foex69QcYszRnhjcgynHcAEy4qfpnV\n\tBdFHTjBQe9ODLEfk4ZR1hvMA7ljLCh7MVO8XF6x/gVVCx+qZOqdMc7GRj0cmfMlsK6S3\n\tgoSJkELVCFBxw6FxlRrPzg267qfvvu4Q15aQFnlzqN1JS9cIifPmZOKxt7w7Kt9IbbEc\n\tOMbQ==","X-Gm-Message-State":"AJIora/b4szuWozwE/CZdTf0xtVvvNt2XPpf2PFfufrtTE48LdFEZo+x\n\tCP0CZxbYqjNYYFOB+WvtDSM87Ssw6hsAyyU3BXjNISZifPphoQ==","X-Google-Smtp-Source":"AGRyM1sK1CQ/j66ajVWyc6RQI+6zFKAmsCy5s2DFsTrpRz01f+tmIrCdtUCcy+EZu3i+BC1BJdSMgkqsZ87d9Tv/q7s=","X-Received":"by 2002:a05:651c:b2b:b0:255:9297:d4d with SMTP id\n\tb43-20020a05651c0b2b00b0025592970d4dmr1836341ljr.66.1655901489960;\n\tWed, 22 Jun 2022 05:38:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20220426091409.1352047-1-chenghaoyang@chromium.org>\n\t<20220426091409.1352047-4-chenghaoyang@chromium.org>\n\t\n\t","In-Reply-To":"","Date":"Wed, 22 Jun 2022 20:37:58 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000017a53f05e2089c77\"","Subject":"Re: [libcamera-devel] [PATCH v3 3/4] Rework JPEG encoder API and\n\tupdate PostProcessorJpeg and EncoderLibJpeg","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","From":"Cheng-Hao Yang via libcamera-devel\n\t","Reply-To":"Cheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24888,"web_url":"https://patchwork.libcamera.org/comment/24888/","msgid":"","date":"2022-09-01T20:09:24","subject":"Re: [libcamera-devel] [PATCH v3 3/4] Rework JPEG encoder API and\n\tupdate PostProcessorJpeg and EncoderLibJpeg","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey,\n\nOn Wed, Jun 22, 2022 at 08:37:58PM +0800, Cheng-Hao Yang wrote:\n> Hi Laurent,\n> \n> Uploaded PATCH v5 and fixed the following:\n> \n> > > > int configure(const libcamera::StreamConfiguration &cfg) override;\n> > > > + int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> > > > + libcamera::Span exifData,\n> > > > + unsigned int quality) override;\n> > > > + int generateThumbnail(\n> > > > + const libcamera::FrameBuffer &source,\n> > > > + const libcamera::Size &targetSize,\n> > > > + unsigned int quality,\n> > > > + std::vector *thumbnail) override;\n> > >\n> > > Same here.\n> > \n> > I think I need your help on the indentation format. Any doc or rule I can\n> > study? Thanks!\n> \n> I was using gmail to view your comment. Now I use `\n> https://patchwork.libcamera.org/patch`, and the tabs are correctly aligned.\n> Done.\n\nI'm afraid I can't really help with getting gmail to display the e-mail\ncontents properly. Maybe you have more leverage than I do there ;-)\nJokes aside, I think most people avoid webmail for patch review.\n\nWhile on the topic of e-mails, could you plese try to reply inline in\nthe patch instead of copying content and answers to the beginning ? It\ngets difficult to get the whole context otherwise.\n\n> > > You now use the same libjpeg encoder instance for both the main image\n> > > and the thumbnail. This leads to EncoderLibJpeg::internalConfigure() being\n> > > called twice for every frame. The function looks fairly cheap so it should\n> > > be fine, but could you confirm that jpeg_set_defaults() doesn't cause a\n> > > large overhead ? Please mention this change in the commit message of the\n> > > appropriate patch, it's an important one.\n> > \n> > Originally for each frame we need at least one call to\n> > jpeg_set_defaults() to encode the thumbnail, and we need two now with patch\n> > v3, so I thought it was fine in terms of the complexity, while you're right\n> > that I'm not sure about the complexity of jpeg_set_defaults(), even looking\n> > into the implementation [1]. I can also modify the code not to configure\n> > twice per frame. WDYT?\n> \n> Done with only one jpeg_set_defaults() being called per frame. Please check.\n\nThank you.\n\n> On Tue, May 3, 2022 at 11:29 AM Cheng-Hao Yang wrote:\n> \n> > Hi Laurent,\n> >\n> > Sorry I forgot to send this reply before going on vacation...\n> >\n> > > The e-mail address should use \"@\" instead of \" at \".\n> > Oh I see! I checked the libcamera archives for examples, and thought\n> > that was intentional haha.\n> > Updated.\n> >\n> > > cfg_ is only used here, I suppose to reconfigure the libjpeg encoder for\n> > the\n> > Is this a deprecated comment?\n> >\n> > > > int configure(const libcamera::StreamConfiguration &cfg)\n> > override;\n> > > > + int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> > > > + libcamera::Span exifData,\n> > > > + unsigned int quality) override;\n> > > > + int generateThumbnail(\n> > > > + const libcamera::FrameBuffer &source,\n> > > > + const libcamera::Size &targetSize,\n> > > > + unsigned int quality,\n> > > > + std::vector *thumbnail) override;\n> >\n> > > Same here.\n> > I think I need your help on the indentation format. Any doc or rule I can\n> > study? Thanks!\n> >\n> > > I'm tempted to simplify this by using conditional compilation (we have a\n> > OS_CHROMEOS macro), the function could then go to post_processor_jpeg.cpp.\n> > Oh I didn't notice there's such a macro to use. Thanks! I think we can get\n> > rid of the PostProcessorJpeg::createEncoder() then.\n> >\n> > > You now use the same libjpeg encoder instance for both the main image\n> > and the thumbnail. This leads to EncoderLibJpeg::internalConfigure() being\n> > called twice for every frame. The function looks fairly cheap so it should\n> > be fine, but could you confirm that jpeg_set_defaults() doesn't cause a\n> > large overhead ? Please mention this change in the commit message of the\n> > appropriate patch, it's an important one.\n> > Originally for each frame we need at least one call to jpeg_set_defaults()\n> > to encode the thumbnail, and we need two now with patch v3, so I thought it\n> > was fine in terms of the complexity, while you're right that I'm not sure\n> > about the complexity of jpeg_set_defaults(), even looking into the\n> > implementation [1]. I can also modify the code not to configure twice per\n> > frame. WDYT?\n> >\n> > [1]: https://github.com/kornelski/libjpeg/blob/master/jcparam.c#L268\n> >\n> >\n> > On Wed, Apr 27, 2022 at 7:44 AM Laurent Pinchart <\n> > laurent.pinchart@ideasonboard.com> wrote:\n> >\n> >> Hi Harvey,\n> >>\n> >> Thank you for the patch.\n> >>\n> >> On Tue, Apr 26, 2022 at 09:14:08AM +0000, Harvey Yang via libcamera-devel\n> >> wrote:\n> >> > To add JEA in the next CL, this CL reworks JPEG encoder API and move\n> >>\n> >> Same comment as in 1/4 for \"CL\" (I won't repeat it for the other patches\n> >> in this series, please update them as applicable).\n> >>\n> >> > thumbnail encoding code into EncoderLibJpeg's implementation, as JEA\n> >> > supports that as well.\n> >> >\n> >> > Signed-off-by: Harvey Yang \n> >>\n> >> The e-mail address should use \"@\" instead of \" at \".\n> >>\n> >> > ---\n> >> > src/android/jpeg/encoder.h | 9 ++-\n> >> > src/android/jpeg/encoder_libjpeg.cpp | 70 +++++++++++++++++++\n> >> > src/android/jpeg/encoder_libjpeg.h | 21 +++++-\n> >> > .../jpeg/generic_post_processor_jpeg.cpp | 14 ++++\n> >> > src/android/jpeg/meson.build | 9 +++\n> >> > src/android/jpeg/post_processor_jpeg.cpp | 60 ++--------------\n> >> > src/android/jpeg/post_processor_jpeg.h | 11 +--\n> >> > src/android/meson.build | 5 +-\n> >>\n> >> This is more reviewable than in the previous version, but there are\n> >> still quite a few changes bundled in the same patch. As a v4 will be\n> >> needed anyway, could you split this further as follows ? Feel free to\n> >> change the order as appropriate.\n> >>\n> >> - Add meson.build file in jpeg/ directory\n> >> - Move thumbnail generate to Encoder class\n> >> - Pass streamBuffer to encode() to prepare for JPEG encoders that need\n> >> access to the HAL buffer handle\n> >> - Add PostProcessorJpeg::SetEncoder()\n> >>\n> >> > 8 files changed, 128 insertions(+), 71 deletions(-)\n> >> > create mode 100644 src/android/jpeg/generic_post_processor_jpeg.cpp\n> >> > create mode 100644 src/android/jpeg/meson.build\n> >> >\n> >> > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> >> > index b974d367..6d527d91 100644\n> >> > --- a/src/android/jpeg/encoder.h\n> >> > +++ b/src/android/jpeg/encoder.h\n> >> > @@ -12,14 +12,19 @@\n> >> > #include \n> >> > #include \n> >> >\n> >> > +#include \"../camera_request.h\"\n> >> > +\n> >> > class Encoder\n> >> > {\n> >> > public:\n> >> > virtual ~Encoder() = default;\n> >> >\n> >> > virtual int configure(const libcamera::StreamConfiguration &cfg)\n> >> = 0;\n> >> > - virtual int encode(const libcamera::FrameBuffer &source,\n> >> > - libcamera::Span destination,\n> >> > + virtual int encode(Camera3RequestDescriptor::StreamBuffer\n> >> *streamBuffer,\n> >> > libcamera::Span exifData,\n> >> > unsigned int quality) = 0;\n> >> > + virtual int generateThumbnail(const libcamera::FrameBuffer\n> >> &source,\n> >> > + const libcamera::Size &targetSize,\n> >> > + unsigned int quality,\n> >> > + std::vector\n> >> *thumbnail) = 0;\n> >> > };\n> >> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp\n> >> b/src/android/jpeg/encoder_libjpeg.cpp\n> >> > index 21a3b33d..b5591e33 100644\n> >> > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> >> > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> >> > @@ -24,6 +24,8 @@\n> >> > #include \"libcamera/internal/formats.h\"\n> >> > #include \"libcamera/internal/mapped_framebuffer.h\"\n> >> >\n> >> > +#include \"../camera_buffer.h\"\n> >> > +\n> >> > using namespace libcamera;\n> >> >\n> >> > LOG_DECLARE_CATEGORY(JPEG)\n> >> > @@ -82,8 +84,17 @@ EncoderLibJpeg::~EncoderLibJpeg()\n> >> > }\n> >> >\n> >> > int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> >> > +{\n> >> > + thumbnailer_.configure(cfg.size, cfg.pixelFormat);\n> >> > + cfg_ = cfg;\n> >>\n> >> I'd store the size and pixel format only, not the full\n> >> StreamConfiguration, as they are all you need. internalConfigure()\n> >> should then take a size and pixel format, which will make it easier to\n> >> use in generateThumbnail().\n> >>\n> >> > +\n> >> > + return internalConfigure(cfg);\n> >> > +}\n> >> > +\n> >> > +int EncoderLibJpeg::internalConfigure(const StreamConfiguration &cfg)\n> >>\n> >> Let's name the function configureEncoder().\n> >>\n> >> > {\n> >> > const struct JPEGPixelFormatInfo info =\n> >> findPixelInfo(cfg.pixelFormat);\n> >> > +\n> >> > if (info.colorSpace == JCS_UNKNOWN)\n> >> > return -ENOTSUP;\n> >> >\n> >> > @@ -178,6 +189,65 @@ void EncoderLibJpeg::compressNV(const\n> >> std::vector> &planes)\n> >> > }\n> >> > }\n> >> >\n> >> > +int EncoderLibJpeg::encode(Camera3RequestDescriptor::StreamBuffer\n> >> *streamBuffer,\n> >> > + libcamera::Span exifData,\n> >> > + unsigned int quality)\n> >> > +{\n> >> > + internalConfigure(cfg_);\n> >>\n> >> cfg_ is only used here, I suppose to reconfigure the libjpeg encoder for\n> >> the\n> >>\n> >> > + return encode(*streamBuffer->srcBuffer,\n> >> streamBuffer->dstBuffer.get()->plane(0), exifData, quality);\n> >>\n> >> Let's shorten the line a bit (we aim for 80 columns as a soft target)\n> >>\n> >> return encode(*streamBuffer->srcBuffer,\n> >> streamBuffer->dstBuffer.get()->plane(0),\n> >> exifData, quality);\n> >>\n> >> > +}\n> >> > +\n> >> > +int EncoderLibJpeg::generateThumbnail(\n> >> > + const libcamera::FrameBuffer &source,\n> >> > + const libcamera::Size &targetSize,\n> >> > + unsigned int quality,\n> >> > + std::vector *thumbnail)\n> >>\n> >> int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer\n> >> &source,\n> >> const libcamera::Size &targetSize,\n> >> unsigned int quality,\n> >> std::vector\n> >> *thumbnail)\n> >>\n> >> to match the coding style.\n> >>\n> >> > +{\n> >> > + /* Stores the raw scaled-down thumbnail bytes. */\n> >> > + std::vector rawThumbnail;\n> >> > +\n> >> > + thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n> >> > +\n> >> > + StreamConfiguration thCfg;\n> >> > + thCfg.size = targetSize;\n> >> > + thCfg.pixelFormat = thumbnailer_.pixelFormat();\n> >> > + int ret = internalConfigure(thCfg);\n> >>\n> >> You now use the same libjpeg encoder instance for both the main image\n> >> and the thumbnail. This leads to EncoderLibJpeg::internalConfigure()\n> >> being called twice for every frame. The function looks fairly cheap so\n> >> it should be fine, but could you confirm that jpeg_set_defaults()\n> >> doesn't cause a large overhead ? Please mention this change in the\n> >> commit message of the appropriate patch, it's an important one.\n> >>\n> >> > +\n> >> > + if (!rawThumbnail.empty() && !ret) {\n> >> > + /*\n> >> > + * \\todo Avoid value-initialization of all elements of the\n> >> > + * vector.\n> >> > + */\n> >> > + thumbnail->resize(rawThumbnail.size());\n> >> > +\n> >> > + /*\n> >> > + * Split planes manually as the encoder expects a vector\n> >> of\n> >> > + * planes.\n> >> > + *\n> >> > + * \\todo Pass a vector of planes directly to\n> >> > + * Thumbnailer::createThumbnailer above and remove the\n> >> manual\n> >> > + * planes split from here.\n> >> > + */\n> >> > + std::vector> thumbnailPlanes;\n> >> > + const PixelFormatInfo &formatNV12 =\n> >> PixelFormatInfo::info(formats::NV12);\n> >> > + size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);\n> >> > + size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);\n> >> > + thumbnailPlanes.push_back({ rawThumbnail.data(),\n> >> yPlaneSize });\n> >> > + thumbnailPlanes.push_back({ rawThumbnail.data() +\n> >> yPlaneSize, uvPlaneSize });\n> >> > +\n> >> > + int jpeg_size = encode(thumbnailPlanes, *thumbnail, {},\n> >> quality);\n> >> > + thumbnail->resize(jpeg_size);\n> >> > +\n> >> > + LOG(JPEG, Debug)\n> >> > + << \"Thumbnail compress returned \"\n> >> > + << jpeg_size << \" bytes\";\n> >> > +\n> >> > + return jpeg_size;\n> >> > + }\n> >> > +\n> >> > + return -1;\n> >> > +}\n> >> > +\n> >> > int EncoderLibJpeg::encode(const FrameBuffer &source, Span\n> >> dest,\n> >> > Span exifData, unsigned int\n> >> quality)\n> >> > {\n> >> > diff --git a/src/android/jpeg/encoder_libjpeg.h\n> >> b/src/android/jpeg/encoder_libjpeg.h\n> >> > index 1b3ac067..56b27bae 100644\n> >> > --- a/src/android/jpeg/encoder_libjpeg.h\n> >> > +++ b/src/android/jpeg/encoder_libjpeg.h\n> >> > @@ -15,6 +15,8 @@\n> >> >\n> >> > #include \n> >> >\n> >> > +#include \"thumbnailer.h\"\n> >> > +\n> >> > class EncoderLibJpeg : public Encoder\n> >> > {\n> >> > public:\n> >> > @@ -22,19 +24,32 @@ public:\n> >> > ~EncoderLibJpeg();\n> >> >\n> >> > int configure(const libcamera::StreamConfiguration &cfg) override;\n> >> > + int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> >> > + libcamera::Span exifData,\n> >> > + unsigned int quality) override;\n> >> > + int generateThumbnail(\n> >> > + const libcamera::FrameBuffer &source,\n> >> > + const libcamera::Size &targetSize,\n> >> > + unsigned int quality,\n> >> > + std::vector *thumbnail) override;\n> >>\n> >> Same here.\n> >>\n> >> > +\n> >> > +private:\n> >> > + int internalConfigure(const libcamera::StreamConfiguration &cfg);\n> >> > +\n> >> > int encode(const libcamera::FrameBuffer &source,\n> >> > libcamera::Span destination,\n> >> > libcamera::Span exifData,\n> >> > - unsigned int quality) override;\n> >> > + unsigned int quality);\n> >> > int encode(const std::vector> &planes,\n> >> > libcamera::Span destination,\n> >> > libcamera::Span exifData,\n> >> > unsigned int quality);\n> >> >\n> >> > -private:\n> >> > void compressRGB(const std::vector>\n> >> &planes);\n> >> > void compressNV(const std::vector>\n> >> &planes);\n> >> >\n> >> > + libcamera::StreamConfiguration cfg_;\n> >> > +\n> >> > struct jpeg_compress_struct compress_;\n> >> > struct jpeg_error_mgr jerr_;\n> >> >\n> >> > @@ -42,4 +57,6 @@ private:\n> >> >\n> >> > bool nv_;\n> >> > bool nvSwap_;\n> >> > +\n> >> > + Thumbnailer thumbnailer_;\n> >> > };\n> >> > diff --git a/src/android/jpeg/generic_post_processor_jpeg.cpp\n> >> b/src/android/jpeg/generic_post_processor_jpeg.cpp\n> >> > new file mode 100644\n> >> > index 00000000..890f6972\n> >> > --- /dev/null\n> >> > +++ b/src/android/jpeg/generic_post_processor_jpeg.cpp\n> >> > @@ -0,0 +1,14 @@\n> >> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> > +/*\n> >> > + * Copyright (C) 2022, Google Inc.\n> >> > + *\n> >> > + * generic_post_processor_jpeg.cpp - Generic JPEG Post Processor\n> >> > + */\n> >> > +\n> >> > +#include \"encoder_libjpeg.h\"\n> >> > +#include \"post_processor_jpeg.h\"\n> >> > +\n> >> > +void PostProcessorJpeg::SetEncoder()\n> >>\n> >> We use camelCase for function names, not CamelCase. Let's name the\n> >> function createEncoder(), as it doesn't just set it but actually creates\n> >> it.\n> >>\n> >> > +{\n> >> > + encoder_ = std::make_unique();\n> >>\n> >> I'm tempted to simplify this by using conditional compilation (we have a\n> >> OS_CHROMEOS macro), the function could then go to\n> >> post_processor_jpeg.cpp.\n> >>\n> >> > +}\n> >> > diff --git a/src/android/jpeg/meson.build b/src/android/jpeg/meson.build\n> >> > new file mode 100644\n> >> > index 00000000..94522dc0\n> >> > --- /dev/null\n> >> > +++ b/src/android/jpeg/meson.build\n> >> > @@ -0,0 +1,9 @@\n> >> > +# SPDX-License-Identifier: CC0-1.0\n> >> > +\n> >> > +android_hal_sources += files([\n> >> > + 'exif.cpp',\n> >> > + 'post_processor_jpeg.cpp'])\n> >>\n> >> The '])' should be on the next line.\n> >>\n> >> > +\n> >> > +android_hal_sources += files(['encoder_libjpeg.cpp',\n> >> > + 'generic_post_processor_jpeg.cpp',\n> >> > + 'thumbnailer.cpp'])\n> >>\n> >> Similar comment here,\n> >>\n> >> android_hal_sources += files([\n> >> 'encoder_libjpeg.cpp',\n> >> 'generic_post_processor_jpeg.cpp',\n> >> 'thumbnailer.cpp',\n> >> ])\n> >>\n> >> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp\n> >> b/src/android/jpeg/post_processor_jpeg.cpp\n> >> > index d72ebc3c..7ceba60e 100644\n> >> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> >> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> >> > @@ -9,10 +9,10 @@\n> >> >\n> >> > #include \n> >> >\n> >> > +#include \"../android_framebuffer.h\"\n> >>\n> >> Is this needed ?\n> >>\n> >> > #include \"../camera_device.h\"\n> >> > #include \"../camera_metadata.h\"\n> >> > #include \"../camera_request.h\"\n> >> > -#include \"encoder_libjpeg.h\"\n> >> > #include \"exif.h\"\n> >> >\n> >> > #include \n> >> > @@ -44,60 +44,11 @@ int PostProcessorJpeg::configure(const\n> >> StreamConfiguration &inCfg,\n> >> >\n> >> > streamSize_ = outCfg.size;\n> >> >\n> >> > - thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> >> > -\n> >> > - encoder_ = std::make_unique();\n> >> > + SetEncoder();\n> >> >\n> >> > return encoder_->configure(inCfg);\n> >> > }\n> >> >\n> >> > -void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> >> > - const Size &targetSize,\n> >> > - unsigned int quality,\n> >> > - std::vector\n> >> *thumbnail)\n> >> > -{\n> >> > - /* Stores the raw scaled-down thumbnail bytes. */\n> >> > - std::vector rawThumbnail;\n> >> > -\n> >> > - thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n> >> > -\n> >> > - StreamConfiguration thCfg;\n> >> > - thCfg.size = targetSize;\n> >> > - thCfg.pixelFormat = thumbnailer_.pixelFormat();\n> >> > - int ret = thumbnailEncoder_.configure(thCfg);\n> >> > -\n> >> > - if (!rawThumbnail.empty() && !ret) {\n> >> > - /*\n> >> > - * \\todo Avoid value-initialization of all elements of the\n> >> > - * vector.\n> >> > - */\n> >> > - thumbnail->resize(rawThumbnail.size());\n> >> > -\n> >> > - /*\n> >> > - * Split planes manually as the encoder expects a vector\n> >> of\n> >> > - * planes.\n> >> > - *\n> >> > - * \\todo Pass a vector of planes directly to\n> >> > - * Thumbnailer::createThumbnailer above and remove the\n> >> manual\n> >> > - * planes split from here.\n> >> > - */\n> >> > - std::vector> thumbnailPlanes;\n> >> > - const PixelFormatInfo &formatNV12 =\n> >> PixelFormatInfo::info(formats::NV12);\n> >> > - size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);\n> >> > - size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);\n> >> > - thumbnailPlanes.push_back({ rawThumbnail.data(),\n> >> yPlaneSize });\n> >> > - thumbnailPlanes.push_back({ rawThumbnail.data() +\n> >> yPlaneSize, uvPlaneSize });\n> >> > -\n> >> > - int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,\n> >> > - *thumbnail, {},\n> >> quality);\n> >> > - thumbnail->resize(jpeg_size);\n> >> > -\n> >> > - LOG(JPEG, Debug)\n> >> > - << \"Thumbnail compress returned \"\n> >> > - << jpeg_size << \" bytes\";\n> >> > - }\n> >> > -}\n> >> > -\n> >> > void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer\n> >> *streamBuffer)\n> >> > {\n> >> > ASSERT(encoder_);\n> >> > @@ -164,8 +115,8 @@ void\n> >> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu\n> >> >\n> >> > if (thumbnailSize != Size(0, 0)) {\n> >> > std::vector thumbnail;\n> >> > - generateThumbnail(source, thumbnailSize, quality,\n> >> &thumbnail);\n> >> > - if (!thumbnail.empty())\n> >> > + ret = encoder_->generateThumbnail(source,\n> >> thumbnailSize, quality, &thumbnail);\n> >> > + if (ret > 0 && !thumbnail.empty())\n> >> > exif.setThumbnail(thumbnail,\n> >> Exif::Compression::JPEG);\n> >> > }\n> >> >\n> >> > @@ -194,8 +145,7 @@ void\n> >> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu\n> >> > const uint8_t quality = ret ? *entry.data.u8 : 95;\n> >> > resultMetadata->addEntry(ANDROID_JPEG_QUALITY, quality);\n> >> >\n> >> > - int jpeg_size = encoder_->encode(source, destination->plane(0),\n> >> > - exif.data(), quality);\n> >> > + int jpeg_size = encoder_->encode(streamBuffer, exif.data(),\n> >> quality);\n> >> > if (jpeg_size < 0) {\n> >> > LOG(JPEG, Error) << \"Failed to encode stream image\";\n> >> > processComplete.emit(streamBuffer,\n> >> PostProcessor::Status::Error);\n> >> > diff --git a/src/android/jpeg/post_processor_jpeg.h\n> >> b/src/android/jpeg/post_processor_jpeg.h\n> >> > index 98309b01..a09f8798 100644\n> >> > --- a/src/android/jpeg/post_processor_jpeg.h\n> >> > +++ b/src/android/jpeg/post_processor_jpeg.h\n> >> > @@ -8,11 +8,11 @@\n> >> > #pragma once\n> >> >\n> >> > #include \"../post_processor.h\"\n> >> > -#include \"encoder_libjpeg.h\"\n> >> > -#include \"thumbnailer.h\"\n> >> >\n> >> > #include \n> >> >\n> >> > +#include \"encoder.h\"\n> >> > +\n> >> > class CameraDevice;\n> >> >\n> >> > class PostProcessorJpeg : public PostProcessor\n> >> > @@ -25,14 +25,9 @@ public:\n> >> > void process(Camera3RequestDescriptor::StreamBuffer\n> >> *streamBuffer) override;\n> >> >\n> >> > private:\n> >> > - void generateThumbnail(const libcamera::FrameBuffer &source,\n> >> > - const libcamera::Size &targetSize,\n> >> > - unsigned int quality,\n> >> > - std::vector *thumbnail);\n> >> > + void SetEncoder();\n> >> >\n> >> > CameraDevice *const cameraDevice_;\n> >> > std::unique_ptr encoder_;\n> >> > libcamera::Size streamSize_;\n> >> > - EncoderLibJpeg thumbnailEncoder_;\n> >> > - Thumbnailer thumbnailer_;\n> >> > };\n> >> > diff --git a/src/android/meson.build b/src/android/meson.build\n> >> > index 27be27bb..026b8b3c 100644\n> >> > --- a/src/android/meson.build\n> >> > +++ b/src/android/meson.build\n> >> > @@ -48,10 +48,6 @@ android_hal_sources = files([\n> >> > 'camera_ops.cpp',\n> >> > 'camera_request.cpp',\n> >> > 'camera_stream.cpp',\n> >> > - 'jpeg/encoder_libjpeg.cpp',\n> >> > - 'jpeg/exif.cpp',\n> >> > - 'jpeg/post_processor_jpeg.cpp',\n> >> > - 'jpeg/thumbnailer.cpp',\n> >> > 'yuv/post_processor_yuv.cpp'\n> >> > ])\n> >> >\n> >> > @@ -59,6 +55,7 @@ android_cpp_args = []\n> >> >\n> >> > subdir('cros')\n> >> > subdir('mm')\n> >> > +subdir('jpeg')\n> >>\n> >> Please keep these alphabetically sorted.\n> >>\n> >> >\n> >> > android_camera_metadata_sources = files([\n> >> > 'metadata/camera_metadata.c',\n> >>\n> >> --\n> >> Regards,\n> >>\n> >> Laurent Pinchart\n> >>\n> >","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 65877C3272\n\tfor ;\n\tThu, 1 Sep 2022 20:09:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1E8661FD6;\n\tThu, 1 Sep 2022 22:09:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D520161FCE\n\tfor ;\n\tThu, 1 Sep 2022 22:09:37 +0200 (CEST)","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 3753A6CD;\n\tThu, 1 Sep 2022 22:09:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662062978;\n\tbh=YRld7SbqI5UVcJYs3BLDdk2pNCzczghC/UbgqKTT3k0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=T7mPWg3eFVeiP8qGxXLHTjkFVjCPVkLX3Lte6Fni7GvsuCEINvhgz9lq29wyGhLSE\n\tUJIhCrPSRu1FAe0asWaku3vrWfqQM54lG+mZlNWTELlmWg9HPE6IlPDovF2uDcRyCh\n\tR+kq3vMYdHbZBOtBXvzSx5augX80ujspJ6G8lf/aX9g3XXYQVEsUeXE9Pyy5e3CKf7\n\t3gEZrHdUNYLrlDhKYfh2X5bFIaJskKBvGYNy8L9Lnf3uNK1OPB+4OS7paSnIlFSvOh\n\th6D7cPZpepHa94LydAz7TWNyhcYBbFBGNoZScXnkl1amTGoZ6deAeiltp5c17h88ra\n\tXzkkZl8oZQ1mw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662062977;\n\tbh=YRld7SbqI5UVcJYs3BLDdk2pNCzczghC/UbgqKTT3k0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=erPLk4Tos6jaRYOuSb5wGrlLQcNn6lKBymz5WMP+mLqsgl/TvEhlorvwDo+MK/Y+1\n\tyhxaD09Bs10vzVPAQr/Ve1xDgx6nl0TyaByhRmVPDbiQG+dfr7vlkikGRQ1NMcLjSu\n\t+qFC4uyy88V+bEKvRWYjEg3srK0ryUMATmuQ65xQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"erPLk4To\"; dkim-atps=neutral","Date":"Thu, 1 Sep 2022 23:09:24 +0300","To":"Cheng-Hao Yang ","Message-ID":"","References":"<20220426091409.1352047-1-chenghaoyang@chromium.org>\n\t<20220426091409.1352047-4-chenghaoyang@chromium.org>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 3/4] Rework JPEG encoder API and\n\tupdate PostProcessorJpeg and EncoderLibJpeg","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24889,"web_url":"https://patchwork.libcamera.org/comment/24889/","msgid":"","date":"2022-09-01T21:07:28","subject":"Re: [libcamera-devel] [PATCH v3 3/4] Rework JPEG encoder API and\n\tupdate PostProcessorJpeg and EncoderLibJpeg","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nOn Fri, Sep 2, 2022 at 4:09 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Harvey,\n>\n> On Wed, Jun 22, 2022 at 08:37:58PM +0800, Cheng-Hao Yang wrote:\n> > Hi Laurent,\n> >\n> > Uploaded PATCH v5 and fixed the following:\n> >\n> > > > > int configure(const libcamera::StreamConfiguration &cfg)\n> override;\n> > > > > + int encode(Camera3RequestDescriptor::StreamBuffer\n> *streamBuffer,\n> > > > > + libcamera::Span exifData,\n> > > > > + unsigned int quality) override;\n> > > > > + int generateThumbnail(\n> > > > > + const libcamera::FrameBuffer &source,\n> > > > > + const libcamera::Size &targetSize,\n> > > > > + unsigned int quality,\n> > > > > + std::vector *thumbnail) override;\n> > > >\n> > > > Same here.\n> > >\n> > > I think I need your help on the indentation format. Any doc or rule I\n> can\n> > > study? Thanks!\n> >\n> > I was using gmail to view your comment. Now I use `\n> > https://patchwork.libcamera.org/patch`\n> , and the tabs are correctly\n> aligned.\n> > Done.\n>\n> I'm afraid I can't really help with getting gmail to display the e-mail\n> contents properly. Maybe you have more leverage than I do there ;-)\n> Jokes aside, I think most people avoid webmail for patch review.\n>\n> While on the topic of e-mails, could you plese try to reply inline in\n> the patch instead of copying content and answers to the beginning ? It\n> gets difficult to get the whole context otherwise.\n>\n>\nRight, I'll start to do that from now on. Thanks for letting me know :)\n\n\n> > > > You now use the same libjpeg encoder instance for both the main image\n> > > > and the thumbnail. This leads to EncoderLibJpeg::internalConfigure()\n> being\n> > > > called twice for every frame. The function looks fairly cheap so it\n> should\n> > > > be fine, but could you confirm that jpeg_set_defaults() doesn't\n> cause a\n> > > > large overhead ? Please mention this change in the commit message of\n> the\n> > > > appropriate patch, it's an important one.\n> > >\n> > > Originally for each frame we need at least one call to\n> > > jpeg_set_defaults() to encode the thumbnail, and we need two now with\n> patch\n> > > v3, so I thought it was fine in terms of the complexity, while you're\n> right\n> > > that I'm not sure about the complexity of jpeg_set_defaults(), even\n> looking\n> > > into the implementation [1]. I can also modify the code not to\n> configure\n> > > twice per frame. WDYT?\n> >\n> > Done with only one jpeg_set_defaults() being called per frame. Please\n> check.\n>\n> Thank you.\n>\n> > On Tue, May 3, 2022 at 11:29 AM Cheng-Hao Yang wrote:\n> >\n> > > Hi Laurent,\n> > >\n> > > Sorry I forgot to send this reply before going on vacation...\n> > >\n> > > > The e-mail address should use \"@\" instead of \" at \".\n> > > Oh I see! I checked the libcamera archives for examples, and thought\n> > > that was intentional haha.\n> > > Updated.\n> > >\n> > > > cfg_ is only used here, I suppose to reconfigure the libjpeg encoder\n> for\n> > > the\n> > > Is this a deprecated comment?\n> > >\n> > > > > int configure(const libcamera::StreamConfiguration &cfg)\n> > > override;\n> > > > > + int encode(Camera3RequestDescriptor::StreamBuffer\n> *streamBuffer,\n> > > > > + libcamera::Span exifData,\n> > > > > + unsigned int quality) override;\n> > > > > + int generateThumbnail(\n> > > > > + const libcamera::FrameBuffer &source,\n> > > > > + const libcamera::Size &targetSize,\n> > > > > + unsigned int quality,\n> > > > > + std::vector *thumbnail) override;\n> > >\n> > > > Same here.\n> > > I think I need your help on the indentation format. Any doc or rule I\n> can\n> > > study? Thanks!\n> > >\n> > > > I'm tempted to simplify this by using conditional compilation (we\n> have a\n> > > OS_CHROMEOS macro), the function could then go to\n> post_processor_jpeg.cpp.\n> > > Oh I didn't notice there's such a macro to use. Thanks! I think we can\n> get\n> > > rid of the PostProcessorJpeg::createEncoder() then.\n> > >\n> > > > You now use the same libjpeg encoder instance for both the main image\n> > > and the thumbnail. This leads to EncoderLibJpeg::internalConfigure()\n> being\n> > > called twice for every frame. The function looks fairly cheap so it\n> should\n> > > be fine, but could you confirm that jpeg_set_defaults() doesn't cause a\n> > > large overhead ? Please mention this change in the commit message of\n> the\n> > > appropriate patch, it's an important one.\n> > > Originally for each frame we need at least one call to\n> jpeg_set_defaults()\n> > > to encode the thumbnail, and we need two now with patch v3, so I\n> thought it\n> > > was fine in terms of the complexity, while you're right that I'm not\n> sure\n> > > about the complexity of jpeg_set_defaults(), even looking into the\n> > > implementation [1]. I can also modify the code not to configure twice\n> per\n> > > frame. WDYT?\n> > >\n> > > [1]: https://github.com/kornelski/libjpeg/blob/master/jcparam.c#L268\n> > >\n> > >\n> > > On Wed, Apr 27, 2022 at 7:44 AM Laurent Pinchart <\n> > > laurent.pinchart@ideasonboard.com> wrote:\n> > >\n> > >> Hi Harvey,\n> > >>\n> > >> Thank you for the patch.\n> > >>\n> > >> On Tue, Apr 26, 2022 at 09:14:08AM +0000, Harvey Yang via\n> libcamera-devel\n> > >> wrote:\n> > >> > To add JEA in the next CL, this CL reworks JPEG encoder API and move\n> > >>\n> > >> Same comment as in 1/4 for \"CL\" (I won't repeat it for the other\n> patches\n> > >> in this series, please update them as applicable).\n> > >>\n> > >> > thumbnail encoding code into EncoderLibJpeg's implementation, as JEA\n> > >> > supports that as well.\n> > >> >\n> > >> > Signed-off-by: Harvey Yang \n> > >>\n> > >> The e-mail address should use \"@\" instead of \" at \".\n> > >>\n> > >> > ---\n> > >> > src/android/jpeg/encoder.h | 9 ++-\n> > >> > src/android/jpeg/encoder_libjpeg.cpp | 70\n> +++++++++++++++++++\n> > >> > src/android/jpeg/encoder_libjpeg.h | 21 +++++-\n> > >> > .../jpeg/generic_post_processor_jpeg.cpp | 14 ++++\n> > >> > src/android/jpeg/meson.build | 9 +++\n> > >> > src/android/jpeg/post_processor_jpeg.cpp | 60 ++--------------\n> > >> > src/android/jpeg/post_processor_jpeg.h | 11 +--\n> > >> > src/android/meson.build | 5 +-\n> > >>\n> > >> This is more reviewable than in the previous version, but there are\n> > >> still quite a few changes bundled in the same patch. As a v4 will be\n> > >> needed anyway, could you split this further as follows ? Feel free to\n> > >> change the order as appropriate.\n> > >>\n> > >> - Add meson.build file in jpeg/ directory\n> > >> - Move thumbnail generate to Encoder class\n> > >> - Pass streamBuffer to encode() to prepare for JPEG encoders that need\n> > >> access to the HAL buffer handle\n> > >> - Add PostProcessorJpeg::SetEncoder()\n> > >>\n> > >> > 8 files changed, 128 insertions(+), 71 deletions(-)\n> > >> > create mode 100644 src/android/jpeg/generic_post_processor_jpeg.cpp\n> > >> > create mode 100644 src/android/jpeg/meson.build\n> > >> >\n> > >> > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> > >> > index b974d367..6d527d91 100644\n> > >> > --- a/src/android/jpeg/encoder.h\n> > >> > +++ b/src/android/jpeg/encoder.h\n> > >> > @@ -12,14 +12,19 @@\n> > >> > #include \n> > >> > #include \n> > >> >\n> > >> > +#include \"../camera_request.h\"\n> > >> > +\n> > >> > class Encoder\n> > >> > {\n> > >> > public:\n> > >> > virtual ~Encoder() = default;\n> > >> >\n> > >> > virtual int configure(const libcamera::StreamConfiguration\n> &cfg)\n> > >> = 0;\n> > >> > - virtual int encode(const libcamera::FrameBuffer &source,\n> > >> > - libcamera::Span destination,\n> > >> > + virtual int encode(Camera3RequestDescriptor::StreamBuffer\n> > >> *streamBuffer,\n> > >> > libcamera::Span exifData,\n> > >> > unsigned int quality) = 0;\n> > >> > + virtual int generateThumbnail(const libcamera::FrameBuffer\n> > >> &source,\n> > >> > + const libcamera::Size\n> &targetSize,\n> > >> > + unsigned int quality,\n> > >> > + std::vector\n> > >> *thumbnail) = 0;\n> > >> > };\n> > >> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp\n> > >> b/src/android/jpeg/encoder_libjpeg.cpp\n> > >> > index 21a3b33d..b5591e33 100644\n> > >> > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > >> > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > >> > @@ -24,6 +24,8 @@\n> > >> > #include \"libcamera/internal/formats.h\"\n> > >> > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > >> >\n> > >> > +#include \"../camera_buffer.h\"\n> > >> > +\n> > >> > using namespace libcamera;\n> > >> >\n> > >> > LOG_DECLARE_CATEGORY(JPEG)\n> > >> > @@ -82,8 +84,17 @@ EncoderLibJpeg::~EncoderLibJpeg()\n> > >> > }\n> > >> >\n> > >> > int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> > >> > +{\n> > >> > + thumbnailer_.configure(cfg.size, cfg.pixelFormat);\n> > >> > + cfg_ = cfg;\n> > >>\n> > >> I'd store the size and pixel format only, not the full\n> > >> StreamConfiguration, as they are all you need. internalConfigure()\n> > >> should then take a size and pixel format, which will make it easier to\n> > >> use in generateThumbnail().\n> > >>\n> > >> > +\n> > >> > + return internalConfigure(cfg);\n> > >> > +}\n> > >> > +\n> > >> > +int EncoderLibJpeg::internalConfigure(const StreamConfiguration\n> &cfg)\n> > >>\n> > >> Let's name the function configureEncoder().\n> > >>\n> > >> > {\n> > >> > const struct JPEGPixelFormatInfo info =\n> > >> findPixelInfo(cfg.pixelFormat);\n> > >> > +\n> > >> > if (info.colorSpace == JCS_UNKNOWN)\n> > >> > return -ENOTSUP;\n> > >> >\n> > >> > @@ -178,6 +189,65 @@ void EncoderLibJpeg::compressNV(const\n> > >> std::vector> &planes)\n> > >> > }\n> > >> > }\n> > >> >\n> > >> > +int EncoderLibJpeg::encode(Camera3RequestDescriptor::StreamBuffer\n> > >> *streamBuffer,\n> > >> > + libcamera::Span exifData,\n> > >> > + unsigned int quality)\n> > >> > +{\n> > >> > + internalConfigure(cfg_);\n> > >>\n> > >> cfg_ is only used here, I suppose to reconfigure the libjpeg encoder\n> for\n> > >> the\n> > >>\n> > >> > + return encode(*streamBuffer->srcBuffer,\n> > >> streamBuffer->dstBuffer.get()->plane(0), exifData, quality);\n> > >>\n> > >> Let's shorten the line a bit (we aim for 80 columns as a soft target)\n> > >>\n> > >> return encode(*streamBuffer->srcBuffer,\n> > >> streamBuffer->dstBuffer.get()->plane(0),\n> > >> exifData, quality);\n> > >>\n> > >> > +}\n> > >> > +\n> > >> > +int EncoderLibJpeg::generateThumbnail(\n> > >> > + const libcamera::FrameBuffer &source,\n> > >> > + const libcamera::Size &targetSize,\n> > >> > + unsigned int quality,\n> > >> > + std::vector *thumbnail)\n> > >>\n> > >> int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer\n> > >> &source,\n> > >> const libcamera::Size\n> &targetSize,\n> > >> unsigned int quality,\n> > >> std::vector\n> > >> *thumbnail)\n> > >>\n> > >> to match the coding style.\n> > >>\n> > >> > +{\n> > >> > + /* Stores the raw scaled-down thumbnail bytes. */\n> > >> > + std::vector rawThumbnail;\n> > >> > +\n> > >> > + thumbnailer_.createThumbnail(source, targetSize,\n> &rawThumbnail);\n> > >> > +\n> > >> > + StreamConfiguration thCfg;\n> > >> > + thCfg.size = targetSize;\n> > >> > + thCfg.pixelFormat = thumbnailer_.pixelFormat();\n> > >> > + int ret = internalConfigure(thCfg);\n> > >>\n> > >> You now use the same libjpeg encoder instance for both the main image\n> > >> and the thumbnail. This leads to EncoderLibJpeg::internalConfigure()\n> > >> being called twice for every frame. The function looks fairly cheap so\n> > >> it should be fine, but could you confirm that jpeg_set_defaults()\n> > >> doesn't cause a large overhead ? Please mention this change in the\n> > >> commit message of the appropriate patch, it's an important one.\n> > >>\n> > >> > +\n> > >> > + if (!rawThumbnail.empty() && !ret) {\n> > >> > + /*\n> > >> > + * \\todo Avoid value-initialization of all elements\n> of the\n> > >> > + * vector.\n> > >> > + */\n> > >> > + thumbnail->resize(rawThumbnail.size());\n> > >> > +\n> > >> > + /*\n> > >> > + * Split planes manually as the encoder expects a\n> vector\n> > >> of\n> > >> > + * planes.\n> > >> > + *\n> > >> > + * \\todo Pass a vector of planes directly to\n> > >> > + * Thumbnailer::createThumbnailer above and remove the\n> > >> manual\n> > >> > + * planes split from here.\n> > >> > + */\n> > >> > + std::vector> thumbnailPlanes;\n> > >> > + const PixelFormatInfo &formatNV12 =\n> > >> PixelFormatInfo::info(formats::NV12);\n> > >> > + size_t yPlaneSize = formatNV12.planeSize(targetSize,\n> 0);\n> > >> > + size_t uvPlaneSize = formatNV12.planeSize(targetSize,\n> 1);\n> > >> > + thumbnailPlanes.push_back({ rawThumbnail.data(),\n> > >> yPlaneSize });\n> > >> > + thumbnailPlanes.push_back({ rawThumbnail.data() +\n> > >> yPlaneSize, uvPlaneSize });\n> > >> > +\n> > >> > + int jpeg_size = encode(thumbnailPlanes, *thumbnail,\n> {},\n> > >> quality);\n> > >> > + thumbnail->resize(jpeg_size);\n> > >> > +\n> > >> > + LOG(JPEG, Debug)\n> > >> > + << \"Thumbnail compress returned \"\n> > >> > + << jpeg_size << \" bytes\";\n> > >> > +\n> > >> > + return jpeg_size;\n> > >> > + }\n> > >> > +\n> > >> > + return -1;\n> > >> > +}\n> > >> > +\n> > >> > int EncoderLibJpeg::encode(const FrameBuffer &source, Span\n> > >> dest,\n> > >> > Span exifData, unsigned int\n> > >> quality)\n> > >> > {\n> > >> > diff --git a/src/android/jpeg/encoder_libjpeg.h\n> > >> b/src/android/jpeg/encoder_libjpeg.h\n> > >> > index 1b3ac067..56b27bae 100644\n> > >> > --- a/src/android/jpeg/encoder_libjpeg.h\n> > >> > +++ b/src/android/jpeg/encoder_libjpeg.h\n> > >> > @@ -15,6 +15,8 @@\n> > >> >\n> > >> > #include \n> > >> >\n> > >> > +#include \"thumbnailer.h\"\n> > >> > +\n> > >> > class EncoderLibJpeg : public Encoder\n> > >> > {\n> > >> > public:\n> > >> > @@ -22,19 +24,32 @@ public:\n> > >> > ~EncoderLibJpeg();\n> > >> >\n> > >> > int configure(const libcamera::StreamConfiguration &cfg)\n> override;\n> > >> > + int encode(Camera3RequestDescriptor::StreamBuffer\n> *streamBuffer,\n> > >> > + libcamera::Span exifData,\n> > >> > + unsigned int quality) override;\n> > >> > + int generateThumbnail(\n> > >> > + const libcamera::FrameBuffer &source,\n> > >> > + const libcamera::Size &targetSize,\n> > >> > + unsigned int quality,\n> > >> > + std::vector *thumbnail) override;\n> > >>\n> > >> Same here.\n> > >>\n> > >> > +\n> > >> > +private:\n> > >> > + int internalConfigure(const libcamera::StreamConfiguration\n> &cfg);\n> > >> > +\n> > >> > int encode(const libcamera::FrameBuffer &source,\n> > >> > libcamera::Span destination,\n> > >> > libcamera::Span exifData,\n> > >> > - unsigned int quality) override;\n> > >> > + unsigned int quality);\n> > >> > int encode(const std::vector>\n> &planes,\n> > >> > libcamera::Span destination,\n> > >> > libcamera::Span exifData,\n> > >> > unsigned int quality);\n> > >> >\n> > >> > -private:\n> > >> > void compressRGB(const std::vector>\n> > >> &planes);\n> > >> > void compressNV(const std::vector>\n> > >> &planes);\n> > >> >\n> > >> > + libcamera::StreamConfiguration cfg_;\n> > >> > +\n> > >> > struct jpeg_compress_struct compress_;\n> > >> > struct jpeg_error_mgr jerr_;\n> > >> >\n> > >> > @@ -42,4 +57,6 @@ private:\n> > >> >\n> > >> > bool nv_;\n> > >> > bool nvSwap_;\n> > >> > +\n> > >> > + Thumbnailer thumbnailer_;\n> > >> > };\n> > >> > diff --git a/src/android/jpeg/generic_post_processor_jpeg.cpp\n> > >> b/src/android/jpeg/generic_post_processor_jpeg.cpp\n> > >> > new file mode 100644\n> > >> > index 00000000..890f6972\n> > >> > --- /dev/null\n> > >> > +++ b/src/android/jpeg/generic_post_processor_jpeg.cpp\n> > >> > @@ -0,0 +1,14 @@\n> > >> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > >> > +/*\n> > >> > + * Copyright (C) 2022, Google Inc.\n> > >> > + *\n> > >> > + * generic_post_processor_jpeg.cpp - Generic JPEG Post Processor\n> > >> > + */\n> > >> > +\n> > >> > +#include \"encoder_libjpeg.h\"\n> > >> > +#include \"post_processor_jpeg.h\"\n> > >> > +\n> > >> > +void PostProcessorJpeg::SetEncoder()\n> > >>\n> > >> We use camelCase for function names, not CamelCase. Let's name the\n> > >> function createEncoder(), as it doesn't just set it but actually\n> creates\n> > >> it.\n> > >>\n> > >> > +{\n> > >> > + encoder_ = std::make_unique();\n> > >>\n> > >> I'm tempted to simplify this by using conditional compilation (we\n> have a\n> > >> OS_CHROMEOS macro), the function could then go to\n> > >> post_processor_jpeg.cpp.\n> > >>\n> > >> > +}\n> > >> > diff --git a/src/android/jpeg/meson.build\n> b/src/android/jpeg/meson.build\n> > >> > new file mode 100644\n> > >> > index 00000000..94522dc0\n> > >> > --- /dev/null\n> > >> > +++ b/src/android/jpeg/meson.build\n> > >> > @@ -0,0 +1,9 @@\n> > >> > +# SPDX-License-Identifier: CC0-1.0\n> > >> > +\n> > >> > +android_hal_sources += files([\n> > >> > + 'exif.cpp',\n> > >> > + 'post_processor_jpeg.cpp'])\n> > >>\n> > >> The '])' should be on the next line.\n> > >>\n> > >> > +\n> > >> > +android_hal_sources += files(['encoder_libjpeg.cpp',\n> > >> > + 'generic_post_processor_jpeg.cpp',\n> > >> > + 'thumbnailer.cpp'])\n> > >>\n> > >> Similar comment here,\n> > >>\n> > >> android_hal_sources += files([\n> > >> 'encoder_libjpeg.cpp',\n> > >> 'generic_post_processor_jpeg.cpp',\n> > >> 'thumbnailer.cpp',\n> > >> ])\n> > >>\n> > >> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp\n> > >> b/src/android/jpeg/post_processor_jpeg.cpp\n> > >> > index d72ebc3c..7ceba60e 100644\n> > >> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > >> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > >> > @@ -9,10 +9,10 @@\n> > >> >\n> > >> > #include \n> > >> >\n> > >> > +#include \"../android_framebuffer.h\"\n> > >>\n> > >> Is this needed ?\n> > >>\n> > >> > #include \"../camera_device.h\"\n> > >> > #include \"../camera_metadata.h\"\n> > >> > #include \"../camera_request.h\"\n> > >> > -#include \"encoder_libjpeg.h\"\n> > >> > #include \"exif.h\"\n> > >> >\n> > >> > #include \n> > >> > @@ -44,60 +44,11 @@ int PostProcessorJpeg::configure(const\n> > >> StreamConfiguration &inCfg,\n> > >> >\n> > >> > streamSize_ = outCfg.size;\n> > >> >\n> > >> > - thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> > >> > -\n> > >> > - encoder_ = std::make_unique();\n> > >> > + SetEncoder();\n> > >> >\n> > >> > return encoder_->configure(inCfg);\n> > >> > }\n> > >> >\n> > >> > -void PostProcessorJpeg::generateThumbnail(const FrameBuffer\n> &source,\n> > >> > - const Size &targetSize,\n> > >> > - unsigned int quality,\n> > >> > - std::vector\n> > >> *thumbnail)\n> > >> > -{\n> > >> > - /* Stores the raw scaled-down thumbnail bytes. */\n> > >> > - std::vector rawThumbnail;\n> > >> > -\n> > >> > - thumbnailer_.createThumbnail(source, targetSize,\n> &rawThumbnail);\n> > >> > -\n> > >> > - StreamConfiguration thCfg;\n> > >> > - thCfg.size = targetSize;\n> > >> > - thCfg.pixelFormat = thumbnailer_.pixelFormat();\n> > >> > - int ret = thumbnailEncoder_.configure(thCfg);\n> > >> > -\n> > >> > - if (!rawThumbnail.empty() && !ret) {\n> > >> > - /*\n> > >> > - * \\todo Avoid value-initialization of all elements\n> of the\n> > >> > - * vector.\n> > >> > - */\n> > >> > - thumbnail->resize(rawThumbnail.size());\n> > >> > -\n> > >> > - /*\n> > >> > - * Split planes manually as the encoder expects a\n> vector\n> > >> of\n> > >> > - * planes.\n> > >> > - *\n> > >> > - * \\todo Pass a vector of planes directly to\n> > >> > - * Thumbnailer::createThumbnailer above and remove the\n> > >> manual\n> > >> > - * planes split from here.\n> > >> > - */\n> > >> > - std::vector> thumbnailPlanes;\n> > >> > - const PixelFormatInfo &formatNV12 =\n> > >> PixelFormatInfo::info(formats::NV12);\n> > >> > - size_t yPlaneSize = formatNV12.planeSize(targetSize,\n> 0);\n> > >> > - size_t uvPlaneSize = formatNV12.planeSize(targetSize,\n> 1);\n> > >> > - thumbnailPlanes.push_back({ rawThumbnail.data(),\n> > >> yPlaneSize });\n> > >> > - thumbnailPlanes.push_back({ rawThumbnail.data() +\n> > >> yPlaneSize, uvPlaneSize });\n> > >> > -\n> > >> > - int jpeg_size =\n> thumbnailEncoder_.encode(thumbnailPlanes,\n> > >> > - *thumbnail,\n> {},\n> > >> quality);\n> > >> > - thumbnail->resize(jpeg_size);\n> > >> > -\n> > >> > - LOG(JPEG, Debug)\n> > >> > - << \"Thumbnail compress returned \"\n> > >> > - << jpeg_size << \" bytes\";\n> > >> > - }\n> > >> > -}\n> > >> > -\n> > >> > void\n> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer\n> > >> *streamBuffer)\n> > >> > {\n> > >> > ASSERT(encoder_);\n> > >> > @@ -164,8 +115,8 @@ void\n> > >> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer\n> *streamBu\n> > >> >\n> > >> > if (thumbnailSize != Size(0, 0)) {\n> > >> > std::vector thumbnail;\n> > >> > - generateThumbnail(source, thumbnailSize,\n> quality,\n> > >> &thumbnail);\n> > >> > - if (!thumbnail.empty())\n> > >> > + ret = encoder_->generateThumbnail(source,\n> > >> thumbnailSize, quality, &thumbnail);\n> > >> > + if (ret > 0 && !thumbnail.empty())\n> > >> > exif.setThumbnail(thumbnail,\n> > >> Exif::Compression::JPEG);\n> > >> > }\n> > >> >\n> > >> > @@ -194,8 +145,7 @@ void\n> > >> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer\n> *streamBu\n> > >> > const uint8_t quality = ret ? *entry.data.u8 : 95;\n> > >> > resultMetadata->addEntry(ANDROID_JPEG_QUALITY, quality);\n> > >> >\n> > >> > - int jpeg_size = encoder_->encode(source,\n> destination->plane(0),\n> > >> > - exif.data(), quality);\n> > >> > + int jpeg_size = encoder_->encode(streamBuffer, exif.data(),\n> > >> quality);\n> > >> > if (jpeg_size < 0) {\n> > >> > LOG(JPEG, Error) << \"Failed to encode stream image\";\n> > >> > processComplete.emit(streamBuffer,\n> > >> PostProcessor::Status::Error);\n> > >> > diff --git a/src/android/jpeg/post_processor_jpeg.h\n> > >> b/src/android/jpeg/post_processor_jpeg.h\n> > >> > index 98309b01..a09f8798 100644\n> > >> > --- a/src/android/jpeg/post_processor_jpeg.h\n> > >> > +++ b/src/android/jpeg/post_processor_jpeg.h\n> > >> > @@ -8,11 +8,11 @@\n> > >> > #pragma once\n> > >> >\n> > >> > #include \"../post_processor.h\"\n> > >> > -#include \"encoder_libjpeg.h\"\n> > >> > -#include \"thumbnailer.h\"\n> > >> >\n> > >> > #include \n> > >> >\n> > >> > +#include \"encoder.h\"\n> > >> > +\n> > >> > class CameraDevice;\n> > >> >\n> > >> > class PostProcessorJpeg : public PostProcessor\n> > >> > @@ -25,14 +25,9 @@ public:\n> > >> > void process(Camera3RequestDescriptor::StreamBuffer\n> > >> *streamBuffer) override;\n> > >> >\n> > >> > private:\n> > >> > - void generateThumbnail(const libcamera::FrameBuffer &source,\n> > >> > - const libcamera::Size &targetSize,\n> > >> > - unsigned int quality,\n> > >> > - std::vector *thumbnail);\n> > >> > + void SetEncoder();\n> > >> >\n> > >> > CameraDevice *const cameraDevice_;\n> > >> > std::unique_ptr encoder_;\n> > >> > libcamera::Size streamSize_;\n> > >> > - EncoderLibJpeg thumbnailEncoder_;\n> > >> > - Thumbnailer thumbnailer_;\n> > >> > };\n> > >> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > >> > index 27be27bb..026b8b3c 100644\n> > >> > --- a/src/android/meson.build\n> > >> > +++ b/src/android/meson.build\n> > >> > @@ -48,10 +48,6 @@ android_hal_sources = files([\n> > >> > 'camera_ops.cpp',\n> > >> > 'camera_request.cpp',\n> > >> > 'camera_stream.cpp',\n> > >> > - 'jpeg/encoder_libjpeg.cpp',\n> > >> > - 'jpeg/exif.cpp',\n> > >> > - 'jpeg/post_processor_jpeg.cpp',\n> > >> > - 'jpeg/thumbnailer.cpp',\n> > >> > 'yuv/post_processor_yuv.cpp'\n> > >> > ])\n> > >> >\n> > >> > @@ -59,6 +55,7 @@ android_cpp_args = []\n> > >> >\n> > >> > subdir('cros')\n> > >> > subdir('mm')\n> > >> > +subdir('jpeg')\n> > >>\n> > >> Please keep these alphabetically sorted.\n> > >>\n> > >> >\n> > >> > android_camera_metadata_sources = files([\n> > >> > 'metadata/camera_metadata.c',\n> > >>\n> > >> --\n> > >> Regards,\n> > >>\n> > >> Laurent Pinchart\n> > >>\n> > >\n>\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","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 2B9C6C0DA4\n\tfor ;\n\tThu, 1 Sep 2022 21:07:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8256D61FD6;\n\tThu, 1 Sep 2022 23:07:42 +0200 (CEST)","from mail-wm1-x335.google.com (mail-wm1-x335.google.com\n\t[IPv6:2a00:1450:4864:20::335])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D883061FB9\n\tfor ;\n\tThu, 1 Sep 2022 23:07:39 +0200 (CEST)","by mail-wm1-x335.google.com with SMTP id\n\tn23-20020a7bc5d7000000b003a62f19b453so2104049wmk.3\n\tfor ;\n\tThu, 01 Sep 2022 14:07:39 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662066462;\n\tbh=JxjmTB4UdJsNG+0qKXEBwX0S9Gm1YByqpyPWMB9RAUM=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=idDhUwcIEPkNH99Vhk+kgITbOB/uNi7gP1hnNRSZftvDyTa6PczhTXkKoeMAF2ebB\n\tba9rz5ij16iuvyHZisTkIyTyZ87jA1MbfXglf6YmwPb+FSOgXI5WAGdn3I/g+uQU+g\n\tnNad9//MNG5hoL/YDjSyYuVqWtA+lZcCSAfO7nT8wot//i4vM7gB+WTL1ua1+m8ggp\n\t7NIRmJMP/IAVbiGR6EyxNin+cbnr7krLzvdt5XMVQ1N6z9U4Kq9y9b76u08CKY67vd\n\txF/+trf9oIZx/Cb9aQCBvsGNXK1aFKg858aqxhEvVOMmHSdinlZo/VgCkqgpBNNdVf\n\tEJDWtAB/hA8hg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc;\n\tbh=Ejf3vc9L5gStvzu/HDeYEMOaLyUovw4IAXuYgcBKHF0=;\n\tb=Xw3JdxlEwefkv5ilg6++aLmD8fXVTi9D9FoT9wbbRVMppRAJ8Zhqq6tY2gxJo2aNRy\n\tKP38vXfgafzgMk6S/32JEVDt5P/D7SdlTcM7rJAo4KdKr4YeuSm7Cs6BiM+5BXz9bTyQ\n\tya9sGoiDUPctH0B3nzShPPPLOi4Ahep+MBC2c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"Xw3JdxlE\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc;\n\tbh=Ejf3vc9L5gStvzu/HDeYEMOaLyUovw4IAXuYgcBKHF0=;\n\tb=oWWFgiwL6SI0EzTUjQ5moQyJXuUnZRZZqYx8AIPRCA3VkdJuPZOXMH2cfpnwPjECoS\n\tliUluiZeNBFSmTaCMBNhOtNY0Tsbi0fNOGACL5zC1+7QSJTVqRBhNxws8snsu1d9mNLA\n\tdChnPPfDIUoqg8flRdErLLN0et21GQb0VtzGVy/w4n6ece0s/QVwM1wPLm/gpn5MmoXf\n\tqUaD+eaNo3TCGLqulsugLffgGWSlRf4GNjTEeH58awFJAg9LqZhvr7oxaWAO5hSM20uR\n\t6myPkT1N7ZQXZvIoAXoEjQmCkgCnzWIafdLJ1uInWoVcBDQhN8lzjwnabsxnpWsanZTJ\n\tELTg==","X-Gm-Message-State":"ACgBeo3RKA28Jvs0VOLual+7Io5aL8rOErtst6UENLXyJTck7mUEiLP8\n\tr2CdgbpmOK7iZE/tdELZ5u9AJJU6vVzGp/6ZCEPZLXTcVq0=","X-Google-Smtp-Source":"AA6agR40PUULGQ5IjZFXVKzTS/BQH2P+51Fd5gx52l4Dbosi5DEoumbKb40w/YN43dlQz5IDn+uSX9PmbfC+mEobJfs=","X-Received":"by 2002:a05:600c:1d1e:b0:3a5:4f8d:743f with SMTP id\n\tl30-20020a05600c1d1e00b003a54f8d743fmr573526wms.121.1662066459351;\n\tThu, 01 Sep 2022 14:07:39 -0700 (PDT)","MIME-Version":"1.0","References":"<20220426091409.1352047-1-chenghaoyang@chromium.org>\n\t<20220426091409.1352047-4-chenghaoyang@chromium.org>\n\t\n\t\n\t\n\t","In-Reply-To":"","Date":"Fri, 2 Sep 2022 05:07:28 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000e735ff05e7a40091\"","Subject":"Re: [libcamera-devel] [PATCH v3 3/4] Rework JPEG encoder API and\n\tupdate PostProcessorJpeg and EncoderLibJpeg","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","From":"Cheng-Hao Yang via libcamera-devel\n\t","Reply-To":"Cheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]