[{"id":12777,"web_url":"https://patchwork.libcamera.org/comment/12777/","msgid":"<67d2a7e4-6e48-8b24-576b-b5b6b2d20432@ideasonboard.com>","date":"2020-09-25T15:03:56","subject":"Re: [libcamera-devel] [PATCH 1/1] android: jpeg: Add a basic NV12\n\timage scaler","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 22/09/2020 09:54, Umang Jain wrote:\n> Add a basic image scaler for NV12 frames being captured. The primary\n> use of this scaler will be to generate a thumbnail image to be embedded\n> as a part of EXIF metadata of the frame.\n> \n> Signed-off-by: Umang Jain <email@uajain.com>\n> ---\n>  src/android/jpeg/encoder_libjpeg.cpp |  10 ++\n>  src/android/jpeg/scaler.cpp          | 137 +++++++++++++++++++++++++++\n>  src/android/jpeg/scaler.h            |  32 +++++++\n>  src/android/meson.build              |   1 +\n>  4 files changed, 180 insertions(+)\n>  create mode 100644 src/android/jpeg/scaler.cpp\n>  create mode 100644 src/android/jpeg/scaler.h\n> \n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index 510613c..9ecf9b1 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 \"scaler.h\"\n>  \n>  #include <fcntl.h>\n>  #include <iomanip>\n> @@ -214,6 +215,15 @@ 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> +\tScaler scaler;\n> +\tlibcamera::Span<uint8_t> thumbnail;\n> +\tscaler.configure(Size (compress_.image_width, compress_.image_height),\n> +\t\t\t /* \\todo: Check for exact thumbnail size required by EXIF spec. */\n> +\t\t         Size (compress_.image_width / 3, compress_.image_height / 3),\n\n\nIndeed, I think this should set explicit sizes, not simply reduce to a\nthird.\n\nhttp://www.fifi.org/doc/jhead/exif-e.html#ExifThumbs\n\nStates, that usually the thumbnails are 160x120 for Exif2.1 or later,\nand are usually encoded as JPG we can use another JPEG compressor\ninstance most likely, but if the RGB format works, that's fine too for now.\n\nI'd hard code the size to 160 x 120 all the same.\n\n> +\t\t\t pixelFormatInfo_);\n> +\tscaler.scaleBuffer(source, thumbnail);\n> +\t/* \\todo: Write thumbnail as part of exifData. */\n> +\n>  \tif (nv_)\n>  \t\tcompressNV(&frame);\n>  \telse\n> diff --git a/src/android/jpeg/scaler.cpp b/src/android/jpeg/scaler.cpp\n> new file mode 100644\n> index 0000000..ff36ece\n> --- /dev/null\n> +++ b/src/android/jpeg/scaler.cpp\n> @@ -0,0 +1,137 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * scaler.cpp - Basic image scaler from NV12 to RGB24 format\n> + */\n> +\n> +#include \"scaler.h\"\n> +\n> +#include <math.h>\n> +#include <string.h>\n> +\n> +#include \"libcamera/internal/log.h\"\n> +#include \"libcamera/internal/file.h\"\n> +\n> +#define RGBSHIFT                8\n> +#ifndef MAX\n> +#define MAX(a,b)                ((a)>(b)?(a):(b))\n> +#endif\n> +#ifndef MIN\n> +#define MIN(a,b)                ((a)<(b)?(a):(b))\n> +#endif\n\n#include <algorithm>\nwill give you std::min, std::max, and std::clamp()\n\n> +#ifndef CLAMP\n> +#define CLAMP(a,low,high)       MAX((low),MIN((high),(a)))\n> +#endif\n\n\n> +#ifndef CLIP\n> +#define CLIP(x)                 CLAMP(x,0,255)\n> +#endif\n\nI think this one is distinct though, Depends on how it's utilised to see\nwhether it's worth just using std::clamp directly.\n\n\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(SCALER)\n> +\n> +Scaler::Scaler()\n> +\t: sourceSize_(0, 0), targetSize_(0,0)\n> +{\n> +}\n> +\n> +Scaler::~Scaler()\n> +{\n> +}\n> +\n> +void Scaler::configure(Size sourceSize, Size targetSize, const PixelFormatInfo *pixelFormatInfo)\n> +{\n> +\tsourceSize_ = sourceSize;\n> +\ttargetSize_ = targetSize;\n> +\tpixelFormatInfo_ = pixelFormatInfo;\n> +}\n> +\n> +static std::string datetime()\n> +{\n> +\ttime_t rawtime;\n> +\tstruct tm *timeinfo;\n> +\tchar buffer[80];\n> +\tstatic unsigned int milliseconds = 0;\n> +\n> +\ttime(&rawtime);\n> +\ttimeinfo = localtime(&rawtime);\n> +\n> +\tstrftime(buffer, 80, \"%d-%m-%Y.%H-%M-%S.\", timeinfo);\n> +\n> +\t/* milliseconds is just a fast hack to ensure unique filenames */\n> +\treturn std::string(buffer) + std::to_string(milliseconds++);\n> +}\n\nI think this function is here for debug, but shouldn't be in the scaler.\n\n> +\n> +/* Handpicked from src/qcam/format_converter.cpp */\n> +static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)\n> +{\n> +\tint c = y - 16;\n> +\tint d = u - 128;\n> +\tint e = v - 128;\n> +\t*r = CLIP(( 298 * c           + 409 * e + 128) >> RGBSHIFT);\n> +\t*g = CLIP(( 298 * c - 100 * d - 208 * e + 128) >> RGBSHIFT);\n> +\t*b = CLIP(( 298 * c + 516 * d           + 128) >> RGBSHIFT);\n> +}\n\nCLIP does look better than std::clamp there.\n\nAs RGBSHIFT is only used here, maybe put it as a constexpr in this function?\n\n\nBut I wonder if we should really be making the scaler do pixel-format\nconversions.\n\nIn the case of the Thumbnail generations it's a nice optimisztion, but\nit doesn't feel right, and leads to the object being an anything to\nanything scaler/convertor.\n\nAs that is possible in some hardware (scale and pixel conversion\ncombined) I'm almost tempted to say it's ok ... but I'm just not sure.\n\nThe alternative would be to take the incoming image, rescale it to the\nthumbnail, and then run that through another JPEG compressor anyway,\nwhich already supports NV12.\n\n\n> +\n> +int Scaler::scaleBuffer(const FrameBuffer *source, Span<uint8_t> &dest)\n> +{\n> +\tMappedFrameBuffer frame(source, PROT_READ);\n> +\tif (!frame.isValid()) {\n> +\t\tLOG(SCALER, Error) << \"Failed to map FrameBuffer : \"\n> +\t\t\t\t   << strerror(frame.error());\n> +\t\treturn frame.error();\n> +\t}\n> +\n> +\tif (strcmp(pixelFormatInfo_->name, \"NV12\") != 0) {\n\nCan you compare against formats::NV12 directly?\n\n \tif (pixelFormatInfo_->format != formats::NV12) {\n\n> +\t\tLOG (SCALER, Info) << \"Source Buffer not in NV12 format, returning...\";\n> +\t\treturn -1;\n\nThis should be in configure() though, not on every call to scaleBuffer().\n\n> +\t}\n> +\n> +\t/* Image scaling block implementing nearest-neighbour algorithm. */\n> +\t{\n\nI'd remove the scoping here and indent left - or if you want to keep it\ndistinct, put it into a function called\n\n  scaleNearestNeighbour(MappedFrameBuffer source*, Span<uint8_t> &dest)\n\nThat would leave scale() only doing the mapping ... but make the code\npaths clear if another scaler implementation was added later...?\n\n\n> +\t\tunsigned int cb_pos = 0;\n> +\t\tunsigned int cr_pos = 1;\n> +\t\tunsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());\n> +\t\tunsigned char *src_c = src + sourceSize_.height * sourceSize_.width;\n> +\t\tunsigned int stride = sourceSize_.width;\n> +\t\tunsigned char *src_y, *src_cb, *src_cr;\n> +\n> +\t\tunsigned int bpp = 3;\n> +\t\tsize_t dstSize = targetSize_.height * targetSize_.width * bpp;\n> +\t\tunsigned char *dst = static_cast<unsigned char *>(malloc(dstSize));\n\nHrm ... so the caller would have to know to free the span it receives.\nThat's not nice as you normally expect a Span to by just a pointer into\nsome space, that you don't have control over.\n\nI think the caller should probably allocate and provide the destination\nmemory.\n\n\n> +\t\tunsigned char *destination = dst;\n> +\t\tint r, g, b;\n> +\n> +\t\tfor (unsigned int y = 0; y < targetSize_.height; y++) {\n> +\t\t\tint32_t sourceY = lround(sourceSize_.height * y / targetSize_.height);\n> +\n> +\t\t\tfor (unsigned int x = 0; x < targetSize_.width; x++) {\n> +\t\t\t\tint32_t sourceX = lround(sourceSize_.width * x / targetSize_.width);\n> +\n> +\t\t\t\tsrc_y = src + stride * sourceY + sourceX;\n> +\t\t\t\tsrc_cb = src_c + (sourceY / 2) * stride + (sourceX / 2) * 2 + cb_pos;\n> +\t\t\t\tsrc_cr = src_c + (sourceY / 2) * stride + (sourceX / 2) * 2 + cr_pos;\n> +\n> +\t\t\t\tyuv_to_rgb(*src_y, *src_cb, *src_cr, &r, &g, &b);\n> +\n> +\t\t\t\tdestination[x * bpp + 0] = r;\n> +\t\t\t\tdestination[x * bpp + 1] = g;\n> +\t\t\t\tdestination[x * bpp + 2] = b;\n> +\t\t\t}\n> +\n> +\t\t\tdestination = destination + bpp * targetSize_.width;\n> +\t\t}\n> +\n> +\t\t/* Helper code: Write the output pixels to a file so we can inspect */\n> +\t\tFile file(\"/tmp/\" + datetime() + \".raw\");\n> +\t\tint32_t ret = file.open(File::WriteOnly);\n> +\t\tret = file.write({ dst, dstSize });\n> +\t\tLOG(SCALER, Info) << \"Wrote \" << ret << \" bytes: \" << targetSize_.width << \"x\" << targetSize_.height;\n> +\n\nWe should keep debug to a separate patch.\n\nIt can be provided in the series with a DNI prefix if it's helpful\nduring review/development - but try to keep the patch targeted for\nintegration clean.\n\n> +\t\t/* Write scaled pixels to dest */\n> +\t\tdest = { dst, dstSize };\n\nIf the caller provides the memory, we shouldn't need to do this, as we\nshould be dealing with fixed size buffers, so the dest should be already\ncorrectly sized.\n\n\n\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> diff --git a/src/android/jpeg/scaler.h b/src/android/jpeg/scaler.h\n> new file mode 100644\n> index 0000000..c68a279\n> --- /dev/null\n> +++ b/src/android/jpeg/scaler.h\n> @@ -0,0 +1,32 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * Basic image scaler from NV12 to RGB24 format\n\nThat's a scaler and pixelformat convertor ;-)\n\n> + */\n> +#ifndef __ANDROID_JPEG_SCALER_H__\n> +#define __ANDROID_JPEG_SCALER_H__\n> +\n> +#include <libcamera/geometry.h>\n> +\n> +#include \"libcamera/internal/buffer.h\"\n> +#include \"libcamera/internal/formats.h\"\n> +\n> +class Scaler\n> +{\n> +public:\n> +\tScaler();\n> +\t~Scaler();\n> +\n> +\tvoid configure(libcamera::Size sourceSize, libcamera::Size targetSize,\n> +\t\t       const libcamera::PixelFormatInfo *pixelFormatInfo);\n> +\tint scaleBuffer(const libcamera::FrameBuffer *source, libcamera::Span<uint8_t> &dest);\n> +\n> +private:\n> +\n> +\tlibcamera::Size sourceSize_;\n> +\tlibcamera::Size targetSize_;\n> +\tconst libcamera::PixelFormatInfo *pixelFormatInfo_;\n> +};\n> +\n> +#endif /* __ANDROID_JPEG_SCALER_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 0293c20..aefb0da 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/scaler.cpp',\n>  ])\n>  \n>  android_camera_metadata_sources = files([\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 093B8C3B5C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 25 Sep 2020 15:04:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C9CB263049;\n\tFri, 25 Sep 2020 17:04:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DED2862FD8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 25 Sep 2020 17:03:59 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5929A2D7;\n\tFri, 25 Sep 2020 17:03:58 +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=\"cO53uJ+3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601046239;\n\tbh=kPmJ9bBxbSuyDCX2NGubmIn/P093leu1c3a5l8kWJIM=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=cO53uJ+3a3JVU/mekQmRb34rse4v77vrqf4/vbSXUrw+W1cGwqqjrPCyroDQ3GuYB\n\tyJqlP7X/fKMWtPeXT8yWrvH+JlKjW9ZpyfvUGe0H4yiAfa5xqhr0a9GboEAoM6NL0r\n\teJ/TKLJ+OzqsitGtl+S99Dbe+Qx0rf4xCwM5ZgH4=","To":"Umang Jain <email@uajain.com>, libcamera-devel@lists.libcamera.org","References":"<20200922085444.15764-1-email@uajain.com>\n\t<20200922085444.15764-2-email@uajain.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<67d2a7e4-6e48-8b24-576b-b5b6b2d20432@ideasonboard.com>","Date":"Fri, 25 Sep 2020 16:03:56 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200922085444.15764-2-email@uajain.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 1/1] android: jpeg: Add a basic NV12\n\timage scaler","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12788,"web_url":"https://patchwork.libcamera.org/comment/12788/","msgid":"<4f080f39-8640-82e2-2174-0420335db570@uajain.com>","date":"2020-09-25T20:36:59","subject":"Re: [libcamera-devel] [PATCH 1/1] android: jpeg: Add a basic NV12\n\timage scaler","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Kieran\n\nThanks for the review\n\nOn 9/25/20 8:33 PM, Kieran Bingham wrote:\n> Hi Umang,\n>\n> On 22/09/2020 09:54, Umang Jain wrote:\n>> Add a basic image scaler for NV12 frames being captured. The primary\n>> use of this scaler will be to generate a thumbnail image to be embedded\n>> as a part of EXIF metadata of the frame.\n>>\n>> Signed-off-by: Umang Jain <email@uajain.com>\n>> ---\n>>   src/android/jpeg/encoder_libjpeg.cpp |  10 ++\n>>   src/android/jpeg/scaler.cpp          | 137 +++++++++++++++++++++++++++\n>>   src/android/jpeg/scaler.h            |  32 +++++++\n>>   src/android/meson.build              |   1 +\n>>   4 files changed, 180 insertions(+)\n>>   create mode 100644 src/android/jpeg/scaler.cpp\n>>   create mode 100644 src/android/jpeg/scaler.h\n>>\n>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n>> index 510613c..9ecf9b1 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 \"scaler.h\"\n>>   \n>>   #include <fcntl.h>\n>>   #include <iomanip>\n>> @@ -214,6 +215,15 @@ 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>> +\tScaler scaler;\n>> +\tlibcamera::Span<uint8_t> thumbnail;\n>> +\tscaler.configure(Size (compress_.image_width, compress_.image_height),\n>> +\t\t\t /* \\todo: Check for exact thumbnail size required by EXIF spec. */\n>> +\t\t         Size (compress_.image_width / 3, compress_.image_height / 3),\n>\n> Indeed, I think this should set explicit sizes, not simply reduce to a\n> third.\n>\n> http://www.fifi.org/doc/jhead/exif-e.html#ExifThumbs\n>\n> States, that usually the thumbnails are 160x120 for Exif2.1 or later,\n> and are usually encoded as JPG we can use another JPEG compressor\n> instance most likely, but if the RGB format works, that's fine too for now.\nThe EXIF standard states that:\n```\nNo limit is placed on the size of thumbnail images. It is optional to \nrecord thumbnails but it is recommended that they be recorded if \npossible, unless hardware or other restrictions preclude this.Thumbnail \ndata does not necessarily have to adopt the same data structure as that \nused for primary images. If, however, the primary images are recorded as \nuncompressed RGB data or as uncompressed YCbCr data, thumbnail images \nshall not be recorded as JPEG compressed data.\n\nWhen thumbnails are recorded in uncompressed format, they are to be \nrecorded in the 1st IFD in conformance with Baseline TIFF Rev. 6.0 RGB \nFull Color Images or TIFF Rev. 6.0 Extensions YCbCr Images.\n```\n(Page 24 - Exif Version 2.31 spec)\n\nSo it seems we can write uncompressed data for thumbnail in RGB888, \nright? It also states YCbCr  too (for uncompressed format) , so maybe we \nwant that? That would take out the 'converter' part of this patch.\nI am not really sure which format to use, so I will move forward as per \nthe advice given in the reviews...\n>\n> I'd hard code the size to 160 x 120 all the same.\nYeah, that would come along in a proper patch.\n>\n>> +\t\t\t pixelFormatInfo_);\n>> +\tscaler.scaleBuffer(source, thumbnail);\n>> +\t/* \\todo: Write thumbnail as part of exifData. */\n>> +\n>>   \tif (nv_)\n>>   \t\tcompressNV(&frame);\n>>   \telse\n>> diff --git a/src/android/jpeg/scaler.cpp b/src/android/jpeg/scaler.cpp\n>> new file mode 100644\n>> index 0000000..ff36ece\n>> --- /dev/null\n>> +++ b/src/android/jpeg/scaler.cpp\n>> @@ -0,0 +1,137 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * scaler.cpp - Basic image scaler from NV12 to RGB24 format\n>> + */\n>> +\n>> +#include \"scaler.h\"\n>> +\n>> +#include <math.h>\n>> +#include <string.h>\n>> +\n>> +#include \"libcamera/internal/log.h\"\n>> +#include \"libcamera/internal/file.h\"\n>> +\n>> +#define RGBSHIFT                8\n>> +#ifndef MAX\n>> +#define MAX(a,b)                ((a)>(b)?(a):(b))\n>> +#endif\n>> +#ifndef MIN\n>> +#define MIN(a,b)                ((a)<(b)?(a):(b))\n>> +#endif\n> #include <algorithm>\n> will give you std::min, std::max, and std::clamp()\n>\n>> +#ifndef CLAMP\n>> +#define CLAMP(a,low,high)       MAX((low),MIN((high),(a)))\n>> +#endif\n>\n>> +#ifndef CLIP\n>> +#define CLIP(x)                 CLAMP(x,0,255)\n>> +#endif\n> I think this one is distinct though, Depends on how it's utilised to see\n> whether it's worth just using std::clamp directly.\n>\n>\n>> +\n>> +using namespace libcamera;\n>> +\n>> +LOG_DEFINE_CATEGORY(SCALER)\n>> +\n>> +Scaler::Scaler()\n>> +\t: sourceSize_(0, 0), targetSize_(0,0)\n>> +{\n>> +}\n>> +\n>> +Scaler::~Scaler()\n>> +{\n>> +}\n>> +\n>> +void Scaler::configure(Size sourceSize, Size targetSize, const PixelFormatInfo *pixelFormatInfo)\n>> +{\n>> +\tsourceSize_ = sourceSize;\n>> +\ttargetSize_ = targetSize;\n>> +\tpixelFormatInfo_ = pixelFormatInfo;\n>> +}\n>> +\n>> +static std::string datetime()\n>> +{\n>> +\ttime_t rawtime;\n>> +\tstruct tm *timeinfo;\n>> +\tchar buffer[80];\n>> +\tstatic unsigned int milliseconds = 0;\n>> +\n>> +\ttime(&rawtime);\n>> +\ttimeinfo = localtime(&rawtime);\n>> +\n>> +\tstrftime(buffer, 80, \"%d-%m-%Y.%H-%M-%S.\", timeinfo);\n>> +\n>> +\t/* milliseconds is just a fast hack to ensure unique filenames */\n>> +\treturn std::string(buffer) + std::to_string(milliseconds++);\n>> +}\n> I think this function is here for debug, but shouldn't be in the scaler.\n>\n>> +\n>> +/* Handpicked from src/qcam/format_converter.cpp */\n>> +static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)\n>> +{\n>> +\tint c = y - 16;\n>> +\tint d = u - 128;\n>> +\tint e = v - 128;\n>> +\t*r = CLIP(( 298 * c           + 409 * e + 128) >> RGBSHIFT);\n>> +\t*g = CLIP(( 298 * c - 100 * d - 208 * e + 128) >> RGBSHIFT);\n>> +\t*b = CLIP(( 298 * c + 516 * d           + 128) >> RGBSHIFT);\n>> +}\n> CLIP does look better than std::clamp there.\n>\n> As RGBSHIFT is only used here, maybe put it as a constexpr in this function?\n>\n>\n> But I wonder if we should really be making the scaler do pixel-format\n> conversions.\n>\n> In the case of the Thumbnail generations it's a nice optimisztion, but\n> it doesn't feel right, and leads to the object being an anything to\n> anything scaler/convertor.\n>\n> As that is possible in some hardware (scale and pixel conversion\n> combined) I'm almost tempted to say it's ok ... but I'm just not sure.\nIf it seems OK and *if* we do end up keeping this scaler + converter \ncombo, I would rename the class as Thumbnailer, instead of Scaler.\n>\n> The alternative would be to take the incoming image, rescale it to the\n> thumbnail, and then run that through another JPEG compressor anyway,\n> which already supports NV12.\nThat sounds an overkill maybe? Not sure... The EXIF spec supports \nuncompressed YCbCr as thumbnail data, so ... we can just scale down our \nNV12.\n\nOne question here to tinker is, what's in-general use-case of the \nthumbnail data? Image previewing? in something like file-manager(s) / \napps. I would love to learn if  they decode this data and then display \nit (like 'raw2rgbgnm' equivalent) maybe? Or they do something completely \ndifferent\n>\n>\n>> +\n>> +int Scaler::scaleBuffer(const FrameBuffer *source, Span<uint8_t> &dest)\n>> +{\n>> +\tMappedFrameBuffer frame(source, PROT_READ);\n>> +\tif (!frame.isValid()) {\n>> +\t\tLOG(SCALER, Error) << \"Failed to map FrameBuffer : \"\n>> +\t\t\t\t   << strerror(frame.error());\n>> +\t\treturn frame.error();\n>> +\t}\n>> +\n>> +\tif (strcmp(pixelFormatInfo_->name, \"NV12\") != 0) {\n> Can you compare against formats::NV12 directly?\n>\n>   \tif (pixelFormatInfo_->format != formats::NV12) {\n>\n>> +\t\tLOG (SCALER, Info) << \"Source Buffer not in NV12 format, returning...\";\n>> +\t\treturn -1;\n> This should be in configure() though, not on every call to scaleBuffer().\nah yeah, Make sense.\n>\n>> +\t}\n>> +\n>> +\t/* Image scaling block implementing nearest-neighbour algorithm. */\n>> +\t{\n> I'd remove the scoping here and indent left - or if you want to keep it\n> distinct, put it into a function called\n>\n>    scaleNearestNeighbour(MappedFrameBuffer source*, Span<uint8_t> &dest)\n>\n> That would leave scale() only doing the mapping ... but make the code\n> paths clear if another scaler implementation was added later...?\n>\n>\n>> +\t\tunsigned int cb_pos = 0;\n>> +\t\tunsigned int cr_pos = 1;\n>> +\t\tunsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());\n>> +\t\tunsigned char *src_c = src + sourceSize_.height * sourceSize_.width;\n>> +\t\tunsigned int stride = sourceSize_.width;\n>> +\t\tunsigned char *src_y, *src_cb, *src_cr;\n>> +\n>> +\t\tunsigned int bpp = 3;\n>> +\t\tsize_t dstSize = targetSize_.height * targetSize_.width * bpp;\n>> +\t\tunsigned char *dst = static_cast<unsigned char *>(malloc(dstSize));\n> Hrm ... so the caller would have to know to free the span it receives.\n> That's not nice as you normally expect a Span to by just a pointer into\n> some space, that you don't have control over.\n>\n> I think the caller should probably allocate and provide the destination\n> memory.\nah indeed, this now seems a leak.\n>\n>\n>> +\t\tunsigned char *destination = dst;\n>> +\t\tint r, g, b;\n>> +\n>> +\t\tfor (unsigned int y = 0; y < targetSize_.height; y++) {\n>> +\t\t\tint32_t sourceY = lround(sourceSize_.height * y / targetSize_.height);\n>> +\n>> +\t\t\tfor (unsigned int x = 0; x < targetSize_.width; x++) {\n>> +\t\t\t\tint32_t sourceX = lround(sourceSize_.width * x / targetSize_.width);\n>> +\n>> +\t\t\t\tsrc_y = src + stride * sourceY + sourceX;\n>> +\t\t\t\tsrc_cb = src_c + (sourceY / 2) * stride + (sourceX / 2) * 2 + cb_pos;\n>> +\t\t\t\tsrc_cr = src_c + (sourceY / 2) * stride + (sourceX / 2) * 2 + cr_pos;\n>> +\n>> +\t\t\t\tyuv_to_rgb(*src_y, *src_cb, *src_cr, &r, &g, &b);\n>> +\n>> +\t\t\t\tdestination[x * bpp + 0] = r;\n>> +\t\t\t\tdestination[x * bpp + 1] = g;\n>> +\t\t\t\tdestination[x * bpp + 2] = b;\n>> +\t\t\t}\n>> +\n>> +\t\t\tdestination = destination + bpp * targetSize_.width;\n>> +\t\t}\n>> +\n>> +\t\t/* Helper code: Write the output pixels to a file so we can inspect */\n>> +\t\tFile file(\"/tmp/\" + datetime() + \".raw\");\n>> +\t\tint32_t ret = file.open(File::WriteOnly);\n>> +\t\tret = file.write({ dst, dstSize });\n>> +\t\tLOG(SCALER, Info) << \"Wrote \" << ret << \" bytes: \" << targetSize_.width << \"x\" << targetSize_.height;\n>> +\n> We should keep debug to a separate patch.\n>\n> It can be provided in the series with a DNI prefix if it's helpful\n> during review/development - but try to keep the patch targeted for\n> integration clean.\n>\n>> +\t\t/* Write scaled pixels to dest */\n>> +\t\tdest = { dst, dstSize };\n> If the caller provides the memory, we shouldn't need to do this, as we\n> should be dealing with fixed size buffers, so the dest should be already\n> correctly sized.\n>\n>\n>\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> diff --git a/src/android/jpeg/scaler.h b/src/android/jpeg/scaler.h\n>> new file mode 100644\n>> index 0000000..c68a279\n>> --- /dev/null\n>> +++ b/src/android/jpeg/scaler.h\n>> @@ -0,0 +1,32 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * Basic image scaler from NV12 to RGB24 format\n> That's a scaler and pixelformat convertor ;-)\nYes, I can imagine now. My preference is to rename this Thumbnailer\nOR\nremove the convertor part and simply scaling down NV12 stream. The EXIF \nspec (if I inferred correctly) supports uncompressed YCbCr for thumbnail \ndata.\n(Page 24 - Exif Version 2.31 spec)\n>\n>> + */\n>> +#ifndef __ANDROID_JPEG_SCALER_H__\n>> +#define __ANDROID_JPEG_SCALER_H__\n>> +\n>> +#include <libcamera/geometry.h>\n>> +\n>> +#include \"libcamera/internal/buffer.h\"\n>> +#include \"libcamera/internal/formats.h\"\n>> +\n>> +class Scaler\n>> +{\n>> +public:\n>> +\tScaler();\n>> +\t~Scaler();\n>> +\n>> +\tvoid configure(libcamera::Size sourceSize, libcamera::Size targetSize,\n>> +\t\t       const libcamera::PixelFormatInfo *pixelFormatInfo);\n>> +\tint scaleBuffer(const libcamera::FrameBuffer *source, libcamera::Span<uint8_t> &dest);\n>> +\n>> +private:\n>> +\n>> +\tlibcamera::Size sourceSize_;\n>> +\tlibcamera::Size targetSize_;\n>> +\tconst libcamera::PixelFormatInfo *pixelFormatInfo_;\n>> +};\n>> +\n>> +#endif /* __ANDROID_JPEG_SCALER_H__ */\n>> diff --git a/src/android/meson.build b/src/android/meson.build\n>> index 0293c20..aefb0da 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/scaler.cpp',\n>>   ])\n>>   \n>>   android_camera_metadata_sources = files([\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 87AF0C3B5C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 25 Sep 2020 20:37:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7F4C6303C;\n\tFri, 25 Sep 2020 22:37:05 +0200 (CEST)","from mail.uajain.com (static.126.159.217.95.clients.your-server.de\n\t[95.217.159.126])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6CABE62FD8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 25 Sep 2020 22:37:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=uajain.com header.i=@uajain.com\n\theader.b=\"XLShfEHy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1601066222; bh=rN2Xwg/EYPhvhcmM4jrAEzQC4TEA7NWlIKxIG8s2MAQ=;\n\th=Subject:To:References:From:In-Reply-To;\n\tb=XLShfEHy+Q52y5EeXQuSj2mhWALR/sTv/sez+FIiTNPzmpPOuuQ7YPhGGGZvrnN4e\n\t1EEDWrz3oH/nMaY6EF8+Gz/otWFmOBp6nhZnqgRs3cxBnD1QC9xI1ty9Hzhee2Pcsd\n\tIVpWpnzHpq2fI1TuzPs/1/iBQC7U0688OqH4cLx7b4Kq09PqddA5O/FEJ7fJ8G0r2x\n\txpAlA/rAWPxFUs+YqZ0L7OuInK1oUxVlaOFZ5DCdzFMbMHd65MUkgg7BhZlatoJMPl\n\taxiEXCrkxeMKC0clC7WH40vBP41Qm05t03c9gnrtPi7G51QOarGecjYg8K0eo2Ay/h\n\talmlhVZvkIE+g==","To":"kieran.bingham@ideasonboard.com, libcamera-devel@lists.libcamera.org","References":"<20200922085444.15764-1-email@uajain.com>\n\t<20200922085444.15764-2-email@uajain.com>\n\t<67d2a7e4-6e48-8b24-576b-b5b6b2d20432@ideasonboard.com>","From":"Umang Jain <email@uajain.com>","Message-ID":"<4f080f39-8640-82e2-2174-0420335db570@uajain.com>","Date":"Sat, 26 Sep 2020 02:06:59 +0530","Mime-Version":"1.0","In-Reply-To":"<67d2a7e4-6e48-8b24-576b-b5b6b2d20432@ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 1/1] android: jpeg: Add a basic NV12\n\timage scaler","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12855,"web_url":"https://patchwork.libcamera.org/comment/12855/","msgid":"<20200929024659.GS14614@pendragon.ideasonboard.com>","date":"2020-09-29T02:46:59","subject":"Re: [libcamera-devel] [PATCH 1/1] android: jpeg: Add a basic NV12\n\timage scaler","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 Sat, Sep 26, 2020 at 02:06:59AM +0530, Umang Jain wrote:\n> On 9/25/20 8:33 PM, Kieran Bingham wrote:\n> > On 22/09/2020 09:54, Umang Jain wrote:\n> >> Add a basic image scaler for NV12 frames being captured. The primary\n> >> use of this scaler will be to generate a thumbnail image to be embedded\n> >> as a part of EXIF metadata of the frame.\n> >>\n> >> Signed-off-by: Umang Jain <email@uajain.com>\n> >> ---\n> >>   src/android/jpeg/encoder_libjpeg.cpp |  10 ++\n> >>   src/android/jpeg/scaler.cpp          | 137 +++++++++++++++++++++++++++\n> >>   src/android/jpeg/scaler.h            |  32 +++++++\n> >>   src/android/meson.build              |   1 +\n> >>   4 files changed, 180 insertions(+)\n> >>   create mode 100644 src/android/jpeg/scaler.cpp\n> >>   create mode 100644 src/android/jpeg/scaler.h\n> >>\n> >> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> >> index 510613c..9ecf9b1 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 \"scaler.h\"\n> >>   \n> >>   #include <fcntl.h>\n> >>   #include <iomanip>\n> >> @@ -214,6 +215,15 @@ 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> >> +\tScaler scaler;\n> >> +\tlibcamera::Span<uint8_t> thumbnail;\n> >> +\tscaler.configure(Size (compress_.image_width, compress_.image_height),\n> >> +\t\t\t /* \\todo: Check for exact thumbnail size required by EXIF spec. */\n> >> +\t\t         Size (compress_.image_width / 3, compress_.image_height / 3),\n> >\n> > Indeed, I think this should set explicit sizes, not simply reduce to a\n> > third.\n> >\n> > http://www.fifi.org/doc/jhead/exif-e.html#ExifThumbs\n> >\n> > States, that usually the thumbnails are 160x120 for Exif2.1 or later,\n> > and are usually encoded as JPG we can use another JPEG compressor\n> > instance most likely, but if the RGB format works, that's fine too for now.\n>\n> The EXIF standard states that:\n> ```\n> No limit is placed on the size of thumbnail images. It is optional to \n> record thumbnails but it is recommended that they be recorded if \n> possible, unless hardware or other restrictions preclude this.Thumbnail \n> data does not necessarily have to adopt the same data structure as that \n> used for primary images. If, however, the primary images are recorded as \n> uncompressed RGB data or as uncompressed YCbCr data, thumbnail images \n> shall not be recorded as JPEG compressed data.\n> \n> When thumbnails are recorded in uncompressed format, they are to be \n> recorded in the 1st IFD in conformance with Baseline TIFF Rev. 6.0 RGB \n> Full Color Images or TIFF Rev. 6.0 Extensions YCbCr Images.\n> ```\n> (Page 24 - Exif Version 2.31 spec)\n> \n> So it seems we can write uncompressed data for thumbnail in RGB888, \n> right? It also states YCbCr  too (for uncompressed format) , so maybe we \n> want that? That would take out the 'converter' part of this patch.\n> I am not really sure which format to use, so I will move forward as per \n> the advice given in the reviews...\n\nBoth should be acceptable. If I had to guess I'd say that RGB would be\nbetter supported, but that's just a guess. However, converting to RGB\nmeans we have to deal with colorspace issues. And if we later encode\nthe thumbnail to JPEG, we'd need YUV anyway. I'm thus tempted to drop\nthe RGB conversion from the scaler.\n\n> > I'd hard code the size to 160 x 120 all the same.\n>\n> Yeah, that would come along in a proper patch.\n\nScaling down by 3 and storing the result in uncompressed form would\nconsume 4MB in RGB. That's a lot for a thumbnail :-) 160x120 is\n57.6 kiB, which is better, but maybe still a good candidate for JPEG\ncompression (don't forget to set the Compression tag in IFD1).\n\nRegarding the size, please make sure to preserve the original aspect\nratio (this is required by section 4.4.2 of Exif v2.32). This means that\n160x120 should be either the minimum or maximum size (e.g. a 16:9 image\nwould be scaled down to either 160x90 or 213x120 - the latter isn't nice\nwith rounding errors, so it could also be rounded to 208x117 or\n224x126). I don't have a strong preference on whether 160x120 should be\nthe minimum or maximum size.\n\n> >> +\t\t\t pixelFormatInfo_);\n> >> +\tscaler.scaleBuffer(source, thumbnail);\n> >> +\t/* \\todo: Write thumbnail as part of exifData. */\n> >> +\n> >>   \tif (nv_)\n> >>   \t\tcompressNV(&frame);\n> >>   \telse\n> >> diff --git a/src/android/jpeg/scaler.cpp b/src/android/jpeg/scaler.cpp\n> >> new file mode 100644\n> >> index 0000000..ff36ece\n> >> --- /dev/null\n> >> +++ b/src/android/jpeg/scaler.cpp\n> >> @@ -0,0 +1,137 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * Copyright (C) 2020, Google Inc.\n> >> + *\n> >> + * scaler.cpp - Basic image scaler from NV12 to RGB24 format\n> >> + */\n> >> +\n> >> +#include \"scaler.h\"\n> >> +\n> >> +#include <math.h>\n> >> +#include <string.h>\n> >> +\n> >> +#include \"libcamera/internal/log.h\"\n> >> +#include \"libcamera/internal/file.h\"\n\nDoesn't checkstyle.py tell you to keep these two headers alphabetically\nsorted ?\n\n> >> +\n> >> +#define RGBSHIFT                8\n> >> +#ifndef MAX\n> >> +#define MAX(a,b)                ((a)>(b)?(a):(b))\n> >> +#endif\n> >> +#ifndef MIN\n> >> +#define MIN(a,b)                ((a)<(b)?(a):(b))\n> >> +#endif\n> >\n> > #include <algorithm>\n> > will give you std::min, std::max, and std::clamp()\n> >\n> >> +#ifndef CLAMP\n> >> +#define CLAMP(a,low,high)       MAX((low),MIN((high),(a)))\n> >> +#endif\n> >\n> >> +#ifndef CLIP\n> >> +#define CLIP(x)                 CLAMP(x,0,255)\n> >> +#endif\n> >\n> > I think this one is distinct though, Depends on how it's utilised to see\n> > whether it's worth just using std::clamp directly.\n> >\n> >> +\n> >> +using namespace libcamera;\n> >> +\n> >> +LOG_DEFINE_CATEGORY(SCALER)\n\ns/SCALER/Scaler/\n\n> >> +\n> >> +Scaler::Scaler()\n> >> +\t: sourceSize_(0, 0), targetSize_(0,0)\n\nNo need for this, the default constructor for Size does the same. You\ncan thus drop the explicit constructor for the Scaler class.\n\n> >> +{\n> >> +}\n> >> +\n> >> +Scaler::~Scaler()\n\nSame here, you can drop the explicit destructor.\n\n> >> +{\n> >> +}\n> >> +\n> >> +void Scaler::configure(Size sourceSize, Size targetSize, const PixelFormatInfo *pixelFormatInfo)\n\nYou could pass sizes as const references.\n\nIf we decide to turn this class into a thumbnail generator, I would also\ncompute the target size internally.\n\n> >> +{\n> >> +\tsourceSize_ = sourceSize;\n> >> +\ttargetSize_ = targetSize;\n> >> +\tpixelFormatInfo_ = pixelFormatInfo;\n> >> +}\n> >> +\n> >> +static std::string datetime()\n> >> +{\n> >> +\ttime_t rawtime;\n> >> +\tstruct tm *timeinfo;\n> >> +\tchar buffer[80];\n> >> +\tstatic unsigned int milliseconds = 0;\n> >> +\n> >> +\ttime(&rawtime);\n> >> +\ttimeinfo = localtime(&rawtime);\n> >> +\n> >> +\tstrftime(buffer, 80, \"%d-%m-%Y.%H-%M-%S.\", timeinfo);\n> >> +\n> >> +\t/* milliseconds is just a fast hack to ensure unique filenames */\n> >> +\treturn std::string(buffer) + std::to_string(milliseconds++);\n> >> +}\n> >\n> > I think this function is here for debug, but shouldn't be in the scaler.\n> >\n> >> +\n> >> +/* Handpicked from src/qcam/format_converter.cpp */\n> >> +static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)\n> >> +{\n> >> +\tint c = y - 16;\n> >> +\tint d = u - 128;\n> >> +\tint e = v - 128;\n> >> +\t*r = CLIP(( 298 * c           + 409 * e + 128) >> RGBSHIFT);\n> >> +\t*g = CLIP(( 298 * c - 100 * d - 208 * e + 128) >> RGBSHIFT);\n> >> +\t*b = CLIP(( 298 * c + 516 * d           + 128) >> RGBSHIFT);\n> >> +}\n> >\n> > CLIP does look better than std::clamp there.\n> >\n> > As RGBSHIFT is only used here, maybe put it as a constexpr in this function?\n> >\n> > But I wonder if we should really be making the scaler do pixel-format\n> > conversions.\n> >\n> > In the case of the Thumbnail generations it's a nice optimisztion, but\n> > it doesn't feel right, and leads to the object being an anything to\n> > anything scaler/convertor.\n> >\n> > As that is possible in some hardware (scale and pixel conversion\n> > combined) I'm almost tempted to say it's ok ... but I'm just not sure.\n>\n> If it seems OK and *if* we do end up keeping this scaler + converter \n> combo, I would rename the class as Thumbnailer, instead of Scaler.\n>\n> > The alternative would be to take the incoming image, rescale it to the\n> > thumbnail, and then run that through another JPEG compressor anyway,\n> > which already supports NV12.\n>\n> That sounds an overkill maybe? Not sure... The EXIF spec supports \n> uncompressed YCbCr as thumbnail data, so ... we can just scale down our \n> NV12.\n> \n> One question here to tinker is, what's in-general use-case of the \n> thumbnail data? Image previewing? in something like file-manager(s) / \n> apps. I would love to learn if  they decode this data and then display \n> it (like 'raw2rgbgnm' equivalent) maybe? Or they do something completely \n> different\n\nThe main use case is preview in gallery or file manager applications,\nyes.\n\n> >> +\n> >> +int Scaler::scaleBuffer(const FrameBuffer *source, Span<uint8_t> &dest)\n> >> +{\n> >> +\tMappedFrameBuffer frame(source, PROT_READ);\n> >> +\tif (!frame.isValid()) {\n> >> +\t\tLOG(SCALER, Error) << \"Failed to map FrameBuffer : \"\n> >> +\t\t\t\t   << strerror(frame.error());\n> >> +\t\treturn frame.error();\n> >> +\t}\n> >> +\n> >> +\tif (strcmp(pixelFormatInfo_->name, \"NV12\") != 0) {\n> >\n> > Can you compare against formats::NV12 directly?\n> >\n> >   \tif (pixelFormatInfo_->format != formats::NV12) {\n> >\n> >> +\t\tLOG (SCALER, Info) << \"Source Buffer not in NV12 format, returning...\";\n> >> +\t\treturn -1;\n> >\n> > This should be in configure() though, not on every call to scaleBuffer().\n>\n> ah yeah, Make sense.\n>\n> >> +\t}\n> >> +\n> >> +\t/* Image scaling block implementing nearest-neighbour algorithm. */\n> >> +\t{\n> >\n> > I'd remove the scoping here and indent left - or if you want to keep it\n> > distinct, put it into a function called\n> >\n> >    scaleNearestNeighbour(MappedFrameBuffer source*, Span<uint8_t> &dest)\n> >\n> > That would leave scale() only doing the mapping ... but make the code\n> > paths clear if another scaler implementation was added later...?\n> >\n> >> +\t\tunsigned int cb_pos = 0;\n> >> +\t\tunsigned int cr_pos = 1;\n> >> +\t\tunsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());\n\nThink should be const.\n\n> >> +\t\tunsigned char *src_c = src + sourceSize_.height * sourceSize_.width;\n\nThis too.\n\n> >> +\t\tunsigned int stride = sourceSize_.width;\n> >> +\t\tunsigned char *src_y, *src_cb, *src_cr;\n\nAnd this too.\n\n> >> +\n> >> +\t\tunsigned int bpp = 3;\n\nconstexpr\n\n> >> +\t\tsize_t dstSize = targetSize_.height * targetSize_.width * bpp;\n> >> +\t\tunsigned char *dst = static_cast<unsigned char *>(malloc(dstSize));\n> >\n> > Hrm ... so the caller would have to know to free the span it receives.\n> > That's not nice as you normally expect a Span to by just a pointer into\n> > some space, that you don't have control over.\n> >\n> > I think the caller should probably allocate and provide the destination\n> > memory.\n\nThat, or we should use appropriate containers. The caller could pass a\npointer to a std::vector<> that would be resized appropriately by this\nfunction, or the function could return a std::vector<>. I think it makes\nsense to calculate the destination size in this class, so I wouldn't\nallocate memory in the caller.\n\n> ah indeed, this now seems a leak.\n>\n> >> +\t\tunsigned char *destination = dst;\n> >> +\t\tint r, g, b;\n> >> +\n> >> +\t\tfor (unsigned int y = 0; y < targetSize_.height; y++) {\n> >> +\t\t\tint32_t sourceY = lround(sourceSize_.height * y / targetSize_.height);\n\nsourceY can never be negative, you can make it an unsigned int.\n\nSize.height and h are all integers, so the result of the division will\nbe an integer, and lround() won't do much. What you want here is\n\n\t\tunsigned int sourceY = (sourceSize_.height * y + targetSize_.height / 2)\n\t\t\t\t     / targetSize_.height;\n\n> >> +\n> >> +\t\t\tfor (unsigned int x = 0; x < targetSize_.width; x++) {\n> >> +\t\t\t\tint32_t sourceX = lround(sourceSize_.width * x / targetSize_.width);\n\nSame here.\n\n> >> +\n> >> +\t\t\t\tsrc_y = src + stride * sourceY + sourceX;\n> >> +\t\t\t\tsrc_cb = src_c + (sourceY / 2) * stride + (sourceX / 2) * 2 + cb_pos;\n> >> +\t\t\t\tsrc_cr = src_c + (sourceY / 2) * stride + (sourceX / 2) * 2 + cr_pos;\n\nInstead of doing this computation for each pixel, you could optimize by\ncalculating the line address before the X loop (src + stride * sourceY,\nsrc_c + sourceY / 2 * stride + cb_pos, and similarly for src_cr) and\njust indexing here\n\n\t\t\t\tyuv_to_rgb(src_y[sourceX], src_cb[sourceX / 2 * 2],\n\t\t\t\t\t   src_cr[sourceX / 2 * 2], &r, &g, &b);\n\nAnd additional optimization would just be to set src_y = src before the\ntwo loops, and adding src_y += stride after the X loop (same for src_cb\nand src_cr).\n\n> >> +\n> >> +\t\t\t\tyuv_to_rgb(*src_y, *src_cb, *src_cr, &r, &g, &b);\n> >> +\n> >> +\t\t\t\tdestination[x * bpp + 0] = r;\n> >> +\t\t\t\tdestination[x * bpp + 1] = g;\n> >> +\t\t\t\tdestination[x * bpp + 2] = b;\n> >> +\t\t\t}\n> >> +\n> >> +\t\t\tdestination = destination + bpp * targetSize_.width;\n> >> +\t\t}\n> >> +\n> >> +\t\t/* Helper code: Write the output pixels to a file so we can inspect */\n> >> +\t\tFile file(\"/tmp/\" + datetime() + \".raw\");\n> >> +\t\tint32_t ret = file.open(File::WriteOnly);\n> >> +\t\tret = file.write({ dst, dstSize });\n> >> +\t\tLOG(SCALER, Info) << \"Wrote \" << ret << \" bytes: \" << targetSize_.width << \"x\" << targetSize_.height;\n> >> +\n> >\n> > We should keep debug to a separate patch.\n> >\n> > It can be provided in the series with a DNI prefix if it's helpful\n> > during review/development - but try to keep the patch targeted for\n> > integration clean.\n> >\n> >> +\t\t/* Write scaled pixels to dest */\n> >> +\t\tdest = { dst, dstSize };\n> >\n> > If the caller provides the memory, we shouldn't need to do this, as we\n> > should be dealing with fixed size buffers, so the dest should be already\n> > correctly sized.\n> >\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> diff --git a/src/android/jpeg/scaler.h b/src/android/jpeg/scaler.h\n> >> new file mode 100644\n> >> index 0000000..c68a279\n> >> --- /dev/null\n> >> +++ b/src/android/jpeg/scaler.h\n> >> @@ -0,0 +1,32 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * Copyright (C) 2020, Google Inc.\n> >> + *\n> >> + * Basic image scaler from NV12 to RGB24 format\n> >\n> > That's a scaler and pixelformat convertor ;-)\n>\n> Yes, I can imagine now. My preference is to rename this Thumbnailer\n> OR\n> remove the convertor part and simply scaling down NV12 stream. The EXIF \n> spec (if I inferred correctly) supports uncompressed YCbCr for thumbnail \n> data.\n> (Page 24 - Exif Version 2.31 spec)\n\nI agree with you. A Thumbnailer could compress to JPEG as well, while a\nScaler would output NV12. There's a use case for splitting the scaler to\na separate component in order to reuse it, but that can be done later on\ntop if/when needed. I think I'd rather go for Thumbnailer for now (also\nbecause a nearest-neighbour scaler, while probably fine for thumbnails,\nwill not be good enough to scale streams for other use cases).\n\n> >> + */\n> >> +#ifndef __ANDROID_JPEG_SCALER_H__\n> >> +#define __ANDROID_JPEG_SCALER_H__\n> >> +\n> >> +#include <libcamera/geometry.h>\n> >> +\n> >> +#include \"libcamera/internal/buffer.h\"\n> >> +#include \"libcamera/internal/formats.h\"\n> >> +\n> >> +class Scaler\n> >> +{\n> >> +public:\n> >> +\tScaler();\n> >> +\t~Scaler();\n> >> +\n> >> +\tvoid configure(libcamera::Size sourceSize, libcamera::Size targetSize,\n> >> +\t\t       const libcamera::PixelFormatInfo *pixelFormatInfo);\n> >> +\tint scaleBuffer(const libcamera::FrameBuffer *source, libcamera::Span<uint8_t> &dest);\n> >> +\n> >> +private:\n> >> +\n> >> +\tlibcamera::Size sourceSize_;\n> >> +\tlibcamera::Size targetSize_;\n> >> +\tconst libcamera::PixelFormatInfo *pixelFormatInfo_;\n> >> +};\n> >> +\n> >> +#endif /* __ANDROID_JPEG_SCALER_H__ */\n> >> diff --git a/src/android/meson.build b/src/android/meson.build\n> >> index 0293c20..aefb0da 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/scaler.cpp',\n> >>   ])\n> >>   \n> >>   android_camera_metadata_sources = files([","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 2AEB9C3B5C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 29 Sep 2020 02:47:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8AE7462140;\n\tTue, 29 Sep 2020 04:47:36 +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 5DAD360394\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 29 Sep 2020 04:47:35 +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 B1F033B;\n\tTue, 29 Sep 2020 04:47:34 +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=\"dZybkK/6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601347654;\n\tbh=ifNSwDqkreo952lLyvpglBw8J7FLDZXTwGthO9q5/zw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dZybkK/6urKGt4drUbP3bJkq8ZWjvqN3Ckgg4My/o/PxaSOYMVHMFkj/4ntaPIyk5\n\t4SDXy255WEFlp0/WywUkp9zCIx73/+0yWRp+J7LU1kP15EVat3xKQn/cTQ6H7y5aJp\n\th69yupDgDaGmsRbm4lBIvr3Y9KFIlSCBhbtbvaOc=","Date":"Tue, 29 Sep 2020 05:46:59 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Umang Jain <email@uajain.com>","Message-ID":"<20200929024659.GS14614@pendragon.ideasonboard.com>","References":"<20200922085444.15764-1-email@uajain.com>\n\t<20200922085444.15764-2-email@uajain.com>\n\t<67d2a7e4-6e48-8b24-576b-b5b6b2d20432@ideasonboard.com>\n\t<4f080f39-8640-82e2-2174-0420335db570@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<4f080f39-8640-82e2-2174-0420335db570@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH 1/1] android: jpeg: Add a basic NV12\n\timage scaler","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]