[{"id":12958,"web_url":"https://patchwork.libcamera.org/comment/12958/","msgid":"<20201004191657.GG8774@pendragon.ideasonboard.com>","date":"2020-10-04T19:16:57","subject":"Re: [libcamera-devel] [PATCH v1 1/3] android: jpeg: Add a basic\n\tNV12 image thumbnailer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Fri, Oct 02, 2020 at 04:04:14PM +0530, Umang Jain wrote:\n> Add a basic image thumbnailer for NV12 frames being captured.\n> It shall generate a thumbnail image to be embedded as a part of\n> EXIF metadata of the frame. The output of the thumbnail will still\n> be NV12.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/jpeg/encoder_libjpeg.cpp | 12 +++\n> src/android/jpeg/thumbnailer.cpp | 106 +++++++++++++++++++++++++++\n> src/android/jpeg/thumbnailer.h | 34 +++++++++\n> src/android/meson.build | 1 +\n> 4 files changed, 153 insertions(+)\n> create mode 100644 src/android/jpeg/thumbnailer.cpp\n> create mode 100644 src/android/jpeg/thumbnailer.h\n> \n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index 510613c..7d097c6 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -6,6 +6,7 @@\n> */\n> \n> #include \"encoder_libjpeg.h\"\n> +#include \"thumbnailer.h\"\n> \n> #include \n> #include \n> @@ -214,6 +215,17 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,\n> \tLOG(JPEG, Debug) << \"JPEG Encode Starting:\" << compress_.image_width\n> \t\t\t << \"x\" << compress_.image_height;\n> \n> +\tThumbnailer thScaler;\n> +\tlibcamera::Span thumbnail;\n> +\tthScaler.configure(Size (compress_.image_width, compress_.image_height),\n> +\t\t\t pixelFormatInfo_->format);\n> +\tthScaler.scaleBuffer(source, thumbnail);\n> +\t/*\n> +\t * \\todo: Discuss if we require scaling here or one level above where\n> +\t * exif data is set (setThumbnail()?).\n> +\t * Setting thumbnailed scaled data seems a bit convulated initially.\n> +\t */\n\nHere's a proposal.\n\nAs the JPEG encoder could also be used to create the thumbnail, I would\ncreate an additional layer. A new JPEGPostProcessor (we can bikeshed the\nname) class would handle the whole JFIF image creation. It would would\ninternally encode the image to JPEG using EncoderLibjpeg, downscale it\nto thumbnail size, compress the thumbnail with EncoderLibjpeg, generate\nthe Exif, and store the JPEG-compressed thumbnail in the Exif. That way,\nJPEG encoding, Exif generation and thumbnail generation are all handled\ninside JPEGPostProcessor and not visible to the CameraDevice or\nCameraStream, and the JPEG encoder itself is kept generic enough to be\nused for both the main image and the thumbnail.\n\n> +\n> \tif (nv_)\n> \t\tcompressNV(&frame);\n> \telse\n> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> new file mode 100644\n> index 0000000..d706ea6\n> --- /dev/null\n> +++ b/src/android/jpeg/thumbnailer.cpp\n> @@ -0,0 +1,106 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * thumbnailer.cpp - Basic image thumbnailer from NV12\n> + */\n> +\n> +#include \"thumbnailer.h\"\n> +#include \"system/graphics.h\"\n\nThis header shouldn't be needed, the implementation of this class should\nnot be Android-specific.\n\n> +\n> +#include \"libcamera/internal/file.h\"\n> +#include \"libcamera/internal/log.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(Thumbnailer)\n> +\n> +Thumbnailer::Thumbnailer()\n> +\t: validConfiguration_(false)\n> +{\n> +}\n> +\n> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)\n> +{\n> +\tsourceSize_ = sourceSize;\n> +\tpixelFormat_ = pixelFormat;\n> +\n> +\tif (pixelFormat_ .fourcc() == HAL_PIXEL_FORMAT_YV12) {\n\nPlease don't use fourcc(), this is completely wrong. PixelFormat stores\na DRM fourcc, and you're comparing it to a completely unrelated value.\nThe PixelFormat class is meant to abstract pixel formats, you must\ncompare it with formats::NV12 instead.\n\nAnd shouldn't the check be reversed ? This accepts all formats except\nNV12.\n\n> +\t\tLOG (Thumbnailer, Error) << \"Failed to configure: Pixel Format \"\n> +\t\t\t\t << pixelFormat_.toString() << \" unsupported.\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tvalidConfiguration_ = true;\n> +}\n\nI'd simplify the API by passing the pixel format and size to the\nscaleBuffer() functionn and remove the configure() function.\n\n> +\n> +/* Compute a thumbnail size with same aspect ratio as source. */\n\nCould you please extend this comment to explain how the size is computed\n? Not a description of the implementation, but the rules that the\nfunction implements.\n\n> +void Thumbnailer::computeThumbnailSize()\n> +{\n> +\tunsigned int baseWidth = 160;\n> +\n> +\tunsigned int width = baseWidth * sourceSize_.width / sourceSize_.height;\n> +\ttargetSize_.width = width - (width % 16);\n> +\ttargetSize_.height = targetSize_.width * sourceSize_.height\n> +\t\t\t / sourceSize_.width;\n> +\tif (targetSize_.height & 1)\n> +\t\ttargetSize_.height++;\n\nWith a source aspect ratio of 4:3, this outputs 208x156 instead of the\nexpected 160x120.\n\n> +}\n> +\n> +int Thumbnailer::scaleBuffer(const FrameBuffer *source, Span &dest)\n> +{\n> +\tMappedFrameBuffer frame(source, PROT_READ);\n\nMappings are expensive. I think the caller should create the\nMappedFrameBuffer and pass it to both the JPEG encoder and the thumbnail\ngenerator.\n\n> +\tif (!frame.isValid()) {\n> +\t\tLOG(Thumbnailer, Error) << \"Failed to map FrameBuffer : \"\n> +\t\t\t\t << strerror(frame.error());\n> +\t\treturn frame.error();\n> +\t}\n> +\n> +\tif (!validConfiguration_) {\n> +\t\tLOG(Thumbnailer, Error) << \"config is unconfigured or invalid.\";\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tcomputeThumbnailSize();\n\nYou can make the function return the size instead of storing it in a\nmember variable.\n\n> +\n> +\tconst unsigned int sw = sourceSize_.width;\n> +\tconst unsigned int sh = sourceSize_.height;\n> +\tconst unsigned int tw = targetSize_.width;\n> +\tconst unsigned int th = targetSize_.height;\n> +\n> +\t/* Image scaling block implementing nearest-neighbour algorithm. */\n> +\tunsigned char *src = static_cast(frame.maps()[0].data());\n> +\tunsigned char *src_c = src + sh * sw;\n> +\tunsigned int cb_pos = 0;\n> +\tunsigned int cr_pos = 1;\n> +\tunsigned char *src_cb, *src_cr;\n> +\n> +\tsize_t dstSize = (th * tw) + ((th/2) * tw);\n\nNo need for parentheses.\n\n> +\tunsigned char *destination = static_cast(malloc(dstSize));\n\nC++ uses new.\n\n> +\tunsigned char *dst = destination;\n> +\tunsigned char *dst_c = destination + th * tw;\n> +\n> +\tfor (unsigned int y = 0; y < th; y+=2) {\n\ns/+=/ += /\n\nThere are a few other locations below that need spaces around operators.\n\n> +\t\tunsigned int sourceY = (sh*y + th/2) / th;\n> +\n> +\t\tsrc_cb = src_c + (sourceY/2) * sw + cb_pos;\n> +\t\tsrc_cr = src_c + (sourceY/2) * sw + cr_pos;\n\nI'd create a new src_y variable to handle the luma plane the same way as\nthe chroma plane.\n\n> +\n> +\t\tfor (unsigned int x = 0; x < tw; x+=2) {\n> +\t\t\tunsigned int sourceX = (sw*x + tw/2) / tw;\n> +\n> +\t\t\tdst[y * tw + x] = src[sw * sourceY + sourceX];\n> +\t\t\tdst[(y+1) * tw + x] = src[sw * (sourceY+1) + sourceX];\n> +\t\t\tdst[y * tw + (x+1)] = src[sw * sourceY + (sourceX+1)];\n> +\t\t\tdst[(y+1) * tw + (x+1)] = src[sw * (sourceY+1) + (sourceX+1)];\n> +\n> +\t\t\tdst_c[(y/2) * tw + x + cb_pos] = src_cb[(sourceX/2) * 2];\n> +\t\t\tdst_c[(y/2) * tw + x + cr_pos] = src_cr[(sourceX/2) * 2];\n\nYou can just do\n\n\t\t\tdst_c[(y/2) * tw + x + 0] = src_c[(sourceX/2) * 2 + 0];\n\t\t\tdst_c[(y/2) * tw + x + 1] = src_c[(sourceX/2) * 2 + 1];\n\nand drop cb_pos, cr_pos, src_cb and src_cr, as the only thing that\nmatters it to copy both pixels.\n\nsrc_c should be handled the same way as src_cb (computed in the outer\nloop), and I would do the same for dst_c (and add a new dst_y).\n\n> +\t\t}\n> +\t}\n> +\n> +\t/* Write scaled pixels to dest */\n> +\tdest = { destination, dstSize };\n\nThis is a very good way to cause memory leaks :-S There's nothing in the\nfunction signature that tells that it allocates memory internally. Let's\ncreate a better interface that makes leaks impossible.\n\n> +\n> +\treturn 0;\n> +}\n> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> new file mode 100644\n> index 0000000..8ba9816\n> --- /dev/null\n> +++ b/src/android/jpeg/thumbnailer.h\n> @@ -0,0 +1,34 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * thumbnailer.h - Basic image thumbnailer from NV12\n> + */\n> +#ifndef __ANDROID_JPEG_THUMBNAILER_H__\n> +#define __ANDROID_JPEG_THUMBNAILER_H__\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/buffer.h\"\n> +#include \"libcamera/internal/formats.h\"\n> +\n> +class Thumbnailer\n> +{\n> +public:\n> +\tThumbnailer();\n> +\n> +\tvoid configure(const libcamera::Size &sourceSize,\n> +\t\t libcamera::PixelFormat pixelFormat);\n> +\tint scaleBuffer(const libcamera::FrameBuffer *source, libcamera::Span &dest);\n> +\n> +private:\n> +\tvoid computeThumbnailSize();\n> +\n> +\tlibcamera::PixelFormat pixelFormat_;\n> +\tlibcamera::Size sourceSize_;\n> +\tlibcamera::Size targetSize_;\n> +\n> +\tbool validConfiguration_;\n> +};\n> +\n> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 0293c20..f35ba04 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -22,6 +22,7 @@ android_hal_sources = files([\n> 'camera_ops.cpp',\n> 'jpeg/encoder_libjpeg.cpp',\n> 'jpeg/exif.cpp',\n> + 'jpeg/thumbnailer.cpp',\n> ])\n> \n> android_camera_metadata_sources = files([","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 294D2C3B5C\n\tfor ;\n\tSun, 4 Oct 2020 19:17:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8E7DA60A00;\n\tSun, 4 Oct 2020 21:17:37 +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 ABF996035F\n\tfor ;\n\tSun, 4 Oct 2020 21:17:36 +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 15DF83B;\n\tSun, 4 Oct 2020 21:17:36 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BZzL/A8w\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601839056;\n\tbh=AfF3br2LAD5+h8HhmAJFKOKgd40yXx98kJIMAwSzIpU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BZzL/A8wd4YXHYpJp4fk1h+WvVZsekfMxmgC+97utCf4beni4+yKnb1ESoT61Ys1f\n\thLJ/ngbU1m89u6FQYIQJrFZYZwY2zuBeeovT5M6wbE5EOFEGjBJr/LAcEsp69MTz1x\n\t4zIzs0DcJbzprpCYPH60XnnSccsRZ5W4NFP7DPG0=","Date":"Sun, 4 Oct 2020 22:16:57 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20201004191657.GG8774@pendragon.ideasonboard.com>","References":"<20201002103416.53623-1-email@uajain.com>\n\t<20201002103416.53623-2-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201002103416.53623-2-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v1 1/3] android: jpeg: Add a basic\n\tNV12 image thumbnailer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12963,"web_url":"https://patchwork.libcamera.org/comment/12963/","msgid":"<20201004204627.GK8774@pendragon.ideasonboard.com>","date":"2020-10-04T20:46:27","subject":"Re: [libcamera-devel] [PATCH v1 1/3] android: jpeg: Add a basic\n\tNV12 image thumbnailer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOne more thing.\n\nOn Sun, Oct 04, 2020 at 10:16:57PM +0300, Laurent Pinchart wrote:\n> On Fri, Oct 02, 2020 at 04:04:14PM +0530, Umang Jain wrote:\n> > Add a basic image thumbnailer for NV12 frames being captured.\n> > It shall generate a thumbnail image to be embedded as a part of\n> > EXIF metadata of the frame. The output of the thumbnail will still\n> > be NV12.\n> > \n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/jpeg/encoder_libjpeg.cpp | 12 +++\n> > src/android/jpeg/thumbnailer.cpp | 106 +++++++++++++++++++++++++++\n> > src/android/jpeg/thumbnailer.h | 34 +++++++++\n> > src/android/meson.build | 1 +\n> > 4 files changed, 153 insertions(+)\n> > create mode 100644 src/android/jpeg/thumbnailer.cpp\n> > create mode 100644 src/android/jpeg/thumbnailer.h\n> > \n> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> > index 510613c..7d097c6 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > @@ -6,6 +6,7 @@\n> > */\n> > \n> > #include \"encoder_libjpeg.h\"\n> > +#include \"thumbnailer.h\"\n> > \n> > #include \n> > #include \n> > @@ -214,6 +215,17 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,\n> > \tLOG(JPEG, Debug) << \"JPEG Encode Starting:\" << compress_.image_width\n> > \t\t\t << \"x\" << compress_.image_height;\n> > \n> > +\tThumbnailer thScaler;\n> > +\tlibcamera::Span thumbnail;\n> > +\tthScaler.configure(Size (compress_.image_width, compress_.image_height),\n> > +\t\t\t pixelFormatInfo_->format);\n> > +\tthScaler.scaleBuffer(source, thumbnail);\n> > +\t/*\n> > +\t * \\todo: Discuss if we require scaling here or one level above where\n> > +\t * exif data is set (setThumbnail()?).\n> > +\t * Setting thumbnailed scaled data seems a bit convulated initially.\n> > +\t */\n> \n> Here's a proposal.\n> \n> As the JPEG encoder could also be used to create the thumbnail, I would\n> create an additional layer. A new JPEGPostProcessor (we can bikeshed the\n> name) class would handle the whole JFIF image creation. It would would\n> internally encode the image to JPEG using EncoderLibjpeg, downscale it\n> to thumbnail size, compress the thumbnail with EncoderLibjpeg, generate\n> the Exif, and store the JPEG-compressed thumbnail in the Exif. That way,\n> JPEG encoding, Exif generation and thumbnail generation are all handled\n> inside JPEGPostProcessor and not visible to the CameraDevice or\n> CameraStream, and the JPEG encoder itself is kept generic enough to be\n> used for both the main image and the thumbnail.\n> \n> > +\n> > \tif (nv_)\n> > \t\tcompressNV(&frame);\n> > \telse\n> > diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> > new file mode 100644\n> > index 0000000..d706ea6\n> > --- /dev/null\n> > +++ b/src/android/jpeg/thumbnailer.cpp\n> > @@ -0,0 +1,106 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * thumbnailer.cpp - Basic image thumbnailer from NV12\n> > + */\n> > +\n> > +#include \"thumbnailer.h\"\n> > +#include \"system/graphics.h\"\n> \n> This header shouldn't be needed, the implementation of this class should\n> not be Android-specific.\n> \n> > +\n> > +#include \"libcamera/internal/file.h\"\n> > +#include \"libcamera/internal/log.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DEFINE_CATEGORY(Thumbnailer)\n> > +\n> > +Thumbnailer::Thumbnailer()\n> > +\t: validConfiguration_(false)\n> > +{\n> > +}\n> > +\n> > +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)\n> > +{\n> > +\tsourceSize_ = sourceSize;\n> > +\tpixelFormat_ = pixelFormat;\n> > +\n> > +\tif (pixelFormat_ .fourcc() == HAL_PIXEL_FORMAT_YV12) {\n> \n> Please don't use fourcc(), this is completely wrong. PixelFormat stores\n> a DRM fourcc, and you're comparing it to a completely unrelated value.\n> The PixelFormat class is meant to abstract pixel formats, you must\n> compare it with formats::NV12 instead.\n> \n> And shouldn't the check be reversed ? This accepts all formats except\n> NV12.\n> \n> > +\t\tLOG (Thumbnailer, Error) << \"Failed to configure: Pixel Format \"\n> > +\t\t\t\t << pixelFormat_.toString() << \" unsupported.\";\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tvalidConfiguration_ = true;\n> > +}\n> \n> I'd simplify the API by passing the pixel format and size to the\n> scaleBuffer() functionn and remove the configure() function.\n> \n> > +\n> > +/* Compute a thumbnail size with same aspect ratio as source. */\n> \n> Could you please extend this comment to explain how the size is computed\n> ? Not a description of the implementation, but the rules that the\n> function implements.\n> \n> > +void Thumbnailer::computeThumbnailSize()\n> > +{\n> > +\tunsigned int baseWidth = 160;\n> > +\n> > +\tunsigned int width = baseWidth * sourceSize_.width / sourceSize_.height;\n> > +\ttargetSize_.width = width - (width % 16);\n> > +\ttargetSize_.height = targetSize_.width * sourceSize_.height\n> > +\t\t\t / sourceSize_.width;\n> > +\tif (targetSize_.height & 1)\n> > +\t\ttargetSize_.height++;\n> \n> With a source aspect ratio of 4:3, this outputs 208x156 instead of the\n> expected 160x120.\n> \n> > +}\n> > +\n> > +int Thumbnailer::scaleBuffer(const FrameBuffer *source, Span &dest)\n> > +{\n> > +\tMappedFrameBuffer frame(source, PROT_READ);\n> \n> Mappings are expensive. I think the caller should create the\n> MappedFrameBuffer and pass it to both the JPEG encoder and the thumbnail\n> generator.\n> \n> > +\tif (!frame.isValid()) {\n> > +\t\tLOG(Thumbnailer, Error) << \"Failed to map FrameBuffer : \"\n> > +\t\t\t\t << strerror(frame.error());\n> > +\t\treturn frame.error();\n> > +\t}\n> > +\n> > +\tif (!validConfiguration_) {\n> > +\t\tLOG(Thumbnailer, Error) << \"config is unconfigured or invalid.\";\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > +\tcomputeThumbnailSize();\n> \n> You can make the function return the size instead of storing it in a\n> member variable.\n> \n> > +\n> > +\tconst unsigned int sw = sourceSize_.width;\n> > +\tconst unsigned int sh = sourceSize_.height;\n> > +\tconst unsigned int tw = targetSize_.width;\n> > +\tconst unsigned int th = targetSize_.height;\n> > +\n> > +\t/* Image scaling block implementing nearest-neighbour algorithm. */\n> > +\tunsigned char *src = static_cast(frame.maps()[0].data());\n> > +\tunsigned char *src_c = src + sh * sw;\n\nThe stride shouldn't be hardcoded to sh, but should come from the stream\nconfiguration. This doesn't have to be implemented yet, a \\todo comment\nwould be fine for now (see encoder_libjpeg.cpp). I would however\nrecommend using PixelFormatInfo to calculate the stride for now (see\nencoder_libjpeg.cpp too :-)).\n\n> > +\tunsigned int cb_pos = 0;\n> > +\tunsigned int cr_pos = 1;\n> > +\tunsigned char *src_cb, *src_cr;\n> > +\n> > +\tsize_t dstSize = (th * tw) + ((th/2) * tw);\n> \n> No need for parentheses.\n> \n> > +\tunsigned char *destination = static_cast(malloc(dstSize));\n> \n> C++ uses new.\n> \n> > +\tunsigned char *dst = destination;\n> > +\tunsigned char *dst_c = destination + th * tw;\n> > +\n> > +\tfor (unsigned int y = 0; y < th; y+=2) {\n> \n> s/+=/ += /\n> \n> There are a few other locations below that need spaces around operators.\n> \n> > +\t\tunsigned int sourceY = (sh*y + th/2) / th;\n> > +\n> > +\t\tsrc_cb = src_c + (sourceY/2) * sw + cb_pos;\n> > +\t\tsrc_cr = src_c + (sourceY/2) * sw + cr_pos;\n> \n> I'd create a new src_y variable to handle the luma plane the same way as\n> the chroma plane.\n> \n> > +\n> > +\t\tfor (unsigned int x = 0; x < tw; x+=2) {\n> > +\t\t\tunsigned int sourceX = (sw*x + tw/2) / tw;\n> > +\n> > +\t\t\tdst[y * tw + x] = src[sw * sourceY + sourceX];\n> > +\t\t\tdst[(y+1) * tw + x] = src[sw * (sourceY+1) + sourceX];\n> > +\t\t\tdst[y * tw + (x+1)] = src[sw * sourceY + (sourceX+1)];\n> > +\t\t\tdst[(y+1) * tw + (x+1)] = src[sw * (sourceY+1) + (sourceX+1)];\n> > +\n> > +\t\t\tdst_c[(y/2) * tw + x + cb_pos] = src_cb[(sourceX/2) * 2];\n> > +\t\t\tdst_c[(y/2) * tw + x + cr_pos] = src_cr[(sourceX/2) * 2];\n> \n> You can just do\n> \n> \t\t\tdst_c[(y/2) * tw + x + 0] = src_c[(sourceX/2) * 2 + 0];\n> \t\t\tdst_c[(y/2) * tw + x + 1] = src_c[(sourceX/2) * 2 + 1];\n> \n> and drop cb_pos, cr_pos, src_cb and src_cr, as the only thing that\n> matters it to copy both pixels.\n> \n> src_c should be handled the same way as src_cb (computed in the outer\n> loop), and I would do the same for dst_c (and add a new dst_y).\n> \n> > +\t\t}\n> > +\t}\n> > +\n> > +\t/* Write scaled pixels to dest */\n> > +\tdest = { destination, dstSize };\n> \n> This is a very good way to cause memory leaks :-S There's nothing in the\n> function signature that tells that it allocates memory internally. Let's\n> create a better interface that makes leaks impossible.\n> \n> > +\n> > +\treturn 0;\n> > +}\n> > diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> > new file mode 100644\n> > index 0000000..8ba9816\n> > --- /dev/null\n> > +++ b/src/android/jpeg/thumbnailer.h\n> > @@ -0,0 +1,34 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * thumbnailer.h - Basic image thumbnailer from NV12\n> > + */\n> > +#ifndef __ANDROID_JPEG_THUMBNAILER_H__\n> > +#define __ANDROID_JPEG_THUMBNAILER_H__\n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/buffer.h\"\n> > +#include \"libcamera/internal/formats.h\"\n> > +\n> > +class Thumbnailer\n> > +{\n> > +public:\n> > +\tThumbnailer();\n> > +\n> > +\tvoid configure(const libcamera::Size &sourceSize,\n> > +\t\t libcamera::PixelFormat pixelFormat);\n> > +\tint scaleBuffer(const libcamera::FrameBuffer *source, libcamera::Span &dest);\n> > +\n> > +private:\n> > +\tvoid computeThumbnailSize();\n> > +\n> > +\tlibcamera::PixelFormat pixelFormat_;\n> > +\tlibcamera::Size sourceSize_;\n> > +\tlibcamera::Size targetSize_;\n> > +\n> > +\tbool validConfiguration_;\n> > +};\n> > +\n> > +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */\n> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > index 0293c20..f35ba04 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -22,6 +22,7 @@ android_hal_sources = files([\n> > 'camera_ops.cpp',\n> > 'jpeg/encoder_libjpeg.cpp',\n> > 'jpeg/exif.cpp',\n> > + 'jpeg/thumbnailer.cpp',\n> > ])\n> > \n> > android_camera_metadata_sources = files([","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A7CC4C3B5D\n\tfor ;\n\tSun, 4 Oct 2020 20:47:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E75563BBF;\n\tSun, 4 Oct 2020 22:47:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E6EBF6035B\n\tfor ;\n\tSun, 4 Oct 2020 22:47:06 +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 65A043B;\n\tSun, 4 Oct 2020 22:47:06 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Shy1vUxP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601844426;\n\tbh=yBMGL6UI2AqsTT6NMkzu/dnCMFxJkmpoNe0L5b5xgV8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Shy1vUxPapN3ejcv4N4yuZLnoZ8u/ys14Jv29hF43tM/mrWGCf8/TTOlPOhz/Itrf\n\temh8cGaiP+9ZMl0psQBXzXc4FSaUhQn0K1ySjHjGoF6Ajx0ghw72lTQw1cafWFKVQy\n\tV4X1CivnB0efsJrEzcdvUMjkmVilgkjPwXFLW++E=","Date":"Sun, 4 Oct 2020 23:46:27 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20201004204627.GK8774@pendragon.ideasonboard.com>","References":"<20201002103416.53623-1-email@uajain.com>\n\t<20201002103416.53623-2-email@uajain.com>\n\t<20201004191657.GG8774@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201004191657.GG8774@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 1/3] android: jpeg: Add a basic\n\tNV12 image thumbnailer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]