[{"id":13490,"web_url":"https://patchwork.libcamera.org/comment/13490/","msgid":"<20201026210533.GC3756@pendragon.ideasonboard.com>","date":"2020-10-26T21:05:33","subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","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 Mon, Oct 26, 2020 at 07:31:34PM +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/exif.cpp | 16 +++-\n> src/android/jpeg/exif.h | 1 +\n> src/android/jpeg/post_processor_jpeg.cpp | 35 +++++++-\n> src/android/jpeg/post_processor_jpeg.h | 8 +-\n> src/android/jpeg/thumbnailer.cpp | 109 +++++++++++++++++++++++\n> src/android/jpeg/thumbnailer.h | 37 ++++++++\n> src/android/meson.build | 1 +\n> 7 files changed, 204 insertions(+), 3 deletions(-)\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/exif.cpp b/src/android/jpeg/exif.cpp\n> index d21534a..24197bd 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -75,8 +75,16 @@ Exif::~Exif()\n> \tif (exifData_)\n> \t\tfree(exifData_);\n> \n> -\tif (data_)\n> +\tif (data_) {\n> +\t\t/*\n> +\t\t * Reset thumbnail data to avoid getting double-freed by\n> +\t\t * libexif. It is owned by the caller (i.e. PostProcessorJpeg).\n> +\t\t */\n> +\t\tdata_->data = nullptr;\n> +\t\tdata_->size = 0;\n> +\n> \t\texif_data_unref(data_);\n> +\t}\n> \n> \tif (mem_)\n> \t\texif_mem_unref(mem_);\n> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)\n> \tsetShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\n> }\n> \n> +void Exif::setThumbnail(std::vector &thumbnail)\n> +{\n> +\tdata_->data = thumbnail.data();\n> +\tdata_->size = thumbnail.size();\n> +}\n> +\n> [[nodiscard]] int Exif::generate()\n> {\n> \tif (exifData_) {\n> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> index 12c27b6..bd54a31 100644\n> --- a/src/android/jpeg/exif.h\n> +++ b/src/android/jpeg/exif.h\n> @@ -26,6 +26,7 @@ public:\n> \n> \tvoid setOrientation(int orientation);\n> \tvoid setSize(const libcamera::Size &size);\n> +\tvoid setThumbnail(std::vector &thumbnail);\n\nYou can pass a Span as the setThumbnail() function\nshouldn't care what storage container is used.\n\nIt's a bit of a dangerous API, as it will store the pointer internally,\nwithout ensuring that the caller keeps the thumbnail valid after the\ncall returns. It's fine, but maybe a comment above the\nExif::setThumbnail() functions to state that the thumbnail must remain\nvalid until the Exif object is destroyed would be a good thing.\n\n> \tvoid setTimestamp(time_t timestamp);\n> \n> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n\nI would have moved this to a separate patch.\n\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index c56f1b2..416e831 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -9,7 +9,6 @@\n> \n> #include \"../camera_device.h\"\n> #include \"../camera_metadata.h\"\n> -#include \"encoder_libjpeg.h\"\n> #include \"exif.h\"\n> \n> #include \n> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> \t}\n> \n> \tstreamSize_ = outCfg.size;\n> +\n> +\tthumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> +\tStreamConfiguration thCfg = inCfg;\n> +\tthCfg.size = thumbnailer_.size();\n> +\tif (thumbnailEncoder_.configure(thCfg) != 0) {\n> +\t\tLOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> +\t\treturn -EINVAL;\n> +\t}\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 std::vector &thumbnail)\n> +{\n> +\t/* Stores the raw scaled-down thumbnail bytes. */\n> +\tstd::vector rawThumbnail;\n> +\n> +\tthumbnailer_.scaleBuffer(source, rawThumbnail);\n> +\n> +\tif (rawThumbnail.data()) {\n\nThis should check for ! .empty() (see additional comments below).\n\n> +\t\tthumbnail.reserve(rawThumbnail.capacity());\n\nYou should call thumbnail.resize(), and use size() instead of capacity()\nbelow, as reserve() allocates memory but doesn't change the size of the\nvector, so it's semantically dangerous to write to the reserved storage\nspace if it hasn't been marked as in use with .resize().\n\n> +\n> +\t\tint jpeg_size = thumbnailEncoder_.encode(\n> +\t\t\t\t{ rawThumbnail.data(), rawThumbnail.capacity() },\n> +\t\t\t\t{ thumbnail.data(), thumbnail.capacity() },\n\nJust pass rawThumbnail and thumbnail, there's an implicit constructor\nfor Span that will turn them into a span using .data() and .size().\n\n> +\t\t\t\t{ });\n> +\t\tthumbnail.resize(jpeg_size);\n> +\n> +\t\tLOG(JPEG, Info) << \"Thumbnail compress returned \"\n> +\t\t\t\t<< jpeg_size << \" bytes\";\n\nWhen log messages don't fit on one line, we usually wrap them as\n\n\t\tLOG(JPEG, Info)\n\t\t\t<< \"Thumbnail compress returned \"\n\t\t\t<< jpeg_size << \" bytes\";\n\nAnd maybe debug instead of info ?\n\n> +\t}\n> +}\n> +\n> int PostProcessorJpeg::process(const FrameBuffer &source,\n> \t\t\t Span destination,\n> \t\t\t CameraMetadata *metadata)\n> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> \t * second, it is good enough.\n> \t */\n> \texif.setTimestamp(std::time(nullptr));\n> +\tstd::vector thumbnail;\n> +\tgenerateThumbnail(source, thumbnail);\n> +\texif.setThumbnail(thumbnail);\n> \tif (exif.generate() != 0)\n> \t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> \n> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> index 3706cec..3894231 100644\n> --- a/src/android/jpeg/post_processor_jpeg.h\n> +++ b/src/android/jpeg/post_processor_jpeg.h\n> @@ -8,12 +8,13 @@\n> #define __ANDROID_POST_PROCESSOR_JPEG_H__\n> \n> #include \"../post_processor.h\"\n> +#include \"encoder_libjpeg.h\"\n> +#include \"thumbnailer.h\"\n> \n> #include \n> \n> #include \"libcamera/internal/buffer.h\"\n> \n> -class Encoder;\n> class CameraDevice;\n> \n> class PostProcessorJpeg : public PostProcessor\n> @@ -28,9 +29,14 @@ public:\n> \t\t CameraMetadata *metadata) override;\n> \n> private:\n> +\tvoid generateThumbnail(const libcamera::FrameBuffer &source,\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\nCould you store this in a std::unique_ptr instead ? The reason\nis that we shouldn't hardcode usage of EncoderLibJpeg, in order to\nsupport different encoders later. You won't need to include\nencoder_libjpeg.h then, and can keep the Encoder forwared declaration.\n\n> +\tThumbnailer thumbnailer_;\n\nThis is good, as I don't expect different thumbnailer types.\n\n> };\n> \n> #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> new file mode 100644\n> index 0000000..f880ffb\n> --- /dev/null\n> +++ b/src/android/jpeg/thumbnailer.cpp\n> @@ -0,0 +1,109 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n\nWrong license.\n\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * thumbnailer.cpp - Simple image thumbnailer\n> + */\n> +\n> +#include \"thumbnailer.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/file.h\"\n\nWhy do you need this header ?\n\n> +#include \"libcamera/internal/log.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(Thumbnailer)\n> +\n> +Thumbnailer::Thumbnailer() : valid_(false)\n\n\t: valid_(false)\n\n> +{\n> +}\n> +\n> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)\n> +{\n> +\tsourceSize_ = sourceSize;\n> +\tpixelFormat_ = pixelFormat;\n> +\n> +\tif (pixelFormat_ != formats::NV12) {\n> +\t\tLOG (Thumbnailer, Error) << \"Failed to configure: Pixel Format \"\n> +\t\t\t\t << pixelFormat_.toString() << \" unsupported.\";\n\n\t\tLOG (Thumbnailer, Error)\n\t\t\t<< \"Failed to configure: Pixel Format \"\n\t\t\t<< pixelFormat_.toString() << \" unsupported.\";\n\n> +\t\treturn;\n> +\t}\n> +\n> +\ttargetSize_ = computeThumbnailSize();\n> +\n> +\tvalid_ = true;\n> +}\n> +\n> +/*\n> + * The Exif specification recommends the width of the thumbnail to be a\n> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height\n\ns/mutiple/multiple/\n\n> + * keeping the aspect ratio same as of the source.\n> + */\n> +Size Thumbnailer::computeThumbnailSize()\n> +{\n> +\tunsigned int targetHeight;\n> +\tunsigned int targetWidth = 160;\n> +\n> +\ttargetHeight = targetWidth * sourceSize_.height / sourceSize_.width;\n> +\n> +\tif (targetHeight & 1)\n> +\t\ttargetHeight++;\n> +\n> +\treturn Size(targetWidth, targetHeight);\n> +}\n> +\n> +void\n> +Thumbnailer::scaleBuffer(const FrameBuffer &source,\n> +\t\t\t std::vector &destination)\n> +{\n> +\tMappedFrameBuffer frame(&source, PROT_READ);\n> +\tif (!frame.isValid()) {\n> +\t\tLOG(Thumbnailer, Error) << \"Failed to map FrameBuffer : \"\n> +\t\t\t\t << strerror(frame.error());\n\n\t\tLOG(Thumbnailer, Error)\n\t\t\t<< \"Failed to map FrameBuffer : \"\n\t\t\t<< strerror(frame.error());\n\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (!valid_) {\n> +\t\tLOG(Thumbnailer, Error) << \"config is unconfigured or invalid.\";\n> +\t\treturn;\n> +\t}\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 char *src_cb, *src_cr;\n> +\tunsigned char *dst_y, *src_y;\n> +\n> +\tsize_t dstSize = (th * tw) + ((th / 2) * tw);\n> +\tdestination.reserve(dstSize);\n\nThis should be\n\n\tdestination.resize(dstSize);\n\nas reserve() allocates memory but doesn't change the size of the vector.\n\n> +\tunsigned char *dst = destination.data();\n> +\tunsigned char *dst_c = dst + th * tw;\n> +\n> +\tfor (unsigned int y = 0; y < th; y += 2) {\n> +\t\tunsigned int sourceY = (sh * y + th / 2) / th;\n> +\n> +\t\tdst_y = dst + y * tw;\n> +\t\tsrc_y = src + sw * sourceY;\n> +\t\tsrc_cb = src_c + (sourceY / 2) * sw + 0;\n> +\t\tsrc_cr = src_c + (sourceY / 2) * sw + 1;\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[x] = src_y[sourceX];\n> +\t\t\tdst_y[tw + x] = src_y[sw + sourceX];\n> +\t\t\tdst_y[x + 1] = src_y[sourceX + 1];\n> +\t\t\tdst_y[tw + x + 1] = src_y[sw + sourceX + 1];\n> +\n> +\t\t\tdst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];\n> +\t\t\tdst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];\n> +\t\t}\n> +\t}\n> + }\n\nExtra space.\n\n> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> new file mode 100644\n> index 0000000..b769619\n> --- /dev/null\n> +++ b/src/android/jpeg/thumbnailer.h\n> @@ -0,0 +1,37 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n\nWrong license.\n\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * thumbnailer.h - Simple image thumbnailer\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> +\tvoid scaleBuffer(const libcamera::FrameBuffer &source,\n> +\t\t\t std::vector &dest);\n\nHow about naming this createThumbnail() or generateThumbnail() ? And the\nfunction should be const.\n\n> +\tlibcamera::Size size() const { return targetSize_; }\n> +\n> +private:\n> +\tlibcamera::Size computeThumbnailSize();\n\nThis can be\n\n\tlibcamera::Size computeThumbnailSize() const;\n\n> +\n> +\tlibcamera::PixelFormat pixelFormat_;\n> +\tlibcamera::Size sourceSize_;\n> +\tlibcamera::Size targetSize_;\n> +\n> +\tbool valid_;\n> +\n\nNo need for a blank line.\n\n> +};\n> +\n> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index f72376a..3d4d3be 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -25,6 +25,7 @@ android_hal_sources = files([\n> 'jpeg/encoder_libjpeg.cpp',\n> 'jpeg/exif.cpp',\n> 'jpeg/post_processor_jpeg.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 5BD52BDB1E\n\tfor ;\n\tMon, 26 Oct 2020 21:06:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E04B262062;\n\tMon, 26 Oct 2020 22:06:21 +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 5BBB160350\n\tfor ;\n\tMon, 26 Oct 2020 22:06:20 +0100 (CET)","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 D2A9F527;\n\tMon, 26 Oct 2020 22:06:19 +0100 (CET)"],"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=\"Wc809A8/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603746380;\n\tbh=k3BYJ3I93eivNG01AMrIdD7EI/EMykmowzxdBkqYUw8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Wc809A8/TPE13/aJFwVYEYcSc9jO5XzTMRrwB3ZdC2dLyaYi7KPE8iCZbrWf4S+g+\n\t4qPsV3yIdVe9n7l8lcYio5djgEQ41gUyH44IxZ64pO8ss//bFRECrPzjYaEV7giYxK\n\tLoVhOpm0ip3uT7Q55eBtfnVSa8jbrSBMZg8Zq5oA=","Date":"Mon, 26 Oct 2020 23:05:33 +0200","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20201026210533.GC3756@pendragon.ideasonboard.com>","References":"<20201026140134.44166-1-email@uajain.com>\n\t<20201026140134.44166-4-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201026140134.44166-4-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","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":13492,"web_url":"https://patchwork.libcamera.org/comment/13492/","msgid":"","date":"2020-10-26T22:02:43","subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent, Umang,\n\nOn 26/10/2020 21:05, Laurent Pinchart wrote:\n> Hi Umang,\n> \n> Thank you for the patch.\n> \n> On Mon, Oct 26, 2020 at 07:31:34PM +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/exif.cpp | 16 +++-\n>> src/android/jpeg/exif.h | 1 +\n>> src/android/jpeg/post_processor_jpeg.cpp | 35 +++++++-\n>> src/android/jpeg/post_processor_jpeg.h | 8 +-\n>> src/android/jpeg/thumbnailer.cpp | 109 +++++++++++++++++++++++\n>> src/android/jpeg/thumbnailer.h | 37 ++++++++\n>> src/android/meson.build | 1 +\n>> 7 files changed, 204 insertions(+), 3 deletions(-)\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/exif.cpp b/src/android/jpeg/exif.cpp\n>> index d21534a..24197bd 100644\n>> --- a/src/android/jpeg/exif.cpp\n>> +++ b/src/android/jpeg/exif.cpp\n>> @@ -75,8 +75,16 @@ Exif::~Exif()\n>> \tif (exifData_)\n>> \t\tfree(exifData_);\n>> \n>> -\tif (data_)\n>> +\tif (data_) {\n>> +\t\t/*\n>> +\t\t * Reset thumbnail data to avoid getting double-freed by\n>> +\t\t * libexif. It is owned by the caller (i.e. PostProcessorJpeg).\n>> +\t\t */\n>> +\t\tdata_->data = nullptr;\n>> +\t\tdata_->size = 0;\n>> +\n>> \t\texif_data_unref(data_);\n>> +\t}\n>> \n>> \tif (mem_)\n>> \t\texif_mem_unref(mem_);\n>> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)\n>> \tsetShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\n>> }\n>> \n>> +void Exif::setThumbnail(std::vector &thumbnail)\n>> +{\n>> +\tdata_->data = thumbnail.data();\n>> +\tdata_->size = thumbnail.size();\n>> +}\n>> +\n>> [[nodiscard]] int Exif::generate()\n>> {\n>> \tif (exifData_) {\n>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n>> index 12c27b6..bd54a31 100644\n>> --- a/src/android/jpeg/exif.h\n>> +++ b/src/android/jpeg/exif.h\n>> @@ -26,6 +26,7 @@ public:\n>> \n>> \tvoid setOrientation(int orientation);\n>> \tvoid setSize(const libcamera::Size &size);\n>> +\tvoid setThumbnail(std::vector &thumbnail);\n> \n> You can pass a Span as the setThumbnail() function\n> shouldn't care what storage container is used.\n> \n> It's a bit of a dangerous API, as it will store the pointer internally,\n> without ensuring that the caller keeps the thumbnail valid after the\n> call returns. It's fine, but maybe a comment above the\n> Exif::setThumbnail() functions to state that the thumbnail must remain\n> valid until the Exif object is destroyed would be a good thing.\n\nI think the comment will help indeed. The only alternative would be to\npass it into generate(), but and refactor generate() to return the span\n... but that's more effort that we need right now. So just a comment\nwill do ;-)\n\n\n\n>> \tvoid setTimestamp(time_t timestamp);\n>> \n>> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> \n> I would have moved this to a separate patch.\n\n\nYes, I'd keep the exif extension for setting the thumbnail, and the\nusage separate.\n\n> \n>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n>> index c56f1b2..416e831 100644\n>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>> @@ -9,7 +9,6 @@\n>> \n>> #include \"../camera_device.h\"\n>> #include \"../camera_metadata.h\"\n>> -#include \"encoder_libjpeg.h\"\n>> #include \"exif.h\"\n>> \n>> #include \n>> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n>> \t}\n>> \n>> \tstreamSize_ = outCfg.size;\n>> +\n>> +\tthumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n>> +\tStreamConfiguration thCfg = inCfg;\n>> +\tthCfg.size = thumbnailer_.size();\n>> +\tif (thumbnailEncoder_.configure(thCfg) != 0) {\n>> +\t\tLOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n>> +\t\treturn -EINVAL;\n>> +\t}\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 std::vector &thumbnail)\n>> +{\n>> +\t/* Stores the raw scaled-down thumbnail bytes. */\n>> +\tstd::vector rawThumbnail;\n>> +\n>> +\tthumbnailer_.scaleBuffer(source, rawThumbnail);\n>> +\n>> +\tif (rawThumbnail.data()) {\n> \n> This should check for ! .empty() (see additional comments below).\n> \n>> +\t\tthumbnail.reserve(rawThumbnail.capacity());\n> \n> You should call thumbnail.resize(), and use size() instead of capacity()\n> below, as reserve() allocates memory but doesn't change the size of the\n> vector, so it's semantically dangerous to write to the reserved storage\n> space if it hasn't been marked as in use with .resize().\n> \n>> +\n>> +\t\tint jpeg_size = thumbnailEncoder_.encode(\n>> +\t\t\t\t{ rawThumbnail.data(), rawThumbnail.capacity() },\n>> +\t\t\t\t{ thumbnail.data(), thumbnail.capacity() },\n> \n> Just pass rawThumbnail and thumbnail, there's an implicit constructor\n> for Span that will turn them into a span using .data() and .size().\n> \n>> +\t\t\t\t{ });\n>> +\t\tthumbnail.resize(jpeg_size);\n>> +\n>> +\t\tLOG(JPEG, Info) << \"Thumbnail compress returned \"\n>> +\t\t\t\t<< jpeg_size << \" bytes\";\n> \n> When log messages don't fit on one line, we usually wrap them as\n> \n> \t\tLOG(JPEG, Info)\n> \t\t\t<< \"Thumbnail compress returned \"\n> \t\t\t<< jpeg_size << \" bytes\";\n> \n> And maybe debug instead of info ?\n> \n>> +\t}\n>> +}\n>> +\n>> int PostProcessorJpeg::process(const FrameBuffer &source,\n>> \t\t\t Span destination,\n>> \t\t\t CameraMetadata *metadata)\n>> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n>> \t * second, it is good enough.\n>> \t */\n>> \texif.setTimestamp(std::time(nullptr));\n>> +\tstd::vector thumbnail;\n>> +\tgenerateThumbnail(source, thumbnail);\n>> +\texif.setThumbnail(thumbnail);\n>> \tif (exif.generate() != 0)\n>> \t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n>> \n>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n>> index 3706cec..3894231 100644\n>> --- a/src/android/jpeg/post_processor_jpeg.h\n>> +++ b/src/android/jpeg/post_processor_jpeg.h\n>> @@ -8,12 +8,13 @@\n>> #define __ANDROID_POST_PROCESSOR_JPEG_H__\n>> \n>> #include \"../post_processor.h\"\n>> +#include \"encoder_libjpeg.h\"\n>> +#include \"thumbnailer.h\"\n>> \n>> #include \n>> \n>> #include \"libcamera/internal/buffer.h\"\n>> \n>> -class Encoder;\n>> class CameraDevice;\n>> \n>> class PostProcessorJpeg : public PostProcessor\n>> @@ -28,9 +29,14 @@ public:\n>> \t\t CameraMetadata *metadata) override;\n>> \n>> private:\n>> +\tvoid generateThumbnail(const libcamera::FrameBuffer &source,\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> \n> Could you store this in a std::unique_ptr instead ? The reason\n> is that we shouldn't hardcode usage of EncoderLibJpeg, in order to\n> support different encoders later. You won't need to include\n> encoder_libjpeg.h then, and can keep the Encoder forwared declaration.\n\n\nI think this was already a unique_ptr in a previous iteration, and I\nsuggested moving it to directly store an instance of the libjpeg encoder.\n\nIn this instance of encoding a thumbnail, when encoding a 160x160 image,\nI would be weary about the overhead of setting up a hardware encode, and\nI'd expect the preparation phases of that to be potentially more\nexpensive than just a software encode. Particularly as this has just\ndone a software rescale, so it would have to cache-flush, when it could\njust use the host-cpu with a hot-cache. (ok, so perhaps later it might\nuse a different scaler too ... but then it's likely a different equation\nanyway)\n\nI have not measured that of course, as we don't yet have a hw-jpeg\nencode post-processor. It will be an interesting test in the future.\n\nBut essentially, I think we're just as well leaving this as a single\ninstance of libjpeg for thumbnails. I think I recall the CrOS HAL using\nlibjpeg as well, but I haven't gone back to double-check.\n\n--\nKieran\n\n\n\n> \n>> +\tThumbnailer thumbnailer_;\n> \n> This is good, as I don't expect different thumbnailer types.\n> \n>> };\n>> \n>> #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n>> new file mode 100644\n>> index 0000000..f880ffb\n>> --- /dev/null\n>> +++ b/src/android/jpeg/thumbnailer.cpp\n>> @@ -0,0 +1,109 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> \n> Wrong license.\n> \n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * thumbnailer.cpp - Simple image thumbnailer\n>> + */\n>> +\n>> +#include \"thumbnailer.h\"\n>> +\n>> +#include \n>> +\n>> +#include \"libcamera/internal/file.h\"\n> \n> Why do you need this header ?\n> \n>> +#include \"libcamera/internal/log.h\"\n>> +\n>> +using namespace libcamera;\n>> +\n>> +LOG_DEFINE_CATEGORY(Thumbnailer)\n>> +\n>> +Thumbnailer::Thumbnailer() : valid_(false)\n> \n> \t: valid_(false)\n> \n>> +{\n>> +}\n>> +\n>> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)\n>> +{\n>> +\tsourceSize_ = sourceSize;\n>> +\tpixelFormat_ = pixelFormat;\n>> +\n>> +\tif (pixelFormat_ != formats::NV12) {\n>> +\t\tLOG (Thumbnailer, Error) << \"Failed to configure: Pixel Format \"\n>> +\t\t\t\t << pixelFormat_.toString() << \" unsupported.\";\n> \n> \t\tLOG (Thumbnailer, Error)\n> \t\t\t<< \"Failed to configure: Pixel Format \"\n> \t\t\t<< pixelFormat_.toString() << \" unsupported.\";\n> \n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\ttargetSize_ = computeThumbnailSize();\n>> +\n>> +\tvalid_ = true;\n>> +}\n>> +\n>> +/*\n>> + * The Exif specification recommends the width of the thumbnail to be a\n>> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height\n> \n> s/mutiple/multiple/\n> \n>> + * keeping the aspect ratio same as of the source.\n>> + */\n>> +Size Thumbnailer::computeThumbnailSize()\n>> +{\n>> +\tunsigned int targetHeight;\n>> +\tunsigned int targetWidth = 160;\n>> +\n>> +\ttargetHeight = targetWidth * sourceSize_.height / sourceSize_.width;\n>> +\n>> +\tif (targetHeight & 1)\n>> +\t\ttargetHeight++;\n>> +\n>> +\treturn Size(targetWidth, targetHeight);\n>> +}\n>> +\n>> +void\n>> +Thumbnailer::scaleBuffer(const FrameBuffer &source,\n>> +\t\t\t std::vector &destination)\n>> +{\n>> +\tMappedFrameBuffer frame(&source, PROT_READ);\n>> +\tif (!frame.isValid()) {\n>> +\t\tLOG(Thumbnailer, Error) << \"Failed to map FrameBuffer : \"\n>> +\t\t\t\t << strerror(frame.error());\n> \n> \t\tLOG(Thumbnailer, Error)\n> \t\t\t<< \"Failed to map FrameBuffer : \"\n> \t\t\t<< strerror(frame.error());\n> \n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tif (!valid_) {\n>> +\t\tLOG(Thumbnailer, Error) << \"config is unconfigured or invalid.\";\n>> +\t\treturn;\n>> +\t}\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 char *src_cb, *src_cr;\n>> +\tunsigned char *dst_y, *src_y;\n>> +\n>> +\tsize_t dstSize = (th * tw) + ((th / 2) * tw);\n>> +\tdestination.reserve(dstSize);\n> \n> This should be\n> \n> \tdestination.resize(dstSize);\n> \n> as reserve() allocates memory but doesn't change the size of the vector.\n> \n>> +\tunsigned char *dst = destination.data();\n>> +\tunsigned char *dst_c = dst + th * tw;\n>> +\n>> +\tfor (unsigned int y = 0; y < th; y += 2) {\n>> +\t\tunsigned int sourceY = (sh * y + th / 2) / th;\n>> +\n>> +\t\tdst_y = dst + y * tw;\n>> +\t\tsrc_y = src + sw * sourceY;\n>> +\t\tsrc_cb = src_c + (sourceY / 2) * sw + 0;\n>> +\t\tsrc_cr = src_c + (sourceY / 2) * sw + 1;\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[x] = src_y[sourceX];\n>> +\t\t\tdst_y[tw + x] = src_y[sw + sourceX];\n>> +\t\t\tdst_y[x + 1] = src_y[sourceX + 1];\n>> +\t\t\tdst_y[tw + x + 1] = src_y[sw + sourceX + 1];\n>> +\n>> +\t\t\tdst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];\n>> +\t\t\tdst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];\n>> +\t\t}\n>> +\t}\n>> + }\n> \n> Extra space.\n> \n>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n>> new file mode 100644\n>> index 0000000..b769619\n>> --- /dev/null\n>> +++ b/src/android/jpeg/thumbnailer.h\n>> @@ -0,0 +1,37 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> \n> Wrong license.\n> \n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * thumbnailer.h - Simple image thumbnailer\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>> +\tvoid scaleBuffer(const libcamera::FrameBuffer &source,\n>> +\t\t\t std::vector &dest);\n> \n> How about naming this createThumbnail() or generateThumbnail() ? And the\n> function should be const.\n> \n>> +\tlibcamera::Size size() const { return targetSize_; }\n>> +\n>> +private:\n>> +\tlibcamera::Size computeThumbnailSize();\n> \n> This can be\n> \n> \tlibcamera::Size computeThumbnailSize() const;\n> \n>> +\n>> +\tlibcamera::PixelFormat pixelFormat_;\n>> +\tlibcamera::Size sourceSize_;\n>> +\tlibcamera::Size targetSize_;\n>> +\n>> +\tbool valid_;\n>> +\n> \n> No need for a blank line.\n> \n>> +};\n>> +\n>> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */\n>> diff --git a/src/android/meson.build b/src/android/meson.build\n>> index f72376a..3d4d3be 100644\n>> --- a/src/android/meson.build\n>> +++ b/src/android/meson.build\n>> @@ -25,6 +25,7 @@ android_hal_sources = files([\n>> 'jpeg/encoder_libjpeg.cpp',\n>> 'jpeg/exif.cpp',\n>> 'jpeg/post_processor_jpeg.cpp',\n>> + 'jpeg/thumbnailer.cpp',\n>> ])\n>> \n>> android_camera_metadata_sources = files([\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 59172C3B5C\n\tfor ;\n\tMon, 26 Oct 2020 22:02:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C18A162064;\n\tMon, 26 Oct 2020 23:02:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D46D360350\n\tfor ;\n\tMon, 26 Oct 2020 23:02:47 +0100 (CET)","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 46D24527;\n\tMon, 26 Oct 2020 23:02:46 +0100 (CET)"],"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=\"nWyIYJ9u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603749766;\n\tbh=xvDT2f9ufXWz3WLqM79fQegszm0Oi2n1utTEmScFT18=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=nWyIYJ9uFqWcRy5SQtak/HS9/pMvJKR0bxak2A6V2PgLwp2H0j7GoU534nbIA93YR\n\t9WopI7qvtSMIpyHPxG8dQUE8b0tmvHdB35yXAbQLDDGiYSNRb2m3Fn2AA6OrpbQU6J\n\txtl7eNrKWaw7i5DAIuDa30SXCT1+z7feGFtGzySg=","To":"Laurent Pinchart ,\n\tUmang Jain ","References":"<20201026140134.44166-1-email@uajain.com>\n\t<20201026140134.44166-4-email@uajain.com>\n\t<20201026210533.GC3756@pendragon.ideasonboard.com>","From":"Kieran Bingham ","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":"","Date":"Mon, 26 Oct 2020 22:02:43 +0000","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":"<20201026210533.GC3756@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","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","Reply-To":"kieran.bingham@ideasonboard.com","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":13493,"web_url":"https://patchwork.libcamera.org/comment/13493/","msgid":"<20201026220818.GF3756@pendragon.ideasonboard.com>","date":"2020-10-26T22:08:18","subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:\n> On 26/10/2020 21:05, Laurent Pinchart wrote:\n> > On Mon, Oct 26, 2020 at 07:31:34PM +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/exif.cpp | 16 +++-\n> >> src/android/jpeg/exif.h | 1 +\n> >> src/android/jpeg/post_processor_jpeg.cpp | 35 +++++++-\n> >> src/android/jpeg/post_processor_jpeg.h | 8 +-\n> >> src/android/jpeg/thumbnailer.cpp | 109 +++++++++++++++++++++++\n> >> src/android/jpeg/thumbnailer.h | 37 ++++++++\n> >> src/android/meson.build | 1 +\n> >> 7 files changed, 204 insertions(+), 3 deletions(-)\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/exif.cpp b/src/android/jpeg/exif.cpp\n> >> index d21534a..24197bd 100644\n> >> --- a/src/android/jpeg/exif.cpp\n> >> +++ b/src/android/jpeg/exif.cpp\n> >> @@ -75,8 +75,16 @@ Exif::~Exif()\n> >> \tif (exifData_)\n> >> \t\tfree(exifData_);\n> >> \n> >> -\tif (data_)\n> >> +\tif (data_) {\n> >> +\t\t/*\n> >> +\t\t * Reset thumbnail data to avoid getting double-freed by\n> >> +\t\t * libexif. It is owned by the caller (i.e. PostProcessorJpeg).\n> >> +\t\t */\n> >> +\t\tdata_->data = nullptr;\n> >> +\t\tdata_->size = 0;\n> >> +\n> >> \t\texif_data_unref(data_);\n> >> +\t}\n> >> \n> >> \tif (mem_)\n> >> \t\texif_mem_unref(mem_);\n> >> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)\n> >> \tsetShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\n> >> }\n> >> \n> >> +void Exif::setThumbnail(std::vector &thumbnail)\n> >> +{\n> >> +\tdata_->data = thumbnail.data();\n> >> +\tdata_->size = thumbnail.size();\n> >> +}\n> >> +\n> >> [[nodiscard]] int Exif::generate()\n> >> {\n> >> \tif (exifData_) {\n> >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> >> index 12c27b6..bd54a31 100644\n> >> --- a/src/android/jpeg/exif.h\n> >> +++ b/src/android/jpeg/exif.h\n> >> @@ -26,6 +26,7 @@ public:\n> >> \n> >> \tvoid setOrientation(int orientation);\n> >> \tvoid setSize(const libcamera::Size &size);\n> >> +\tvoid setThumbnail(std::vector &thumbnail);\n> > \n> > You can pass a Span as the setThumbnail() function\n> > shouldn't care what storage container is used.\n> > \n> > It's a bit of a dangerous API, as it will store the pointer internally,\n> > without ensuring that the caller keeps the thumbnail valid after the\n> > call returns. It's fine, but maybe a comment above the\n> > Exif::setThumbnail() functions to state that the thumbnail must remain\n> > valid until the Exif object is destroyed would be a good thing.\n> \n> I think the comment will help indeed. The only alternative would be to\n> pass it into generate(), but and refactor generate() to return the span\n> ... but that's more effort that we need right now. So just a comment\n> will do ;-)\n> \n> \n> \n> >> \tvoid setTimestamp(time_t timestamp);\n> >> \n> >> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> > \n> > I would have moved this to a separate patch.\n> \n> \n> Yes, I'd keep the exif extension for setting the thumbnail, and the\n> usage separate.\n> \n> > \n> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> >> index c56f1b2..416e831 100644\n> >> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> >> @@ -9,7 +9,6 @@\n> >> \n> >> #include \"../camera_device.h\"\n> >> #include \"../camera_metadata.h\"\n> >> -#include \"encoder_libjpeg.h\"\n> >> #include \"exif.h\"\n> >> \n> >> #include \n> >> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> >> \t}\n> >> \n> >> \tstreamSize_ = outCfg.size;\n> >> +\n> >> +\tthumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> >> +\tStreamConfiguration thCfg = inCfg;\n> >> +\tthCfg.size = thumbnailer_.size();\n> >> +\tif (thumbnailEncoder_.configure(thCfg) != 0) {\n> >> +\t\tLOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> >> +\t\treturn -EINVAL;\n> >> +\t}\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 std::vector &thumbnail)\n> >> +{\n> >> +\t/* Stores the raw scaled-down thumbnail bytes. */\n> >> +\tstd::vector rawThumbnail;\n> >> +\n> >> +\tthumbnailer_.scaleBuffer(source, rawThumbnail);\n> >> +\n> >> +\tif (rawThumbnail.data()) {\n> > \n> > This should check for ! .empty() (see additional comments below).\n> > \n> >> +\t\tthumbnail.reserve(rawThumbnail.capacity());\n> > \n> > You should call thumbnail.resize(), and use size() instead of capacity()\n> > below, as reserve() allocates memory but doesn't change the size of the\n> > vector, so it's semantically dangerous to write to the reserved storage\n> > space if it hasn't been marked as in use with .resize().\n> > \n> >> +\n> >> +\t\tint jpeg_size = thumbnailEncoder_.encode(\n> >> +\t\t\t\t{ rawThumbnail.data(), rawThumbnail.capacity() },\n> >> +\t\t\t\t{ thumbnail.data(), thumbnail.capacity() },\n> > \n> > Just pass rawThumbnail and thumbnail, there's an implicit constructor\n> > for Span that will turn them into a span using .data() and .size().\n> > \n> >> +\t\t\t\t{ });\n> >> +\t\tthumbnail.resize(jpeg_size);\n> >> +\n> >> +\t\tLOG(JPEG, Info) << \"Thumbnail compress returned \"\n> >> +\t\t\t\t<< jpeg_size << \" bytes\";\n> > \n> > When log messages don't fit on one line, we usually wrap them as\n> > \n> > \t\tLOG(JPEG, Info)\n> > \t\t\t<< \"Thumbnail compress returned \"\n> > \t\t\t<< jpeg_size << \" bytes\";\n> > \n> > And maybe debug instead of info ?\n> > \n> >> +\t}\n> >> +}\n> >> +\n> >> int PostProcessorJpeg::process(const FrameBuffer &source,\n> >> \t\t\t Span destination,\n> >> \t\t\t CameraMetadata *metadata)\n> >> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> >> \t * second, it is good enough.\n> >> \t */\n> >> \texif.setTimestamp(std::time(nullptr));\n> >> +\tstd::vector thumbnail;\n> >> +\tgenerateThumbnail(source, thumbnail);\n> >> +\texif.setThumbnail(thumbnail);\n> >> \tif (exif.generate() != 0)\n> >> \t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> >> \n> >> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> >> index 3706cec..3894231 100644\n> >> --- a/src/android/jpeg/post_processor_jpeg.h\n> >> +++ b/src/android/jpeg/post_processor_jpeg.h\n> >> @@ -8,12 +8,13 @@\n> >> #define __ANDROID_POST_PROCESSOR_JPEG_H__\n> >> \n> >> #include \"../post_processor.h\"\n> >> +#include \"encoder_libjpeg.h\"\n> >> +#include \"thumbnailer.h\"\n> >> \n> >> #include \n> >> \n> >> #include \"libcamera/internal/buffer.h\"\n> >> \n> >> -class Encoder;\n> >> class CameraDevice;\n> >> \n> >> class PostProcessorJpeg : public PostProcessor\n> >> @@ -28,9 +29,14 @@ public:\n> >> \t\t CameraMetadata *metadata) override;\n> >> \n> >> private:\n> >> +\tvoid generateThumbnail(const libcamera::FrameBuffer &source,\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> > \n> > Could you store this in a std::unique_ptr instead ? The reason\n> > is that we shouldn't hardcode usage of EncoderLibJpeg, in order to\n> > support different encoders later. You won't need to include\n> > encoder_libjpeg.h then, and can keep the Encoder forwared declaration.\n> \n> I think this was already a unique_ptr in a previous iteration, and I\n> suggested moving it to directly store an instance of the libjpeg encoder.\n> \n> In this instance of encoding a thumbnail, when encoding a 160x160 image,\n> I would be weary about the overhead of setting up a hardware encode, and\n> I'd expect the preparation phases of that to be potentially more\n> expensive than just a software encode. Particularly as this has just\n> done a software rescale, so it would have to cache-flush, when it could\n> just use the host-cpu with a hot-cache. (ok, so perhaps later it might\n> use a different scaler too ... but then it's likely a different equation\n> anyway)\n> \n> I have not measured that of course, as we don't yet have a hw-jpeg\n> encode post-processor. It will be an interesting test in the future.\n> \n> But essentially, I think we're just as well leaving this as a single\n> instance of libjpeg for thumbnails. I think I recall the CrOS HAL using\n> libjpeg as well, but I haven't gone back to double-check.\n\nFair enough, those are good points. I'm fine if the code is kept as-is\n(I wouldn't mind a unique_ptr of course :-)). Umang, up to you.\n\n> >> +\tThumbnailer thumbnailer_;\n> > \n> > This is good, as I don't expect different thumbnailer types.\n> > \n> >> };\n> >> \n> >> #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n> >> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> >> new file mode 100644\n> >> index 0000000..f880ffb\n> >> --- /dev/null\n> >> +++ b/src/android/jpeg/thumbnailer.cpp\n> >> @@ -0,0 +1,109 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > \n> > Wrong license.\n> > \n> >> +/*\n> >> + * Copyright (C) 2020, Google Inc.\n> >> + *\n> >> + * thumbnailer.cpp - Simple image thumbnailer\n> >> + */\n> >> +\n> >> +#include \"thumbnailer.h\"\n> >> +\n> >> +#include \n> >> +\n> >> +#include \"libcamera/internal/file.h\"\n> > \n> > Why do you need this header ?\n> > \n> >> +#include \"libcamera/internal/log.h\"\n> >> +\n> >> +using namespace libcamera;\n> >> +\n> >> +LOG_DEFINE_CATEGORY(Thumbnailer)\n> >> +\n> >> +Thumbnailer::Thumbnailer() : valid_(false)\n> > \n> > \t: valid_(false)\n> > \n> >> +{\n> >> +}\n> >> +\n> >> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)\n> >> +{\n> >> +\tsourceSize_ = sourceSize;\n> >> +\tpixelFormat_ = pixelFormat;\n> >> +\n> >> +\tif (pixelFormat_ != formats::NV12) {\n> >> +\t\tLOG (Thumbnailer, Error) << \"Failed to configure: Pixel Format \"\n> >> +\t\t\t\t << pixelFormat_.toString() << \" unsupported.\";\n> > \n> > \t\tLOG (Thumbnailer, Error)\n> > \t\t\t<< \"Failed to configure: Pixel Format \"\n> > \t\t\t<< pixelFormat_.toString() << \" unsupported.\";\n> > \n> >> +\t\treturn;\n> >> +\t}\n> >> +\n> >> +\ttargetSize_ = computeThumbnailSize();\n> >> +\n> >> +\tvalid_ = true;\n> >> +}\n> >> +\n> >> +/*\n> >> + * The Exif specification recommends the width of the thumbnail to be a\n> >> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height\n> > \n> > s/mutiple/multiple/\n> > \n> >> + * keeping the aspect ratio same as of the source.\n> >> + */\n> >> +Size Thumbnailer::computeThumbnailSize()\n> >> +{\n> >> +\tunsigned int targetHeight;\n> >> +\tunsigned int targetWidth = 160;\n> >> +\n> >> +\ttargetHeight = targetWidth * sourceSize_.height / sourceSize_.width;\n> >> +\n> >> +\tif (targetHeight & 1)\n> >> +\t\ttargetHeight++;\n> >> +\n> >> +\treturn Size(targetWidth, targetHeight);\n> >> +}\n> >> +\n> >> +void\n> >> +Thumbnailer::scaleBuffer(const FrameBuffer &source,\n> >> +\t\t\t std::vector &destination)\n> >> +{\n> >> +\tMappedFrameBuffer frame(&source, PROT_READ);\n> >> +\tif (!frame.isValid()) {\n> >> +\t\tLOG(Thumbnailer, Error) << \"Failed to map FrameBuffer : \"\n> >> +\t\t\t\t << strerror(frame.error());\n> > \n> > \t\tLOG(Thumbnailer, Error)\n> > \t\t\t<< \"Failed to map FrameBuffer : \"\n> > \t\t\t<< strerror(frame.error());\n> > \n> >> +\t\treturn;\n> >> +\t}\n> >> +\n> >> +\tif (!valid_) {\n> >> +\t\tLOG(Thumbnailer, Error) << \"config is unconfigured or invalid.\";\n> >> +\t\treturn;\n> >> +\t}\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 char *src_cb, *src_cr;\n> >> +\tunsigned char *dst_y, *src_y;\n> >> +\n> >> +\tsize_t dstSize = (th * tw) + ((th / 2) * tw);\n> >> +\tdestination.reserve(dstSize);\n> > \n> > This should be\n> > \n> > \tdestination.resize(dstSize);\n> > \n> > as reserve() allocates memory but doesn't change the size of the vector.\n> > \n> >> +\tunsigned char *dst = destination.data();\n> >> +\tunsigned char *dst_c = dst + th * tw;\n> >> +\n> >> +\tfor (unsigned int y = 0; y < th; y += 2) {\n> >> +\t\tunsigned int sourceY = (sh * y + th / 2) / th;\n> >> +\n> >> +\t\tdst_y = dst + y * tw;\n> >> +\t\tsrc_y = src + sw * sourceY;\n> >> +\t\tsrc_cb = src_c + (sourceY / 2) * sw + 0;\n> >> +\t\tsrc_cr = src_c + (sourceY / 2) * sw + 1;\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[x] = src_y[sourceX];\n> >> +\t\t\tdst_y[tw + x] = src_y[sw + sourceX];\n> >> +\t\t\tdst_y[x + 1] = src_y[sourceX + 1];\n> >> +\t\t\tdst_y[tw + x + 1] = src_y[sw + sourceX + 1];\n> >> +\n> >> +\t\t\tdst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];\n> >> +\t\t\tdst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];\n> >> +\t\t}\n> >> +\t}\n> >> + }\n> > \n> > Extra space.\n> > \n> >> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> >> new file mode 100644\n> >> index 0000000..b769619\n> >> --- /dev/null\n> >> +++ b/src/android/jpeg/thumbnailer.h\n> >> @@ -0,0 +1,37 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > \n> > Wrong license.\n> > \n> >> +/*\n> >> + * Copyright (C) 2020, Google Inc.\n> >> + *\n> >> + * thumbnailer.h - Simple image thumbnailer\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> >> +\tvoid scaleBuffer(const libcamera::FrameBuffer &source,\n> >> +\t\t\t std::vector &dest);\n> > \n> > How about naming this createThumbnail() or generateThumbnail() ? And the\n> > function should be const.\n> > \n> >> +\tlibcamera::Size size() const { return targetSize_; }\n> >> +\n> >> +private:\n> >> +\tlibcamera::Size computeThumbnailSize();\n> > \n> > This can be\n> > \n> > \tlibcamera::Size computeThumbnailSize() const;\n> > \n> >> +\n> >> +\tlibcamera::PixelFormat pixelFormat_;\n> >> +\tlibcamera::Size sourceSize_;\n> >> +\tlibcamera::Size targetSize_;\n> >> +\n> >> +\tbool valid_;\n> >> +\n> > \n> > No need for a blank line.\n> > \n> >> +};\n> >> +\n> >> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */\n> >> diff --git a/src/android/meson.build b/src/android/meson.build\n> >> index f72376a..3d4d3be 100644\n> >> --- a/src/android/meson.build\n> >> +++ b/src/android/meson.build\n> >> @@ -25,6 +25,7 @@ android_hal_sources = files([\n> >> 'jpeg/encoder_libjpeg.cpp',\n> >> 'jpeg/exif.cpp',\n> >> 'jpeg/post_processor_jpeg.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 95BBDBDB1E\n\tfor ;\n\tMon, 26 Oct 2020 22:09:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0CE7362062;\n\tMon, 26 Oct 2020 23:09:08 +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 4342A60350\n\tfor ;\n\tMon, 26 Oct 2020 23:09:06 +0100 (CET)","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 B516A527;\n\tMon, 26 Oct 2020 23:09:05 +0100 (CET)"],"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=\"OTji9xHX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603750145;\n\tbh=a16rLxJ3ijC1q0QE1zz/WoNqZbUfxMYbtyf54BjW1+w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OTji9xHXXk+B6LFnx+EqG6R/UBAvizypKo5r+rNR5C1WTDcfb5Yq3sB9OE6PzSs8p\n\tlWpRogEPFIWFfzNUIXczIoZItiKeLuEFClUxPbWDXuhdI6YKhj3k4Ky4h+5YUBvfbW\n\tFpZYPkc2uBJ/ydinUUlmKJl6+WY0j7ZZb/q8k6PI=","Date":"Tue, 27 Oct 2020 00:08:18 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20201026220818.GF3756@pendragon.ideasonboard.com>","References":"<20201026140134.44166-1-email@uajain.com>\n\t<20201026140134.44166-4-email@uajain.com>\n\t<20201026210533.GC3756@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","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":13505,"web_url":"https://patchwork.libcamera.org/comment/13505/","msgid":"","date":"2020-10-27T05:21:35","subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang,\n\nOn Tue, Oct 27, 2020 at 7:09 AM Laurent Pinchart\n wrote:\n>\n> Hi Kieran,\n>\n> On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:\n> > On 26/10/2020 21:05, Laurent Pinchart wrote:\n> > > On Mon, Oct 26, 2020 at 07:31:34PM +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/exif.cpp | 16 +++-\n> > >> src/android/jpeg/exif.h | 1 +\n> > >> src/android/jpeg/post_processor_jpeg.cpp | 35 +++++++-\n> > >> src/android/jpeg/post_processor_jpeg.h | 8 +-\n> > >> src/android/jpeg/thumbnailer.cpp | 109 +++++++++++++++++++++++\n> > >> src/android/jpeg/thumbnailer.h | 37 ++++++++\n> > >> src/android/meson.build | 1 +\n> > >> 7 files changed, 204 insertions(+), 3 deletions(-)\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/exif.cpp b/src/android/jpeg/exif.cpp\n> > >> index d21534a..24197bd 100644\n> > >> --- a/src/android/jpeg/exif.cpp\n> > >> +++ b/src/android/jpeg/exif.cpp\n> > >> @@ -75,8 +75,16 @@ Exif::~Exif()\n> > >> if (exifData_)\n> > >> free(exifData_);\n> > >>\n> > >> - if (data_)\n> > >> + if (data_) {\n> > >> + /*\n> > >> + * Reset thumbnail data to avoid getting double-freed by\n> > >> + * libexif. It is owned by the caller (i.e. PostProcessorJpeg).\n> > >> + */\n> > >> + data_->data = nullptr;\n> > >> + data_->size = 0;\n> > >> +\n> > >> exif_data_unref(data_);\n> > >> + }\n> > >>\n> > >> if (mem_)\n> > >> exif_mem_unref(mem_);\n> > >> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)\n> > >> setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\n> > >> }\n> > >>\n> > >> +void Exif::setThumbnail(std::vector &thumbnail)\n> > >> +{\n> > >> + data_->data = thumbnail.data();\n> > >> + data_->size = thumbnail.size();\n> > >> +}\n> > >> +\n> > >> [[nodiscard]] int Exif::generate()\n> > >> {\n> > >> if (exifData_) {\n> > >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> > >> index 12c27b6..bd54a31 100644\n> > >> --- a/src/android/jpeg/exif.h\n> > >> +++ b/src/android/jpeg/exif.h\n> > >> @@ -26,6 +26,7 @@ public:\n> > >>\n> > >> void setOrientation(int orientation);\n> > >> void setSize(const libcamera::Size &size);\n> > >> + void setThumbnail(std::vector &thumbnail);\n> > >\n> > > You can pass a Span as the setThumbnail() function\n> > > shouldn't care what storage container is used.\n> > >\n> > > It's a bit of a dangerous API, as it will store the pointer internally,\n> > > without ensuring that the caller keeps the thumbnail valid after the\n> > > call returns. It's fine, but maybe a comment above the\n> > > Exif::setThumbnail() functions to state that the thumbnail must remain\n> > > valid until the Exif object is destroyed would be a good thing.\n> >\n> > I think the comment will help indeed. The only alternative would be to\n> > pass it into generate(), but and refactor generate() to return the span\n> > ... but that's more effort that we need right now. So just a comment\n> > will do ;-)\n> >\n> >\n> >\n> > >> void setTimestamp(time_t timestamp);\n> > >>\n> > >> libcamera::Span data() const { return { exifData_, size_ }; }\n> > >\n> > > I would have moved this to a separate patch.\n> >\n> >\n> > Yes, I'd keep the exif extension for setting the thumbnail, and the\n> > usage separate.\n> >\n> > >\n> > >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > >> index c56f1b2..416e831 100644\n> > >> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > >> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > >> @@ -9,7 +9,6 @@\n> > >>\n> > >> #include \"../camera_device.h\"\n> > >> #include \"../camera_metadata.h\"\n> > >> -#include \"encoder_libjpeg.h\"\n> > >> #include \"exif.h\"\n> > >>\n> > >> #include \n> > >> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > >> }\n> > >>\n> > >> streamSize_ = outCfg.size;\n> > >> +\n> > >> + thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> > >> + StreamConfiguration thCfg = inCfg;\n> > >> + thCfg.size = thumbnailer_.size();\n> > >> + if (thumbnailEncoder_.configure(thCfg) != 0) {\n> > >> + LOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> > >> + return -EINVAL;\n> > >> + }\n> > >> +\n> > >> encoder_ = std::make_unique();\n> > >>\n> > >> return encoder_->configure(inCfg);\n> > >> }\n> > >>\n> > >> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> > >> + std::vector &thumbnail)\n\nstd::vector* thumbnail\nWe would rather use a pointer than reference if a function changes it.\nSame for other places.\n\n> > >> +{\n> > >> + /* Stores the raw scaled-down thumbnail bytes. */\n> > >> + std::vector rawThumbnail;\n> > >> +\n> > >> + thumbnailer_.scaleBuffer(source, rawThumbnail);\n> > >> +\n> > >> + if (rawThumbnail.data()) {\n> > >\n> > > This should check for ! .empty() (see additional comments below).\n> > >\n> > >> + thumbnail.reserve(rawThumbnail.capacity());\n> > >\n> > > You should call thumbnail.resize(), and use size() instead of capacity()\n> > > below, as reserve() allocates memory but doesn't change the size of the\n> > > vector, so it's semantically dangerous to write to the reserved storage\n> > > space if it hasn't been marked as in use with .resize().\n> > >\n> > >> +\n> > >> + int jpeg_size = thumbnailEncoder_.encode(\n> > >> + { rawThumbnail.data(), rawThumbnail.capacity() },\n> > >> + { thumbnail.data(), thumbnail.capacity() },\n> > >\n> > > Just pass rawThumbnail and thumbnail, there's an implicit constructor\n> > > for Span that will turn them into a span using .data() and .size().\n> > >\n> > >> + { });\n> > >> + thumbnail.resize(jpeg_size);\n> > >> +\n> > >> + LOG(JPEG, Info) << \"Thumbnail compress returned \"\n> > >> + << jpeg_size << \" bytes\";\n> > >\n> > > When log messages don't fit on one line, we usually wrap them as\n> > >\n> > > LOG(JPEG, Info)\n> > > << \"Thumbnail compress returned \"\n> > > << jpeg_size << \" bytes\";\n> > >\n> > > And maybe debug instead of info ?\n> > >\n> > >> + }\n> > >> +}\n\nShall this function handle a failure of jpeg encode?\n\n> > >> +\n> > >> int PostProcessorJpeg::process(const FrameBuffer &source,\n> > >> Span destination,\n> > >> CameraMetadata *metadata)\n> > >> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > >> * second, it is good enough.\n> > >> */\n> > >> exif.setTimestamp(std::time(nullptr));\n> > >> + std::vector thumbnail;\n> > >> + generateThumbnail(source, thumbnail);\n> > >> + exif.setThumbnail(thumbnail);\n> > >> if (exif.generate() != 0)\n> > >> LOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> > >>\n> > >> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> > >> index 3706cec..3894231 100644\n> > >> --- a/src/android/jpeg/post_processor_jpeg.h\n> > >> +++ b/src/android/jpeg/post_processor_jpeg.h\n> > >> @@ -8,12 +8,13 @@\n> > >> #define __ANDROID_POST_PROCESSOR_JPEG_H__\n> > >>\n> > >> #include \"../post_processor.h\"\n> > >> +#include \"encoder_libjpeg.h\"\n> > >> +#include \"thumbnailer.h\"\n> > >>\n> > >> #include \n> > >>\n> > >> #include \"libcamera/internal/buffer.h\"\n> > >>\n> > >> -class Encoder;\n> > >> class CameraDevice;\n> > >>\n> > >> class PostProcessorJpeg : public PostProcessor\n> > >> @@ -28,9 +29,14 @@ public:\n> > >> CameraMetadata *metadata) override;\n> > >>\n> > >> private:\n> > >> + void generateThumbnail(const libcamera::FrameBuffer &source,\n> > >> + std::vector &thumbnail);\n> > >> +\n> > >> CameraDevice *const cameraDevice_;\n> > >> std::unique_ptr encoder_;\n> > >> libcamera::Size streamSize_;\n> > >> + EncoderLibJpeg thumbnailEncoder_;\n> > >\n> > > Could you store this in a std::unique_ptr instead ? The reason\n> > > is that we shouldn't hardcode usage of EncoderLibJpeg, in order to\n> > > support different encoders later. You won't need to include\n> > > encoder_libjpeg.h then, and can keep the Encoder forwared declaration.\n> >\n> > I think this was already a unique_ptr in a previous iteration, and I\n> > suggested moving it to directly store an instance of the libjpeg encoder.\n> >\n> > In this instance of encoding a thumbnail, when encoding a 160x160 image,\n> > I would be weary about the overhead of setting up a hardware encode, and\n> > I'd expect the preparation phases of that to be potentially more\n> > expensive than just a software encode. Particularly as this has just\n> > done a software rescale, so it would have to cache-flush, when it could\n> > just use the host-cpu with a hot-cache. (ok, so perhaps later it might\n> > use a different scaler too ... but then it's likely a different equation\n> > anyway)\n> >\n> > I have not measured that of course, as we don't yet have a hw-jpeg\n> > encode post-processor. It will be an interesting test in the future.\n> >\n> > But essentially, I think we're just as well leaving this as a single\n> > instance of libjpeg for thumbnails. I think I recall the CrOS HAL using\n> > libjpeg as well, but I haven't gone back to double-check.\n>\n> Fair enough, those are good points. I'm fine if the code is kept as-is\n> (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.\n>\n\nAlthough either is fine to me, I wonder why the encoder for the\nthumbnail is within the thumbnailer.\nSince the thumbnailer is currently being used for thumbnail, it seems\nto be reasonable to move the encoder to it.\nWhat do you think?\n\n\n> > >> + Thumbnailer thumbnailer_;\n> > >\n> > > This is good, as I don't expect different thumbnailer types.\n> > >\n> > >> };\n> > >>\n> > >> #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n> > >> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> > >> new file mode 100644\n> > >> index 0000000..f880ffb\n> > >> --- /dev/null\n> > >> +++ b/src/android/jpeg/thumbnailer.cpp\n> > >> @@ -0,0 +1,109 @@\n> > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > >\n> > > Wrong license.\n> > >\n> > >> +/*\n> > >> + * Copyright (C) 2020, Google Inc.\n> > >> + *\n> > >> + * thumbnailer.cpp - Simple image thumbnailer\n> > >> + */\n> > >> +\n> > >> +#include \"thumbnailer.h\"\n> > >> +\n> > >> +#include \n> > >> +\n> > >> +#include \"libcamera/internal/file.h\"\n> > >\n> > > Why do you need this header ?\n> > >\n> > >> +#include \"libcamera/internal/log.h\"\n> > >> +\n> > >> +using namespace libcamera;\n> > >> +\n> > >> +LOG_DEFINE_CATEGORY(Thumbnailer)\n> > >> +\n> > >> +Thumbnailer::Thumbnailer() : valid_(false)\n> > >\n> > > : valid_(false)\n> > >\n> > >> +{\n> > >> +}\n> > >> +\n> > >> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)\n> > >> +{\n> > >> + sourceSize_ = sourceSize;\n> > >> + pixelFormat_ = pixelFormat;\n> > >> +\n> > >> + if (pixelFormat_ != formats::NV12) {\n> > >> + LOG (Thumbnailer, Error) << \"Failed to configure: Pixel Format \"\n> > >> + << pixelFormat_.toString() << \" unsupported.\";\n> > >\n> > > LOG (Thumbnailer, Error)\n> > > << \"Failed to configure: Pixel Format \"\n> > > << pixelFormat_.toString() << \" unsupported.\";\n> > >\n> > >> + return;\n> > >> + }\n> > >> +\n> > >> + targetSize_ = computeThumbnailSize();\n> > >> +\n> > >> + valid_ = true;\n> > >> +}\n> > >> +\n> > >> +/*\n> > >> + * The Exif specification recommends the width of the thumbnail to be a\n> > >> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height\n> > >\n> > > s/mutiple/multiple/\n> > >\n> > >> + * keeping the aspect ratio same as of the source.\n> > >> + */\n> > >> +Size Thumbnailer::computeThumbnailSize()\n> > >> +{\n> > >> + unsigned int targetHeight;\n> > >> + unsigned int targetWidth = 160;\n\nnit: constexpr unsigned int kTargetWidth = 160;\n\n> > >> +\n> > >> + targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;\n> > >> +\n> > >> + if (targetHeight & 1)\n> > >> + targetHeight++;\n> > >> +\n> > >> + return Size(targetWidth, targetHeight);\n> > >> +}\n> > >> +\n> > >> +void\n> > >> +Thumbnailer::scaleBuffer(const FrameBuffer &source,\n> > >> + std::vector &destination)\n> > >> +{\n> > >> + MappedFrameBuffer frame(&source, PROT_READ);\n> > >> + if (!frame.isValid()) {\n> > >> + LOG(Thumbnailer, Error) << \"Failed to map FrameBuffer : \"\n> > >> + << strerror(frame.error());\n> > >\n> > > LOG(Thumbnailer, Error)\n> > > << \"Failed to map FrameBuffer : \"\n> > > << strerror(frame.error());\n> > >\n> > >> + return;\n> > >> + }\n> > >> +\n> > >> + if (!valid_) {\n> > >> + LOG(Thumbnailer, Error) << \"config is unconfigured or invalid.\";\n> > >> + return;\n> > >> + }\n> > >> +\n> > >> + const unsigned int sw = sourceSize_.width;\n> > >> + const unsigned int sh = sourceSize_.height;\n> > >> + const unsigned int tw = targetSize_.width;\n> > >> + const unsigned int th = targetSize_.height;\n> > >> +\n> > >> + /* Image scaling block implementing nearest-neighbour algorithm. */\n> > >> + unsigned char *src = static_cast(frame.maps()[0].data());\n> > >> + unsigned char *src_c = src + sh * sw;\n> > >> + unsigned char *src_cb, *src_cr;\n> > >> + unsigned char *dst_y, *src_y;\n> > >> +\n> > >> + size_t dstSize = (th * tw) + ((th / 2) * tw);\n> > >> + destination.reserve(dstSize);\n> > >\n> > > This should be\n> > >\n> > > destination.resize(dstSize);\n> > >\n> > > as reserve() allocates memory but doesn't change the size of the vector.\n> > >\n> > >> + unsigned char *dst = destination.data();\n> > >> + unsigned char *dst_c = dst + th * tw;\n> > >> +\n> > >> + for (unsigned int y = 0; y < th; y += 2) {\n> > >> + unsigned int sourceY = (sh * y + th / 2) / th;\n> > >> +\n> > >> + dst_y = dst + y * tw;\n> > >> + src_y = src + sw * sourceY;\n> > >> + src_cb = src_c + (sourceY / 2) * sw + 0;\n> > >> + src_cr = src_c + (sourceY / 2) * sw + 1;\n> > >> +\n> > >> + for (unsigned int x = 0; x < tw; x += 2) {\n> > >> + unsigned int sourceX = (sw * x + tw / 2) / tw;\n> > >> +\n> > >> + dst_y[x] = src_y[sourceX];\n> > >> + dst_y[tw + x] = src_y[sw + sourceX];\n> > >> + dst_y[x + 1] = src_y[sourceX + 1];\n> > >> + dst_y[tw + x + 1] = src_y[sw + sourceX + 1];\n> > >> +\n> > >> + dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];\n> > >> + dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];\n> > >> + }\n> > >> + }\n> > >> + }\n> > >\n> > > Extra space.\n> > >\n> > >> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> > >> new file mode 100644\n> > >> index 0000000..b769619\n> > >> --- /dev/null\n> > >> +++ b/src/android/jpeg/thumbnailer.h\n> > >> @@ -0,0 +1,37 @@\n> > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > >\n> > > Wrong license.\n> > >\n> > >> +/*\n> > >> + * Copyright (C) 2020, Google Inc.\n> > >> + *\n> > >> + * thumbnailer.h - Simple image thumbnailer\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> > >> + Thumbnailer();\n> > >> +\n> > >> + void configure(const libcamera::Size &sourceSize,\n> > >> + libcamera::PixelFormat pixelFormat);\n> > >> + void scaleBuffer(const libcamera::FrameBuffer &source,\n> > >> + std::vector &dest);\n> > >\n> > > How about naming this createThumbnail() or generateThumbnail() ? And the\n> > > function should be const.\n> > >\n> > >> + libcamera::Size size() const { return targetSize_; }\n\nnit: to reduce unnecessary copy, let's return const reference.\nconst libcamera::Size& computeThumbanilSize() const;\n\nBest Regards,\n-Hiro\n> > >> +\n> > >> +private:\n> > >> + libcamera::Size computeThumbnailSize();\n> > >\n> > > This can be\n> > >\n> > > libcamera::Size computeThumbnailSize() const;\n> > >\n> > >> +\n> > >> + libcamera::PixelFormat pixelFormat_;\n> > >> + libcamera::Size sourceSize_;\n> > >> + libcamera::Size targetSize_;\n> > >> +\n> > >> + bool valid_;\n> > >> +\n> > >\n> > > No need for a blank line.\n> > >\n> > >> +};\n> > >> +\n> > >> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */\n> > >> diff --git a/src/android/meson.build b/src/android/meson.build\n> > >> index f72376a..3d4d3be 100644\n> > >> --- a/src/android/meson.build\n> > >> +++ b/src/android/meson.build\n> > >> @@ -25,6 +25,7 @@ android_hal_sources = files([\n> > >> 'jpeg/encoder_libjpeg.cpp',\n> > >> 'jpeg/exif.cpp',\n> > >> 'jpeg/post_processor_jpeg.cpp',\n> > >> + 'jpeg/thumbnailer.cpp',\n> > >> ])\n> > >>\n> > >> android_camera_metadata_sources = files([\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 DFE98C3B5C\n\tfor ;\n\tTue, 27 Oct 2020 05:21:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D2C862067;\n\tTue, 27 Oct 2020 06:21:48 +0100 (CET)","from mail-ej1-x642.google.com (mail-ej1-x642.google.com\n\t[IPv6:2a00:1450:4864:20::642])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D15676034C\n\tfor ;\n\tTue, 27 Oct 2020 06:21:46 +0100 (CET)","by mail-ej1-x642.google.com with SMTP id c15so437468ejs.0\n\tfor ;\n\tMon, 26 Oct 2020 22:21:46 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"dKPo/MMW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=zMOcWUVbIdTj/BBFvHklVg7yOrdkNiU0YUjr/mcqL3I=;\n\tb=dKPo/MMWLQG/aLz7EiNAvfaCKwfB3M20nc0wCSnp5Tl7hTIib9Ux0TMsSn57TRl7wc\n\t12si1c6cKBIAwfw5+4/mUj5X0DbNP1msu+8RtFzPTrdUcNnEkOLo7XTT8B9VD1jtFfyW\n\txwNFYTfkFvvvky/n7ricvSAZ/oXYMCs3X4CTI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=zMOcWUVbIdTj/BBFvHklVg7yOrdkNiU0YUjr/mcqL3I=;\n\tb=Uk18ekXuvrebrTIrsQL8gH9LLHr+yprISOZzrGvCRAWwG2ACd8U0UKfubhvZ98LGSO\n\tLouZylmPKXfDVd7pWtcDq1gNfCFLPiX71p9szLlVfCNHsgbF7q25krKvlhysbbZllkwf\n\taW0NPVh3R+gGj4XKSGjgi7BIIUq9ZUsfi2tve8OAy2e0aTlF8BMag4sPPFbNo6ltLvoy\n\thyeyMlkrgg9wUoC+fbLv6iX+av6sm6DA4vxNnn87aKkPlpACmcCS0riQFOp2OxGepI6q\n\tSNVuXSwCIeKjsbQbtjmY2ctW+9f8x2wyOf8qPV5nPJvRoSTLC40QaptUwFk6wftEvGnB\n\tlL6g==","X-Gm-Message-State":"AOAM533h8aq/xaIEEYhEcrY5YbBpxO72egIZSeslErA5ivVR29CdJW0u\n\tC0D3szmHndEBs1eP00TYLiwYP46Sp0qS0QO/wnxJRf2yw9DNrg==","X-Google-Smtp-Source":"ABdhPJyXcCIvSUtNaUy7Vi7FqonGLZ3PwdeJ9320ooIdpRz4UkUjixp12x5GvQwFxNq8/zTGopM1hbxbTLfuDBYaKGc=","X-Received":"by 2002:a17:906:401b:: with SMTP id\n\tv27mr630504ejj.475.1603776106269; \n\tMon, 26 Oct 2020 22:21:46 -0700 (PDT)","MIME-Version":"1.0","References":"<20201026140134.44166-1-email@uajain.com>\n\t<20201026140134.44166-4-email@uajain.com>\n\t<20201026210533.GC3756@pendragon.ideasonboard.com>\n\t\n\t<20201026220818.GF3756@pendragon.ideasonboard.com>","In-Reply-To":"<20201026220818.GF3756@pendragon.ideasonboard.com>","From":"Hirokazu Honda ","Date":"Tue, 27 Oct 2020 14:21:35 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","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":13506,"web_url":"https://patchwork.libcamera.org/comment/13506/","msgid":"","date":"2020-10-27T06:53:02","subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang,\n\nJust few additional comments.\n\nOn Tue, Oct 27, 2020 at 2:21 PM Hirokazu Honda wrote:\n>\n> Hi Umang,\n>\n> On Tue, Oct 27, 2020 at 7:09 AM Laurent Pinchart\n> wrote:\n> >\n> > Hi Kieran,\n> >\n> > On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:\n> > > On 26/10/2020 21:05, Laurent Pinchart wrote:\n> > > > On Mon, Oct 26, 2020 at 07:31:34PM +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/exif.cpp | 16 +++-\n> > > >> src/android/jpeg/exif.h | 1 +\n> > > >> src/android/jpeg/post_processor_jpeg.cpp | 35 +++++++-\n> > > >> src/android/jpeg/post_processor_jpeg.h | 8 +-\n> > > >> src/android/jpeg/thumbnailer.cpp | 109 +++++++++++++++++++++++\n> > > >> src/android/jpeg/thumbnailer.h | 37 ++++++++\n> > > >> src/android/meson.build | 1 +\n> > > >> 7 files changed, 204 insertions(+), 3 deletions(-)\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/exif.cpp b/src/android/jpeg/exif.cpp\n> > > >> index d21534a..24197bd 100644\n> > > >> --- a/src/android/jpeg/exif.cpp\n> > > >> +++ b/src/android/jpeg/exif.cpp\n> > > >> @@ -75,8 +75,16 @@ Exif::~Exif()\n> > > >> if (exifData_)\n> > > >> free(exifData_);\n> > > >>\n> > > >> - if (data_)\n> > > >> + if (data_) {\n> > > >> + /*\n> > > >> + * Reset thumbnail data to avoid getting double-freed by\n> > > >> + * libexif. It is owned by the caller (i.e. PostProcessorJpeg).\n> > > >> + */\n> > > >> + data_->data = nullptr;\n> > > >> + data_->size = 0;\n> > > >> +\n> > > >> exif_data_unref(data_);\n> > > >> + }\n> > > >>\n> > > >> if (mem_)\n> > > >> exif_mem_unref(mem_);\n> > > >> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)\n> > > >> setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\n> > > >> }\n> > > >>\n> > > >> +void Exif::setThumbnail(std::vector &thumbnail)\n> > > >> +{\n> > > >> + data_->data = thumbnail.data();\n> > > >> + data_->size = thumbnail.size();\n> > > >> +}\n> > > >> +\n> > > >> [[nodiscard]] int Exif::generate()\n> > > >> {\n> > > >> if (exifData_) {\n> > > >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> > > >> index 12c27b6..bd54a31 100644\n> > > >> --- a/src/android/jpeg/exif.h\n> > > >> +++ b/src/android/jpeg/exif.h\n> > > >> @@ -26,6 +26,7 @@ public:\n> > > >>\n> > > >> void setOrientation(int orientation);\n> > > >> void setSize(const libcamera::Size &size);\n> > > >> + void setThumbnail(std::vector &thumbnail);\n> > > >\n> > > > You can pass a Span as the setThumbnail() function\n> > > > shouldn't care what storage container is used.\n> > > >\n> > > > It's a bit of a dangerous API, as it will store the pointer internally,\n> > > > without ensuring that the caller keeps the thumbnail valid after the\n> > > > call returns. It's fine, but maybe a comment above the\n> > > > Exif::setThumbnail() functions to state that the thumbnail must remain\n> > > > valid until the Exif object is destroyed would be a good thing.\n> > >\n> > > I think the comment will help indeed. The only alternative would be to\n> > > pass it into generate(), but and refactor generate() to return the span\n> > > ... but that's more effort that we need right now. So just a comment\n> > > will do ;-)\n> > >\n> > >\n> > >\n> > > >> void setTimestamp(time_t timestamp);\n> > > >>\n> > > >> libcamera::Span data() const { return { exifData_, size_ }; }\n> > > >\n> > > > I would have moved this to a separate patch.\n> > >\n> > >\n> > > Yes, I'd keep the exif extension for setting the thumbnail, and the\n> > > usage separate.\n> > >\n> > > >\n> > > >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > > >> index c56f1b2..416e831 100644\n> > > >> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > >> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > >> @@ -9,7 +9,6 @@\n> > > >>\n> > > >> #include \"../camera_device.h\"\n> > > >> #include \"../camera_metadata.h\"\n> > > >> -#include \"encoder_libjpeg.h\"\n> > > >> #include \"exif.h\"\n> > > >>\n> > > >> #include \n> > > >> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > > >> }\n> > > >>\n> > > >> streamSize_ = outCfg.size;\n> > > >> +\n> > > >> + thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> > > >> + StreamConfiguration thCfg = inCfg;\n> > > >> + thCfg.size = thumbnailer_.size();\n> > > >> + if (thumbnailEncoder_.configure(thCfg) != 0) {\n> > > >> + LOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> > > >> + return -EINVAL;\n> > > >> + }\n> > > >> +\n> > > >> encoder_ = std::make_unique();\n> > > >>\n> > > >> return encoder_->configure(inCfg);\n> > > >> }\n> > > >>\n> > > >> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> > > >> + std::vector &thumbnail)\n>\n> std::vector* thumbnail\n> We would rather use a pointer than reference if a function changes it.\n> Same for other places.\n>\n> > > >> +{\n> > > >> + /* Stores the raw scaled-down thumbnail bytes. */\n> > > >> + std::vector rawThumbnail;\n> > > >> +\n> > > >> + thumbnailer_.scaleBuffer(source, rawThumbnail);\n> > > >> +\n> > > >> + if (rawThumbnail.data()) {\n> > > >\n> > > > This should check for ! .empty() (see additional comments below).\n> > > >\n> > > >> + thumbnail.reserve(rawThumbnail.capacity());\n> > > >\n> > > > You should call thumbnail.resize(), and use size() instead of capacity()\n> > > > below, as reserve() allocates memory but doesn't change the size of the\n> > > > vector, so it's semantically dangerous to write to the reserved storage\n> > > > space if it hasn't been marked as in use with .resize().\n> > > >\n> > > >> +\n> > > >> + int jpeg_size = thumbnailEncoder_.encode(\n> > > >> + { rawThumbnail.data(), rawThumbnail.capacity() },\n> > > >> + { thumbnail.data(), thumbnail.capacity() },\n> > > >\n> > > > Just pass rawThumbnail and thumbnail, there's an implicit constructor\n> > > > for Span that will turn them into a span using .data() and .size().\n> > > >\n> > > >> + { });\n> > > >> + thumbnail.resize(jpeg_size);\n> > > >> +\n> > > >> + LOG(JPEG, Info) << \"Thumbnail compress returned \"\n> > > >> + << jpeg_size << \" bytes\";\n> > > >\n> > > > When log messages don't fit on one line, we usually wrap them as\n> > > >\n> > > > LOG(JPEG, Info)\n> > > > << \"Thumbnail compress returned \"\n> > > > << jpeg_size << \" bytes\";\n> > > >\n> > > > And maybe debug instead of info ?\n> > > >\n> > > >> + }\n> > > >> +}\n>\n> Shall this function handle a failure of jpeg encode?\n>\n> > > >> +\n> > > >> int PostProcessorJpeg::process(const FrameBuffer &source,\n> > > >> Span destination,\n> > > >> CameraMetadata *metadata)\n> > > >> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > > >> * second, it is good enough.\n> > > >> */\n> > > >> exif.setTimestamp(std::time(nullptr));\n> > > >> + std::vector thumbnail;\n> > > >> + generateThumbnail(source, thumbnail);\n> > > >> + exif.setThumbnail(thumbnail);\n> > > >> if (exif.generate() != 0)\n> > > >> LOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> > > >>\n> > > >> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> > > >> index 3706cec..3894231 100644\n> > > >> --- a/src/android/jpeg/post_processor_jpeg.h\n> > > >> +++ b/src/android/jpeg/post_processor_jpeg.h\n> > > >> @@ -8,12 +8,13 @@\n> > > >> #define __ANDROID_POST_PROCESSOR_JPEG_H__\n> > > >>\n> > > >> #include \"../post_processor.h\"\n> > > >> +#include \"encoder_libjpeg.h\"\n> > > >> +#include \"thumbnailer.h\"\n> > > >>\n> > > >> #include \n> > > >>\n> > > >> #include \"libcamera/internal/buffer.h\"\n> > > >>\n> > > >> -class Encoder;\n> > > >> class CameraDevice;\n> > > >>\n> > > >> class PostProcessorJpeg : public PostProcessor\n> > > >> @@ -28,9 +29,14 @@ public:\n> > > >> CameraMetadata *metadata) override;\n> > > >>\n> > > >> private:\n> > > >> + void generateThumbnail(const libcamera::FrameBuffer &source,\n> > > >> + std::vector &thumbnail);\n> > > >> +\n> > > >> CameraDevice *const cameraDevice_;\n> > > >> std::unique_ptr encoder_;\n> > > >> libcamera::Size streamSize_;\n> > > >> + EncoderLibJpeg thumbnailEncoder_;\n> > > >\n> > > > Could you store this in a std::unique_ptr instead ? The reason\n> > > > is that we shouldn't hardcode usage of EncoderLibJpeg, in order to\n> > > > support different encoders later. You won't need to include\n> > > > encoder_libjpeg.h then, and can keep the Encoder forwared declaration.\n> > >\n> > > I think this was already a unique_ptr in a previous iteration, and I\n> > > suggested moving it to directly store an instance of the libjpeg encoder.\n> > >\n> > > In this instance of encoding a thumbnail, when encoding a 160x160 image,\n> > > I would be weary about the overhead of setting up a hardware encode, and\n> > > I'd expect the preparation phases of that to be potentially more\n> > > expensive than just a software encode. Particularly as this has just\n> > > done a software rescale, so it would have to cache-flush, when it could\n> > > just use the host-cpu with a hot-cache. (ok, so perhaps later it might\n> > > use a different scaler too ... but then it's likely a different equation\n> > > anyway)\n> > >\n> > > I have not measured that of course, as we don't yet have a hw-jpeg\n> > > encode post-processor. It will be an interesting test in the future.\n> > >\n> > > But essentially, I think we're just as well leaving this as a single\n> > > instance of libjpeg for thumbnails. I think I recall the CrOS HAL using\n> > > libjpeg as well, but I haven't gone back to double-check.\n> >\n> > Fair enough, those are good points. I'm fine if the code is kept as-is\n> > (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.\n> >\n>\n> Although either is fine to me, I wonder why the encoder for the\n> thumbnail is within the thumbnailer.\n> Since the thumbnailer is currently being used for thumbnail, it seems\n> to be reasonable to move the encoder to it.\n> What do you think?\n>\n>\n> > > >> + Thumbnailer thumbnailer_;\n> > > >\n> > > > This is good, as I don't expect different thumbnailer types.\n> > > >\n> > > >> };\n> > > >>\n> > > >> #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n> > > >> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> > > >> new file mode 100644\n> > > >> index 0000000..f880ffb\n> > > >> --- /dev/null\n> > > >> +++ b/src/android/jpeg/thumbnailer.cpp\n> > > >> @@ -0,0 +1,109 @@\n> > > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > >\n> > > > Wrong license.\n> > > >\n> > > >> +/*\n> > > >> + * Copyright (C) 2020, Google Inc.\n> > > >> + *\n> > > >> + * thumbnailer.cpp - Simple image thumbnailer\n> > > >> + */\n> > > >> +\n> > > >> +#include \"thumbnailer.h\"\n> > > >> +\n> > > >> +#include \n> > > >> +\n> > > >> +#include \"libcamera/internal/file.h\"\n> > > >\n> > > > Why do you need this header ?\n> > > >\n> > > >> +#include \"libcamera/internal/log.h\"\n> > > >> +\n> > > >> +using namespace libcamera;\n> > > >> +\n> > > >> +LOG_DEFINE_CATEGORY(Thumbnailer)\n> > > >> +\n> > > >> +Thumbnailer::Thumbnailer() : valid_(false)\n> > > >\n> > > > : valid_(false)\n> > > >\n> > > >> +{\n> > > >> +}\n> > > >> +\n> > > >> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)\n> > > >> +{\n> > > >> + sourceSize_ = sourceSize;\n> > > >> + pixelFormat_ = pixelFormat;\n> > > >> +\n> > > >> + if (pixelFormat_ != formats::NV12) {\n> > > >> + LOG (Thumbnailer, Error) << \"Failed to configure: Pixel Format \"\n> > > >> + << pixelFormat_.toString() << \" unsupported.\";\n> > > >\n> > > > LOG (Thumbnailer, Error)\n> > > > << \"Failed to configure: Pixel Format \"\n> > > > << pixelFormat_.toString() << \" unsupported.\";\n> > > >\n> > > >> + return;\n> > > >> + }\n> > > >> +\n> > > >> + targetSize_ = computeThumbnailSize();\n> > > >> +\n> > > >> + valid_ = true;\n> > > >> +}\n> > > >> +\n> > > >> +/*\n\nShall we check sourceSize is an even dimension and smaller than targetSize_?\n\n> > > >> + * The Exif specification recommends the width of the thumbnail to be a\n> > > >> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height\n> > > >\n> > > > s/mutiple/multiple/\n> > > >\n> > > >> + * keeping the aspect ratio same as of the source.\n> > > >> + */\n> > > >> +Size Thumbnailer::computeThumbnailSize()\n> > > >> +{\n> > > >> + unsigned int targetHeight;\n> > > >> + unsigned int targetWidth = 160;\n>\n> nit: constexpr unsigned int kTargetWidth = 160;\n>\n> > > >> +\n> > > >> + targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;\n> > > >> +\n> > > >> + if (targetHeight & 1)\n> > > >> + targetHeight++;\n> > > >> +\n> > > >> + return Size(targetWidth, targetHeight);\n> > > >> +}\n> > > >> +\n> > > >> +void\n> > > >> +Thumbnailer::scaleBuffer(const FrameBuffer &source,\n> > > >> + std::vector &destination)\n> > > >> +{\n> > > >> + MappedFrameBuffer frame(&source, PROT_READ);\n> > > >> + if (!frame.isValid()) {\n> > > >> + LOG(Thumbnailer, Error) << \"Failed to map FrameBuffer : \"\n> > > >> + << strerror(frame.error());\n> > > >\n> > > > LOG(Thumbnailer, Error)\n> > > > << \"Failed to map FrameBuffer : \"\n> > > > << strerror(frame.error());\n> > > >\n> > > >> + return;\n> > > >> + }\n> > > >> +\n> > > >> + if (!valid_) {\n> > > >> + LOG(Thumbnailer, Error) << \"config is unconfigured or invalid.\";\n> > > >> + return;\n> > > >> + }\n> > > >> +\n> > > >> + const unsigned int sw = sourceSize_.width;\n> > > >> + const unsigned int sh = sourceSize_.height;\n> > > >> + const unsigned int tw = targetSize_.width;\n> > > >> + const unsigned int th = targetSize_.height;\n> > > >> +\n\nassert(tw % 2 == 0 && th % 2 == 0);\n\n> > > >> + /* Image scaling block implementing nearest-neighbour algorithm. */\n> > > >> + unsigned char *src = static_cast(frame.maps()[0].data());\n\nShould we check just in case, frame.size() is sourceSize_?\n\n> > > >> + unsigned char *src_c = src + sh * sw;\n> > > >> + unsigned char *src_cb, *src_cr;\n> > > >> + unsigned char *dst_y, *src_y;\n> > > >> +\n\nnit: Follow variable naming rule? srcY, srcCb, and so on.\n\n> > > >> + size_t dstSize = (th * tw) + ((th / 2) * tw);\n> > > >> + destination.reserve(dstSize);\n> > > >\n> > > > This should be\n> > > >\n> > > > destination.resize(dstSize);\n> > > >\n> > > > as reserve() allocates memory but doesn't change the size of the vector.\n> > > >\n> > > >> + unsigned char *dst = destination.data();\n> > > >> + unsigned char *dst_c = dst + th * tw;\n> > > >> +\n> > > >> + for (unsigned int y = 0; y < th; y += 2) {\n> > > >> + unsigned int sourceY = (sh * y + th / 2) / th;\n> > > >> +\n\nI don't understand well why \"+ th / 2\" is required.\nCould you explain?\n\nBest Regards,\n-Hiro\n> > > >> + dst_y = dst + y * tw;\n> > > >> + src_y = src + sw * sourceY;\n> > > >> + src_cb = src_c + (sourceY / 2) * sw + 0;\n> > > >> + src_cr = src_c + (sourceY / 2) * sw + 1;\n> > > >> +\n> > > >> + for (unsigned int x = 0; x < tw; x += 2) {\n> > > >> + unsigned int sourceX = (sw * x + tw / 2) / tw;\n> > > >> +\n> > > >> + dst_y[x] = src_y[sourceX];\n> > > >> + dst_y[tw + x] = src_y[sw + sourceX];\n> > > >> + dst_y[x + 1] = src_y[sourceX + 1];\n> > > >> + dst_y[tw + x + 1] = src_y[sw + sourceX + 1];\n> > > >> +\n> > > >> + dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];\n> > > >> + dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];\n> > > >> + }\n> > > >> + }\n> > > >> + }\n> > > >\n> > > > Extra space.\n> > > >\n> > > >> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> > > >> new file mode 100644\n> > > >> index 0000000..b769619\n> > > >> --- /dev/null\n> > > >> +++ b/src/android/jpeg/thumbnailer.h\n> > > >> @@ -0,0 +1,37 @@\n> > > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > >\n> > > > Wrong license.\n> > > >\n> > > >> +/*\n> > > >> + * Copyright (C) 2020, Google Inc.\n> > > >> + *\n> > > >> + * thumbnailer.h - Simple image thumbnailer\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> > > >> + Thumbnailer();\n> > > >> +\n> > > >> + void configure(const libcamera::Size &sourceSize,\n> > > >> + libcamera::PixelFormat pixelFormat);\n> > > >> + void scaleBuffer(const libcamera::FrameBuffer &source,\n> > > >> + std::vector &dest);\n> > > >\n> > > > How about naming this createThumbnail() or generateThumbnail() ? And the\n> > > > function should be const.\n> > > >\n> > > >> + libcamera::Size size() const { return targetSize_; }\n>\n> nit: to reduce unnecessary copy, let's return const reference.\n> const libcamera::Size& computeThumbanilSize() const;\n>\n> Best Regards,\n> -Hiro\n> > > >> +\n> > > >> +private:\n> > > >> + libcamera::Size computeThumbnailSize();\n> > > >\n> > > > This can be\n> > > >\n> > > > libcamera::Size computeThumbnailSize() const;\n> > > >\n> > > >> +\n> > > >> + libcamera::PixelFormat pixelFormat_;\n> > > >> + libcamera::Size sourceSize_;\n> > > >> + libcamera::Size targetSize_;\n> > > >> +\n> > > >> + bool valid_;\n> > > >> +\n> > > >\n> > > > No need for a blank line.\n> > > >\n> > > >> +};\n> > > >> +\n> > > >> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */\n> > > >> diff --git a/src/android/meson.build b/src/android/meson.build\n> > > >> index f72376a..3d4d3be 100644\n> > > >> --- a/src/android/meson.build\n> > > >> +++ b/src/android/meson.build\n> > > >> @@ -25,6 +25,7 @@ android_hal_sources = files([\n> > > >> 'jpeg/encoder_libjpeg.cpp',\n> > > >> 'jpeg/exif.cpp',\n> > > >> 'jpeg/post_processor_jpeg.cpp',\n> > > >> + 'jpeg/thumbnailer.cpp',\n> > > >> ])\n> > > >>\n> > > >> android_camera_metadata_sources = files([\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 EA4B4BDB1E\n\tfor ;\n\tTue, 27 Oct 2020 06:53:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7804862069;\n\tTue, 27 Oct 2020 07:53:16 +0100 (CET)","from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com\n\t[IPv6:2a00:1450:4864:20::52d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 27D376034C\n\tfor ;\n\tTue, 27 Oct 2020 07:53:14 +0100 (CET)","by mail-ed1-x52d.google.com with SMTP id x1so301419eds.1\n\tfor ;\n\tMon, 26 Oct 2020 23:53:14 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"fJZhj4dL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=YsoZ9/qKAhsDbhzBer7qbSkIdi3Amu+L2B6s4JSX6d8=;\n\tb=fJZhj4dLUzI4+NyCpaVd5LeHvax86TRykJKbAhyiFpydMhT+l+lckyG7DHph25OILN\n\tYdyUTxQsW1bdaOC9XcXst8QZI7HBTlqGcvkVtb0rozkAsrPzftpHtE6SypjQYkimLYtb\n\tFPf3TdAe3wBHGbjxRQzTTSjMMxz9evM+6YBbY=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=YsoZ9/qKAhsDbhzBer7qbSkIdi3Amu+L2B6s4JSX6d8=;\n\tb=MCL8bAk4XaTGkemK28o7Lqf08/kIFMVfPxjufPn76Ivjdenxq1/It4O0jR5+wca1Ar\n\tKtJOZ4HorZJjMLaddmLZIiWptzt81zzrwivc5Y4KCdEG+jk/EiAC1wSNDWZKs88SMfB2\n\t2tA9prlDwXd7okdjeN3w+4S7MWAOTCuFW2hAz4ljO56NUFqX6B+W9PyAqipgO0lncVEU\n\tBDLGthtHAvAVW0VTF6riBisZDOFoPpvAxBAvjr8dDmfNAI/ojE3CB9B8EscPOxPeOF9s\n\t8Ml9Kg0HNtrr0T1eNbZcboTq+RXwr1fT8O1+ucx71mV5cM5wQ0ltbqRYSAmCB1qNcAsL\n\tUFNA==","X-Gm-Message-State":"AOAM531IhbR2ePTTW/YauIOR7ucCXOpXpuM/+dH1IXAoHo/MUGcHLlIV\n\tw3cf/oW6rat4e+BRxcw4c/4D6dV1rluUQwGCBBXSRw==","X-Google-Smtp-Source":"ABdhPJw/IX2TtgpGDW8mn0T1VkjH133BVWJ9sMNvOG3/q4L6SLDgA1tLUvDzNLuEYWnbKgfbUw8+MpXTaoinNWC+c7k=","X-Received":"by 2002:a50:c40c:: with SMTP id v12mr686063edf.233.1603781593221;\n\tMon, 26 Oct 2020 23:53:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20201026140134.44166-1-email@uajain.com>\n\t<20201026140134.44166-4-email@uajain.com>\n\t<20201026210533.GC3756@pendragon.ideasonboard.com>\n\t\n\t<20201026220818.GF3756@pendragon.ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 27 Oct 2020 15:53:02 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","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":13507,"web_url":"https://patchwork.libcamera.org/comment/13507/","msgid":"","date":"2020-10-27T07:34:34","subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Tue, Oct 27, 2020 at 3:53 PM Hirokazu Honda wrote:\n>\n> Hi Umang,\n>\n> Just few additional comments.\n>\n> On Tue, Oct 27, 2020 at 2:21 PM Hirokazu Honda wrote:\n> >\n> > Hi Umang,\n> >\n> > On Tue, Oct 27, 2020 at 7:09 AM Laurent Pinchart\n> > wrote:\n> > >\n> > > Hi Kieran,\n> > >\n> > > On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:\n> > > > On 26/10/2020 21:05, Laurent Pinchart wrote:\n> > > > > On Mon, Oct 26, 2020 at 07:31:34PM +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/exif.cpp | 16 +++-\n> > > > >> src/android/jpeg/exif.h | 1 +\n> > > > >> src/android/jpeg/post_processor_jpeg.cpp | 35 +++++++-\n> > > > >> src/android/jpeg/post_processor_jpeg.h | 8 +-\n> > > > >> src/android/jpeg/thumbnailer.cpp | 109 +++++++++++++++++++++++\n> > > > >> src/android/jpeg/thumbnailer.h | 37 ++++++++\n> > > > >> src/android/meson.build | 1 +\n> > > > >> 7 files changed, 204 insertions(+), 3 deletions(-)\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/exif.cpp b/src/android/jpeg/exif.cpp\n> > > > >> index d21534a..24197bd 100644\n> > > > >> --- a/src/android/jpeg/exif.cpp\n> > > > >> +++ b/src/android/jpeg/exif.cpp\n> > > > >> @@ -75,8 +75,16 @@ Exif::~Exif()\n> > > > >> if (exifData_)\n> > > > >> free(exifData_);\n> > > > >>\n> > > > >> - if (data_)\n> > > > >> + if (data_) {\n> > > > >> + /*\n> > > > >> + * Reset thumbnail data to avoid getting double-freed by\n> > > > >> + * libexif. It is owned by the caller (i.e. PostProcessorJpeg).\n> > > > >> + */\n> > > > >> + data_->data = nullptr;\n> > > > >> + data_->size = 0;\n> > > > >> +\n> > > > >> exif_data_unref(data_);\n> > > > >> + }\n> > > > >>\n> > > > >> if (mem_)\n> > > > >> exif_mem_unref(mem_);\n> > > > >> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)\n> > > > >> setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\n> > > > >> }\n> > > > >>\n> > > > >> +void Exif::setThumbnail(std::vector &thumbnail)\n> > > > >> +{\n> > > > >> + data_->data = thumbnail.data();\n> > > > >> + data_->size = thumbnail.size();\n> > > > >> +}\n> > > > >> +\n> > > > >> [[nodiscard]] int Exif::generate()\n> > > > >> {\n> > > > >> if (exifData_) {\n> > > > >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> > > > >> index 12c27b6..bd54a31 100644\n> > > > >> --- a/src/android/jpeg/exif.h\n> > > > >> +++ b/src/android/jpeg/exif.h\n> > > > >> @@ -26,6 +26,7 @@ public:\n> > > > >>\n> > > > >> void setOrientation(int orientation);\n> > > > >> void setSize(const libcamera::Size &size);\n> > > > >> + void setThumbnail(std::vector &thumbnail);\n> > > > >\n> > > > > You can pass a Span as the setThumbnail() function\n> > > > > shouldn't care what storage container is used.\n> > > > >\n> > > > > It's a bit of a dangerous API, as it will store the pointer internally,\n> > > > > without ensuring that the caller keeps the thumbnail valid after the\n> > > > > call returns. It's fine, but maybe a comment above the\n> > > > > Exif::setThumbnail() functions to state that the thumbnail must remain\n> > > > > valid until the Exif object is destroyed would be a good thing.\n> > > >\n> > > > I think the comment will help indeed. The only alternative would be to\n> > > > pass it into generate(), but and refactor generate() to return the span\n> > > > ... but that's more effort that we need right now. So just a comment\n> > > > will do ;-)\n> > > >\n> > > >\n> > > >\n> > > > >> void setTimestamp(time_t timestamp);\n> > > > >>\n> > > > >> libcamera::Span data() const { return { exifData_, size_ }; }\n> > > > >\n> > > > > I would have moved this to a separate patch.\n> > > >\n> > > >\n> > > > Yes, I'd keep the exif extension for setting the thumbnail, and the\n> > > > usage separate.\n> > > >\n> > > > >\n> > > > >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > >> index c56f1b2..416e831 100644\n> > > > >> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > > >> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > >> @@ -9,7 +9,6 @@\n> > > > >>\n> > > > >> #include \"../camera_device.h\"\n> > > > >> #include \"../camera_metadata.h\"\n> > > > >> -#include \"encoder_libjpeg.h\"\n> > > > >> #include \"exif.h\"\n> > > > >>\n> > > > >> #include \n> > > > >> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > > > >> }\n> > > > >>\n> > > > >> streamSize_ = outCfg.size;\n> > > > >> +\n> > > > >> + thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> > > > >> + StreamConfiguration thCfg = inCfg;\n> > > > >> + thCfg.size = thumbnailer_.size();\n> > > > >> + if (thumbnailEncoder_.configure(thCfg) != 0) {\n> > > > >> + LOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> > > > >> + return -EINVAL;\n> > > > >> + }\n> > > > >> +\n> > > > >> encoder_ = std::make_unique();\n> > > > >>\n> > > > >> return encoder_->configure(inCfg);\n> > > > >> }\n> > > > >>\n> > > > >> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> > > > >> + std::vector &thumbnail)\n> >\n> > std::vector* thumbnail\n> > We would rather use a pointer than reference if a function changes it.\n> > Same for other places.\n> >\n> > > > >> +{\n> > > > >> + /* Stores the raw scaled-down thumbnail bytes. */\n> > > > >> + std::vector rawThumbnail;\n> > > > >> +\n> > > > >> + thumbnailer_.scaleBuffer(source, rawThumbnail);\n> > > > >> +\n> > > > >> + if (rawThumbnail.data()) {\n> > > > >\n> > > > > This should check for ! .empty() (see additional comments below).\n> > > > >\n> > > > >> + thumbnail.reserve(rawThumbnail.capacity());\n> > > > >\n> > > > > You should call thumbnail.resize(), and use size() instead of capacity()\n> > > > > below, as reserve() allocates memory but doesn't change the size of the\n> > > > > vector, so it's semantically dangerous to write to the reserved storage\n> > > > > space if it hasn't been marked as in use with .resize().\n> > > > >\n> > > > >> +\n> > > > >> + int jpeg_size = thumbnailEncoder_.encode(\n> > > > >> + { rawThumbnail.data(), rawThumbnail.capacity() },\n> > > > >> + { thumbnail.data(), thumbnail.capacity() },\n> > > > >\n> > > > > Just pass rawThumbnail and thumbnail, there's an implicit constructor\n> > > > > for Span that will turn them into a span using .data() and .size().\n> > > > >\n> > > > >> + { });\n> > > > >> + thumbnail.resize(jpeg_size);\n> > > > >> +\n> > > > >> + LOG(JPEG, Info) << \"Thumbnail compress returned \"\n> > > > >> + << jpeg_size << \" bytes\";\n> > > > >\n> > > > > When log messages don't fit on one line, we usually wrap them as\n> > > > >\n> > > > > LOG(JPEG, Info)\n> > > > > << \"Thumbnail compress returned \"\n> > > > > << jpeg_size << \" bytes\";\n> > > > >\n> > > > > And maybe debug instead of info ?\n> > > > >\n> > > > >> + }\n> > > > >> +}\n> >\n> > Shall this function handle a failure of jpeg encode?\n> >\n> > > > >> +\n> > > > >> int PostProcessorJpeg::process(const FrameBuffer &source,\n> > > > >> Span destination,\n> > > > >> CameraMetadata *metadata)\n> > > > >> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > > > >> * second, it is good enough.\n> > > > >> */\n> > > > >> exif.setTimestamp(std::time(nullptr));\n> > > > >> + std::vector thumbnail;\n> > > > >> + generateThumbnail(source, thumbnail);\n> > > > >> + exif.setThumbnail(thumbnail);\n> > > > >> if (exif.generate() != 0)\n> > > > >> LOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> > > > >>\n> > > > >> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> > > > >> index 3706cec..3894231 100644\n> > > > >> --- a/src/android/jpeg/post_processor_jpeg.h\n> > > > >> +++ b/src/android/jpeg/post_processor_jpeg.h\n> > > > >> @@ -8,12 +8,13 @@\n> > > > >> #define __ANDROID_POST_PROCESSOR_JPEG_H__\n> > > > >>\n> > > > >> #include \"../post_processor.h\"\n> > > > >> +#include \"encoder_libjpeg.h\"\n> > > > >> +#include \"thumbnailer.h\"\n> > > > >>\n> > > > >> #include \n> > > > >>\n> > > > >> #include \"libcamera/internal/buffer.h\"\n> > > > >>\n> > > > >> -class Encoder;\n> > > > >> class CameraDevice;\n> > > > >>\n> > > > >> class PostProcessorJpeg : public PostProcessor\n> > > > >> @@ -28,9 +29,14 @@ public:\n> > > > >> CameraMetadata *metadata) override;\n> > > > >>\n> > > > >> private:\n> > > > >> + void generateThumbnail(const libcamera::FrameBuffer &source,\n> > > > >> + std::vector &thumbnail);\n> > > > >> +\n> > > > >> CameraDevice *const cameraDevice_;\n> > > > >> std::unique_ptr encoder_;\n> > > > >> libcamera::Size streamSize_;\n> > > > >> + EncoderLibJpeg thumbnailEncoder_;\n> > > > >\n> > > > > Could you store this in a std::unique_ptr instead ? The reason\n> > > > > is that we shouldn't hardcode usage of EncoderLibJpeg, in order to\n> > > > > support different encoders later. You won't need to include\n> > > > > encoder_libjpeg.h then, and can keep the Encoder forwared declaration.\n> > > >\n> > > > I think this was already a unique_ptr in a previous iteration, and I\n> > > > suggested moving it to directly store an instance of the libjpeg encoder.\n> > > >\n> > > > In this instance of encoding a thumbnail, when encoding a 160x160 image,\n> > > > I would be weary about the overhead of setting up a hardware encode, and\n> > > > I'd expect the preparation phases of that to be potentially more\n> > > > expensive than just a software encode. Particularly as this has just\n> > > > done a software rescale, so it would have to cache-flush, when it could\n> > > > just use the host-cpu with a hot-cache. (ok, so perhaps later it might\n> > > > use a different scaler too ... but then it's likely a different equation\n> > > > anyway)\n> > > >\n> > > > I have not measured that of course, as we don't yet have a hw-jpeg\n> > > > encode post-processor. It will be an interesting test in the future.\n> > > >\n> > > > But essentially, I think we're just as well leaving this as a single\n> > > > instance of libjpeg for thumbnails. I think I recall the CrOS HAL using\n> > > > libjpeg as well, but I haven't gone back to double-check.\n> > >\n> > > Fair enough, those are good points. I'm fine if the code is kept as-is\n> > > (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.\n> > >\n> >\n> > Although either is fine to me, I wonder why the encoder for the\n> > thumbnail is within the thumbnailer.\n> > Since the thumbnailer is currently being used for thumbnail, it seems\n> > to be reasonable to move the encoder to it.\n> > What do you think?\n> >\n> >\n> > > > >> + Thumbnailer thumbnailer_;\n> > > > >\n> > > > > This is good, as I don't expect different thumbnailer types.\n> > > > >\n> > > > >> };\n> > > > >>\n> > > > >> #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n> > > > >> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> > > > >> new file mode 100644\n> > > > >> index 0000000..f880ffb\n> > > > >> --- /dev/null\n> > > > >> +++ b/src/android/jpeg/thumbnailer.cpp\n> > > > >> @@ -0,0 +1,109 @@\n> > > > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > >\n> > > > > Wrong license.\n> > > > >\n> > > > >> +/*\n> > > > >> + * Copyright (C) 2020, Google Inc.\n> > > > >> + *\n> > > > >> + * thumbnailer.cpp - Simple image thumbnailer\n> > > > >> + */\n> > > > >> +\n> > > > >> +#include \"thumbnailer.h\"\n> > > > >> +\n> > > > >> +#include \n> > > > >> +\n> > > > >> +#include \"libcamera/internal/file.h\"\n> > > > >\n> > > > > Why do you need this header ?\n> > > > >\n> > > > >> +#include \"libcamera/internal/log.h\"\n> > > > >> +\n> > > > >> +using namespace libcamera;\n> > > > >> +\n> > > > >> +LOG_DEFINE_CATEGORY(Thumbnailer)\n> > > > >> +\n> > > > >> +Thumbnailer::Thumbnailer() : valid_(false)\n> > > > >\n> > > > > : valid_(false)\n> > > > >\n> > > > >> +{\n> > > > >> +}\n> > > > >> +\n> > > > >> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)\n> > > > >> +{\n> > > > >> + sourceSize_ = sourceSize;\n> > > > >> + pixelFormat_ = pixelFormat;\n> > > > >> +\n> > > > >> + if (pixelFormat_ != formats::NV12) {\n> > > > >> + LOG (Thumbnailer, Error) << \"Failed to configure: Pixel Format \"\n> > > > >> + << pixelFormat_.toString() << \" unsupported.\";\n> > > > >\n> > > > > LOG (Thumbnailer, Error)\n> > > > > << \"Failed to configure: Pixel Format \"\n> > > > > << pixelFormat_.toString() << \" unsupported.\";\n> > > > >\n> > > > >> + return;\n> > > > >> + }\n> > > > >> +\n> > > > >> + targetSize_ = computeThumbnailSize();\n> > > > >> +\n> > > > >> + valid_ = true;\n> > > > >> +}\n> > > > >> +\n> > > > >> +/*\n>\n> Shall we check sourceSize is an even dimension and smaller than targetSize_?\n>\n> > > > >> + * The Exif specification recommends the width of the thumbnail to be a\n> > > > >> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height\n> > > > >\n> > > > > s/mutiple/multiple/\n> > > > >\n> > > > >> + * keeping the aspect ratio same as of the source.\n> > > > >> + */\n> > > > >> +Size Thumbnailer::computeThumbnailSize()\n> > > > >> +{\n> > > > >> + unsigned int targetHeight;\n> > > > >> + unsigned int targetWidth = 160;\n> >\n> > nit: constexpr unsigned int kTargetWidth = 160;\n> >\n> > > > >> +\n> > > > >> + targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;\n> > > > >> +\n> > > > >> + if (targetHeight & 1)\n> > > > >> + targetHeight++;\n> > > > >> +\n> > > > >> + return Size(targetWidth, targetHeight);\n> > > > >> +}\n> > > > >> +\n> > > > >> +void\n> > > > >> +Thumbnailer::scaleBuffer(const FrameBuffer &source,\n> > > > >> + std::vector &destination)\n> > > > >> +{\n> > > > >> + MappedFrameBuffer frame(&source, PROT_READ);\n> > > > >> + if (!frame.isValid()) {\n> > > > >> + LOG(Thumbnailer, Error) << \"Failed to map FrameBuffer : \"\n> > > > >> + << strerror(frame.error());\n> > > > >\n> > > > > LOG(Thumbnailer, Error)\n> > > > > << \"Failed to map FrameBuffer : \"\n> > > > > << strerror(frame.error());\n> > > > >\n> > > > >> + return;\n> > > > >> + }\n> > > > >> +\n> > > > >> + if (!valid_) {\n> > > > >> + LOG(Thumbnailer, Error) << \"config is unconfigured or invalid.\";\n> > > > >> + return;\n> > > > >> + }\n> > > > >> +\n> > > > >> + const unsigned int sw = sourceSize_.width;\n> > > > >> + const unsigned int sh = sourceSize_.height;\n> > > > >> + const unsigned int tw = targetSize_.width;\n> > > > >> + const unsigned int th = targetSize_.height;\n> > > > >> +\n>\n> assert(tw % 2 == 0 && th % 2 == 0);\n>\n> > > > >> + /* Image scaling block implementing nearest-neighbour algorithm. */\n> > > > >> + unsigned char *src = static_cast(frame.maps()[0].data());\n>\n> Should we check just in case, frame.size() is sourceSize_?\n>\n> > > > >> + unsigned char *src_c = src + sh * sw;\n> > > > >> + unsigned char *src_cb, *src_cr;\n> > > > >> + unsigned char *dst_y, *src_y;\n> > > > >> +\n>\n> nit: Follow variable naming rule? srcY, srcCb, and so on.\n>\n> > > > >> + size_t dstSize = (th * tw) + ((th / 2) * tw);\n> > > > >> + destination.reserve(dstSize);\n> > > > >\n> > > > > This should be\n> > > > >\n> > > > > destination.resize(dstSize);\n> > > > >\n> > > > > as reserve() allocates memory but doesn't change the size of the vector.\n> > > > >\n> > > > >> + unsigned char *dst = destination.data();\n> > > > >> + unsigned char *dst_c = dst + th * tw;\n> > > > >> +\n> > > > >> + for (unsigned int y = 0; y < th; y += 2) {\n> > > > >> + unsigned int sourceY = (sh * y + th / 2) / th;\n> > > > >> +\n>\n> I don't understand well why \"+ th / 2\" is required.\n> Could you explain?\n>\n\nPlease ignore. I found that this addition is for round to the nearest integer.\n\n> Best Regards,\n> -Hiro\n> > > > >> + dst_y = dst + y * tw;\n> > > > >> + src_y = src + sw * sourceY;\n> > > > >> + src_cb = src_c + (sourceY / 2) * sw + 0;\n> > > > >> + src_cr = src_c + (sourceY / 2) * sw + 1;\n> > > > >> +\n> > > > >> + for (unsigned int x = 0; x < tw; x += 2) {\n> > > > >> + unsigned int sourceX = (sw * x + tw / 2) / tw;\n> > > > >> +\n> > > > >> + dst_y[x] = src_y[sourceX];\n> > > > >> + dst_y[tw + x] = src_y[sw + sourceX];\n> > > > >> + dst_y[x + 1] = src_y[sourceX + 1];\n> > > > >> + dst_y[tw + x + 1] = src_y[sw + sourceX + 1];\n> > > > >> +\n> > > > >> + dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];\n> > > > >> + dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];\n> > > > >> + }\n> > > > >> + }\n> > > > >> + }\n> > > > >\n> > > > > Extra space.\n> > > > >\n> > > > >> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> > > > >> new file mode 100644\n> > > > >> index 0000000..b769619\n> > > > >> --- /dev/null\n> > > > >> +++ b/src/android/jpeg/thumbnailer.h\n> > > > >> @@ -0,0 +1,37 @@\n> > > > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > >\n> > > > > Wrong license.\n> > > > >\n> > > > >> +/*\n> > > > >> + * Copyright (C) 2020, Google Inc.\n> > > > >> + *\n> > > > >> + * thumbnailer.h - Simple image thumbnailer\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> > > > >> + Thumbnailer();\n> > > > >> +\n> > > > >> + void configure(const libcamera::Size &sourceSize,\n> > > > >> + libcamera::PixelFormat pixelFormat);\n> > > > >> + void scaleBuffer(const libcamera::FrameBuffer &source,\n> > > > >> + std::vector &dest);\n> > > > >\n> > > > > How about naming this createThumbnail() or generateThumbnail() ? And the\n> > > > > function should be const.\n> > > > >\n> > > > >> + libcamera::Size size() const { return targetSize_; }\n> >\n> > nit: to reduce unnecessary copy, let's return const reference.\n> > const libcamera::Size& computeThumbanilSize() const;\n> >\n> > Best Regards,\n> > -Hiro\n> > > > >> +\n> > > > >> +private:\n> > > > >> + libcamera::Size computeThumbnailSize();\n> > > > >\n> > > > > This can be\n> > > > >\n> > > > > libcamera::Size computeThumbnailSize() const;\n> > > > >\n> > > > >> +\n> > > > >> + libcamera::PixelFormat pixelFormat_;\n> > > > >> + libcamera::Size sourceSize_;\n> > > > >> + libcamera::Size targetSize_;\n> > > > >> +\n> > > > >> + bool valid_;\n> > > > >> +\n> > > > >\n> > > > > No need for a blank line.\n> > > > >\n> > > > >> +};\n> > > > >> +\n> > > > >> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */\n> > > > >> diff --git a/src/android/meson.build b/src/android/meson.build\n> > > > >> index f72376a..3d4d3be 100644\n> > > > >> --- a/src/android/meson.build\n> > > > >> +++ b/src/android/meson.build\n> > > > >> @@ -25,6 +25,7 @@ android_hal_sources = files([\n> > > > >> 'jpeg/encoder_libjpeg.cpp',\n> > > > >> 'jpeg/exif.cpp',\n> > > > >> 'jpeg/post_processor_jpeg.cpp',\n> > > > >> + 'jpeg/thumbnailer.cpp',\n> > > > >> ])\n> > > > >>\n> > > > >> android_camera_metadata_sources = files([\n> > >\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel","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 80FB0C3B5C\n\tfor ;\n\tTue, 27 Oct 2020 07:34:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 120186206A;\n\tTue, 27 Oct 2020 08:34:47 +0100 (CET)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81A2762053\n\tfor ;\n\tTue, 27 Oct 2020 08:34:45 +0100 (CET)","by mail-ej1-x633.google.com with SMTP id bn26so775480ejb.6\n\tfor ;\n\tTue, 27 Oct 2020 00:34:45 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"mTR8beek\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=q5Uwr5z/Tl7satGSvmT/MTGO6E1DPTpuRFethMdL9uw=;\n\tb=mTR8beekIdHhPpMhCLI4vpD7UoFFvz98T2WK5Jxb/mp5prTC61UHp9gpg/8fM9DDEf\n\t8sAbOXNAlTG7a0YPNmbok1KNSxkthtwVKc4SyPTUDgTUyu7WXJgLVnB+GHxvGgEf4x7i\n\tQxOo1cTc2uL4GnzkZncu8EDeVPOXFVjxbaZgw=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=q5Uwr5z/Tl7satGSvmT/MTGO6E1DPTpuRFethMdL9uw=;\n\tb=dkmVKbKuoTe3RAdSEM/Ji4d+SbA20uv51d5xuDBw1Vr3xKeCsr4IiZAoArwOd6JUff\n\tYhPzBxk7mBtAxrZ15k/DHPC3wxclUFcopzBNxSSBMj/hvnUk4V4ccZV8+jAbXOP2Q5yB\n\t16AmfK4S4+55cJ0qMY1Okgl9zcRtTjEYtPB67nVx+/B+iX4LIwzp8tqrev9ucMoFnYgd\n\tpJZ+KiQ6uYWdARrruP5dlRNan77AIL89IhKYeErbAwtm1haf1yIxE3GXihMHWAPO25L+\n\t3bcyxmSAgSrj2UmEZnwUtcGtxm+c8cOtX7/wkD+zq95J4Nx2/NSC1xBmeEFkjHZ0xBRj\n\t6weQ==","X-Gm-Message-State":"AOAM532DBRFMS66ZzfuYV+3zrwMRyam5GCuXxqg8VgZTkopJrpj1dLRV\n\t1YlA9mo78sLXrIyFSchc242Cl5H40zJ+MJf6TDkwlQ==","X-Google-Smtp-Source":"ABdhPJzZ/daBqcIRl7MpmMcnXdISvNAfz+p8JY2SD/+09YR1/+FoJLy8UIbyZGIK2PGA9VOt/QEhxu1OqKe767Gt8fQ=","X-Received":"by 2002:a17:906:6b92:: with SMTP id\n\tl18mr1133775ejr.292.1603784084750; \n\tTue, 27 Oct 2020 00:34:44 -0700 (PDT)","MIME-Version":"1.0","References":"<20201026140134.44166-1-email@uajain.com>\n\t<20201026140134.44166-4-email@uajain.com>\n\t<20201026210533.GC3756@pendragon.ideasonboard.com>\n\t\n\t<20201026220818.GF3756@pendragon.ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 27 Oct 2020 16:34:34 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","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":13508,"web_url":"https://patchwork.libcamera.org/comment/13508/","msgid":"","date":"2020-10-27T07:40:24","subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Hiro,\n\nOn 10/27/20 10:51 AM, Hirokazu Honda wrote:\n> Hi Umang,\n>\n> On Tue, Oct 27, 2020 at 7:09 AM Laurent Pinchart\n> wrote:\n>> Hi Kieran,\n>>\n>> On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:\n>>> On 26/10/2020 21:05, Laurent Pinchart wrote:\n>>>> On Mon, Oct 26, 2020 at 07:31:34PM +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/exif.cpp | 16 +++-\n>>>>> src/android/jpeg/exif.h | 1 +\n>>>>> src/android/jpeg/post_processor_jpeg.cpp | 35 +++++++-\n>>>>> src/android/jpeg/post_processor_jpeg.h | 8 +-\n>>>>> src/android/jpeg/thumbnailer.cpp | 109 +++++++++++++++++++++++\n>>>>> src/android/jpeg/thumbnailer.h | 37 ++++++++\n>>>>> src/android/meson.build | 1 +\n>>>>> 7 files changed, 204 insertions(+), 3 deletions(-)\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/exif.cpp b/src/android/jpeg/exif.cpp\n>>>>> index d21534a..24197bd 100644\n>>>>> --- a/src/android/jpeg/exif.cpp\n>>>>> +++ b/src/android/jpeg/exif.cpp\n>>>>> @@ -75,8 +75,16 @@ Exif::~Exif()\n>>>>> if (exifData_)\n>>>>> free(exifData_);\n>>>>>\n>>>>> - if (data_)\n>>>>> + if (data_) {\n>>>>> + /*\n>>>>> + * Reset thumbnail data to avoid getting double-freed by\n>>>>> + * libexif. It is owned by the caller (i.e. PostProcessorJpeg).\n>>>>> + */\n>>>>> + data_->data = nullptr;\n>>>>> + data_->size = 0;\n>>>>> +\n>>>>> exif_data_unref(data_);\n>>>>> + }\n>>>>>\n>>>>> if (mem_)\n>>>>> exif_mem_unref(mem_);\n>>>>> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)\n>>>>> setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\n>>>>> }\n>>>>>\n>>>>> +void Exif::setThumbnail(std::vector &thumbnail)\n>>>>> +{\n>>>>> + data_->data = thumbnail.data();\n>>>>> + data_->size = thumbnail.size();\n>>>>> +}\n>>>>> +\n>>>>> [[nodiscard]] int Exif::generate()\n>>>>> {\n>>>>> if (exifData_) {\n>>>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n>>>>> index 12c27b6..bd54a31 100644\n>>>>> --- a/src/android/jpeg/exif.h\n>>>>> +++ b/src/android/jpeg/exif.h\n>>>>> @@ -26,6 +26,7 @@ public:\n>>>>>\n>>>>> void setOrientation(int orientation);\n>>>>> void setSize(const libcamera::Size &size);\n>>>>> + void setThumbnail(std::vector &thumbnail);\n>>>> You can pass a Span as the setThumbnail() function\n>>>> shouldn't care what storage container is used.\n>>>>\n>>>> It's a bit of a dangerous API, as it will store the pointer internally,\n>>>> without ensuring that the caller keeps the thumbnail valid after the\n>>>> call returns. It's fine, but maybe a comment above the\n>>>> Exif::setThumbnail() functions to state that the thumbnail must remain\n>>>> valid until the Exif object is destroyed would be a good thing.\n>>> I think the comment will help indeed. The only alternative would be to\n>>> pass it into generate(), but and refactor generate() to return the span\n>>> ... but that's more effort that we need right now. So just a comment\n>>> will do ;-)\n>>>\n>>>\n>>>\n>>>>> void setTimestamp(time_t timestamp);\n>>>>>\n>>>>> libcamera::Span data() const { return { exifData_, size_ }; }\n>>>> I would have moved this to a separate patch.\n>>> Yes, I'd keep the exif extension for setting the thumbnail, and the\n>>> usage separate.\n>>>\n>>>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n>>>>> index c56f1b2..416e831 100644\n>>>>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n>>>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>>>>> @@ -9,7 +9,6 @@\n>>>>>\n>>>>> #include \"../camera_device.h\"\n>>>>> #include \"../camera_metadata.h\"\n>>>>> -#include \"encoder_libjpeg.h\"\n>>>>> #include \"exif.h\"\n>>>>>\n>>>>> #include \n>>>>> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n>>>>> }\n>>>>>\n>>>>> streamSize_ = outCfg.size;\n>>>>> +\n>>>>> + thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n>>>>> + StreamConfiguration thCfg = inCfg;\n>>>>> + thCfg.size = thumbnailer_.size();\n>>>>> + if (thumbnailEncoder_.configure(thCfg) != 0) {\n>>>>> + LOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n>>>>> + return -EINVAL;\n>>>>> + }\n>>>>> +\n>>>>> encoder_ = std::make_unique();\n>>>>>\n>>>>> return encoder_->configure(inCfg);\n>>>>> }\n>>>>>\n>>>>> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n>>>>> + std::vector &thumbnail)\n> std::vector* thumbnail\n> We would rather use a pointer than reference if a function changes it.\n> Same for other places.\nAck.\n>>>>> +{\n>>>>> + /* Stores the raw scaled-down thumbnail bytes. */\n>>>>> + std::vector rawThumbnail;\n>>>>> +\n>>>>> + thumbnailer_.scaleBuffer(source, rawThumbnail);\n>>>>> +\n>>>>> + if (rawThumbnail.data()) {\n>>>> This should check for ! .empty() (see additional comments below).\n>>>>\n>>>>> + thumbnail.reserve(rawThumbnail.capacity());\n>>>> You should call thumbnail.resize(), and use size() instead of capacity()\n>>>> below, as reserve() allocates memory but doesn't change the size of the\n>>>> vector, so it's semantically dangerous to write to the reserved storage\n>>>> space if it hasn't been marked as in use with .resize().\n>>>>\n>>>>> +\n>>>>> + int jpeg_size = thumbnailEncoder_.encode(\n>>>>> + { rawThumbnail.data(), rawThumbnail.capacity() },\n>>>>> + { thumbnail.data(), thumbnail.capacity() },\n>>>> Just pass rawThumbnail and thumbnail, there's an implicit constructor\n>>>> for Span that will turn them into a span using .data() and .size().\n>>>>\n>>>>> + { });\n>>>>> + thumbnail.resize(jpeg_size);\n>>>>> +\n>>>>> + LOG(JPEG, Info) << \"Thumbnail compress returned \"\n>>>>> + << jpeg_size << \" bytes\";\n>>>> When log messages don't fit on one line, we usually wrap them as\n>>>>\n>>>> LOG(JPEG, Info)\n>>>> << \"Thumbnail compress returned \"\n>>>> << jpeg_size << \" bytes\";\n>>>>\n>>>> And maybe debug instead of info ?\n>>>>\n>>>>> + }\n>>>>> +}\n> Shall this function handle a failure of jpeg encode?\nIdeally it should but I am wondering in what style. I would prefer that \nchecking for the (thumbnail data != 0) before calling \nExif::setThumbnail() below. Since, when the encoding fails, jpeg_size \nwill get 0, and the thumbnail vector(placeholder for our thumbnail data) \nwill get resized to '0'.\n>>>>> +\n>>>>> int PostProcessorJpeg::process(const FrameBuffer &source,\n>>>>> Span destination,\n>>>>> CameraMetadata *metadata)\n>>>>> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n>>>>> * second, it is good enough.\n>>>>> */\n>>>>> exif.setTimestamp(std::time(nullptr));\n>>>>> + std::vector thumbnail;\n>>>>> + generateThumbnail(source, thumbnail);\n>>>>> + exif.setThumbnail(thumbnail);\n>>>>> if (exif.generate() != 0)\n>>>>> LOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n>>>>>\n>>>>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n>>>>> index 3706cec..3894231 100644\n>>>>> --- a/src/android/jpeg/post_processor_jpeg.h\n>>>>> +++ b/src/android/jpeg/post_processor_jpeg.h\n>>>>> @@ -8,12 +8,13 @@\n>>>>> #define __ANDROID_POST_PROCESSOR_JPEG_H__\n>>>>>\n>>>>> #include \"../post_processor.h\"\n>>>>> +#include \"encoder_libjpeg.h\"\n>>>>> +#include \"thumbnailer.h\"\n>>>>>\n>>>>> #include \n>>>>>\n>>>>> #include \"libcamera/internal/buffer.h\"\n>>>>>\n>>>>> -class Encoder;\n>>>>> class CameraDevice;\n>>>>>\n>>>>> class PostProcessorJpeg : public PostProcessor\n>>>>> @@ -28,9 +29,14 @@ public:\n>>>>> CameraMetadata *metadata) override;\n>>>>>\n>>>>> private:\n>>>>> + void generateThumbnail(const libcamera::FrameBuffer &source,\n>>>>> + std::vector &thumbnail);\n>>>>> +\n>>>>> CameraDevice *const cameraDevice_;\n>>>>> std::unique_ptr encoder_;\n>>>>> libcamera::Size streamSize_;\n>>>>> + EncoderLibJpeg thumbnailEncoder_;\n>>>> Could you store this in a std::unique_ptr instead ? The reason\n>>>> is that we shouldn't hardcode usage of EncoderLibJpeg, in order to\n>>>> support different encoders later. You won't need to include\n>>>> encoder_libjpeg.h then, and can keep the Encoder forwared declaration.\n>>> I think this was already a unique_ptr in a previous iteration, and I\n>>> suggested moving it to directly store an instance of the libjpeg encoder.\n>>>\n>>> In this instance of encoding a thumbnail, when encoding a 160x160 image,\n>>> I would be weary about the overhead of setting up a hardware encode, and\n>>> I'd expect the preparation phases of that to be potentially more\n>>> expensive than just a software encode. Particularly as this has just\n>>> done a software rescale, so it would have to cache-flush, when it could\n>>> just use the host-cpu with a hot-cache. (ok, so perhaps later it might\n>>> use a different scaler too ... but then it's likely a different equation\n>>> anyway)\n>>>\n>>> I have not measured that of course, as we don't yet have a hw-jpeg\n>>> encode post-processor. It will be an interesting test in the future.\n>>>\n>>> But essentially, I think we're just as well leaving this as a single\n>>> instance of libjpeg for thumbnails. I think I recall the CrOS HAL using\n>>> libjpeg as well, but I haven't gone back to double-check.\n>> Fair enough, those are good points. I'm fine if the code is kept as-is\n>> (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.\n>>\n> Although either is fine to me, I wonder why the encoder for the\n> thumbnail is within the thumbnailer.\nIt is not within the encoder. It is inside the PostProcessorJpeg\n> Since the thumbnailer is currently being used for thumbnail, it seems\n> to be reasonable to move the encoder to it.\n> What do you think?\nIf you mean moving the thumbnail-encoder to `class Thumbnailer` - hmm, \nthat could also work I suppose. However,  I might not be very \nenthusiastic about it, as PostProcessorJpeg captures the entire flow at \none place : e.g. exif generation, creating a thumbnail, encoding it and \nfinally encoding the entire frame. I like this.\n\nIf others support moving the thumbnail-encoder to Thumbnailer, that's \nalso fine! (Also, could be done on top, as an independent refactor)\n>\n>>>>> + Thumbnailer thumbnailer_;\n>>>> This is good, as I don't expect different thumbnailer types.\n>>>>\n>>>>> };\n>>>>>\n>>>>> #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n>>>>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n>>>>> new file mode 100644\n>>>>> index 0000000..f880ffb\n>>>>> --- /dev/null\n>>>>> +++ b/src/android/jpeg/thumbnailer.cpp\n>>>>> @@ -0,0 +1,109 @@\n>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>> Wrong license.\n>>>>\n>>>>> +/*\n>>>>> + * Copyright (C) 2020, Google Inc.\n>>>>> + *\n>>>>> + * thumbnailer.cpp - Simple image thumbnailer\n>>>>> + */\n>>>>> +\n>>>>> +#include \"thumbnailer.h\"\n>>>>> +\n>>>>> +#include \n>>>>> +\n>>>>> +#include \"libcamera/internal/file.h\"\n>>>> Why do you need this header ?\n>>>>\n>>>>> +#include \"libcamera/internal/log.h\"\n>>>>> +\n>>>>> +using namespace libcamera;\n>>>>> +\n>>>>> +LOG_DEFINE_CATEGORY(Thumbnailer)\n>>>>> +\n>>>>> +Thumbnailer::Thumbnailer() : valid_(false)\n>>>> : valid_(false)\n>>>>\n>>>>> +{\n>>>>> +}\n>>>>> +\n>>>>> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)\n>>>>> +{\n>>>>> + sourceSize_ = sourceSize;\n>>>>> + pixelFormat_ = pixelFormat;\n>>>>> +\n>>>>> + if (pixelFormat_ != formats::NV12) {\n>>>>> + LOG (Thumbnailer, Error) << \"Failed to configure: Pixel Format \"\n>>>>> + << pixelFormat_.toString() << \" unsupported.\";\n>>>> LOG (Thumbnailer, Error)\n>>>> << \"Failed to configure: Pixel Format \"\n>>>> << pixelFormat_.toString() << \" unsupported.\";\n>>>>\n>>>>> + return;\n>>>>> + }\n>>>>> +\n>>>>> + targetSize_ = computeThumbnailSize();\n>>>>> +\n>>>>> + valid_ = true;\n>>>>> +}\n>>>>> +\n>>>>> +/*\n>>>>> + * The Exif specification recommends the width of the thumbnail to be a\n>>>>> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height\n>>>> s/mutiple/multiple/\n>>>>\n>>>>> + * keeping the aspect ratio same as of the source.\n>>>>> + */\n>>>>> +Size Thumbnailer::computeThumbnailSize()\n>>>>> +{\n>>>>> + unsigned int targetHeight;\n>>>>> + unsigned int targetWidth = 160;\n> nit: constexpr unsigned int kTargetWidth = 160;\n>\n>>>>> +\n>>>>> + targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;\n>>>>> +\n>>>>> + if (targetHeight & 1)\n>>>>> + targetHeight++;\n>>>>> +\n>>>>> + return Size(targetWidth, targetHeight);\n>>>>> +}\n>>>>> +\n>>>>> +void\n>>>>> +Thumbnailer::scaleBuffer(const FrameBuffer &source,\n>>>>> + std::vector &destination)\n>>>>> +{\n>>>>> + MappedFrameBuffer frame(&source, PROT_READ);\n>>>>> + if (!frame.isValid()) {\n>>>>> + LOG(Thumbnailer, Error) << \"Failed to map FrameBuffer : \"\n>>>>> + << strerror(frame.error());\n>>>> LOG(Thumbnailer, Error)\n>>>> << \"Failed to map FrameBuffer : \"\n>>>> << strerror(frame.error());\n>>>>\n>>>>> + return;\n>>>>> + }\n>>>>> +\n>>>>> + if (!valid_) {\n>>>>> + LOG(Thumbnailer, Error) << \"config is unconfigured or invalid.\";\n>>>>> + return;\n>>>>> + }\n>>>>> +\n>>>>> + const unsigned int sw = sourceSize_.width;\n>>>>> + const unsigned int sh = sourceSize_.height;\n>>>>> + const unsigned int tw = targetSize_.width;\n>>>>> + const unsigned int th = targetSize_.height;\n>>>>> +\n>>>>> + /* Image scaling block implementing nearest-neighbour algorithm. */\n>>>>> + unsigned char *src = static_cast(frame.maps()[0].data());\n>>>>> + unsigned char *src_c = src + sh * sw;\n>>>>> + unsigned char *src_cb, *src_cr;\n>>>>> + unsigned char *dst_y, *src_y;\n>>>>> +\n>>>>> + size_t dstSize = (th * tw) + ((th / 2) * tw);\n>>>>> + destination.reserve(dstSize);\n>>>> This should be\n>>>>\n>>>> destination.resize(dstSize);\n>>>>\n>>>> as reserve() allocates memory but doesn't change the size of the vector.\n>>>>\n>>>>> + unsigned char *dst = destination.data();\n>>>>> + unsigned char *dst_c = dst + th * tw;\n>>>>> +\n>>>>> + for (unsigned int y = 0; y < th; y += 2) {\n>>>>> + unsigned int sourceY = (sh * y + th / 2) / th;\n>>>>> +\n>>>>> + dst_y = dst + y * tw;\n>>>>> + src_y = src + sw * sourceY;\n>>>>> + src_cb = src_c + (sourceY / 2) * sw + 0;\n>>>>> + src_cr = src_c + (sourceY / 2) * sw + 1;\n>>>>> +\n>>>>> + for (unsigned int x = 0; x < tw; x += 2) {\n>>>>> + unsigned int sourceX = (sw * x + tw / 2) / tw;\n>>>>> +\n>>>>> + dst_y[x] = src_y[sourceX];\n>>>>> + dst_y[tw + x] = src_y[sw + sourceX];\n>>>>> + dst_y[x + 1] = src_y[sourceX + 1];\n>>>>> + dst_y[tw + x + 1] = src_y[sw + sourceX + 1];\n>>>>> +\n>>>>> + dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];\n>>>>> + dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];\n>>>>> + }\n>>>>> + }\n>>>>> + }\n>>>> Extra space.\n>>>>\n>>>>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n>>>>> new file mode 100644\n>>>>> index 0000000..b769619\n>>>>> --- /dev/null\n>>>>> +++ b/src/android/jpeg/thumbnailer.h\n>>>>> @@ -0,0 +1,37 @@\n>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>> Wrong license.\n>>>>\n>>>>> +/*\n>>>>> + * Copyright (C) 2020, Google Inc.\n>>>>> + *\n>>>>> + * thumbnailer.h - Simple image thumbnailer\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>>>>> + Thumbnailer();\n>>>>> +\n>>>>> + void configure(const libcamera::Size &sourceSize,\n>>>>> + libcamera::PixelFormat pixelFormat);\n>>>>> + void scaleBuffer(const libcamera::FrameBuffer &source,\n>>>>> + std::vector &dest);\n>>>> How about naming this createThumbnail() or generateThumbnail() ? And the\n>>>> function should be const.\n>>>>\n>>>>> + libcamera::Size size() const { return targetSize_; }\n> nit: to reduce unnecessary copy, let's return const reference.\n> const libcamera::Size& computeThumbanilSize() const;\n>\n> Best Regards,\n> -Hiro\n>>>>> +\n>>>>> +private:\n>>>>> + libcamera::Size computeThumbnailSize();\n>>>> This can be\n>>>>\n>>>> libcamera::Size computeThumbnailSize() const;\n>>>>\n>>>>> +\n>>>>> + libcamera::PixelFormat pixelFormat_;\n>>>>> + libcamera::Size sourceSize_;\n>>>>> + libcamera::Size targetSize_;\n>>>>> +\n>>>>> + bool valid_;\n>>>>> +\n>>>> No need for a blank line.\n>>>>\n>>>>> +};\n>>>>> +\n>>>>> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */\n>>>>> diff --git a/src/android/meson.build b/src/android/meson.build\n>>>>> index f72376a..3d4d3be 100644\n>>>>> --- a/src/android/meson.build\n>>>>> +++ b/src/android/meson.build\n>>>>> @@ -25,6 +25,7 @@ android_hal_sources = files([\n>>>>> 'jpeg/encoder_libjpeg.cpp',\n>>>>> 'jpeg/exif.cpp',\n>>>>> 'jpeg/post_processor_jpeg.cpp',\n>>>>> + 'jpeg/thumbnailer.cpp',\n>>>>> ])\n>>>>>\n>>>>> android_camera_metadata_sources = files([\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 7E71ABDB1E\n\tfor ;\n\tTue, 27 Oct 2020 07:40:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D51A062070;\n\tTue, 27 Oct 2020 08:40:30 +0100 (CET)","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 EAC5362053\n\tfor ;\n\tTue, 27 Oct 2020 08:40:28 +0100 (CET)"],"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=\"AiqsfJIJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1603784428; bh=Ap/XN1Eu6X3/A+6COzEB5EHntO5X8HZTVboGPFwZDn8=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=AiqsfJIJBOJ5RCxBrLBDogrGZwEcK7SHomuU482Nag3KP2cmMM/dJGNo08lFcmId1\n\toOVirHnaKtlYHIoqGq1URMLcZMYWD3oI6PJ65Dq7vvBWpf4ufQBynwyUzYmGEG3J0g\n\tSC8xWKsqm5HmWYAp8KyfsV1S3bRw4vSuaY7oz/Nr6JdZam6UglE4BifSCVRd6R+jP1\n\twdIOJ4FUOZ4xkMmalMIebo1uChLIglOmIt7TrpGanLocTg21SVujnF7tJEDSgC8maU\n\tB15b7qv8wW+5Eiu2lfP81X5z5pt20H1hSEyCaLX2aZ7TaVoebQxSOIQ3uM99rBwF1k\n\tf0SJEnCl54OcQ==","To":"Hirokazu Honda ,\n\tLaurent Pinchart ","References":"<20201026140134.44166-1-email@uajain.com>\n\t<20201026140134.44166-4-email@uajain.com>\n\t<20201026210533.GC3756@pendragon.ideasonboard.com>\n\t\n\t<20201026220818.GF3756@pendragon.ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Tue, 27 Oct 2020 13:10:24 +0530","Mime-Version":"1.0","In-Reply-To":"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","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-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13509,"web_url":"https://patchwork.libcamera.org/comment/13509/","msgid":"","date":"2020-10-27T08:04:00","subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Tue, Oct 27, 2020 at 4:40 PM Umang Jain wrote:\n>\n> Hi Hiro,\n>\n> On 10/27/20 10:51 AM, Hirokazu Honda wrote:\n> > Hi Umang,\n> >\n> > On Tue, Oct 27, 2020 at 7:09 AM Laurent Pinchart\n> > wrote:\n> >> Hi Kieran,\n> >>\n> >> On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:\n> >>> On 26/10/2020 21:05, Laurent Pinchart wrote:\n> >>>> On Mon, Oct 26, 2020 at 07:31:34PM +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/exif.cpp | 16 +++-\n> >>>>> src/android/jpeg/exif.h | 1 +\n> >>>>> src/android/jpeg/post_processor_jpeg.cpp | 35 +++++++-\n> >>>>> src/android/jpeg/post_processor_jpeg.h | 8 +-\n> >>>>> src/android/jpeg/thumbnailer.cpp | 109 +++++++++++++++++++++++\n> >>>>> src/android/jpeg/thumbnailer.h | 37 ++++++++\n> >>>>> src/android/meson.build | 1 +\n> >>>>> 7 files changed, 204 insertions(+), 3 deletions(-)\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/exif.cpp b/src/android/jpeg/exif.cpp\n> >>>>> index d21534a..24197bd 100644\n> >>>>> --- a/src/android/jpeg/exif.cpp\n> >>>>> +++ b/src/android/jpeg/exif.cpp\n> >>>>> @@ -75,8 +75,16 @@ Exif::~Exif()\n> >>>>> if (exifData_)\n> >>>>> free(exifData_);\n> >>>>>\n> >>>>> - if (data_)\n> >>>>> + if (data_) {\n> >>>>> + /*\n> >>>>> + * Reset thumbnail data to avoid getting double-freed by\n> >>>>> + * libexif. It is owned by the caller (i.e. PostProcessorJpeg).\n> >>>>> + */\n> >>>>> + data_->data = nullptr;\n> >>>>> + data_->size = 0;\n> >>>>> +\n> >>>>> exif_data_unref(data_);\n> >>>>> + }\n> >>>>>\n> >>>>> if (mem_)\n> >>>>> exif_mem_unref(mem_);\n> >>>>> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)\n> >>>>> setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\n> >>>>> }\n> >>>>>\n> >>>>> +void Exif::setThumbnail(std::vector &thumbnail)\n> >>>>> +{\n> >>>>> + data_->data = thumbnail.data();\n> >>>>> + data_->size = thumbnail.size();\n> >>>>> +}\n> >>>>> +\n> >>>>> [[nodiscard]] int Exif::generate()\n> >>>>> {\n> >>>>> if (exifData_) {\n> >>>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> >>>>> index 12c27b6..bd54a31 100644\n> >>>>> --- a/src/android/jpeg/exif.h\n> >>>>> +++ b/src/android/jpeg/exif.h\n> >>>>> @@ -26,6 +26,7 @@ public:\n> >>>>>\n> >>>>> void setOrientation(int orientation);\n> >>>>> void setSize(const libcamera::Size &size);\n> >>>>> + void setThumbnail(std::vector &thumbnail);\n> >>>> You can pass a Span as the setThumbnail() function\n> >>>> shouldn't care what storage container is used.\n> >>>>\n> >>>> It's a bit of a dangerous API, as it will store the pointer internally,\n> >>>> without ensuring that the caller keeps the thumbnail valid after the\n> >>>> call returns. It's fine, but maybe a comment above the\n> >>>> Exif::setThumbnail() functions to state that the thumbnail must remain\n> >>>> valid until the Exif object is destroyed would be a good thing.\n> >>> I think the comment will help indeed. The only alternative would be to\n> >>> pass it into generate(), but and refactor generate() to return the span\n> >>> ... but that's more effort that we need right now. So just a comment\n> >>> will do ;-)\n> >>>\n> >>>\n> >>>\n> >>>>> void setTimestamp(time_t timestamp);\n> >>>>>\n> >>>>> libcamera::Span data() const { return { exifData_, size_ }; }\n> >>>> I would have moved this to a separate patch.\n> >>> Yes, I'd keep the exif extension for setting the thumbnail, and the\n> >>> usage separate.\n> >>>\n> >>>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> >>>>> index c56f1b2..416e831 100644\n> >>>>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> >>>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> >>>>> @@ -9,7 +9,6 @@\n> >>>>>\n> >>>>> #include \"../camera_device.h\"\n> >>>>> #include \"../camera_metadata.h\"\n> >>>>> -#include \"encoder_libjpeg.h\"\n> >>>>> #include \"exif.h\"\n> >>>>>\n> >>>>> #include \n> >>>>> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> >>>>> }\n> >>>>>\n> >>>>> streamSize_ = outCfg.size;\n> >>>>> +\n> >>>>> + thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> >>>>> + StreamConfiguration thCfg = inCfg;\n> >>>>> + thCfg.size = thumbnailer_.size();\n> >>>>> + if (thumbnailEncoder_.configure(thCfg) != 0) {\n> >>>>> + LOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> >>>>> + return -EINVAL;\n> >>>>> + }\n> >>>>> +\n> >>>>> encoder_ = std::make_unique();\n> >>>>>\n> >>>>> return encoder_->configure(inCfg);\n> >>>>> }\n> >>>>>\n> >>>>> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> >>>>> + std::vector &thumbnail)\n> > std::vector* thumbnail\n> > We would rather use a pointer than reference if a function changes it.\n> > Same for other places.\n> Ack.\n> >>>>> +{\n> >>>>> + /* Stores the raw scaled-down thumbnail bytes. */\n> >>>>> + std::vector rawThumbnail;\n> >>>>> +\n> >>>>> + thumbnailer_.scaleBuffer(source, rawThumbnail);\n> >>>>> +\n> >>>>> + if (rawThumbnail.data()) {\n> >>>> This should check for ! .empty() (see additional comments below).\n> >>>>\n> >>>>> + thumbnail.reserve(rawThumbnail.capacity());\n> >>>> You should call thumbnail.resize(), and use size() instead of capacity()\n> >>>> below, as reserve() allocates memory but doesn't change the size of the\n> >>>> vector, so it's semantically dangerous to write to the reserved storage\n> >>>> space if it hasn't been marked as in use with .resize().\n> >>>>\n> >>>>> +\n> >>>>> + int jpeg_size = thumbnailEncoder_.encode(\n> >>>>> + { rawThumbnail.data(), rawThumbnail.capacity() },\n> >>>>> + { thumbnail.data(), thumbnail.capacity() },\n> >>>> Just pass rawThumbnail and thumbnail, there's an implicit constructor\n> >>>> for Span that will turn them into a span using .data() and .size().\n> >>>>\n> >>>>> + { });\n> >>>>> + thumbnail.resize(jpeg_size);\n> >>>>> +\n> >>>>> + LOG(JPEG, Info) << \"Thumbnail compress returned \"\n> >>>>> + << jpeg_size << \" bytes\";\n> >>>> When log messages don't fit on one line, we usually wrap them as\n> >>>>\n> >>>> LOG(JPEG, Info)\n> >>>> << \"Thumbnail compress returned \"\n> >>>> << jpeg_size << \" bytes\";\n> >>>>\n> >>>> And maybe debug instead of info ?\n> >>>>\n> >>>>> + }\n> >>>>> +}\n> > Shall this function handle a failure of jpeg encode?\n> Ideally it should but I am wondering in what style. I would prefer that\n> checking for the (thumbnail data != 0) before calling\n> Exif::setThumbnail() below. Since, when the encoding fails, jpeg_size\n> will get 0, and the thumbnail vector(placeholder for our thumbnail data)\n> will get resized to '0'.\n\nAck.\n\n> >>>>> +\n> >>>>> int PostProcessorJpeg::process(const FrameBuffer &source,\n> >>>>> Span destination,\n> >>>>> CameraMetadata *metadata)\n> >>>>> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> >>>>> * second, it is good enough.\n> >>>>> */\n> >>>>> exif.setTimestamp(std::time(nullptr));\n> >>>>> + std::vector thumbnail;\n> >>>>> + generateThumbnail(source, thumbnail);\n> >>>>> + exif.setThumbnail(thumbnail);\n> >>>>> if (exif.generate() != 0)\n> >>>>> LOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> >>>>>\n> >>>>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> >>>>> index 3706cec..3894231 100644\n> >>>>> --- a/src/android/jpeg/post_processor_jpeg.h\n> >>>>> +++ b/src/android/jpeg/post_processor_jpeg.h\n> >>>>> @@ -8,12 +8,13 @@\n> >>>>> #define __ANDROID_POST_PROCESSOR_JPEG_H__\n> >>>>>\n> >>>>> #include \"../post_processor.h\"\n> >>>>> +#include \"encoder_libjpeg.h\"\n> >>>>> +#include \"thumbnailer.h\"\n> >>>>>\n> >>>>> #include \n> >>>>>\n> >>>>> #include \"libcamera/internal/buffer.h\"\n> >>>>>\n> >>>>> -class Encoder;\n> >>>>> class CameraDevice;\n> >>>>>\n> >>>>> class PostProcessorJpeg : public PostProcessor\n> >>>>> @@ -28,9 +29,14 @@ public:\n> >>>>> CameraMetadata *metadata) override;\n> >>>>>\n> >>>>> private:\n> >>>>> + void generateThumbnail(const libcamera::FrameBuffer &source,\n> >>>>> + std::vector &thumbnail);\n> >>>>> +\n> >>>>> CameraDevice *const cameraDevice_;\n> >>>>> std::unique_ptr encoder_;\n> >>>>> libcamera::Size streamSize_;\n> >>>>> + EncoderLibJpeg thumbnailEncoder_;\n> >>>> Could you store this in a std::unique_ptr instead ? The reason\n> >>>> is that we shouldn't hardcode usage of EncoderLibJpeg, in order to\n> >>>> support different encoders later. You won't need to include\n> >>>> encoder_libjpeg.h then, and can keep the Encoder forwared declaration.\n> >>> I think this was already a unique_ptr in a previous iteration, and I\n> >>> suggested moving it to directly store an instance of the libjpeg encoder.\n> >>>\n> >>> In this instance of encoding a thumbnail, when encoding a 160x160 image,\n> >>> I would be weary about the overhead of setting up a hardware encode, and\n> >>> I'd expect the preparation phases of that to be potentially more\n> >>> expensive than just a software encode. Particularly as this has just\n> >>> done a software rescale, so it would have to cache-flush, when it could\n> >>> just use the host-cpu with a hot-cache. (ok, so perhaps later it might\n> >>> use a different scaler too ... but then it's likely a different equation\n> >>> anyway)\n> >>>\n> >>> I have not measured that of course, as we don't yet have a hw-jpeg\n> >>> encode post-processor. It will be an interesting test in the future.\n> >>>\n> >>> But essentially, I think we're just as well leaving this as a single\n> >>> instance of libjpeg for thumbnails. I think I recall the CrOS HAL using\n> >>> libjpeg as well, but I haven't gone back to double-check.\n> >> Fair enough, those are good points. I'm fine if the code is kept as-is\n> >> (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.\n> >>\n> > Although either is fine to me, I wonder why the encoder for the\n> > thumbnail is within the thumbnailer.\n> It is not within the encoder. It is inside the PostProcessorJpeg\n\nSorry, I meant why ... is NOT within ....\n\n> > Since the thumbnailer is currently being used for thumbnail, it seems\n> > to be reasonable to move the encoder to it.\n> > What do you think?\n> If you mean moving the thumbnail-encoder to `class Thumbnailer` - hmm,\n> that could also work I suppose. However, I might not be very\n> enthusiastic about it, as PostProcessorJpeg captures the entire flow at\n> one place : e.g. exif generation, creating a thumbnail, encoding it and\n> finally encoding the entire frame. I like this.\n>\n> If others support moving the thumbnail-encoder to Thumbnailer, that's\n> also fine! (Also, could be done on top, as an independent refactor)\n\nI would like to hear others opinions here.\nI would move it to Thumbnailer, so that\n1.) a reader can understand at a glance the encoder is used for\ngenerating thumbnails, and\n2.) thumbnail is always meant JPEG here and then it makes more sense\nthat thumbnailer outputs JPEG.\n\nEspecially regarding 2, will the thumbnail possibly be other formats?\n\nBest Regards,\n-Hiro\n> >\n> >>>>> + Thumbnailer thumbnailer_;\n> >>>> This is good, as I don't expect different thumbnailer types.\n> >>>>\n> >>>>> };\n> >>>>>\n> >>>>> #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n> >>>>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> >>>>> new file mode 100644\n> >>>>> index 0000000..f880ffb\n> >>>>> --- /dev/null\n> >>>>> +++ b/src/android/jpeg/thumbnailer.cpp\n> >>>>> @@ -0,0 +1,109 @@\n> >>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>>> Wrong license.\n> >>>>\n> >>>>> +/*\n> >>>>> + * Copyright (C) 2020, Google Inc.\n> >>>>> + *\n> >>>>> + * thumbnailer.cpp - Simple image thumbnailer\n> >>>>> + */\n> >>>>> +\n> >>>>> +#include \"thumbnailer.h\"\n> >>>>> +\n> >>>>> +#include \n> >>>>> +\n> >>>>> +#include \"libcamera/internal/file.h\"\n> >>>> Why do you need this header ?\n> >>>>\n> >>>>> +#include \"libcamera/internal/log.h\"\n> >>>>> +\n> >>>>> +using namespace libcamera;\n> >>>>> +\n> >>>>> +LOG_DEFINE_CATEGORY(Thumbnailer)\n> >>>>> +\n> >>>>> +Thumbnailer::Thumbnailer() : valid_(false)\n> >>>> : valid_(false)\n> >>>>\n> >>>>> +{\n> >>>>> +}\n> >>>>> +\n> >>>>> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)\n> >>>>> +{\n> >>>>> + sourceSize_ = sourceSize;\n> >>>>> + pixelFormat_ = pixelFormat;\n> >>>>> +\n> >>>>> + if (pixelFormat_ != formats::NV12) {\n> >>>>> + LOG (Thumbnailer, Error) << \"Failed to configure: Pixel Format \"\n> >>>>> + << pixelFormat_.toString() << \" unsupported.\";\n> >>>> LOG (Thumbnailer, Error)\n> >>>> << \"Failed to configure: Pixel Format \"\n> >>>> << pixelFormat_.toString() << \" unsupported.\";\n> >>>>\n> >>>>> + return;\n> >>>>> + }\n> >>>>> +\n> >>>>> + targetSize_ = computeThumbnailSize();\n> >>>>> +\n> >>>>> + valid_ = true;\n> >>>>> +}\n> >>>>> +\n> >>>>> +/*\n> >>>>> + * The Exif specification recommends the width of the thumbnail to be a\n> >>>>> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height\n> >>>> s/mutiple/multiple/\n> >>>>\n> >>>>> + * keeping the aspect ratio same as of the source.\n> >>>>> + */\n> >>>>> +Size Thumbnailer::computeThumbnailSize()\n> >>>>> +{\n> >>>>> + unsigned int targetHeight;\n> >>>>> + unsigned int targetWidth = 160;\n> > nit: constexpr unsigned int kTargetWidth = 160;\n> >\n> >>>>> +\n> >>>>> + targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;\n> >>>>> +\n> >>>>> + if (targetHeight & 1)\n> >>>>> + targetHeight++;\n> >>>>> +\n> >>>>> + return Size(targetWidth, targetHeight);\n> >>>>> +}\n> >>>>> +\n> >>>>> +void\n> >>>>> +Thumbnailer::scaleBuffer(const FrameBuffer &source,\n> >>>>> + std::vector &destination)\n> >>>>> +{\n> >>>>> + MappedFrameBuffer frame(&source, PROT_READ);\n> >>>>> + if (!frame.isValid()) {\n> >>>>> + LOG(Thumbnailer, Error) << \"Failed to map FrameBuffer : \"\n> >>>>> + << strerror(frame.error());\n> >>>> LOG(Thumbnailer, Error)\n> >>>> << \"Failed to map FrameBuffer : \"\n> >>>> << strerror(frame.error());\n> >>>>\n> >>>>> + return;\n> >>>>> + }\n> >>>>> +\n> >>>>> + if (!valid_) {\n> >>>>> + LOG(Thumbnailer, Error) << \"config is unconfigured or invalid.\";\n> >>>>> + return;\n> >>>>> + }\n> >>>>> +\n> >>>>> + const unsigned int sw = sourceSize_.width;\n> >>>>> + const unsigned int sh = sourceSize_.height;\n> >>>>> + const unsigned int tw = targetSize_.width;\n> >>>>> + const unsigned int th = targetSize_.height;\n> >>>>> +\n> >>>>> + /* Image scaling block implementing nearest-neighbour algorithm. */\n> >>>>> + unsigned char *src = static_cast(frame.maps()[0].data());\n> >>>>> + unsigned char *src_c = src + sh * sw;\n> >>>>> + unsigned char *src_cb, *src_cr;\n> >>>>> + unsigned char *dst_y, *src_y;\n> >>>>> +\n> >>>>> + size_t dstSize = (th * tw) + ((th / 2) * tw);\n> >>>>> + destination.reserve(dstSize);\n> >>>> This should be\n> >>>>\n> >>>> destination.resize(dstSize);\n> >>>>\n> >>>> as reserve() allocates memory but doesn't change the size of the vector.\n> >>>>\n> >>>>> + unsigned char *dst = destination.data();\n> >>>>> + unsigned char *dst_c = dst + th * tw;\n> >>>>> +\n> >>>>> + for (unsigned int y = 0; y < th; y += 2) {\n> >>>>> + unsigned int sourceY = (sh * y + th / 2) / th;\n> >>>>> +\n> >>>>> + dst_y = dst + y * tw;\n> >>>>> + src_y = src + sw * sourceY;\n> >>>>> + src_cb = src_c + (sourceY / 2) * sw + 0;\n> >>>>> + src_cr = src_c + (sourceY / 2) * sw + 1;\n> >>>>> +\n> >>>>> + for (unsigned int x = 0; x < tw; x += 2) {\n> >>>>> + unsigned int sourceX = (sw * x + tw / 2) / tw;\n> >>>>> +\n> >>>>> + dst_y[x] = src_y[sourceX];\n> >>>>> + dst_y[tw + x] = src_y[sw + sourceX];\n> >>>>> + dst_y[x + 1] = src_y[sourceX + 1];\n> >>>>> + dst_y[tw + x + 1] = src_y[sw + sourceX + 1];\n> >>>>> +\n> >>>>> + dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];\n> >>>>> + dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];\n> >>>>> + }\n> >>>>> + }\n> >>>>> + }\n> >>>> Extra space.\n> >>>>\n> >>>>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> >>>>> new file mode 100644\n> >>>>> index 0000000..b769619\n> >>>>> --- /dev/null\n> >>>>> +++ b/src/android/jpeg/thumbnailer.h\n> >>>>> @@ -0,0 +1,37 @@\n> >>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>>> Wrong license.\n> >>>>\n> >>>>> +/*\n> >>>>> + * Copyright (C) 2020, Google Inc.\n> >>>>> + *\n> >>>>> + * thumbnailer.h - Simple image thumbnailer\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> >>>>> + Thumbnailer();\n> >>>>> +\n> >>>>> + void configure(const libcamera::Size &sourceSize,\n> >>>>> + libcamera::PixelFormat pixelFormat);\n> >>>>> + void scaleBuffer(const libcamera::FrameBuffer &source,\n> >>>>> + std::vector &dest);\n> >>>> How about naming this createThumbnail() or generateThumbnail() ? And the\n> >>>> function should be const.\n> >>>>\n> >>>>> + libcamera::Size size() const { return targetSize_; }\n> > nit: to reduce unnecessary copy, let's return const reference.\n> > const libcamera::Size& computeThumbanilSize() const;\n> >\n> > Best Regards,\n> > -Hiro\n> >>>>> +\n> >>>>> +private:\n> >>>>> + libcamera::Size computeThumbnailSize();\n> >>>> This can be\n> >>>>\n> >>>> libcamera::Size computeThumbnailSize() const;\n> >>>>\n> >>>>> +\n> >>>>> + libcamera::PixelFormat pixelFormat_;\n> >>>>> + libcamera::Size sourceSize_;\n> >>>>> + libcamera::Size targetSize_;\n> >>>>> +\n> >>>>> + bool valid_;\n> >>>>> +\n> >>>> No need for a blank line.\n> >>>>\n> >>>>> +};\n> >>>>> +\n> >>>>> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */\n> >>>>> diff --git a/src/android/meson.build b/src/android/meson.build\n> >>>>> index f72376a..3d4d3be 100644\n> >>>>> --- a/src/android/meson.build\n> >>>>> +++ b/src/android/meson.build\n> >>>>> @@ -25,6 +25,7 @@ android_hal_sources = files([\n> >>>>> 'jpeg/encoder_libjpeg.cpp',\n> >>>>> 'jpeg/exif.cpp',\n> >>>>> 'jpeg/post_processor_jpeg.cpp',\n> >>>>> + 'jpeg/thumbnailer.cpp',\n> >>>>> ])\n> >>>>>\n> >>>>> android_camera_metadata_sources = files([\n> >> --\n> >> Regards,\n> >>\n> >> Laurent Pinchart\n> >> _______________________________________________\n> >> libcamera-devel mailing list\n> >> libcamera-devel@lists.libcamera.org\n> >> https://lists.libcamera.org/listinfo/libcamera-devel\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 00A93C3B5C\n\tfor ;\n\tTue, 27 Oct 2020 08:04:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7BB8362074;\n\tTue, 27 Oct 2020 09:04:14 +0100 (CET)","from mail-ed1-x541.google.com (mail-ed1-x541.google.com\n\t[IPv6:2a00:1450:4864:20::541])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 790646052E\n\tfor ;\n\tTue, 27 Oct 2020 09:04:12 +0100 (CET)","by mail-ed1-x541.google.com with SMTP id o18so456944edq.4\n\tfor ;\n\tTue, 27 Oct 2020 01:04:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"H02qKKrR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=1ZcjcxRSYuH2z9drKRlUEAw7ASKCdPF5P9iKx1pbJLk=;\n\tb=H02qKKrRcb8YhCXRAnFlwzWtTNQYO70kstnLfaIKOJ8m7roXBgeHfXebYfAUctZB3f\n\t7VWEpQaKDyja2cWOd7VdPFpqLAFdqGr/ziedL9w6ZcEqfOIiMW/fl4rGMTidczxfx5xH\n\t2R7IFmML4kBUmSxU9RtK7BlTISKa8SwIUzt0A=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=1ZcjcxRSYuH2z9drKRlUEAw7ASKCdPF5P9iKx1pbJLk=;\n\tb=IjniT7AbW+JwTs5GShNvvaRwoX0Y1192ieLWkpTZBTA5cHn19qQmkkBWqee8y9YreB\n\trT2QGwAn8UtGaUIyq+QccFCYddFSm6GVLqY4gZl35mkmzXDy9UlRrpH54RpPULRrbdO/\n\tFSKqAR0UkMOvsN6ciq61+9FL/6v45SsRFIbdxtSBK/5O5YmmK7YNUVaxEKUTkkht8+tK\n\tr8+gOL9bXfOvD4E5FbddRaqO2QdQFKOqvul6SrNKh9gH32KMiWFTEjKvbviGEEC2oaCF\n\tg9YHIGizvRRh3JCh51OdsAea+gDiGnecXno2pWlrGMRu0ENpjPn/GaMGt4dQktgRxgnG\n\tX/OQ==","X-Gm-Message-State":"AOAM533XFXqpqrVsutMS+A76CL3bi9QwLl4yK2bh7d39ddO53jykHecN\n\t2TjW2AsO4uphJqact2ikWWdoespqpgXYES5eEBXW/F4Kw/M=","X-Google-Smtp-Source":"ABdhPJxJNLlfWCtTsiIK0eVqvGGl/qzQnyI+RaB8fwBe6l+X9bIfig8WjL9+MuGTLOK9vREZi147PqZ8UeIVwW1hAow=","X-Received":"by 2002:a05:6402:d0d:: with SMTP id\n\teb13mr967442edb.244.1603785851646; \n\tTue, 27 Oct 2020 01:04:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20201026140134.44166-1-email@uajain.com>\n\t<20201026140134.44166-4-email@uajain.com>\n\t<20201026210533.GC3756@pendragon.ideasonboard.com>\n\t\n\t<20201026220818.GF3756@pendragon.ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 27 Oct 2020 17:04:00 +0900","Message-ID":"","To":"Umang Jain ","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","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":13518,"web_url":"https://patchwork.libcamera.org/comment/13518/","msgid":"","date":"2020-10-27T20:40:46","subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Laurent,\n\nOn 10/27/20 3:38 AM, Laurent Pinchart wrote:\n> Hi Kieran,\n>\n> On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:\n>> On 26/10/2020 21:05, Laurent Pinchart wrote:\n>>> On Mon, Oct 26, 2020 at 07:31:34PM +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/exif.cpp | 16 +++-\n>>>> src/android/jpeg/exif.h | 1 +\n>>>> src/android/jpeg/post_processor_jpeg.cpp | 35 +++++++-\n>>>> src/android/jpeg/post_processor_jpeg.h | 8 +-\n>>>> src/android/jpeg/thumbnailer.cpp | 109 +++++++++++++++++++++++\n>>>> src/android/jpeg/thumbnailer.h | 37 ++++++++\n>>>> src/android/meson.build | 1 +\n>>>> 7 files changed, 204 insertions(+), 3 deletions(-)\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/exif.cpp b/src/android/jpeg/exif.cpp\n>>>> index d21534a..24197bd 100644\n>>>> --- a/src/android/jpeg/exif.cpp\n>>>> +++ b/src/android/jpeg/exif.cpp\n>>>> @@ -75,8 +75,16 @@ Exif::~Exif()\n>>>> \tif (exifData_)\n>>>> \t\tfree(exifData_);\n>>>> \n>>>> -\tif (data_)\n>>>> +\tif (data_) {\n>>>> +\t\t/*\n>>>> +\t\t * Reset thumbnail data to avoid getting double-freed by\n>>>> +\t\t * libexif. It is owned by the caller (i.e. PostProcessorJpeg).\n>>>> +\t\t */\n>>>> +\t\tdata_->data = nullptr;\n>>>> +\t\tdata_->size = 0;\n>>>> +\n>>>> \t\texif_data_unref(data_);\n>>>> +\t}\n>>>> \n>>>> \tif (mem_)\n>>>> \t\texif_mem_unref(mem_);\n>>>> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)\n>>>> \tsetShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\n>>>> }\n>>>> \n>>>> +void Exif::setThumbnail(std::vector &thumbnail)\n>>>> +{\n>>>> +\tdata_->data = thumbnail.data();\n>>>> +\tdata_->size = thumbnail.size();\n>>>> +}\n>>>> +\n>>>> [[nodiscard]] int Exif::generate()\n>>>> {\n>>>> \tif (exifData_) {\n>>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n>>>> index 12c27b6..bd54a31 100644\n>>>> --- a/src/android/jpeg/exif.h\n>>>> +++ b/src/android/jpeg/exif.h\n>>>> @@ -26,6 +26,7 @@ public:\n>>>> \n>>>> \tvoid setOrientation(int orientation);\n>>>> \tvoid setSize(const libcamera::Size &size);\n>>>> +\tvoid setThumbnail(std::vector &thumbnail);\n>>> You can pass a Span as the setThumbnail() function\n>>> shouldn't care what storage container is used.\n>>>\n>>> It's a bit of a dangerous API, as it will store the pointer internally,\n>>> without ensuring that the caller keeps the thumbnail valid after the\n>>> call returns. It's fine, but maybe a comment above the\n>>> Exif::setThumbnail() functions to state that the thumbnail must remain\n>>> valid until the Exif object is destroyed would be a good thing.\n>> I think the comment will help indeed. The only alternative would be to\n>> pass it into generate(), but and refactor generate() to return the span\n>> ... but that's more effort that we need right now. So just a comment\n>> will do ;-)\n>>\n>>\n>>\n>>>> \tvoid setTimestamp(time_t timestamp);\n>>>> \n>>>> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n>>> I would have moved this to a separate patch.\n>>\n>> Yes, I'd keep the exif extension for setting the thumbnail, and the\n>> usage separate.\n>>\n>>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n>>>> index c56f1b2..416e831 100644\n>>>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n>>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>>>> @@ -9,7 +9,6 @@\n>>>> \n>>>> #include \"../camera_device.h\"\n>>>> #include \"../camera_metadata.h\"\n>>>> -#include \"encoder_libjpeg.h\"\n>>>> #include \"exif.h\"\n>>>> \n>>>> #include \n>>>> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n>>>> \t}\n>>>> \n>>>> \tstreamSize_ = outCfg.size;\n>>>> +\n>>>> +\tthumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n>>>> +\tStreamConfiguration thCfg = inCfg;\n>>>> +\tthCfg.size = thumbnailer_.size();\n>>>> +\tif (thumbnailEncoder_.configure(thCfg) != 0) {\n>>>> +\t\tLOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n>>>> +\t\treturn -EINVAL;\n>>>> +\t}\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 std::vector &thumbnail)\n>>>> +{\n>>>> +\t/* Stores the raw scaled-down thumbnail bytes. */\n>>>> +\tstd::vector rawThumbnail;\n>>>> +\n>>>> +\tthumbnailer_.scaleBuffer(source, rawThumbnail);\n>>>> +\n>>>> +\tif (rawThumbnail.data()) {\n>>> This should check for ! .empty() (see additional comments below).\n>>>\n>>>> +\t\tthumbnail.reserve(rawThumbnail.capacity());\n>>> You should call thumbnail.resize(), and use size() instead of capacity()\n>>> below, as reserve() allocates memory but doesn't change the size of the\n>>> vector, so it's semantically dangerous to write to the reserved storage\n>>> space if it hasn't been marked as in use with .resize().\n>>>\n>>>> +\n>>>> +\t\tint jpeg_size = thumbnailEncoder_.encode(\n>>>> +\t\t\t\t{ rawThumbnail.data(), rawThumbnail.capacity() },\n>>>> +\t\t\t\t{ thumbnail.data(), thumbnail.capacity() },\n>>> Just pass rawThumbnail and thumbnail, there's an implicit constructor\n>>> for Span that will turn them into a span using .data() and .size().\n>>>\n>>>> +\t\t\t\t{ });\n>>>> +\t\tthumbnail.resize(jpeg_size);\n>>>> +\n>>>> +\t\tLOG(JPEG, Info) << \"Thumbnail compress returned \"\n>>>> +\t\t\t\t<< jpeg_size << \" bytes\";\n>>> When log messages don't fit on one line, we usually wrap them as\n>>>\n>>> \t\tLOG(JPEG, Info)\n>>> \t\t\t<< \"Thumbnail compress returned \"\n>>> \t\t\t<< jpeg_size << \" bytes\";\n>>>\n>>> And maybe debug instead of info ?\n>>>\n>>>> +\t}\n>>>> +}\n>>>> +\n>>>> int PostProcessorJpeg::process(const FrameBuffer &source,\n>>>> \t\t\t Span destination,\n>>>> \t\t\t CameraMetadata *metadata)\n>>>> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n>>>> \t * second, it is good enough.\n>>>> \t */\n>>>> \texif.setTimestamp(std::time(nullptr));\n>>>> +\tstd::vector thumbnail;\n>>>> +\tgenerateThumbnail(source, thumbnail);\n>>>> +\texif.setThumbnail(thumbnail);\n>>>> \tif (exif.generate() != 0)\n>>>> \t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n>>>> \n>>>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n>>>> index 3706cec..3894231 100644\n>>>> --- a/src/android/jpeg/post_processor_jpeg.h\n>>>> +++ b/src/android/jpeg/post_processor_jpeg.h\n>>>> @@ -8,12 +8,13 @@\n>>>> #define __ANDROID_POST_PROCESSOR_JPEG_H__\n>>>> \n>>>> #include \"../post_processor.h\"\n>>>> +#include \"encoder_libjpeg.h\"\n>>>> +#include \"thumbnailer.h\"\n>>>> \n>>>> #include \n>>>> \n>>>> #include \"libcamera/internal/buffer.h\"\n>>>> \n>>>> -class Encoder;\n>>>> class CameraDevice;\n>>>> \n>>>> class PostProcessorJpeg : public PostProcessor\n>>>> @@ -28,9 +29,14 @@ public:\n>>>> \t\t CameraMetadata *metadata) override;\n>>>> \n>>>> private:\n>>>> +\tvoid generateThumbnail(const libcamera::FrameBuffer &source,\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>>> Could you store this in a std::unique_ptr instead ? The reason\n>>> is that we shouldn't hardcode usage of EncoderLibJpeg, in order to\n>>> support different encoders later. You won't need to include\n>>> encoder_libjpeg.h then, and can keep the Encoder forwared declaration.\n>> I think this was already a unique_ptr in a previous iteration, and I\n>> suggested moving it to directly store an instance of the libjpeg encoder.\n>>\n>> In this instance of encoding a thumbnail, when encoding a 160x160 image,\n>> I would be weary about the overhead of setting up a hardware encode, and\n>> I'd expect the preparation phases of that to be potentially more\n>> expensive than just a software encode. Particularly as this has just\n>> done a software rescale, so it would have to cache-flush, when it could\n>> just use the host-cpu with a hot-cache. (ok, so perhaps later it might\n>> use a different scaler too ... but then it's likely a different equation\n>> anyway)\n>>\n>> I have not measured that of course, as we don't yet have a hw-jpeg\n>> encode post-processor. It will be an interesting test in the future.\n>>\n>> But essentially, I think we're just as well leaving this as a single\n>> instance of libjpeg for thumbnails. I think I recall the CrOS HAL using\n>> libjpeg as well, but I haven't gone back to double-check.\n> Fair enough, those are good points. I'm fine if the code is kept as-is\n> (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.\nDo you specfically suggest using unique_ptr, to make use of forward \ndeclaration, in this instance of EncoderLibJpeg(see a diff below)? I am \ncurious to ask, because in order to make it work with incomplete \ntypes(i.e. forward declared), I need to insert destructor declaration in \npost_processor_jpeg.h and define a (blank)destructor in \npost_processor_jpeg.cpp\n\nI learnt this and can see it in action. Ref: \nhttps://stackoverflow.com/a/42158611\n\n-----\ndiff --git a/src/android/jpeg/post_processor_jpeg.h \nb/src/android/jpeg/post_processor_jpeg.h\nindex 3706cec..8b36a21 100644\n--- a/src/android/jpeg/post_processor_jpeg.h\n+++ b/src/android/jpeg/post_processor_jpeg.h\n@@ -8,12 +8,14 @@\n  #define __ANDROID_POST_PROCESSOR_JPEG_H__\n\n  #include \"../post_processor.h\"\n+#include \"thumbnailer.h\"\n\n  #include \n\n  #include \"libcamera/internal/buffer.h\"\n\n  class Encoder;\n+class EncoderLibJpeg;\n  class CameraDevice;\n\n  class PostProcessorJpeg : public PostProcessor\n@@ -28,9 +30,14 @@ public:\n                     CameraMetadata *metadata) override;\n\n  private:\n+       void generateThumbnail(const libcamera::FrameBuffer &source,\n+                              std::vector *thumbnail);\n+\n         CameraDevice *const cameraDevice_;\n         std::unique_ptr encoder_;\n         libcamera::Size streamSize_;\n+       std::unique_ptr thumbnailEncoder_;\n+       Thumbnailer thumbnailer_;\n  };\n\n>\n>>>> +\tThumbnailer thumbnailer_;\n>>> This is good, as I don't expect different thumbnailer types.\n>>>\n>>>> };\n>>>> \n>>>> #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n>>>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n>>>> new file mode 100644\n>>>> index 0000000..f880ffb\n>>>> --- /dev/null\n>>>> +++ b/src/android/jpeg/thumbnailer.cpp\n>>>> @@ -0,0 +1,109 @@\n>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>> Wrong license.\n>>>\n>>>> +/*\n>>>> + * Copyright (C) 2020, Google Inc.\n>>>> + *\n>>>> + * thumbnailer.cpp - Simple image thumbnailer\n>>>> + */\n>>>> +\n>>>> +#include \"thumbnailer.h\"\n>>>> +\n>>>> +#include \n>>>> +\n>>>> +#include \"libcamera/internal/file.h\"\n>>> Why do you need this header ?\n>>>\n>>>> +#include \"libcamera/internal/log.h\"\n>>>> +\n>>>> +using namespace libcamera;\n>>>> +\n>>>> +LOG_DEFINE_CATEGORY(Thumbnailer)\n>>>> +\n>>>> +Thumbnailer::Thumbnailer() : valid_(false)\n>>> \t: valid_(false)\n>>>\n>>>> +{\n>>>> +}\n>>>> +\n>>>> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)\n>>>> +{\n>>>> +\tsourceSize_ = sourceSize;\n>>>> +\tpixelFormat_ = pixelFormat;\n>>>> +\n>>>> +\tif (pixelFormat_ != formats::NV12) {\n>>>> +\t\tLOG (Thumbnailer, Error) << \"Failed to configure: Pixel Format \"\n>>>> +\t\t\t\t << pixelFormat_.toString() << \" unsupported.\";\n>>> \t\tLOG (Thumbnailer, Error)\n>>> \t\t\t<< \"Failed to configure: Pixel Format \"\n>>> \t\t\t<< pixelFormat_.toString() << \" unsupported.\";\n>>>\n>>>> +\t\treturn;\n>>>> +\t}\n>>>> +\n>>>> +\ttargetSize_ = computeThumbnailSize();\n>>>> +\n>>>> +\tvalid_ = true;\n>>>> +}\n>>>> +\n>>>> +/*\n>>>> + * The Exif specification recommends the width of the thumbnail to be a\n>>>> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height\n>>> s/mutiple/multiple/\n>>>\n>>>> + * keeping the aspect ratio same as of the source.\n>>>> + */\n>>>> +Size Thumbnailer::computeThumbnailSize()\n>>>> +{\n>>>> +\tunsigned int targetHeight;\n>>>> +\tunsigned int targetWidth = 160;\n>>>> +\n>>>> +\ttargetHeight = targetWidth * sourceSize_.height / sourceSize_.width;\n>>>> +\n>>>> +\tif (targetHeight & 1)\n>>>> +\t\ttargetHeight++;\n>>>> +\n>>>> +\treturn Size(targetWidth, targetHeight);\n>>>> +}\n>>>> +\n>>>> +void\n>>>> +Thumbnailer::scaleBuffer(const FrameBuffer &source,\n>>>> +\t\t\t std::vector &destination)\n>>>> +{\n>>>> +\tMappedFrameBuffer frame(&source, PROT_READ);\n>>>> +\tif (!frame.isValid()) {\n>>>> +\t\tLOG(Thumbnailer, Error) << \"Failed to map FrameBuffer : \"\n>>>> +\t\t\t\t << strerror(frame.error());\n>>> \t\tLOG(Thumbnailer, Error)\n>>> \t\t\t<< \"Failed to map FrameBuffer : \"\n>>> \t\t\t<< strerror(frame.error());\n>>>\n>>>> +\t\treturn;\n>>>> +\t}\n>>>> +\n>>>> +\tif (!valid_) {\n>>>> +\t\tLOG(Thumbnailer, Error) << \"config is unconfigured or invalid.\";\n>>>> +\t\treturn;\n>>>> +\t}\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 char *src_cb, *src_cr;\n>>>> +\tunsigned char *dst_y, *src_y;\n>>>> +\n>>>> +\tsize_t dstSize = (th * tw) + ((th / 2) * tw);\n>>>> +\tdestination.reserve(dstSize);\n>>> This should be\n>>>\n>>> \tdestination.resize(dstSize);\n>>>\n>>> as reserve() allocates memory but doesn't change the size of the vector.\n>>>\n>>>> +\tunsigned char *dst = destination.data();\n>>>> +\tunsigned char *dst_c = dst + th * tw;\n>>>> +\n>>>> +\tfor (unsigned int y = 0; y < th; y += 2) {\n>>>> +\t\tunsigned int sourceY = (sh * y + th / 2) / th;\n>>>> +\n>>>> +\t\tdst_y = dst + y * tw;\n>>>> +\t\tsrc_y = src + sw * sourceY;\n>>>> +\t\tsrc_cb = src_c + (sourceY / 2) * sw + 0;\n>>>> +\t\tsrc_cr = src_c + (sourceY / 2) * sw + 1;\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[x] = src_y[sourceX];\n>>>> +\t\t\tdst_y[tw + x] = src_y[sw + sourceX];\n>>>> +\t\t\tdst_y[x + 1] = src_y[sourceX + 1];\n>>>> +\t\t\tdst_y[tw + x + 1] = src_y[sw + sourceX + 1];\n>>>> +\n>>>> +\t\t\tdst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];\n>>>> +\t\t\tdst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];\n>>>> +\t\t}\n>>>> +\t}\n>>>> + }\n>>> Extra space.\n>>>\n>>>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n>>>> new file mode 100644\n>>>> index 0000000..b769619\n>>>> --- /dev/null\n>>>> +++ b/src/android/jpeg/thumbnailer.h\n>>>> @@ -0,0 +1,37 @@\n>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>> Wrong license.\n>>>\n>>>> +/*\n>>>> + * Copyright (C) 2020, Google Inc.\n>>>> + *\n>>>> + * thumbnailer.h - Simple image thumbnailer\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>>>> +\tvoid scaleBuffer(const libcamera::FrameBuffer &source,\n>>>> +\t\t\t std::vector &dest);\n>>> How about naming this createThumbnail() or generateThumbnail() ? And the\n>>> function should be const.\n>>>\n>>>> +\tlibcamera::Size size() const { return targetSize_; }\n>>>> +\n>>>> +private:\n>>>> +\tlibcamera::Size computeThumbnailSize();\n>>> This can be\n>>>\n>>> \tlibcamera::Size computeThumbnailSize() const;\n>>>\n>>>> +\n>>>> +\tlibcamera::PixelFormat pixelFormat_;\n>>>> +\tlibcamera::Size sourceSize_;\n>>>> +\tlibcamera::Size targetSize_;\n>>>> +\n>>>> +\tbool valid_;\n>>>> +\n>>> No need for a blank line.\n>>>\n>>>> +};\n>>>> +\n>>>> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */\n>>>> diff --git a/src/android/meson.build b/src/android/meson.build\n>>>> index f72376a..3d4d3be 100644\n>>>> --- a/src/android/meson.build\n>>>> +++ b/src/android/meson.build\n>>>> @@ -25,6 +25,7 @@ android_hal_sources = files([\n>>>> 'jpeg/encoder_libjpeg.cpp',\n>>>> 'jpeg/exif.cpp',\n>>>> 'jpeg/post_processor_jpeg.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 B8452BDB1E\n\tfor ;\n\tTue, 27 Oct 2020 20:40:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38D21621C2;\n\tTue, 27 Oct 2020 21:40:53 +0100 (CET)","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 1A2836205F\n\tfor ;\n\tTue, 27 Oct 2020 21:40:51 +0100 (CET)"],"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=\"dE5G9iV9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1603831250; bh=mw4IAwo12x/cQgOWAF2gEhoTelgXy05aSK6R2FD7B6Y=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=dE5G9iV94H20RTbR9VZse/aY8Il82Oohs1Sjr17m/DmgMOv7DKlO8mORvx5OrIWmg\n\tb/Sv4LS3/i5P8EAm1RdWwMm+SHguhteHluFwtyvNhCU/gt1wrs4KgdsjkGDdXJb+Fj\n\tYcbgMurb6ydvojjAHiWDDtIsjuATroL3CNd52A54SoANN6LL0+Ggn+KWztAXanuOii\n\tpl9lsI0HVC25uyQtKhRyD+1D2S4JPAQa58gFPlyUcpZqCyfF89hQraJCmGEL2TRliG\n\t9Pej1ZKOgM9sLS2QoO38kVEkRvEF4zI00+g4FwMfrjGhLPlT2NNXjdlKRg9z6THybm\n\t+tZ2KM0iTpaJA==","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20201026140134.44166-1-email@uajain.com>\n\t<20201026140134.44166-4-email@uajain.com>\n\t<20201026210533.GC3756@pendragon.ideasonboard.com>\n\t\n\t<20201026220818.GF3756@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Wed, 28 Oct 2020 02:10:46 +0530","Mime-Version":"1.0","In-Reply-To":"<20201026220818.GF3756@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","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-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13521,"web_url":"https://patchwork.libcamera.org/comment/13521/","msgid":"<20201028010749.GG3967@pendragon.ideasonboard.com>","date":"2020-10-28T01:07:49","subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro-san,\n\nOn Tue, Oct 27, 2020 at 05:04:00PM +0900, Hirokazu Honda wrote:\n> On Tue, Oct 27, 2020 at 4:40 PM Umang Jain wrote:\n> > On 10/27/20 10:51 AM, Hirokazu Honda wrote:\n> > > On Tue, Oct 27, 2020 at 7:09 AM Laurent Pinchart wrote:\n> > >> On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:\n> > >>> On 26/10/2020 21:05, Laurent Pinchart wrote:\n> > >>>> On Mon, Oct 26, 2020 at 07:31:34PM +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/exif.cpp | 16 +++-\n> > >>>>> src/android/jpeg/exif.h | 1 +\n> > >>>>> src/android/jpeg/post_processor_jpeg.cpp | 35 +++++++-\n> > >>>>> src/android/jpeg/post_processor_jpeg.h | 8 +-\n> > >>>>> src/android/jpeg/thumbnailer.cpp | 109 +++++++++++++++++++++++\n> > >>>>> src/android/jpeg/thumbnailer.h | 37 ++++++++\n> > >>>>> src/android/meson.build | 1 +\n> > >>>>> 7 files changed, 204 insertions(+), 3 deletions(-)\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/exif.cpp b/src/android/jpeg/exif.cpp\n> > >>>>> index d21534a..24197bd 100644\n> > >>>>> --- a/src/android/jpeg/exif.cpp\n> > >>>>> +++ b/src/android/jpeg/exif.cpp\n> > >>>>> @@ -75,8 +75,16 @@ Exif::~Exif()\n> > >>>>> if (exifData_)\n> > >>>>> free(exifData_);\n> > >>>>>\n> > >>>>> - if (data_)\n> > >>>>> + if (data_) {\n> > >>>>> + /*\n> > >>>>> + * Reset thumbnail data to avoid getting double-freed by\n> > >>>>> + * libexif. It is owned by the caller (i.e. PostProcessorJpeg).\n> > >>>>> + */\n> > >>>>> + data_->data = nullptr;\n> > >>>>> + data_->size = 0;\n> > >>>>> +\n> > >>>>> exif_data_unref(data_);\n> > >>>>> + }\n> > >>>>>\n> > >>>>> if (mem_)\n> > >>>>> exif_mem_unref(mem_);\n> > >>>>> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)\n> > >>>>> setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\n> > >>>>> }\n> > >>>>>\n> > >>>>> +void Exif::setThumbnail(std::vector &thumbnail)\n> > >>>>> +{\n> > >>>>> + data_->data = thumbnail.data();\n> > >>>>> + data_->size = thumbnail.size();\n> > >>>>> +}\n> > >>>>> +\n> > >>>>> [[nodiscard]] int Exif::generate()\n> > >>>>> {\n> > >>>>> if (exifData_) {\n> > >>>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> > >>>>> index 12c27b6..bd54a31 100644\n> > >>>>> --- a/src/android/jpeg/exif.h\n> > >>>>> +++ b/src/android/jpeg/exif.h\n> > >>>>> @@ -26,6 +26,7 @@ public:\n> > >>>>>\n> > >>>>> void setOrientation(int orientation);\n> > >>>>> void setSize(const libcamera::Size &size);\n> > >>>>> + void setThumbnail(std::vector &thumbnail);\n> > >>>> You can pass a Span as the setThumbnail() function\n> > >>>> shouldn't care what storage container is used.\n> > >>>>\n> > >>>> It's a bit of a dangerous API, as it will store the pointer internally,\n> > >>>> without ensuring that the caller keeps the thumbnail valid after the\n> > >>>> call returns. It's fine, but maybe a comment above the\n> > >>>> Exif::setThumbnail() functions to state that the thumbnail must remain\n> > >>>> valid until the Exif object is destroyed would be a good thing.\n> > >>> I think the comment will help indeed. The only alternative would be to\n> > >>> pass it into generate(), but and refactor generate() to return the span\n> > >>> ... but that's more effort that we need right now. So just a comment\n> > >>> will do ;-)\n> > >>>\n> > >>>\n> > >>>\n> > >>>>> void setTimestamp(time_t timestamp);\n> > >>>>>\n> > >>>>> libcamera::Span data() const { return { exifData_, size_ }; }\n> > >>>> I would have moved this to a separate patch.\n> > >>> Yes, I'd keep the exif extension for setting the thumbnail, and the\n> > >>> usage separate.\n> > >>>\n> > >>>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > >>>>> index c56f1b2..416e831 100644\n> > >>>>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > >>>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > >>>>> @@ -9,7 +9,6 @@\n> > >>>>>\n> > >>>>> #include \"../camera_device.h\"\n> > >>>>> #include \"../camera_metadata.h\"\n> > >>>>> -#include \"encoder_libjpeg.h\"\n> > >>>>> #include \"exif.h\"\n> > >>>>>\n> > >>>>> #include \n> > >>>>> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > >>>>> }\n> > >>>>>\n> > >>>>> streamSize_ = outCfg.size;\n> > >>>>> +\n> > >>>>> + thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> > >>>>> + StreamConfiguration thCfg = inCfg;\n> > >>>>> + thCfg.size = thumbnailer_.size();\n> > >>>>> + if (thumbnailEncoder_.configure(thCfg) != 0) {\n> > >>>>> + LOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> > >>>>> + return -EINVAL;\n> > >>>>> + }\n> > >>>>> +\n> > >>>>> encoder_ = std::make_unique();\n> > >>>>>\n> > >>>>> return encoder_->configure(inCfg);\n> > >>>>> }\n> > >>>>>\n> > >>>>> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> > >>>>> + std::vector &thumbnail)\n> > > std::vector* thumbnail\n> > > We would rather use a pointer than reference if a function changes it.\n> > > Same for other places.\n> > Ack.\n> > >>>>> +{\n> > >>>>> + /* Stores the raw scaled-down thumbnail bytes. */\n> > >>>>> + std::vector rawThumbnail;\n> > >>>>> +\n> > >>>>> + thumbnailer_.scaleBuffer(source, rawThumbnail);\n> > >>>>> +\n> > >>>>> + if (rawThumbnail.data()) {\n> > >>>> This should check for ! .empty() (see additional comments below).\n> > >>>>\n> > >>>>> + thumbnail.reserve(rawThumbnail.capacity());\n> > >>>> You should call thumbnail.resize(), and use size() instead of capacity()\n> > >>>> below, as reserve() allocates memory but doesn't change the size of the\n> > >>>> vector, so it's semantically dangerous to write to the reserved storage\n> > >>>> space if it hasn't been marked as in use with .resize().\n> > >>>>\n> > >>>>> +\n> > >>>>> + int jpeg_size = thumbnailEncoder_.encode(\n> > >>>>> + { rawThumbnail.data(), rawThumbnail.capacity() },\n> > >>>>> + { thumbnail.data(), thumbnail.capacity() },\n> > >>>> Just pass rawThumbnail and thumbnail, there's an implicit constructor\n> > >>>> for Span that will turn them into a span using .data() and .size().\n> > >>>>\n> > >>>>> + { });\n> > >>>>> + thumbnail.resize(jpeg_size);\n> > >>>>> +\n> > >>>>> + LOG(JPEG, Info) << \"Thumbnail compress returned \"\n> > >>>>> + << jpeg_size << \" bytes\";\n> > >>>> When log messages don't fit on one line, we usually wrap them as\n> > >>>>\n> > >>>> LOG(JPEG, Info)\n> > >>>> << \"Thumbnail compress returned \"\n> > >>>> << jpeg_size << \" bytes\";\n> > >>>>\n> > >>>> And maybe debug instead of info ?\n> > >>>>\n> > >>>>> + }\n> > >>>>> +}\n> > > Shall this function handle a failure of jpeg encode?\n> > Ideally it should but I am wondering in what style. I would prefer that\n> > checking for the (thumbnail data != 0) before calling\n> > Exif::setThumbnail() below. Since, when the encoding fails, jpeg_size\n> > will get 0, and the thumbnail vector(placeholder for our thumbnail data)\n> > will get resized to '0'.\n> \n> Ack.\n> \n> > >>>>> +\n> > >>>>> int PostProcessorJpeg::process(const FrameBuffer &source,\n> > >>>>> Span destination,\n> > >>>>> CameraMetadata *metadata)\n> > >>>>> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > >>>>> * second, it is good enough.\n> > >>>>> */\n> > >>>>> exif.setTimestamp(std::time(nullptr));\n> > >>>>> + std::vector thumbnail;\n> > >>>>> + generateThumbnail(source, thumbnail);\n> > >>>>> + exif.setThumbnail(thumbnail);\n> > >>>>> if (exif.generate() != 0)\n> > >>>>> LOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> > >>>>>\n> > >>>>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> > >>>>> index 3706cec..3894231 100644\n> > >>>>> --- a/src/android/jpeg/post_processor_jpeg.h\n> > >>>>> +++ b/src/android/jpeg/post_processor_jpeg.h\n> > >>>>> @@ -8,12 +8,13 @@\n> > >>>>> #define __ANDROID_POST_PROCESSOR_JPEG_H__\n> > >>>>>\n> > >>>>> #include \"../post_processor.h\"\n> > >>>>> +#include \"encoder_libjpeg.h\"\n> > >>>>> +#include \"thumbnailer.h\"\n> > >>>>>\n> > >>>>> #include \n> > >>>>>\n> > >>>>> #include \"libcamera/internal/buffer.h\"\n> > >>>>>\n> > >>>>> -class Encoder;\n> > >>>>> class CameraDevice;\n> > >>>>>\n> > >>>>> class PostProcessorJpeg : public PostProcessor\n> > >>>>> @@ -28,9 +29,14 @@ public:\n> > >>>>> CameraMetadata *metadata) override;\n> > >>>>>\n> > >>>>> private:\n> > >>>>> + void generateThumbnail(const libcamera::FrameBuffer &source,\n> > >>>>> + std::vector &thumbnail);\n> > >>>>> +\n> > >>>>> CameraDevice *const cameraDevice_;\n> > >>>>> std::unique_ptr encoder_;\n> > >>>>> libcamera::Size streamSize_;\n> > >>>>> + EncoderLibJpeg thumbnailEncoder_;\n> > >>>> Could you store this in a std::unique_ptr instead ? The reason\n> > >>>> is that we shouldn't hardcode usage of EncoderLibJpeg, in order to\n> > >>>> support different encoders later. You won't need to include\n> > >>>> encoder_libjpeg.h then, and can keep the Encoder forwared declaration.\n> > >>> I think this was already a unique_ptr in a previous iteration, and I\n> > >>> suggested moving it to directly store an instance of the libjpeg encoder.\n> > >>>\n> > >>> In this instance of encoding a thumbnail, when encoding a 160x160 image,\n> > >>> I would be weary about the overhead of setting up a hardware encode, and\n> > >>> I'd expect the preparation phases of that to be potentially more\n> > >>> expensive than just a software encode. Particularly as this has just\n> > >>> done a software rescale, so it would have to cache-flush, when it could\n> > >>> just use the host-cpu with a hot-cache. (ok, so perhaps later it might\n> > >>> use a different scaler too ... but then it's likely a different equation\n> > >>> anyway)\n> > >>>\n> > >>> I have not measured that of course, as we don't yet have a hw-jpeg\n> > >>> encode post-processor. It will be an interesting test in the future.\n> > >>>\n> > >>> But essentially, I think we're just as well leaving this as a single\n> > >>> instance of libjpeg for thumbnails. I think I recall the CrOS HAL using\n> > >>> libjpeg as well, but I haven't gone back to double-check.\n> > >>\n> > >> Fair enough, those are good points. I'm fine if the code is kept as-is\n> > >> (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.\n> > >\n> > > Although either is fine to me, I wonder why the encoder for the\n> > > thumbnail is within the thumbnailer.\n> >\n> > It is not within the encoder. It is inside the PostProcessorJpeg\n> \n> Sorry, I meant why ... is NOT within ....\n> \n> > > Since the thumbnailer is currently being used for thumbnail, it seems\n> > > to be reasonable to move the encoder to it.\n> > > What do you think?\n> >\n> > If you mean moving the thumbnail-encoder to `class Thumbnailer` - hmm,\n> > that could also work I suppose. However, I might not be very\n> > enthusiastic about it, as PostProcessorJpeg captures the entire flow at\n> > one place : e.g. exif generation, creating a thumbnail, encoding it and\n> > finally encoding the entire frame. I like this.\n> >\n> > If others support moving the thumbnail-encoder to Thumbnailer, that's\n> > also fine! (Also, could be done on top, as an independent refactor)\n> \n> I would like to hear others opinions here.\n> I would move it to Thumbnailer, so that\n> 1.) a reader can understand at a glance the encoder is used for\n> generating thumbnails, and\n> 2.) thumbnail is always meant JPEG here and then it makes more sense\n> that thumbnailer outputs JPEG.\n> \n> Especially regarding 2, will the thumbnail possibly be other formats?\n\nThumbnails can also be stored in uncompressed form (the only combination\nthat is not allowed is storing compressed thumbnails in a TIFF container\nfor an uncompressed image). We have no plan at this point to support\nthis though, but if we realize that the size gain from the JPEG\ncompression of the thumbnail is minimal compared to the extra CPU time\nto encode it, we could consider dropping the compression.\n\n> > >>>>> + Thumbnailer thumbnailer_;\n> > >>>>\n> > >>>> This is good, as I don't expect different thumbnailer types.\n> > >>>>\n> > >>>>> };\n> > >>>>>\n> > >>>>> #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n> > >>>>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> > >>>>> new file mode 100644\n> > >>>>> index 0000000..f880ffb\n> > >>>>> --- /dev/null\n> > >>>>> +++ b/src/android/jpeg/thumbnailer.cpp\n> > >>>>> @@ -0,0 +1,109 @@\n> > >>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > >>>> Wrong license.\n> > >>>>\n> > >>>>> +/*\n> > >>>>> + * Copyright (C) 2020, Google Inc.\n> > >>>>> + *\n> > >>>>> + * thumbnailer.cpp - Simple image thumbnailer\n> > >>>>> + */\n> > >>>>> +\n> > >>>>> +#include \"thumbnailer.h\"\n> > >>>>> +\n> > >>>>> +#include \n> > >>>>> +\n> > >>>>> +#include \"libcamera/internal/file.h\"\n> > >>>> Why do you need this header ?\n> > >>>>\n> > >>>>> +#include \"libcamera/internal/log.h\"\n> > >>>>> +\n> > >>>>> +using namespace libcamera;\n> > >>>>> +\n> > >>>>> +LOG_DEFINE_CATEGORY(Thumbnailer)\n> > >>>>> +\n> > >>>>> +Thumbnailer::Thumbnailer() : valid_(false)\n> > >>>> : valid_(false)\n> > >>>>\n> > >>>>> +{\n> > >>>>> +}\n> > >>>>> +\n> > >>>>> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)\n> > >>>>> +{\n> > >>>>> + sourceSize_ = sourceSize;\n> > >>>>> + pixelFormat_ = pixelFormat;\n> > >>>>> +\n> > >>>>> + if (pixelFormat_ != formats::NV12) {\n> > >>>>> + LOG (Thumbnailer, Error) << \"Failed to configure: Pixel Format \"\n> > >>>>> + << pixelFormat_.toString() << \" unsupported.\";\n> > >>>> LOG (Thumbnailer, Error)\n> > >>>> << \"Failed to configure: Pixel Format \"\n> > >>>> << pixelFormat_.toString() << \" unsupported.\";\n> > >>>>\n> > >>>>> + return;\n> > >>>>> + }\n> > >>>>> +\n> > >>>>> + targetSize_ = computeThumbnailSize();\n> > >>>>> +\n> > >>>>> + valid_ = true;\n> > >>>>> +}\n> > >>>>> +\n> > >>>>> +/*\n> > >>>>> + * The Exif specification recommends the width of the thumbnail to be a\n> > >>>>> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height\n> > >>>> s/mutiple/multiple/\n> > >>>>\n> > >>>>> + * keeping the aspect ratio same as of the source.\n> > >>>>> + */\n> > >>>>> +Size Thumbnailer::computeThumbnailSize()\n> > >>>>> +{\n> > >>>>> + unsigned int targetHeight;\n> > >>>>> + unsigned int targetWidth = 160;\n> > > nit: constexpr unsigned int kTargetWidth = 160;\n> > >\n> > >>>>> +\n> > >>>>> + targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;\n> > >>>>> +\n> > >>>>> + if (targetHeight & 1)\n> > >>>>> + targetHeight++;\n> > >>>>> +\n> > >>>>> + return Size(targetWidth, targetHeight);\n> > >>>>> +}\n> > >>>>> +\n> > >>>>> +void\n> > >>>>> +Thumbnailer::scaleBuffer(const FrameBuffer &source,\n> > >>>>> + std::vector &destination)\n> > >>>>> +{\n> > >>>>> + MappedFrameBuffer frame(&source, PROT_READ);\n> > >>>>> + if (!frame.isValid()) {\n> > >>>>> + LOG(Thumbnailer, Error) << \"Failed to map FrameBuffer : \"\n> > >>>>> + << strerror(frame.error());\n> > >>>> LOG(Thumbnailer, Error)\n> > >>>> << \"Failed to map FrameBuffer : \"\n> > >>>> << strerror(frame.error());\n> > >>>>\n> > >>>>> + return;\n> > >>>>> + }\n> > >>>>> +\n> > >>>>> + if (!valid_) {\n> > >>>>> + LOG(Thumbnailer, Error) << \"config is unconfigured or invalid.\";\n> > >>>>> + return;\n> > >>>>> + }\n> > >>>>> +\n> > >>>>> + const unsigned int sw = sourceSize_.width;\n> > >>>>> + const unsigned int sh = sourceSize_.height;\n> > >>>>> + const unsigned int tw = targetSize_.width;\n> > >>>>> + const unsigned int th = targetSize_.height;\n> > >>>>> +\n> > >>>>> + /* Image scaling block implementing nearest-neighbour algorithm. */\n> > >>>>> + unsigned char *src = static_cast(frame.maps()[0].data());\n> > >>>>> + unsigned char *src_c = src + sh * sw;\n> > >>>>> + unsigned char *src_cb, *src_cr;\n> > >>>>> + unsigned char *dst_y, *src_y;\n> > >>>>> +\n> > >>>>> + size_t dstSize = (th * tw) + ((th / 2) * tw);\n> > >>>>> + destination.reserve(dstSize);\n> > >>>> This should be\n> > >>>>\n> > >>>> destination.resize(dstSize);\n> > >>>>\n> > >>>> as reserve() allocates memory but doesn't change the size of the vector.\n> > >>>>\n> > >>>>> + unsigned char *dst = destination.data();\n> > >>>>> + unsigned char *dst_c = dst + th * tw;\n> > >>>>> +\n> > >>>>> + for (unsigned int y = 0; y < th; y += 2) {\n> > >>>>> + unsigned int sourceY = (sh * y + th / 2) / th;\n> > >>>>> +\n> > >>>>> + dst_y = dst + y * tw;\n> > >>>>> + src_y = src + sw * sourceY;\n> > >>>>> + src_cb = src_c + (sourceY / 2) * sw + 0;\n> > >>>>> + src_cr = src_c + (sourceY / 2) * sw + 1;\n> > >>>>> +\n> > >>>>> + for (unsigned int x = 0; x < tw; x += 2) {\n> > >>>>> + unsigned int sourceX = (sw * x + tw / 2) / tw;\n> > >>>>> +\n> > >>>>> + dst_y[x] = src_y[sourceX];\n> > >>>>> + dst_y[tw + x] = src_y[sw + sourceX];\n> > >>>>> + dst_y[x + 1] = src_y[sourceX + 1];\n> > >>>>> + dst_y[tw + x + 1] = src_y[sw + sourceX + 1];\n> > >>>>> +\n> > >>>>> + dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];\n> > >>>>> + dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];\n> > >>>>> + }\n> > >>>>> + }\n> > >>>>> + }\n> > >>>> Extra space.\n> > >>>>\n> > >>>>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> > >>>>> new file mode 100644\n> > >>>>> index 0000000..b769619\n> > >>>>> --- /dev/null\n> > >>>>> +++ b/src/android/jpeg/thumbnailer.h\n> > >>>>> @@ -0,0 +1,37 @@\n> > >>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > >>>> Wrong license.\n> > >>>>\n> > >>>>> +/*\n> > >>>>> + * Copyright (C) 2020, Google Inc.\n> > >>>>> + *\n> > >>>>> + * thumbnailer.h - Simple image thumbnailer\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> > >>>>> + Thumbnailer();\n> > >>>>> +\n> > >>>>> + void configure(const libcamera::Size &sourceSize,\n> > >>>>> + libcamera::PixelFormat pixelFormat);\n> > >>>>> + void scaleBuffer(const libcamera::FrameBuffer &source,\n> > >>>>> + std::vector &dest);\n> > >>>> How about naming this createThumbnail() or generateThumbnail() ? And the\n> > >>>> function should be const.\n> > >>>>\n> > >>>>> + libcamera::Size size() const { return targetSize_; }\n> > >\n> > > nit: to reduce unnecessary copy, let's return const reference.\n> > > const libcamera::Size& computeThumbanilSize() const;\n> > >\n> > >>>>> +\n> > >>>>> +private:\n> > >>>>> + libcamera::Size computeThumbnailSize();\n> > >>>> This can be\n> > >>>>\n> > >>>> libcamera::Size computeThumbnailSize() const;\n> > >>>>\n> > >>>>> +\n> > >>>>> + libcamera::PixelFormat pixelFormat_;\n> > >>>>> + libcamera::Size sourceSize_;\n> > >>>>> + libcamera::Size targetSize_;\n> > >>>>> +\n> > >>>>> + bool valid_;\n> > >>>>> +\n> > >>>> No need for a blank line.\n> > >>>>\n> > >>>>> +};\n> > >>>>> +\n> > >>>>> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */\n> > >>>>> diff --git a/src/android/meson.build b/src/android/meson.build\n> > >>>>> index f72376a..3d4d3be 100644\n> > >>>>> --- a/src/android/meson.build\n> > >>>>> +++ b/src/android/meson.build\n> > >>>>> @@ -25,6 +25,7 @@ android_hal_sources = files([\n> > >>>>> 'jpeg/encoder_libjpeg.cpp',\n> > >>>>> 'jpeg/exif.cpp',\n> > >>>>> 'jpeg/post_processor_jpeg.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 42E66BDB1E\n\tfor ;\n\tWed, 28 Oct 2020 01:08:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2EB162244;\n\tWed, 28 Oct 2020 02:08:39 +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 B59EC6034B\n\tfor ;\n\tWed, 28 Oct 2020 02:08:37 +0100 (CET)","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 0180E99A;\n\tWed, 28 Oct 2020 02:08:36 +0100 (CET)"],"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=\"rXlPfaJ8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603847317;\n\tbh=SGgrrf4P4G51urjAC+9LbBedlfzETb78Gb/k2JucYeg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rXlPfaJ8o+46zPoy6MAhycY5jdgi2hs0buZFAwiizSJ0gssYJis2eyVOoI2g21dLm\n\tOk3GiDQKZelrt+439Tif8xMqvfd+O8qmDsIpYAgl5ILDjUnqWaJ3FrubzD5sXhOsUs\n\tHhMK/ek1pj0OxLbRyEBpirygafbv2q2qaNlI7h+w=","Date":"Wed, 28 Oct 2020 03:07:49 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"<20201028010749.GG3967@pendragon.ideasonboard.com>","References":"<20201026140134.44166-1-email@uajain.com>\n\t<20201026140134.44166-4-email@uajain.com>\n\t<20201026210533.GC3756@pendragon.ideasonboard.com>\n\t\n\t<20201026220818.GF3756@pendragon.ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","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":13522,"web_url":"https://patchwork.libcamera.org/comment/13522/","msgid":"<20201028011113.GH3967@pendragon.ideasonboard.com>","date":"2020-10-28T01:11:13","subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Oct 28, 2020 at 02:10:46AM +0530, Umang Jain wrote:\n> On 10/27/20 3:38 AM, Laurent Pinchart wrote:\n> > On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:\n> >> On 26/10/2020 21:05, Laurent Pinchart wrote:\n> >>> On Mon, Oct 26, 2020 at 07:31:34PM +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/exif.cpp | 16 +++-\n> >>>> src/android/jpeg/exif.h | 1 +\n> >>>> src/android/jpeg/post_processor_jpeg.cpp | 35 +++++++-\n> >>>> src/android/jpeg/post_processor_jpeg.h | 8 +-\n> >>>> src/android/jpeg/thumbnailer.cpp | 109 +++++++++++++++++++++++\n> >>>> src/android/jpeg/thumbnailer.h | 37 ++++++++\n> >>>> src/android/meson.build | 1 +\n> >>>> 7 files changed, 204 insertions(+), 3 deletions(-)\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/exif.cpp b/src/android/jpeg/exif.cpp\n> >>>> index d21534a..24197bd 100644\n> >>>> --- a/src/android/jpeg/exif.cpp\n> >>>> +++ b/src/android/jpeg/exif.cpp\n> >>>> @@ -75,8 +75,16 @@ Exif::~Exif()\n> >>>> \tif (exifData_)\n> >>>> \t\tfree(exifData_);\n> >>>> \n> >>>> -\tif (data_)\n> >>>> +\tif (data_) {\n> >>>> +\t\t/*\n> >>>> +\t\t * Reset thumbnail data to avoid getting double-freed by\n> >>>> +\t\t * libexif. It is owned by the caller (i.e. PostProcessorJpeg).\n> >>>> +\t\t */\n> >>>> +\t\tdata_->data = nullptr;\n> >>>> +\t\tdata_->size = 0;\n> >>>> +\n> >>>> \t\texif_data_unref(data_);\n> >>>> +\t}\n> >>>> \n> >>>> \tif (mem_)\n> >>>> \t\texif_mem_unref(mem_);\n> >>>> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)\n> >>>> \tsetShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\n> >>>> }\n> >>>> \n> >>>> +void Exif::setThumbnail(std::vector &thumbnail)\n> >>>> +{\n> >>>> +\tdata_->data = thumbnail.data();\n> >>>> +\tdata_->size = thumbnail.size();\n> >>>> +}\n> >>>> +\n> >>>> [[nodiscard]] int Exif::generate()\n> >>>> {\n> >>>> \tif (exifData_) {\n> >>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> >>>> index 12c27b6..bd54a31 100644\n> >>>> --- a/src/android/jpeg/exif.h\n> >>>> +++ b/src/android/jpeg/exif.h\n> >>>> @@ -26,6 +26,7 @@ public:\n> >>>> \n> >>>> \tvoid setOrientation(int orientation);\n> >>>> \tvoid setSize(const libcamera::Size &size);\n> >>>> +\tvoid setThumbnail(std::vector &thumbnail);\n> >>> You can pass a Span as the setThumbnail() function\n> >>> shouldn't care what storage container is used.\n> >>>\n> >>> It's a bit of a dangerous API, as it will store the pointer internally,\n> >>> without ensuring that the caller keeps the thumbnail valid after the\n> >>> call returns. It's fine, but maybe a comment above the\n> >>> Exif::setThumbnail() functions to state that the thumbnail must remain\n> >>> valid until the Exif object is destroyed would be a good thing.\n> >> I think the comment will help indeed. The only alternative would be to\n> >> pass it into generate(), but and refactor generate() to return the span\n> >> ... but that's more effort that we need right now. So just a comment\n> >> will do ;-)\n> >>\n> >>\n> >>\n> >>>> \tvoid setTimestamp(time_t timestamp);\n> >>>> \n> >>>> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> >>> I would have moved this to a separate patch.\n> >>\n> >> Yes, I'd keep the exif extension for setting the thumbnail, and the\n> >> usage separate.\n> >>\n> >>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> >>>> index c56f1b2..416e831 100644\n> >>>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> >>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> >>>> @@ -9,7 +9,6 @@\n> >>>> \n> >>>> #include \"../camera_device.h\"\n> >>>> #include \"../camera_metadata.h\"\n> >>>> -#include \"encoder_libjpeg.h\"\n> >>>> #include \"exif.h\"\n> >>>> \n> >>>> #include \n> >>>> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> >>>> \t}\n> >>>> \n> >>>> \tstreamSize_ = outCfg.size;\n> >>>> +\n> >>>> +\tthumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> >>>> +\tStreamConfiguration thCfg = inCfg;\n> >>>> +\tthCfg.size = thumbnailer_.size();\n> >>>> +\tif (thumbnailEncoder_.configure(thCfg) != 0) {\n> >>>> +\t\tLOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> >>>> +\t\treturn -EINVAL;\n> >>>> +\t}\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 std::vector &thumbnail)\n> >>>> +{\n> >>>> +\t/* Stores the raw scaled-down thumbnail bytes. */\n> >>>> +\tstd::vector rawThumbnail;\n> >>>> +\n> >>>> +\tthumbnailer_.scaleBuffer(source, rawThumbnail);\n> >>>> +\n> >>>> +\tif (rawThumbnail.data()) {\n> >>> This should check for ! .empty() (see additional comments below).\n> >>>\n> >>>> +\t\tthumbnail.reserve(rawThumbnail.capacity());\n> >>> You should call thumbnail.resize(), and use size() instead of capacity()\n> >>> below, as reserve() allocates memory but doesn't change the size of the\n> >>> vector, so it's semantically dangerous to write to the reserved storage\n> >>> space if it hasn't been marked as in use with .resize().\n> >>>\n> >>>> +\n> >>>> +\t\tint jpeg_size = thumbnailEncoder_.encode(\n> >>>> +\t\t\t\t{ rawThumbnail.data(), rawThumbnail.capacity() },\n> >>>> +\t\t\t\t{ thumbnail.data(), thumbnail.capacity() },\n> >>> Just pass rawThumbnail and thumbnail, there's an implicit constructor\n> >>> for Span that will turn them into a span using .data() and .size().\n> >>>\n> >>>> +\t\t\t\t{ });\n> >>>> +\t\tthumbnail.resize(jpeg_size);\n> >>>> +\n> >>>> +\t\tLOG(JPEG, Info) << \"Thumbnail compress returned \"\n> >>>> +\t\t\t\t<< jpeg_size << \" bytes\";\n> >>> When log messages don't fit on one line, we usually wrap them as\n> >>>\n> >>> \t\tLOG(JPEG, Info)\n> >>> \t\t\t<< \"Thumbnail compress returned \"\n> >>> \t\t\t<< jpeg_size << \" bytes\";\n> >>>\n> >>> And maybe debug instead of info ?\n> >>>\n> >>>> +\t}\n> >>>> +}\n> >>>> +\n> >>>> int PostProcessorJpeg::process(const FrameBuffer &source,\n> >>>> \t\t\t Span destination,\n> >>>> \t\t\t CameraMetadata *metadata)\n> >>>> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> >>>> \t * second, it is good enough.\n> >>>> \t */\n> >>>> \texif.setTimestamp(std::time(nullptr));\n> >>>> +\tstd::vector thumbnail;\n> >>>> +\tgenerateThumbnail(source, thumbnail);\n> >>>> +\texif.setThumbnail(thumbnail);\n> >>>> \tif (exif.generate() != 0)\n> >>>> \t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> >>>> \n> >>>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> >>>> index 3706cec..3894231 100644\n> >>>> --- a/src/android/jpeg/post_processor_jpeg.h\n> >>>> +++ b/src/android/jpeg/post_processor_jpeg.h\n> >>>> @@ -8,12 +8,13 @@\n> >>>> #define __ANDROID_POST_PROCESSOR_JPEG_H__\n> >>>> \n> >>>> #include \"../post_processor.h\"\n> >>>> +#include \"encoder_libjpeg.h\"\n> >>>> +#include \"thumbnailer.h\"\n> >>>> \n> >>>> #include \n> >>>> \n> >>>> #include \"libcamera/internal/buffer.h\"\n> >>>> \n> >>>> -class Encoder;\n> >>>> class CameraDevice;\n> >>>> \n> >>>> class PostProcessorJpeg : public PostProcessor\n> >>>> @@ -28,9 +29,14 @@ public:\n> >>>> \t\t CameraMetadata *metadata) override;\n> >>>> \n> >>>> private:\n> >>>> +\tvoid generateThumbnail(const libcamera::FrameBuffer &source,\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> >>> Could you store this in a std::unique_ptr instead ? The reason\n> >>> is that we shouldn't hardcode usage of EncoderLibJpeg, in order to\n> >>> support different encoders later. You won't need to include\n> >>> encoder_libjpeg.h then, and can keep the Encoder forwared declaration.\n> >> I think this was already a unique_ptr in a previous iteration, and I\n> >> suggested moving it to directly store an instance of the libjpeg encoder.\n> >>\n> >> In this instance of encoding a thumbnail, when encoding a 160x160 image,\n> >> I would be weary about the overhead of setting up a hardware encode, and\n> >> I'd expect the preparation phases of that to be potentially more\n> >> expensive than just a software encode. Particularly as this has just\n> >> done a software rescale, so it would have to cache-flush, when it could\n> >> just use the host-cpu with a hot-cache. (ok, so perhaps later it might\n> >> use a different scaler too ... but then it's likely a different equation\n> >> anyway)\n> >>\n> >> I have not measured that of course, as we don't yet have a hw-jpeg\n> >> encode post-processor. It will be an interesting test in the future.\n> >>\n> >> But essentially, I think we're just as well leaving this as a single\n> >> instance of libjpeg for thumbnails. I think I recall the CrOS HAL using\n> >> libjpeg as well, but I haven't gone back to double-check.\n> >\n> > Fair enough, those are good points. I'm fine if the code is kept as-is\n> > (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.\n>\n> Do you specfically suggest using unique_ptr, to make use of forward \n> declaration, in this instance of EncoderLibJpeg(see a diff below)? I am \n> curious to ask, because in order to make it work with incomplete \n> types(i.e. forward declared), I need to insert destructor declaration in \n> post_processor_jpeg.h and define a (blank)destructor in \n> post_processor_jpeg.cpp\n\nI was thinking of storing a unique pointer to Encoder, not\nEncoderLibJpeg, even if we always instantiate a EncoderLibJpeg, but that\ndoesn't matter much.\n\n> I learnt this and can see it in action. Ref: \n> https://stackoverflow.com/a/42158611\n> \n> -----\n> diff --git a/src/android/jpeg/post_processor_jpeg.h \n> b/src/android/jpeg/post_processor_jpeg.h\n> index 3706cec..8b36a21 100644\n> --- a/src/android/jpeg/post_processor_jpeg.h\n> +++ b/src/android/jpeg/post_processor_jpeg.h\n> @@ -8,12 +8,14 @@\n>  #define __ANDROID_POST_PROCESSOR_JPEG_H__\n> \n>  #include \"../post_processor.h\"\n> +#include \"thumbnailer.h\"\n> \n>  #include \n> \n>  #include \"libcamera/internal/buffer.h\"\n> \n>  class Encoder;\n> +class EncoderLibJpeg;\n>  class CameraDevice;\n> \n>  class PostProcessorJpeg : public PostProcessor\n> @@ -28,9 +30,14 @@ public:\n>                     CameraMetadata *metadata) override;\n> \n>  private:\n> +       void generateThumbnail(const libcamera::FrameBuffer &source,\n> +                              std::vector *thumbnail);\n> +\n>         CameraDevice *const cameraDevice_;\n>         std::unique_ptr encoder_;\n>         libcamera::Size streamSize_;\n> +       std::unique_ptr thumbnailEncoder_;\n> +       Thumbnailer thumbnailer_;\n>  };\n> \n>\n> >>>> +\tThumbnailer thumbnailer_;\n> >>> This is good, as I don't expect different thumbnailer types.\n> >>>\n> >>>> };\n> >>>> \n> >>>> #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n> >>>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> >>>> new file mode 100644\n> >>>> index 0000000..f880ffb\n> >>>> --- /dev/null\n> >>>> +++ b/src/android/jpeg/thumbnailer.cpp\n> >>>> @@ -0,0 +1,109 @@\n> >>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>> Wrong license.\n> >>>\n> >>>> +/*\n> >>>> + * Copyright (C) 2020, Google Inc.\n> >>>> + *\n> >>>> + * thumbnailer.cpp - Simple image thumbnailer\n> >>>> + */\n> >>>> +\n> >>>> +#include \"thumbnailer.h\"\n> >>>> +\n> >>>> +#include \n> >>>> +\n> >>>> +#include \"libcamera/internal/file.h\"\n> >>> Why do you need this header ?\n> >>>\n> >>>> +#include \"libcamera/internal/log.h\"\n> >>>> +\n> >>>> +using namespace libcamera;\n> >>>> +\n> >>>> +LOG_DEFINE_CATEGORY(Thumbnailer)\n> >>>> +\n> >>>> +Thumbnailer::Thumbnailer() : valid_(false)\n> >>> \t: valid_(false)\n> >>>\n> >>>> +{\n> >>>> +}\n> >>>> +\n> >>>> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)\n> >>>> +{\n> >>>> +\tsourceSize_ = sourceSize;\n> >>>> +\tpixelFormat_ = pixelFormat;\n> >>>> +\n> >>>> +\tif (pixelFormat_ != formats::NV12) {\n> >>>> +\t\tLOG (Thumbnailer, Error) << \"Failed to configure: Pixel Format \"\n> >>>> +\t\t\t\t << pixelFormat_.toString() << \" unsupported.\";\n> >>> \t\tLOG (Thumbnailer, Error)\n> >>> \t\t\t<< \"Failed to configure: Pixel Format \"\n> >>> \t\t\t<< pixelFormat_.toString() << \" unsupported.\";\n> >>>\n> >>>> +\t\treturn;\n> >>>> +\t}\n> >>>> +\n> >>>> +\ttargetSize_ = computeThumbnailSize();\n> >>>> +\n> >>>> +\tvalid_ = true;\n> >>>> +}\n> >>>> +\n> >>>> +/*\n> >>>> + * The Exif specification recommends the width of the thumbnail to be a\n> >>>> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height\n> >>> s/mutiple/multiple/\n> >>>\n> >>>> + * keeping the aspect ratio same as of the source.\n> >>>> + */\n> >>>> +Size Thumbnailer::computeThumbnailSize()\n> >>>> +{\n> >>>> +\tunsigned int targetHeight;\n> >>>> +\tunsigned int targetWidth = 160;\n> >>>> +\n> >>>> +\ttargetHeight = targetWidth * sourceSize_.height / sourceSize_.width;\n> >>>> +\n> >>>> +\tif (targetHeight & 1)\n> >>>> +\t\ttargetHeight++;\n> >>>> +\n> >>>> +\treturn Size(targetWidth, targetHeight);\n> >>>> +}\n> >>>> +\n> >>>> +void\n> >>>> +Thumbnailer::scaleBuffer(const FrameBuffer &source,\n> >>>> +\t\t\t std::vector &destination)\n> >>>> +{\n> >>>> +\tMappedFrameBuffer frame(&source, PROT_READ);\n> >>>> +\tif (!frame.isValid()) {\n> >>>> +\t\tLOG(Thumbnailer, Error) << \"Failed to map FrameBuffer : \"\n> >>>> +\t\t\t\t << strerror(frame.error());\n> >>> \t\tLOG(Thumbnailer, Error)\n> >>> \t\t\t<< \"Failed to map FrameBuffer : \"\n> >>> \t\t\t<< strerror(frame.error());\n> >>>\n> >>>> +\t\treturn;\n> >>>> +\t}\n> >>>> +\n> >>>> +\tif (!valid_) {\n> >>>> +\t\tLOG(Thumbnailer, Error) << \"config is unconfigured or invalid.\";\n> >>>> +\t\treturn;\n> >>>> +\t}\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 char *src_cb, *src_cr;\n> >>>> +\tunsigned char *dst_y, *src_y;\n> >>>> +\n> >>>> +\tsize_t dstSize = (th * tw) + ((th / 2) * tw);\n> >>>> +\tdestination.reserve(dstSize);\n> >>> This should be\n> >>>\n> >>> \tdestination.resize(dstSize);\n> >>>\n> >>> as reserve() allocates memory but doesn't change the size of the vector.\n> >>>\n> >>>> +\tunsigned char *dst = destination.data();\n> >>>> +\tunsigned char *dst_c = dst + th * tw;\n> >>>> +\n> >>>> +\tfor (unsigned int y = 0; y < th; y += 2) {\n> >>>> +\t\tunsigned int sourceY = (sh * y + th / 2) / th;\n> >>>> +\n> >>>> +\t\tdst_y = dst + y * tw;\n> >>>> +\t\tsrc_y = src + sw * sourceY;\n> >>>> +\t\tsrc_cb = src_c + (sourceY / 2) * sw + 0;\n> >>>> +\t\tsrc_cr = src_c + (sourceY / 2) * sw + 1;\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[x] = src_y[sourceX];\n> >>>> +\t\t\tdst_y[tw + x] = src_y[sw + sourceX];\n> >>>> +\t\t\tdst_y[x + 1] = src_y[sourceX + 1];\n> >>>> +\t\t\tdst_y[tw + x + 1] = src_y[sw + sourceX + 1];\n> >>>> +\n> >>>> +\t\t\tdst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];\n> >>>> +\t\t\tdst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];\n> >>>> +\t\t}\n> >>>> +\t}\n> >>>> + }\n> >>> Extra space.\n> >>>\n> >>>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> >>>> new file mode 100644\n> >>>> index 0000000..b769619\n> >>>> --- /dev/null\n> >>>> +++ b/src/android/jpeg/thumbnailer.h\n> >>>> @@ -0,0 +1,37 @@\n> >>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>> Wrong license.\n> >>>\n> >>>> +/*\n> >>>> + * Copyright (C) 2020, Google Inc.\n> >>>> + *\n> >>>> + * thumbnailer.h - Simple image thumbnailer\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> >>>> +\tvoid scaleBuffer(const libcamera::FrameBuffer &source,\n> >>>> +\t\t\t std::vector &dest);\n> >>> How about naming this createThumbnail() or generateThumbnail() ? And the\n> >>> function should be const.\n> >>>\n> >>>> +\tlibcamera::Size size() const { return targetSize_; }\n> >>>> +\n> >>>> +private:\n> >>>> +\tlibcamera::Size computeThumbnailSize();\n> >>> This can be\n> >>>\n> >>> \tlibcamera::Size computeThumbnailSize() const;\n> >>>\n> >>>> +\n> >>>> +\tlibcamera::PixelFormat pixelFormat_;\n> >>>> +\tlibcamera::Size sourceSize_;\n> >>>> +\tlibcamera::Size targetSize_;\n> >>>> +\n> >>>> +\tbool valid_;\n> >>>> +\n> >>> No need for a blank line.\n> >>>\n> >>>> +};\n> >>>> +\n> >>>> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */\n> >>>> diff --git a/src/android/meson.build b/src/android/meson.build\n> >>>> index f72376a..3d4d3be 100644\n> >>>> --- a/src/android/meson.build\n> >>>> +++ b/src/android/meson.build\n> >>>> @@ -25,6 +25,7 @@ android_hal_sources = files([\n> >>>> 'jpeg/encoder_libjpeg.cpp',\n> >>>> 'jpeg/exif.cpp',\n> >>>> 'jpeg/post_processor_jpeg.cpp',\n> >>>> + 'jpeg/thumbnailer.cpp',\n> >>>> ])\n> >>>> \n> >>>> android_camera_metadata_sources = files([\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 275B1C3B5C\n\tfor ;\n\tWed, 28 Oct 2020 01:12:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AEFA26223C;\n\tWed, 28 Oct 2020 02:12:03 +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 4110E6034B\n\tfor ;\n\tWed, 28 Oct 2020 02:12:02 +0100 (CET)","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 A315399A;\n\tWed, 28 Oct 2020 02:12:01 +0100 (CET)"],"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=\"rlUfe4JV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603847521;\n\tbh=QmgsvTSI1R3HcfZOCfe3ivesOtj6YqlPIn6bFwKy3mw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rlUfe4JVGQiPT7ADlfThabykGd9NsgwLx+G/GNXHs3JJ9QgB/aC7Ei3pBy1LwDaGH\n\tNcefCQXUpaotP7HpfX/2xS8p+ifrxm2Sys7fKy2R9H8ba8Qm7pStuMpU1sCvEuMIIg\n\teAlUiT5uyhtaqXiYfhsyX9yIz/GbODWWeMvyLAI4=","Date":"Wed, 28 Oct 2020 03:11:13 +0200","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20201028011113.GH3967@pendragon.ideasonboard.com>","References":"<20201026140134.44166-1-email@uajain.com>\n\t<20201026140134.44166-4-email@uajain.com>\n\t<20201026210533.GC3756@pendragon.ideasonboard.com>\n\t\n\t<20201026220818.GF3756@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a\n\tJPEG-encoded thumbnail","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]