[{"id":26220,"web_url":"https://patchwork.libcamera.org/comment/26220/","msgid":"","date":"2023-01-15T23:13:23","subject":"Re: [libcamera-devel] [PATCH v8 4/7] Add an internal Encoder class\n\tin 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\nThe subject requires a prefix. \"android: jpeg: \" would be appropriate.\nSame comment for all patches in this series.\n\nOn Wed, Dec 14, 2022 at 09:33:27AM +0000, Harvey Yang via libcamera-devel wrote:\n> To move the thumbnail encoder into EncoderLibJpeg in the following\n> patch, this patch adds a wrapper/internal Encoder class that allows\n> EncoderLibJpeg to have more than one encoder to tackle both captures\n> and thumbnails.\n\nI'd write \"to tackle encoding of both captured images and their\nthumbnails\".\n\n> Signed-off-by: Harvey Yang \n> ---\n> src/android/jpeg/encoder_libjpeg.cpp | 32 +++++++++++++++++++++-------\n> src/android/jpeg/encoder_libjpeg.h | 30 ++++++++++++++++++++------\n> 2 files changed, 47 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index fd62bd9c..d849547f 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -68,7 +68,16 @@ const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)\n> \n> } /* namespace */\n> \n> -EncoderLibJpeg::EncoderLibJpeg()\n> +EncoderLibJpeg::EncoderLibJpeg() = default;\n> +\n> +EncoderLibJpeg::~EncoderLibJpeg() = default;\n> +\n> +int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> +{\n> +\treturn encoder_.configure(cfg);\n> +}\n> +\n> +EncoderLibJpeg::Encoder::Encoder()\n> {\n> \t/* \\todo Expand error handling coverage with a custom handler. */\n> \tcompress_.err = jpeg_std_error(&jerr_);\n> @@ -76,12 +85,12 @@ EncoderLibJpeg::EncoderLibJpeg()\n> \tjpeg_create_compress(&compress_);\n> }\n> \n> -EncoderLibJpeg::~EncoderLibJpeg()\n> +EncoderLibJpeg::Encoder::~Encoder()\n> {\n> \tjpeg_destroy_compress(&compress_);\n> }\n> \n> -int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> +int EncoderLibJpeg::Encoder::configure(const StreamConfiguration &cfg)\n> {\n> \tconst struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);\n> \tif (info.colorSpace == JCS_UNKNOWN)\n> @@ -103,7 +112,7 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> \treturn 0;\n> }\n> \n> -void EncoderLibJpeg::compressRGB(const std::vector> &planes)\n> +void EncoderLibJpeg::Encoder::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> @@ -121,7 +130,7 @@ void EncoderLibJpeg::compressRGB(const std::vector> &planes)\n> * Compress the incoming buffer from a supported NV format.\n> * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.\n> */\n> -void EncoderLibJpeg::compressNV(const std::vector> &planes)\n> +void EncoderLibJpeg::Encoder::compressNV(const std::vector> &planes)\n> {\n> \tuint8_t tmprowbuf[compress_.image_width * 3];\n> \n> @@ -188,12 +197,19 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n> \t\treturn frame.error();\n> \t}\n> \n> -\treturn encode(frame.planes(), dest, exifData, quality);\n> +\treturn encoder_.encode(frame.planes(), dest, exifData, quality);\n> }\n> \n> int EncoderLibJpeg::encode(const std::vector> &src,\n\nLet's not mix-n-match the EncoderLibJpeg and EncoderLibJpeg::Encoder\nfunctions\n\n> -\t\t\t Span dest, Span exifData,\n> -\t\t\t unsigned int quality)\n> +\t\t\t\t Span dest, Span exifData,\n> +\t\t\t\t unsigned int quality)\n\n\nWrong indentation. This looks like an unneeded change.\n\n> +{\n> +\treturn encoder_.encode(src, std::move(dest), std::move(exifData), quality);\n\nI don't think you need std::move here.\n\n> +}\n> +\n> +int EncoderLibJpeg::Encoder::encode(const std::vector> &src,\n> +\t\t\t\t Span dest, Span exifData,\n> +\t\t\t\t unsigned int quality)\n> {\n> \tunsigned char *destination = dest.data();\n> \tunsigned long size = dest.size();\n> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> index 1b3ac067..97182b96 100644\n> --- a/src/android/jpeg/encoder_libjpeg.h\n> +++ b/src/android/jpeg/encoder_libjpeg.h\n> @@ -32,14 +32,30 @@ public:\n> \t\t unsigned int quality);\n> \n> private:\n> -\tvoid compressRGB(const std::vector> &planes);\n> -\tvoid compressNV(const std::vector> &planes);\n> +\tclass Encoder\n> +\t{\n> +\tpublic:\n> +\t\tEncoder();\n> +\t\t~Encoder();\n> \n> -\tstruct jpeg_compress_struct compress_;\n> -\tstruct jpeg_error_mgr jerr_;\n> +\t\tint encode(const std::vector> &planes,\n> +\t\t\t libcamera::Span destination,\n> +\t\t\t libcamera::Span exifData,\n> +\t\t\t unsigned int quality);\n> +\t\tint configure(const libcamera::StreamConfiguration &cfg);\n\nPlease declare the functions in the same order as in the EncoderLibJpeg\nclass, with configure before encode, and define functions in the same\norder in the .cpp file.\n\nI can handle all these changes when applying the patch if nothing else\nin the series requires sending a new version. Otherwise, after fixing\nthese, you can add my \n\nReviewed-by: Laurent Pinchart \n\n> \n> -\tconst libcamera::PixelFormatInfo *pixelFormatInfo_;\n> +\tprivate:\n> +\t\tvoid compressRGB(const std::vector> &planes);\n> +\t\tvoid compressNV(const std::vector> &planes);\n> \n> -\tbool nv_;\n> -\tbool nvSwap_;\n> +\t\tstruct jpeg_compress_struct compress_;\n> +\t\tstruct jpeg_error_mgr jerr_;\n> +\n> +\t\tconst libcamera::PixelFormatInfo *pixelFormatInfo_;\n> +\n> +\t\tbool nv_;\n> +\t\tbool nvSwap_;\n> +\t};\n> +\n> +\tEncoder encoder_;\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 B8503BD87C\n\tfor ;\n\tSun, 15 Jan 2023 23:13:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 10B63625E4;\n\tMon, 16 Jan 2023 00:13:26 +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 2737D625D0\n\tfor ;\n\tMon, 16 Jan 2023 00:13:24 +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 31344997;\n\tMon, 16 Jan 2023 00:13:23 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673824406;\n\tbh=IBfQhEmo4aJr0A/kd6fqNtCo+sjEV7YVyFuOYZZs82w=;\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=idt/rucPPIbNurzU45ccyK/YlV2eWlqa4stBHehrLwaz6Fa2j6C+YxeECA0wYxDUG\n\t7tEdJRyDgrwo2RKdAG+KK5KxPpDoDqDjyEpvWyd3+yD50YpBUfwAzQ651+ZrQIABWN\n\tOxpOclBwbqI7la4yEOAzGWmBjWEo15GmfhnI9rk8f4yOaOuisZChhw5sVu6EMGTDBs\n\t9IiDxEYqJS6i0buwSUA2+x3enmcDc6gcx9boTXCz7gdgJUjAzwkDkv0j9FWvi1CV86\n\tVih5AQRA2vcSeJuMME2/9duqn3pH1jsut0WXmUbE/b6DvIXAU3aqSTRNvs27fe99CR\n\tvu8XsW+gVoQ1Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1673824403;\n\tbh=IBfQhEmo4aJr0A/kd6fqNtCo+sjEV7YVyFuOYZZs82w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M0SkxqQ0a4vr1awgc5MPC9f6NRtXV53d7Q/6M7p5Qf8ukgIIkt7tmEV7aFV3q50HN\n\tv0AJphs74NvQytNxaJ2BdTD56zIs4495v7exgP+hKPuiDuoxhAMdDMVaMm1ENuOPT/\n\txs1kBtiX7IXJJ+J1DnR6HLE9Fs3FbrSoEKGYQTdQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"M0SkxqQ0\"; dkim-atps=neutral","Date":"Mon, 16 Jan 2023 01:13:23 +0200","To":"Harvey Yang ","Message-ID":"","References":"<20221214093330.3345421-1-chenghaoyang@google.com>\n\t<20221214093330.3345421-5-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221214093330.3345421-5-chenghaoyang@google.com>","Subject":"Re: [libcamera-devel] [PATCH v8 4/7] Add an internal Encoder class\n\tin 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":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]