[{"id":26013,"web_url":"https://patchwork.libcamera.org/comment/26013/","msgid":"","date":"2022-12-07T04:24:36","subject":"Re: [libcamera-devel] [PATCH v7 4/6] Move generateThumbnail from\n\tPostProcessorJpeg to Encoder","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 Thu, Dec 01, 2022 at 09:27:31AM +0000, Harvey Yang via libcamera-devel wrote:\n> From: Harvey Yang \n> \n> In the following patch, generateThumbnail will have a different\n> implementation in the jea encoder. Therefore, this patch moves the\n> generateThumbnail function from PostProcessorJpeg to Encoder.\n> \n> Signed-off-by: Harvey Yang \n> ---\n> src/android/jpeg/encoder.h | 5 +\n> src/android/jpeg/encoder_libjpeg.cpp | 122 ++++++++++++++++++-----\n> src/android/jpeg/encoder_libjpeg.h | 28 +++++-\n> src/android/jpeg/post_processor_jpeg.cpp | 54 +---------\n> src/android/jpeg/post_processor_jpeg.h | 11 +-\n> 5 files changed, 130 insertions(+), 90 deletions(-)\n> \n> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> index b974d367..7dc1ee27 100644\n> --- a/src/android/jpeg/encoder.h\n> +++ b/src/android/jpeg/encoder.h\n> @@ -22,4 +22,9 @@ public:\n> \t\t\t libcamera::Span destination,\n> \t\t\t libcamera::Span exifData,\n> \t\t\t unsigned int quality) = 0;\n> +\tvirtual int generateThumbnail(\n> +\t\tconst libcamera::FrameBuffer &source,\n> +\t\tconst libcamera::Size &targetSize,\n> +\t\tunsigned int quality,\n> +\t\tstd::vector *thumbnail) = 0;\n\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\nto be consistent with the coding style. Same below where applicable.\n\n> };\n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index fd62bd9c..cc37fde3 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -71,29 +71,43 @@ const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)\n> EncoderLibJpeg::EncoderLibJpeg()\n> {\n> \t/* \\todo Expand error handling coverage with a custom handler. */\n> -\tcompress_.err = jpeg_std_error(&jerr_);\n> +\timage_data_.compress.err = jpeg_std_error(&image_data_.jerr);\n> +\tthumbnail_data_.compress.err = jpeg_std_error(&thumbnail_data_.jerr);\n> \n> -\tjpeg_create_compress(&compress_);\n> +\tjpeg_create_compress(&image_data_.compress);\n> +\tjpeg_create_compress(&thumbnail_data_.compress);\n> }\n> \n> EncoderLibJpeg::~EncoderLibJpeg()\n> {\n> -\tjpeg_destroy_compress(&compress_);\n> +\tjpeg_destroy_compress(&image_data_.compress);\n> +\tjpeg_destroy_compress(&thumbnail_data_.compress);\n> }\n> \n> int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> {\n> -\tconst struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);\n> +\tthumbnailer_.configure(cfg.size, cfg.pixelFormat);\n> +\n> +\treturn configureEncoder(&image_data_.compress, cfg.pixelFormat,\n> +\t\t\t\tcfg.size);\n> +}\n> +\n> +int EncoderLibJpeg::configureEncoder(struct jpeg_compress_struct *compress,\n> +\t\t\t\t libcamera::PixelFormat pixelFormat,\n> +\t\t\t\t libcamera::Size size)\n> +{\n> +\tconst struct JPEGPixelFormatInfo info = findPixelInfo(pixelFormat);\n> +\n> \tif (info.colorSpace == JCS_UNKNOWN)\n> \t\treturn -ENOTSUP;\n> \n> -\tcompress_.image_width = cfg.size.width;\n> -\tcompress_.image_height = cfg.size.height;\n> -\tcompress_.in_color_space = info.colorSpace;\n> +\tcompress->image_width = size.width;\n> +\tcompress->image_height = size.height;\n> +\tcompress->in_color_space = info.colorSpace;\n> \n> -\tcompress_.input_components = info.colorSpace == JCS_GRAYSCALE ? 1 : 3;\n> +\tcompress->input_components = info.colorSpace == JCS_GRAYSCALE ? 1 : 3;\n> \n> -\tjpeg_set_defaults(&compress_);\n> +\tjpeg_set_defaults(compress);\n> \n> \tpixelFormatInfo_ = &info.pixelFormatInfo;\n> \n> @@ -107,13 +121,13 @@ void EncoderLibJpeg::compressRGB(const std::vector> &planes)\n> {\n> \tunsigned char *src = const_cast(planes[0].data());\n> \t/* \\todo Stride information should come from buffer configuration. */\n> -\tunsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0);\n> +\tunsigned int stride = pixelFormatInfo_->stride(compress_->image_width, 0);\n> \n> \tJSAMPROW row_pointer[1];\n> \n> -\twhile (compress_.next_scanline < compress_.image_height) {\n> -\t\trow_pointer[0] = &src[compress_.next_scanline * stride];\n> -\t\tjpeg_write_scanlines(&compress_, row_pointer, 1);\n> +\twhile (compress_->next_scanline < compress_->image_height) {\n> +\t\trow_pointer[0] = &src[compress_->next_scanline * stride];\n> +\t\tjpeg_write_scanlines(compress_, row_pointer, 1);\n> \t}\n> }\n> \n> @@ -123,7 +137,7 @@ void EncoderLibJpeg::compressRGB(const std::vector> &planes)\n> */\n> void EncoderLibJpeg::compressNV(const std::vector> &planes)\n> {\n> -\tuint8_t tmprowbuf[compress_.image_width * 3];\n> +\tuint8_t tmprowbuf[compress_->image_width * 3];\n> \n> \t/*\n> \t * \\todo Use the raw api, and only unpack the cb/cr samples to new line\n> @@ -133,10 +147,10 @@ void EncoderLibJpeg::compressNV(const std::vector> &planes)\n> \t * Possible hints at:\n> \t * https://sourceforge.net/p/libjpeg/mailman/message/30815123/\n> \t */\n> -\tunsigned int y_stride = pixelFormatInfo_->stride(compress_.image_width, 0);\n> -\tunsigned int c_stride = pixelFormatInfo_->stride(compress_.image_width, 1);\n> +\tunsigned int y_stride = pixelFormatInfo_->stride(compress_->image_width, 0);\n> +\tunsigned int c_stride = pixelFormatInfo_->stride(compress_->image_width, 1);\n> \n> -\tunsigned int horzSubSample = 2 * compress_.image_width / c_stride;\n> +\tunsigned int horzSubSample = 2 * compress_->image_width / c_stride;\n> \tunsigned int vertSubSample = pixelFormatInfo_->planes[1].verticalSubSampling;\n> \n> \tunsigned int c_inc = horzSubSample == 1 ? 2 : 0;\n> @@ -149,14 +163,14 @@ void EncoderLibJpeg::compressNV(const std::vector> &planes)\n> \tJSAMPROW row_pointer[1];\n> \trow_pointer[0] = &tmprowbuf[0];\n> \n> -\tfor (unsigned int y = 0; y < compress_.image_height; y++) {\n> +\tfor (unsigned int y = 0; y < compress_->image_height; y++) {\n> \t\tunsigned char *dst = &tmprowbuf[0];\n> \n> \t\tconst unsigned char *src_y = src + y * y_stride;\n> \t\tconst unsigned char *src_cb = src_c + (y / vertSubSample) * c_stride + cb_pos;\n> \t\tconst unsigned char *src_cr = src_c + (y / vertSubSample) * c_stride + cr_pos;\n> \n> -\t\tfor (unsigned int x = 0; x < compress_.image_width; x += 2) {\n> +\t\tfor (unsigned int x = 0; x < compress_->image_width; x += 2) {\n> \t\t\tdst[0] = *src_y;\n> \t\t\tdst[1] = *src_cb;\n> \t\t\tdst[2] = *src_cr;\n> @@ -174,13 +188,67 @@ void EncoderLibJpeg::compressNV(const std::vector> &planes)\n> \t\t\tdst += 3;\n> \t\t}\n> \n> -\t\tjpeg_write_scanlines(&compress_, row_pointer, 1);\n> +\t\tjpeg_write_scanlines(compress_, row_pointer, 1);\n> \t}\n> }\n> \n> +int 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> +{\n> +\t/* Stores the raw scaled-down thumbnail bytes. */\n> +\tstd::vector rawThumbnail;\n> +\n> +\tthumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n> +\n> +\tint ret = configureEncoder(&thumbnail_data_.compress,\n> +\t\t\t\t thumbnailer_.pixelFormat(), targetSize);\n> +\tcompress_ = &thumbnail_data_.compress;\n\nThis is all becoming a bit of spaghetti code with the compress_ pointer.\nIt feels like you're bundling two classes into one. It would be cleaner,\nI think, to create an internal JpegEncoder class (similar to your\nJpegData, I'd name it EncoderLibJpeg::Encoder) that handles one encoder\n(moving all the private data members of EncoderLibJpeg to that class).\nYou could then instantiate it twice as private data members of\nEncoderLibJpeg. What do you think ? The refactoring of EncoderLibJpeg\nshould go in first, before moving the thumbnailer to it.\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 =\n> +\t\t\tPixelFormatInfo::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,\n> +\t\t\t\t\t uvPlaneSize });\n> +\n> +\t\tint jpeg_size = encode(thumbnailPlanes, *thumbnail, {},\n> +\t\t\t\t quality);\n\ncamelCase (I know it was like this already, but let's fix it).\n\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\nAlso while at it, retur\n\n> +}\n> +\n> int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n> \t\t\t Span exifData, unsigned int quality)\n> {\n> +\tcompress_ = &image_data_.compress;\n> +\n> \tMappedFrameBuffer frame(&source, MappedFrameBuffer::MapFlag::Read);\n> \tif (!frame.isValid()) {\n> \t\tLOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n> @@ -198,7 +266,7 @@ int EncoderLibJpeg::encode(const std::vector> &src,\n> \tunsigned char *destination = dest.data();\n> \tunsigned long size = dest.size();\n> \n> -\tjpeg_set_quality(&compress_, quality, TRUE);\n> +\tjpeg_set_quality(compress_, quality, TRUE);\n> \n> \t/*\n> \t * The jpeg_mem_dest will reallocate if the required size is not\n> @@ -208,18 +276,18 @@ int EncoderLibJpeg::encode(const std::vector> &src,\n> \t * \\todo Implement our own custom memory destination to prevent\n> \t * reallocation and prefer failure with correct reporting.\n> \t */\n> -\tjpeg_mem_dest(&compress_, &destination, &size);\n> +\tjpeg_mem_dest(compress_, &destination, &size);\n> \n> -\tjpeg_start_compress(&compress_, TRUE);\n> +\tjpeg_start_compress(compress_, TRUE);\n> \n> \tif (exifData.size())\n> \t\t/* Store Exif data in the JPEG_APP1 data block. */\n> -\t\tjpeg_write_marker(&compress_, JPEG_APP0 + 1,\n> +\t\tjpeg_write_marker(compress_, JPEG_APP0 + 1,\n> \t\t\t\t static_cast(exifData.data()),\n> \t\t\t\t exifData.size());\n> \n> -\tLOG(JPEG, Debug) << \"JPEG Encode Starting:\" << compress_.image_width\n> -\t\t\t << \"x\" << compress_.image_height;\n> +\tLOG(JPEG, Debug) << \"JPEG Encode Starting:\" << compress_->image_width\n> +\t\t\t << \"x\" << compress_->image_height;\n> \n> \tASSERT(src.size() == pixelFormatInfo_->numPlanes());\n> \n> @@ -228,7 +296,7 @@ int EncoderLibJpeg::encode(const std::vector> &src,\n> \telse\n> \t\tcompressRGB(src);\n> \n> -\tjpeg_finish_compress(&compress_);\n> +\tjpeg_finish_compress(compress_);\n> \n> \treturn size;\n> }\n> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> index 1b3ac067..1ec2ba13 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> @@ -26,20 +28,40 @@ public:\n> \t\t libcamera::Span destination,\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\nThis patch would be easier to review if it didn't bundle a change of\nreturn type for this function. Furthermore, it should be explained in\nthe commit message. Is the return type change needed here, or later in\nthe series ? If later in the series, I'd split it to a separate patch.\nIn general, please try to have only one change per patch to simplify\nreview.\n\n> +\n> +private:\n> +\tstruct JpegData {\n> +\t\tstruct jpeg_compress_struct compress;\n> +\t\tstruct jpeg_error_mgr jerr;\n> +\t};\n> +\n> +\tint configureEncoder(struct jpeg_compress_struct *compress,\n> +\t\t\t libcamera::PixelFormat pixelFormat,\n> +\t\t\t libcamera::Size size);\n> +\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> -\tstruct jpeg_compress_struct compress_;\n> -\tstruct jpeg_error_mgr jerr_;\n> +\tJpegData image_data_;\n> +\tJpegData thumbnail_data_;\n\ncamelCase for both.\n\n> +\n> +\t// |&image_data_.compress| or |&thumbnail_data_.compress|.\n\nC-style comments (/* ... */).\n\n> +\tstruct jpeg_compress_struct *compress_;\n> \n> \tconst libcamera::PixelFormatInfo *pixelFormatInfo_;\n> \n> \tbool nv_;\n> \tbool nvSwap_;\n> +\n> +\tThumbnailer thumbnailer_;\n> };\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 0cf56716..60eebb11 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\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> \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,9 @@ 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,\n> +\t\t\t\t\t\t\t quality, &thumbnail);\n> +\t\t\tif (ret > 0 && !thumbnail.empty())\n\nDo you need to check both conditions ? Also, the generateThumbnail()\nfunction returns -1 in case of error or the JPEG data size otherwise,\nwhile you only check ret > 0 here. I'd return a bool from the function\nand check that only, moving the thumbnail.empty() check (if needed)\ninside generateThumbnail() to simplify the API.\n\n> \t\t\t\texif.setThumbnail(std::move(thumbnail), Exif::Compression::JPEG);\n> \t\t}\n> \n> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> index 98309b01..55b23d7d 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\nYou can add a\n\nclass Encoder;\n\ndeclaration instead (just below CameraDevice).\n\n> class CameraDevice;\n> \n> class PostProcessorJpeg : public PostProcessor\n> @@ -25,14 +25,7 @@ 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> -\n> \tCameraDevice *const cameraDevice_;\n> \tstd::unique_ptr encoder_;\n> \tlibcamera::Size streamSize_;\n> -\tEncoderLibJpeg thumbnailEncoder_;\n> -\tThumbnailer thumbnailer_;\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 56F89BE08B\n\tfor ;\n\tWed, 7 Dec 2022 04:24:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 836946333F;\n\tWed, 7 Dec 2022 05:24:41 +0100 (CET)","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 89A7061F1C\n\tfor ;\n\tWed, 7 Dec 2022 05:24:39 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CC6FE87B;\n\tWed, 7 Dec 2022 05:24:38 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1670387081;\n\tbh=dk3NgIqdDJEqK+qOFrZpsfAl+4uylIMpOOi0opfg6qE=;\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=C8Ohy5thdfD1wyDTOkpvPWFkrXlXzmEiuGrc57nxZ4KPf3GRzPoMGo3rKz1PD+M0Y\n\toF3YXBrhX1a9fuYiGbFkWycrZf9g/Ln7TFw2MEIQGFxyoRC5Hn5DVUoPjq9TXiDE+E\n\t+Kaa2OuOJkVhJXio4YPn4nF7ISP881D82rJATu3RRCnwt5RzINgJHJEQOm0qoES5Mj\n\tfJqmdN5jtmFZHDy9HmTtcOh4DCM+yihllkkyxMDtAQk8tBo4rrygtWv0XLJW/zhhKq\n\tMfD+sJV0Ci20igE5fgNB19LtrLBBEgfsMOSxiwiPcMrnG9oLaDTX1LUre5cerjRtFq\n\tfNSY+hPdReCKw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1670387079;\n\tbh=dk3NgIqdDJEqK+qOFrZpsfAl+4uylIMpOOi0opfg6qE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=McKLSyVB7W6tjTWwd6KAVwJDjiLTDErRrVFQ7j8sQ6OBwEUqZ1l1Pt/wdY1vSb0w7\n\t+aIhryyZj3qrjgWO9j2as+VbCdu1/LT+ycBCEaziDH5ym/ETWcMWawMPZSxVEhhgDp\n\ti8/PxtN6cAkEWG7VWuHpoBhZRKuCxk18FsdPCURI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"McKLSyVB\"; dkim-atps=neutral","Date":"Wed, 7 Dec 2022 06:24:36 +0200","To":"Harvey Yang ","Message-ID":"","References":"<20221201092733.2042078-1-chenghaoyang@google.com>\n\t<20221201092733.2042078-5-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221201092733.2042078-5-chenghaoyang@google.com>","Subject":"Re: [libcamera-devel] [PATCH v7 4/6] Move generateThumbnail from\n\tPostProcessorJpeg to Encoder","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":26067,"web_url":"https://patchwork.libcamera.org/comment/26067/","msgid":"","date":"2022-12-14T09:35:35","subject":"Re: [libcamera-devel] [PATCH v7 4/6] Move generateThumbnail from\n\tPostProcessorJpeg to Encoder","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thank you Laurent for the review!\n\nOn Wed, Dec 7, 2022 at 12:24 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Harvey,\n>\n> Thank you for the patch.\n>\n> On Thu, Dec 01, 2022 at 09:27:31AM +0000, Harvey Yang via libcamera-devel\n> wrote:\n> > From: Harvey Yang \n> >\n> > In the following patch, generateThumbnail will have a different\n> > implementation in the jea encoder. Therefore, this patch moves the\n> > generateThumbnail function from PostProcessorJpeg to Encoder.\n> >\n> > Signed-off-by: Harvey Yang \n> > ---\n> > src/android/jpeg/encoder.h | 5 +\n> > src/android/jpeg/encoder_libjpeg.cpp | 122 ++++++++++++++++++-----\n> > src/android/jpeg/encoder_libjpeg.h | 28 +++++-\n> > src/android/jpeg/post_processor_jpeg.cpp | 54 +---------\n> > src/android/jpeg/post_processor_jpeg.h | 11 +-\n> > 5 files changed, 130 insertions(+), 90 deletions(-)\n> >\n> > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> > index b974d367..7dc1ee27 100644\n> > --- a/src/android/jpeg/encoder.h\n> > +++ b/src/android/jpeg/encoder.h\n> > @@ -22,4 +22,9 @@ public:\n> > libcamera::Span destination,\n> > libcamera::Span exifData,\n> > unsigned int quality) = 0;\n> > + virtual int generateThumbnail(\n> > + const libcamera::FrameBuffer &source,\n> > + const libcamera::Size &targetSize,\n> > + unsigned int quality,\n> > + std::vector *thumbnail) = 0;\n>\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> to be consistent with the coding style. Same below where applicable.\n>\n>\nDone.\n\n\n> > };\n> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp\n> b/src/android/jpeg/encoder_libjpeg.cpp\n> > index fd62bd9c..cc37fde3 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > @@ -71,29 +71,43 @@ const struct JPEGPixelFormatInfo\n> &findPixelInfo(const PixelFormat &format)\n> > EncoderLibJpeg::EncoderLibJpeg()\n> > {\n> > /* \\todo Expand error handling coverage with a custom handler. */\n> > - compress_.err = jpeg_std_error(&jerr_);\n> > + image_data_.compress.err = jpeg_std_error(&image_data_.jerr);\n> > + thumbnail_data_.compress.err =\n> jpeg_std_error(&thumbnail_data_.jerr);\n> >\n> > - jpeg_create_compress(&compress_);\n> > + jpeg_create_compress(&image_data_.compress);\n> > + jpeg_create_compress(&thumbnail_data_.compress);\n> > }\n> >\n> > EncoderLibJpeg::~EncoderLibJpeg()\n> > {\n> > - jpeg_destroy_compress(&compress_);\n> > + jpeg_destroy_compress(&image_data_.compress);\n> > + jpeg_destroy_compress(&thumbnail_data_.compress);\n> > }\n> >\n> > int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> > {\n> > - const struct JPEGPixelFormatInfo info =\n> findPixelInfo(cfg.pixelFormat);\n> > + thumbnailer_.configure(cfg.size, cfg.pixelFormat);\n> > +\n> > + return configureEncoder(&image_data_.compress, cfg.pixelFormat,\n> > + cfg.size);\n> > +}\n> > +\n> > +int EncoderLibJpeg::configureEncoder(struct jpeg_compress_struct\n> *compress,\n> > + libcamera::PixelFormat pixelFormat,\n> > + libcamera::Size size)\n> > +{\n> > + const struct JPEGPixelFormatInfo info = findPixelInfo(pixelFormat);\n> > +\n> > if (info.colorSpace == JCS_UNKNOWN)\n> > return -ENOTSUP;\n> >\n> > - compress_.image_width = cfg.size.width;\n> > - compress_.image_height = cfg.size.height;\n> > - compress_.in_color_space = info.colorSpace;\n> > + compress->image_width = size.width;\n> > + compress->image_height = size.height;\n> > + compress->in_color_space = info.colorSpace;\n> >\n> > - compress_.input_components = info.colorSpace == JCS_GRAYSCALE ? 1\n> : 3;\n> > + compress->input_components = info.colorSpace == JCS_GRAYSCALE ? 1\n> : 3;\n> >\n> > - jpeg_set_defaults(&compress_);\n> > + jpeg_set_defaults(compress);\n> >\n> > pixelFormatInfo_ = &info.pixelFormatInfo;\n> >\n> > @@ -107,13 +121,13 @@ void EncoderLibJpeg::compressRGB(const\n> std::vector> &planes)\n> > {\n> > unsigned char *src = const_cast(planes[0].data());\n> > /* \\todo Stride information should come from buffer configuration.\n> */\n> > - unsigned int stride =\n> pixelFormatInfo_->stride(compress_.image_width, 0);\n> > + unsigned int stride =\n> pixelFormatInfo_->stride(compress_->image_width, 0);\n> >\n> > JSAMPROW row_pointer[1];\n> >\n> > - while (compress_.next_scanline < compress_.image_height) {\n> > - row_pointer[0] = &src[compress_.next_scanline * stride];\n> > - jpeg_write_scanlines(&compress_, row_pointer, 1);\n> > + while (compress_->next_scanline < compress_->image_height) {\n> > + row_pointer[0] = &src[compress_->next_scanline * stride];\n> > + jpeg_write_scanlines(compress_, row_pointer, 1);\n> > }\n> > }\n> >\n> > @@ -123,7 +137,7 @@ void EncoderLibJpeg::compressRGB(const\n> std::vector> &planes)\n> > */\n> > void EncoderLibJpeg::compressNV(const std::vector>\n> &planes)\n> > {\n> > - uint8_t tmprowbuf[compress_.image_width * 3];\n> > + uint8_t tmprowbuf[compress_->image_width * 3];\n> >\n> > /*\n> > * \\todo Use the raw api, and only unpack the cb/cr samples to new\n> line\n> > @@ -133,10 +147,10 @@ void EncoderLibJpeg::compressNV(const\n> std::vector> &planes)\n> > * Possible hints at:\n> > * https://sourceforge.net/p/libjpeg/mailman/message/30815123/\n> > */\n> > - unsigned int y_stride =\n> pixelFormatInfo_->stride(compress_.image_width, 0);\n> > - unsigned int c_stride =\n> pixelFormatInfo_->stride(compress_.image_width, 1);\n> > + unsigned int y_stride =\n> pixelFormatInfo_->stride(compress_->image_width, 0);\n> > + unsigned int c_stride =\n> pixelFormatInfo_->stride(compress_->image_width, 1);\n> >\n> > - unsigned int horzSubSample = 2 * compress_.image_width / c_stride;\n> > + unsigned int horzSubSample = 2 * compress_->image_width / c_stride;\n> > unsigned int vertSubSample =\n> pixelFormatInfo_->planes[1].verticalSubSampling;\n> >\n> > unsigned int c_inc = horzSubSample == 1 ? 2 : 0;\n> > @@ -149,14 +163,14 @@ void EncoderLibJpeg::compressNV(const\n> std::vector> &planes)\n> > JSAMPROW row_pointer[1];\n> > row_pointer[0] = &tmprowbuf[0];\n> >\n> > - for (unsigned int y = 0; y < compress_.image_height; y++) {\n> > + for (unsigned int y = 0; y < compress_->image_height; y++) {\n> > unsigned char *dst = &tmprowbuf[0];\n> >\n> > const unsigned char *src_y = src + y * y_stride;\n> > const unsigned char *src_cb = src_c + (y / vertSubSample)\n> * c_stride + cb_pos;\n> > const unsigned char *src_cr = src_c + (y / vertSubSample)\n> * c_stride + cr_pos;\n> >\n> > - for (unsigned int x = 0; x < compress_.image_width; x +=\n> 2) {\n> > + for (unsigned int x = 0; x < compress_->image_width; x +=\n> 2) {\n> > dst[0] = *src_y;\n> > dst[1] = *src_cb;\n> > dst[2] = *src_cr;\n> > @@ -174,13 +188,67 @@ void EncoderLibJpeg::compressNV(const\n> std::vector> &planes)\n> > dst += 3;\n> > }\n> >\n> > - jpeg_write_scanlines(&compress_, row_pointer, 1);\n> > + jpeg_write_scanlines(compress_, row_pointer, 1);\n> > }\n> > }\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> > + /* Stores the raw scaled-down thumbnail bytes. */\n> > + std::vector rawThumbnail;\n> > +\n> > + thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n> > +\n> > + int ret = configureEncoder(&thumbnail_data_.compress,\n> > + thumbnailer_.pixelFormat(), targetSize);\n> > + compress_ = &thumbnail_data_.compress;\n>\n> This is all becoming a bit of spaghetti code with the compress_ pointer.\n> It feels like you're bundling two classes into one. It would be cleaner,\n> I think, to create an internal JpegEncoder class (similar to your\n> JpegData, I'd name it EncoderLibJpeg::Encoder) that handles one encoder\n> (moving all the private data members of EncoderLibJpeg to that class).\n> You could then instantiate it twice as private data members of\n> EncoderLibJpeg. What do you think ? The refactoring of EncoderLibJpeg\n> should go in first, before moving the thumbnailer to it.\n>\n>\nYes, you're absolutely right. This version actually has conflicts in\nconfiguring\nthe capture and thumbnail with shared member variables.\n\nUpdated with your suggestion that EncoderLibJpeg is a wrapper that consists\nof two real encoders for captures and thumbnails respectively.\n\nI also separate this patch into two: the first adds the internal encoder,\nand the\nsecond moves the thumbnailer and add the |thumbnailEncoder_| in\nEncoderLibJpeg.\n\nPlease check if I missed anything.\n\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,\n> > + uvPlaneSize });\n> > +\n> > + int jpeg_size = encode(thumbnailPlanes, *thumbnail, {},\n> > + quality);\n>\n> camelCase (I know it was like this already, but let's fix it).\n>\n>\nDone.\n\n\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> Also while at it, retur\n>\n>\n`return -EINVAL;`?\n\n\n> > +}\n> > +\n> > int EncoderLibJpeg::encode(const FrameBuffer &source, Span\n> dest,\n> > Span exifData, unsigned int\n> quality)\n> > {\n> > + compress_ = &image_data_.compress;\n> > +\n> > MappedFrameBuffer frame(&source, MappedFrameBuffer::MapFlag::Read);\n> > if (!frame.isValid()) {\n> > LOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n> > @@ -198,7 +266,7 @@ int EncoderLibJpeg::encode(const\n> std::vector> &src,\n> > unsigned char *destination = dest.data();\n> > unsigned long size = dest.size();\n> >\n> > - jpeg_set_quality(&compress_, quality, TRUE);\n> > + jpeg_set_quality(compress_, quality, TRUE);\n> >\n> > /*\n> > * The jpeg_mem_dest will reallocate if the required size is not\n> > @@ -208,18 +276,18 @@ int EncoderLibJpeg::encode(const\n> std::vector> &src,\n> > * \\todo Implement our own custom memory destination to prevent\n> > * reallocation and prefer failure with correct reporting.\n> > */\n> > - jpeg_mem_dest(&compress_, &destination, &size);\n> > + jpeg_mem_dest(compress_, &destination, &size);\n> >\n> > - jpeg_start_compress(&compress_, TRUE);\n> > + jpeg_start_compress(compress_, TRUE);\n> >\n> > if (exifData.size())\n> > /* Store Exif data in the JPEG_APP1 data block. */\n> > - jpeg_write_marker(&compress_, JPEG_APP0 + 1,\n> > + jpeg_write_marker(compress_, JPEG_APP0 + 1,\n> > static_cast *>(exifData.data()),\n> > exifData.size());\n> >\n> > - LOG(JPEG, Debug) << \"JPEG Encode Starting:\" <<\n> compress_.image_width\n> > - << \"x\" << compress_.image_height;\n> > + LOG(JPEG, Debug) << \"JPEG Encode Starting:\" <<\n> compress_->image_width\n> > + << \"x\" << compress_->image_height;\n> >\n> > ASSERT(src.size() == pixelFormatInfo_->numPlanes());\n> >\n> > @@ -228,7 +296,7 @@ int EncoderLibJpeg::encode(const\n> std::vector> &src,\n> > else\n> > compressRGB(src);\n> >\n> > - jpeg_finish_compress(&compress_);\n> > + jpeg_finish_compress(compress_);\n> >\n> > return size;\n> > }\n> > diff --git a/src/android/jpeg/encoder_libjpeg.h\n> b/src/android/jpeg/encoder_libjpeg.h\n> > index 1b3ac067..1ec2ba13 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> > @@ -26,20 +28,40 @@ public:\n> > libcamera::Span destination,\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> This patch would be easier to review if it didn't bundle a change of\n> return type for this function. Furthermore, it should be explained in\n> the commit message. Is the return type change needed here, or later in\n> the series ? If later in the series, I'd split it to a separate patch.\n> In general, please try to have only one change per patch to simplify\n> review.\n>\n>\nRight, the return value is actually not needed. Removed.\n\n\n> > +\n> > +private:\n> > + struct JpegData {\n> > + struct jpeg_compress_struct compress;\n> > + struct jpeg_error_mgr jerr;\n> > + };\n> > +\n> > + int configureEncoder(struct jpeg_compress_struct *compress,\n> > + libcamera::PixelFormat pixelFormat,\n> > + libcamera::Size size);\n> > +\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> > - struct jpeg_compress_struct compress_;\n> > - struct jpeg_error_mgr jerr_;\n> > + JpegData image_data_;\n> > + JpegData thumbnail_data_;\n>\n> camelCase for both.\n>\n>\nRemoved.\n\n\n> > +\n> > + // |&image_data_.compress| or |&thumbnail_data_.compress|.\n>\n> C-style comments (/* ... */).\n>\n> > + struct jpeg_compress_struct *compress_;\n> >\n> > const libcamera::PixelFormatInfo *pixelFormatInfo_;\n> >\n> > bool nv_;\n> > bool nvSwap_;\n> > +\n> > + Thumbnailer thumbnailer_;\n> > };\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp\n> b/src/android/jpeg/post_processor_jpeg.cpp\n> > index 0cf56716..60eebb11 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\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> >\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,9 @@ 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,\n> > + quality,\n> &thumbnail);\n> > + if (ret > 0 && !thumbnail.empty())\n>\n> Do you need to check both conditions ? Also, the generateThumbnail()\n> function returns -1 in case of error or the JPEG data size otherwise,\n> while you only check ret > 0 here. I'd return a bool from the function\n> and check that only, moving the thumbnail.empty() check (if needed)\n> inside generateThumbnail() to simplify the API.\n>\n>\nRemoved the return value.\n\n\n> > exif.setThumbnail(std::move(thumbnail),\n> Exif::Compression::JPEG);\n> > }\n> >\n> > diff --git a/src/android/jpeg/post_processor_jpeg.h\n> b/src/android/jpeg/post_processor_jpeg.h\n> > index 98309b01..55b23d7d 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>\n> You can add a\n>\n> class Encoder;\n>\n> declaration instead (just below CameraDevice).\n>\n> > class CameraDevice;\n> >\n> > class PostProcessorJpeg : public PostProcessor\n> > @@ -25,14 +25,7 @@ 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> > -\n> > CameraDevice *const cameraDevice_;\n> > std::unique_ptr encoder_;\n> > libcamera::Size streamSize_;\n> > - EncoderLibJpeg thumbnailEncoder_;\n> > - Thumbnailer thumbnailer_;\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 7B688C31E9\n\tfor ;\n\tWed, 14 Dec 2022 09:35:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3528763362;\n\tWed, 14 Dec 2022 10:35:49 +0100 (CET)","from mail-vs1-xe34.google.com (mail-vs1-xe34.google.com\n\t[IPv6:2607:f8b0:4864:20::e34])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6215C603D0\n\tfor ;\n\tWed, 14 Dec 2022 10:35:47 +0100 (CET)","by mail-vs1-xe34.google.com with SMTP id b189so17292637vsc.10\n\tfor ;\n\tWed, 14 Dec 2022 01:35:47 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671010549;\n\tbh=TttuGFSAV8Pzm6yP50JUqF+cgGPbSAHwfSZbJF7iKoY=;\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=yAppY0MvKX1UDif3D090H8Bv0gDMGYD34EDdYXJ5wTNoewzbOcFUT+/TQkVCVlKVW\n\tiP3gOkW6ZpTG/PmmRGaU0OBpkMHqtTFQ458+aV9Ba8eiS1m2hZTQvrjtZs4Dom+rXr\n\tUbJ9yzsfr0mkngXplF9GlfGaFxIypGhwk4BWdvkigRDlbNbAfyxbLqE+YU1YcdzjDH\n\tLOCPabdFr3SkQFmMvfZMNeel4PCslq5FimqFKd4GWAiaWQHVGvsMM61lZk85qNMifo\n\t6bH9aO6QTch4cJj+YaR5JO5ikWULBixkfbQYpDhWP7ogqoDolIbyQWgGjslHbQWPka\n\tX3Xa9RPPsjOVw==","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:subject:date:message-id:reply-to;\n\tbh=rM5OjdMuiydXZFJE5pHHdtcm7DUc1TSYJKl0Wyp0H3o=;\n\tb=FUeZMfUQju27luSiZE3KBcyETSiztFSRUW4qpRHqv3PWqmA4evZwveR4lf+wCL9Woh\n\tpD2Oe7wo6y5qLDB0eJYtR84TYEPs6o81FXXlgl6FeqqLe6PmkHyGltudupe09dcd9x35\n\tqv91EV2fT/HVg3VpzQXAvGCTmNb+ovfLC8afI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"FUeZMfUQ\"; \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:subject:date:message-id\n\t:reply-to;\n\tbh=rM5OjdMuiydXZFJE5pHHdtcm7DUc1TSYJKl0Wyp0H3o=;\n\tb=2hcMsbHayrx9Bzzf/jnY9X7372Dzxsxcvhps6N9u5JZErTqajFa8oKOnresgNan7CF\n\taWhR+X3l0PYbIGv3vlQeZo6mciDFHjbiTVjaolA5RCOGVuKz4tIgNgG8mBZcGNAA3/iV\n\tPdp0vSEU84Z1lhMuUmjykxszAfcuya1w4zGlqZdPsS/A5LUnY1QA0ksGhBSQ+0/2J8N2\n\tMMRK/kodY41gSQ3QgsI9/K2pZ8TwfAWPYzj8DpGqU8zvC+JcrdQvJaqVq1cZaU+wfTnf\n\tpvp+c8xZ5/EhVkzqBFQfwf1aCcF8HkWk2bacwYO7aELxH/OvIzs/guN4P141JQwU0DUe\n\t9LWA==","X-Gm-Message-State":"ANoB5pks2AQVrtGqAYfnkaJjk5qI3o5K+XauHqmuGcaLBBGBrcu6dRgp\n\tN7CvWy8lTm4BIrvjCFUimSVgXI6/hspcyArVLKsYXw==","X-Google-Smtp-Source":"AA0mqf7HYltNZe6iqfOyPDl3aEJOutcJ6/HG+p9V7MaRdgLRmnPZt4YQijouZ06WWcMWy6NaQjIlNDD6gk4ZORNM+L8=","X-Received":"by 2002:a05:6102:31ba:b0:3b1:4914:c553 with SMTP id\n\td26-20020a05610231ba00b003b14914c553mr12194957vsh.26.1671010546176;\n\tWed, 14 Dec 2022 01:35:46 -0800 (PST)","MIME-Version":"1.0","References":"<20221201092733.2042078-1-chenghaoyang@google.com>\n\t<20221201092733.2042078-5-chenghaoyang@google.com>\n\t","In-Reply-To":"","Date":"Wed, 14 Dec 2022 17:35:35 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000056a4305efc67609\"","Subject":"Re: [libcamera-devel] [PATCH v7 4/6] Move generateThumbnail from\n\tPostProcessorJpeg to Encoder","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\" "}}]