[{"id":13113,"web_url":"https://patchwork.libcamera.org/comment/13113/","msgid":"<20201008192240.GE3939@pendragon.ideasonboard.com>","date":"2020-10-08T19:22:40","subject":"Re: [libcamera-devel] [RFC PATCH 2/3] android: jpeg: Port to\n\tPostProcessor interface","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 Thu, Oct 08, 2020 at 07:40:37PM +0530, Umang Jain wrote:\n> Remove the existing Encoder interface completely and use the\n> PostProcessor interface instead.\n\nI think we need to keep the Encoder interface. We want to support other\nJPEG encoders than the libjpeg-based implementation. Creating a JPEG\npost-processor is the right thing to do, but it should still rely on the\nexisting encoder interface for the actual JPEG encoding. From a\nCameraDevice point of view, only the PostProcessor interface will be\nvisible.\n\n> Now the ::encode() function will be called by PostProcessor::process()\n> internally and will not be directly exposed in CameraStream. Similarly,\n> other operations can be introduced internally in the PostProcessorJpeg,\n> and can be called through its process() interface.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.h | 1 -\n> src/android/camera_stream.cpp | 74 +++------------\n> src/android/camera_stream.h | 9 +-\n> src/android/jpeg/encoder.h | 25 -----\n> ...er_libjpeg.cpp => post_processor_jpeg.cpp} | 92 +++++++++++++++++--\n> ...ncoder_libjpeg.h => post_processor_jpeg.h} | 27 ++++--\n> src/android/meson.build | 2 +-\n> 7 files changed, 122 insertions(+), 108 deletions(-)\n> delete mode 100644 src/android/jpeg/encoder.h\n> rename src/android/jpeg/{encoder_libjpeg.cpp => post_processor_jpeg.cpp} (67%)\n> rename src/android/jpeg/{encoder_libjpeg.h => post_processor_jpeg.h} (55%)\n> \n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 777d1a3..25de12e 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -25,7 +25,6 @@\n> #include \"libcamera/internal/message.h\"\n> \n> #include \"camera_stream.h\"\n> -#include \"jpeg/encoder.h\"\n> \n> class CameraMetadata;\n> \n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index eac1480..ed3bb41 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -9,9 +9,7 @@\n> \n> #include \"camera_device.h\"\n> #include \"camera_metadata.h\"\n> -#include \"jpeg/encoder.h\"\n> -#include \"jpeg/encoder_libjpeg.h\"\n> -#include \"jpeg/exif.h\"\n> +#include \"jpeg/post_processor_jpeg.h\"\n> \n> using namespace libcamera;\n> \n> @@ -24,8 +22,15 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> {\n> \tconfig_ = cameraDevice_->cameraConfiguration();\n> \n> -\tif (type_ == Type::Internal || type_ == Type::Mapped)\n> -\t\tencoder_ = std::make_unique();\n> +\tif (type_ == Type::Internal || type_ == Type::Mapped) {\n> +\t\t/*\n> +\t\t * \\todo There might be multiple post-processors. The logic\n> +\t\t * which should be instantiated here, is deferred for future.\n> +\t\t * For now, we only have PostProcessJpeg and that is what we\n\ns/PostProcessJpeg/PostProcessorJpeg/\n\n> +\t\t * will instantiate here.\n> +\t\t */\n> +\t\tpostProcessor_ = std::make_unique();\n> +\t}\n> \n> \tif (type == Type::Internal) {\n> \t\tallocator_ = std::make_unique(cameraDevice_->camera());\n> @@ -45,8 +50,8 @@ Stream *CameraStream::stream() const\n> \n> int CameraStream::configure()\n> {\n> -\tif (encoder_) {\n> -\t\tint ret = encoder_->configure(configuration());\n> +\tif (postProcessor_) {\n> +\t\tint ret = postProcessor_->configure(configuration());\n> \t\tif (ret)\n> \t\t\treturn ret;\n> \t}\n> @@ -69,60 +74,11 @@ int CameraStream::configure()\n> int CameraStream::process(const libcamera::FrameBuffer &source,\n> \t\t\t MappedCamera3Buffer *dest, CameraMetadata *metadata)\n> {\n> -\tif (!encoder_)\n> +\tif (!postProcessor_)\n> \t\treturn 0;\n> \n> -\t/* Set EXIF metadata for various tags. */\n> -\tExif exif;\n> -\t/* \\todo Set Make and Model from external vendor tags. */\n> -\texif.setMake(\"libcamera\");\n> -\texif.setModel(\"cameraModel\");\n> -\texif.setOrientation(cameraDevice_->orientation());\n> -\texif.setSize(configuration().size);\n> -\t/*\n> -\t * We set the frame's EXIF timestamp as the time of encode.\n> -\t * Since the precision we need for EXIF timestamp is only one\n> -\t * second, it is good enough.\n> -\t */\n> -\texif.setTimestamp(std::time(nullptr));\n> -\tif (exif.generate() != 0)\n> -\t\tLOG(HAL, Error) << \"Failed to generate valid EXIF data\";\n> -\n> -\tint jpeg_size = encoder_->encode(&source, dest->maps()[0], exif.data());\n> -\tif (jpeg_size < 0) {\n> -\t\tLOG(HAL, Error) << \"Failed to encode stream image\";\n> -\t\treturn jpeg_size;\n> -\t}\n> -\n> -\t/*\n> -\t * Fill in the JPEG blob header.\n> -\t *\n> -\t * The mapped size of the buffer is being returned as\n> -\t * substantially larger than the requested JPEG_MAX_SIZE\n> -\t * (which is referenced from maxJpegBufferSize_). Utilise\n> -\t * this static size to ensure the correct offset of the blob is\n> -\t * determined.\n> -\t *\n> -\t * \\todo Investigate if the buffer size mismatch is an issue or\n> -\t * expected behaviour.\n> -\t */\n> -\tuint8_t *resultPtr = dest->maps()[0].data() +\n> -\t\t\t cameraDevice_->maxJpegBufferSize() -\n> -\t\t\t sizeof(struct camera3_jpeg_blob);\n> -\tauto *blob = reinterpret_cast(resultPtr);\n> -\tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n> -\tblob->jpeg_size = jpeg_size;\n> -\n> -\t/* Update the JPEG result Metadata. */\n> -\tmetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);\n> -\n> -\tconst uint32_t jpeg_quality = 95;\n> -\tmetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n> -\n> -\tconst uint32_t jpeg_orientation = 0;\n> -\tmetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n> -\n> -\treturn 0;\n> +\treturn postProcessor_->process(&source, dest->maps()[0],\n> +\t\t\t\t metadata, cameraDevice_);\n\nThere are at least two options to keep the interface generic, avoiding\nvariadic arguments. One of them is to explicitly pass the the camera\ndevice to the process function. Another option is to pass it to the\nPostProcessorJpeg constructor, and store it internally. I'd go for the\nlatter.\n\n> }\n> \n> FrameBuffer *CameraStream::getBuffer()\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index 8df0101..8d0f2e3 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -19,7 +19,12 @@\n> #include \n> #include \n> \n> -class Encoder;\n> +/*\n> + * \\todo Ideally we want to replace this include with forward-declaration.\n> + * If we do that. currently we get a compile error.\n> + */\n\nLet's fix them :-) What compilation error do you get ?\n\n> +#include \"post_processor.h\"\n> +\n> class CameraDevice;\n> class CameraMetadata;\n> class MappedCamera3Buffer;\n> @@ -135,7 +140,7 @@ private:\n> \t */\n> \tunsigned int index_;\n> \n> -\tstd::unique_ptr encoder_;\n> +\tstd::unique_ptr postProcessor_;\n> \tstd::unique_ptr allocator_;\n> \tstd::vector buffers_;\n> \t/*\n> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> deleted file mode 100644\n> index cf26d67..0000000\n> --- a/src/android/jpeg/encoder.h\n> +++ /dev/null\n> @@ -1,25 +0,0 @@\n> -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * encoder.h - Image encoding interface\n> - */\n> -#ifndef __ANDROID_JPEG_ENCODER_H__\n> -#define __ANDROID_JPEG_ENCODER_H__\n> -\n> -#include \n> -#include \n> -#include \n> -\n> -class Encoder\n> -{\n> -public:\n> -\tvirtual ~Encoder() {};\n> -\n> -\tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> -\tvirtual int encode(const libcamera::FrameBuffer *source,\n> -\t\t\t const libcamera::Span &destination,\n> -\t\t\t const libcamera::Span &exifData) = 0;\n> -};\n> -\n> -#endif /* __ANDROID_JPEG_ENCODER_H__ */\n\nAs mentioned above, this should be kept, and used by the JPEG\npost-processor.\n\n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> similarity index 67%\n> rename from src/android/jpeg/encoder_libjpeg.cpp\n> rename to src/android/jpeg/post_processor_jpeg.cpp\n> index 510613c..eeb4e95 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -2,10 +2,14 @@\n> /*\n> * Copyright (C) 2020, Google Inc.\n> *\n> - * encoder_libjpeg.cpp - JPEG encoding using libjpeg native API\n> + * post_processor_jpeg.cpp - JPEG Post Processor\n> */\n> \n> -#include \"encoder_libjpeg.h\"\n> +#include \"post_processor_jpeg.h\"\n> +\n> +#include \"exif.h\"\n> +\n> +#include \"../camera_device.h\"\n> \n> #include \n> #include \n> @@ -25,6 +29,14 @@\n> \n> using namespace libcamera;\n> \n> +#define extract_va_arg(type, arg, last) \\\n> +{ \\\n> + va_list ap; \\\n> + va_start(ap, last); \\\n> + arg = va_arg(ap, type); \\\n> + va_end(ap); \\\n> +}\n> +\n> LOG_DEFINE_CATEGORY(JPEG)\n> \n> namespace {\n> @@ -67,7 +79,7 @@ const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)\n> \n> } /* namespace */\n> \n> -EncoderLibJpeg::EncoderLibJpeg()\n> +PostProcessorJpeg::PostProcessorJpeg()\n> \t: quality_(95)\n> {\n> \t/* \\todo Expand error handling coverage with a custom handler. */\n> @@ -76,12 +88,12 @@ EncoderLibJpeg::EncoderLibJpeg()\n> \tjpeg_create_compress(&compress_);\n> }\n> \n> -EncoderLibJpeg::~EncoderLibJpeg()\n> +PostProcessorJpeg::~PostProcessorJpeg()\n> {\n> \tjpeg_destroy_compress(&compress_);\n> }\n> \n> -int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> +int PostProcessorJpeg::configure(const StreamConfiguration &cfg)\n> {\n> \tconst struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);\n> \tif (info.colorSpace == JCS_UNKNOWN)\n> @@ -104,7 +116,67 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> \treturn 0;\n> }\n> \n> -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)\n> +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> +\t\t\t const libcamera::Span &destination,\n> +\t\t\t CameraMetadata *metadata, ...)\n> +{\n> +\tCameraDevice *device = nullptr;\n> +\textract_va_arg(CameraDevice *, device, metadata);\n> +\n> +\t/* Set EXIF metadata for various tags. */\n> +\tExif exif;\n> +\t/* \\todo Set Make and Model from external vendor tags. */\n> +\texif.setMake(\"libcamera\");\n> +\texif.setModel(\"cameraModel\");\n> +\texif.setOrientation(device->orientation());\n> +\texif.setSize(Size {compress_.image_width, compress_.image_height});\n> +\t/*\n> +\t * We set the frame's EXIF timestamp as the time of encode.\n> +\t * Since the precision we need for EXIF timestamp is only one\n> +\t * second, it is good enough.\n> +\t */\n> +\texif.setTimestamp(std::time(nullptr));\n> +\tif (exif.generate() != 0)\n> +\t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> +\n> +\tint jpeg_size = encode(source, destination, exif.data());\n> +\tif (jpeg_size < 0) {\n> +\t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n> +\t\treturn jpeg_size;\n> +\t}\n> +\n> +\t/*\n> +\t * Fill in the JPEG blob header.\n> +\t *\n> +\t * The mapped size of the buffer is being returned as\n> +\t * substantially larger than the requested JPEG_MAX_SIZE\n> +\t * (which is referenced from maxJpegBufferSize_). Utilise\n> +\t * this static size to ensure the correct offset of the blob is\n> +\t * determined.\n> +\t *\n> +\t * \\todo Investigate if the buffer size mismatch is an issue or\n> +\t * expected behaviour.\n> +\t */\n> +\tuint8_t *resultPtr = destination.data() +\n> +\t\t\t device->maxJpegBufferSize() -\n> +\t\t\t sizeof(struct camera3_jpeg_blob);\n> +\tauto *blob = reinterpret_cast(resultPtr);\n> +\tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n> +\tblob->jpeg_size = jpeg_size;\n> +\n> +\t/* Update the JPEG result Metadata. */\n> +\tmetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);\n> +\n> +\tconst uint32_t jpeg_quality = 95;\n> +\tmetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n> +\n> +\tconst uint32_t jpeg_orientation = 0;\n> +\tmetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n> +\n> +\treturn 0;\n> +}\n> +\n> +void PostProcessorJpeg::compressRGB(const libcamera::MappedBuffer *frame)\n> {\n> \tunsigned char *src = static_cast(frame->maps()[0].data());\n> \t/* \\todo Stride information should come from buffer configuration. */\n> @@ -122,7 +194,7 @@ void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)\n> * Compress the incoming buffer from a supported NV format.\n> * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.\n> */\n> -void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> +void PostProcessorJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> {\n> \tuint8_t tmprowbuf[compress_.image_width * 3];\n> \n> @@ -179,9 +251,9 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> \t}\n> }\n> \n> -int EncoderLibJpeg::encode(const FrameBuffer *source,\n> -\t\t\t const libcamera::Span &dest,\n> -\t\t\t const libcamera::Span &exifData)\n> +int PostProcessorJpeg::encode(const FrameBuffer *source,\n> +\t\t\t const libcamera::Span &dest,\n> +\t\t\t const libcamera::Span &exifData)\n> {\n> \tMappedFrameBuffer frame(source, PROT_READ);\n> \tif (!frame.isValid()) {\n> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> similarity index 55%\n> rename from src/android/jpeg/encoder_libjpeg.h\n> rename to src/android/jpeg/post_processor_jpeg.h\n> index 1e8df05..7f9ce70 100644\n> --- a/src/android/jpeg/encoder_libjpeg.h\n> +++ b/src/android/jpeg/post_processor_jpeg.h\n> @@ -2,30 +2,37 @@\n> /*\n> * Copyright (C) 2020, Google Inc.\n> *\n> - * encoder_libjpeg.h - JPEG encoding using libjpeg\n> + * post_processor_jpeg.h - JPEG Post Processor\n> */\n> -#ifndef __ANDROID_JPEG_ENCODER_LIBJPEG_H__\n> -#define __ANDROID_JPEG_ENCODER_LIBJPEG_H__\n> +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__\n> +#define __ANDROID_POST_PROCESSOR_JPEG_H__\n> \n> -#include \"encoder.h\"\n> +#include \"../post_processor.h\"\n> +#include \"../camera_metadata.h\"\n> \n> #include \"libcamera/internal/buffer.h\"\n> #include \"libcamera/internal/formats.h\"\n> \n> #include \n> \n> -class EncoderLibJpeg : public Encoder\n> +class PostProcessorJpeg : public PostProcessor\n> {\n> public:\n> -\tEncoderLibJpeg();\n> -\t~EncoderLibJpeg();\n> +\tPostProcessorJpeg();\n> +\t~PostProcessorJpeg();\n> \n> \tint configure(const libcamera::StreamConfiguration &cfg) override;\n> +\tint process(const libcamera::FrameBuffer *source,\n> +\t\t const libcamera::Span &destination,\n> +\t\t CameraMetadata *metadata,\n> +\t\t ...) override;\n> +\n> +\n> +private:\n> \tint encode(const libcamera::FrameBuffer *source,\n> \t\t const libcamera::Span &destination,\n> -\t\t const libcamera::Span &exifData) override;\n> +\t\t const libcamera::Span &exifData);\n> \n> -private:\n> \tvoid compressRGB(const libcamera::MappedBuffer *frame);\n> \tvoid compressNV(const libcamera::MappedBuffer *frame);\n> \n> @@ -40,4 +47,4 @@ private:\n> \tbool nvSwap_;\n> };\n> \n> -#endif /* __ANDROID_JPEG_ENCODER_LIBJPEG_H__ */\n> +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 802bb89..02b3b47 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -21,8 +21,8 @@ android_hal_sources = files([\n> 'camera_metadata.cpp',\n> 'camera_ops.cpp',\n> 'camera_stream.cpp',\n> - 'jpeg/encoder_libjpeg.cpp',\n> 'jpeg/exif.cpp',\n> + 'jpeg/post_processor_jpeg.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 CB9A0BEEE0\n\tfor ;\n\tThu, 8 Oct 2020 19:23:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 61F89605D1;\n\tThu, 8 Oct 2020 21:23:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1193660361\n\tfor ;\n\tThu, 8 Oct 2020 21:23:24 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7490C59E;\n\tThu, 8 Oct 2020 21:23:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ich5psTL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602185003;\n\tbh=jKLKkqXn+ivOmBVLNcC7AHhDTSdc+4LsuFXXW0CUxhA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ich5psTLnp5jgu4EYgR7Ejxl0SBgLQNkN6l3eA7c5y47Zsd9Witd91sxkTyn6SA8W\n\tqqBcWKx3+MK9XV9pKMW+X+DRogZhIoaA8mt7SKxhobroysGgaCl57NdccJXHuSDoyL\n\t7k95LJuN1UsyXeLVEb0AtpV2sw+bz9G7DH05xUYo=","Date":"Thu, 8 Oct 2020 22:22:40 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20201008192240.GE3939@pendragon.ideasonboard.com>","References":"<20201008141038.83425-1-email@uajain.com>\n\t<20201008141038.83425-3-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201008141038.83425-3-email@uajain.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 2/3] android: jpeg: Port to\n\tPostProcessor interface","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":13120,"web_url":"https://patchwork.libcamera.org/comment/13120/","msgid":"","date":"2020-10-09T04:34:18","subject":"Re: [libcamera-devel] [RFC PATCH 2/3] android: jpeg: Port to\n\tPostProcessor interface","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Laurent,\n\nThanks for the review.\n\nOn 10/9/20 12:52 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Thu, Oct 08, 2020 at 07:40:37PM +0530, Umang Jain wrote:\n>> Remove the existing Encoder interface completely and use the\n>> PostProcessor interface instead.\n> I think we need to keep the Encoder interface. We want to support other\n> JPEG encoders than the libjpeg-based implementation. Creating a JPEG\n> post-processor is the right thing to do, but it should still rely on the\n> existing encoder interface for the actual JPEG encoding. From a\n> CameraDevice point of view, only the PostProcessor interface will be\n> visible.\nOh. I didn't see that. I assumed we will have only one? encoder and even \nif we had more, it would simply encapsulated within a private function \nas PostProcessorJpeg::encodeViaX() or whatever. I will bring back the \nEncoder interface if you say so.\n\nIndeed, CameraDevice point-of-view, we only want to expose the \nPostProcessor interface.\n>\n>> Now the ::encode() function will be called by PostProcessor::process()\n>> internally and will not be directly exposed in CameraStream. Similarly,\n>> other operations can be introduced internally in the PostProcessorJpeg,\n>> and can be called through its process() interface.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.h | 1 -\n>> src/android/camera_stream.cpp | 74 +++------------\n>> src/android/camera_stream.h | 9 +-\n>> src/android/jpeg/encoder.h | 25 -----\n>> ...er_libjpeg.cpp => post_processor_jpeg.cpp} | 92 +++++++++++++++++--\n>> ...ncoder_libjpeg.h => post_processor_jpeg.h} | 27 ++++--\n>> src/android/meson.build | 2 +-\n>> 7 files changed, 122 insertions(+), 108 deletions(-)\n>> delete mode 100644 src/android/jpeg/encoder.h\n>> rename src/android/jpeg/{encoder_libjpeg.cpp => post_processor_jpeg.cpp} (67%)\n>> rename src/android/jpeg/{encoder_libjpeg.h => post_processor_jpeg.h} (55%)\n>>\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index 777d1a3..25de12e 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -25,7 +25,6 @@\n>> #include \"libcamera/internal/message.h\"\n>> \n>> #include \"camera_stream.h\"\n>> -#include \"jpeg/encoder.h\"\n>> \n>> class CameraMetadata;\n>> \n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index eac1480..ed3bb41 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -9,9 +9,7 @@\n>> \n>> #include \"camera_device.h\"\n>> #include \"camera_metadata.h\"\n>> -#include \"jpeg/encoder.h\"\n>> -#include \"jpeg/encoder_libjpeg.h\"\n>> -#include \"jpeg/exif.h\"\n>> +#include \"jpeg/post_processor_jpeg.h\"\n>> \n>> using namespace libcamera;\n>> \n>> @@ -24,8 +22,15 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n>> {\n>> \tconfig_ = cameraDevice_->cameraConfiguration();\n>> \n>> -\tif (type_ == Type::Internal || type_ == Type::Mapped)\n>> -\t\tencoder_ = std::make_unique();\n>> +\tif (type_ == Type::Internal || type_ == Type::Mapped) {\n>> +\t\t/*\n>> +\t\t * \\todo There might be multiple post-processors. The logic\n>> +\t\t * which should be instantiated here, is deferred for future.\n>> +\t\t * For now, we only have PostProcessJpeg and that is what we\n> s/PostProcessJpeg/PostProcessorJpeg/\n>\n>> +\t\t * will instantiate here.\n>> +\t\t */\n>> +\t\tpostProcessor_ = std::make_unique();\n>> +\t}\n>> \n>> \tif (type == Type::Internal) {\n>> \t\tallocator_ = std::make_unique(cameraDevice_->camera());\n>> @@ -45,8 +50,8 @@ Stream *CameraStream::stream() const\n>> \n>> int CameraStream::configure()\n>> {\n>> -\tif (encoder_) {\n>> -\t\tint ret = encoder_->configure(configuration());\n>> +\tif (postProcessor_) {\n>> +\t\tint ret = postProcessor_->configure(configuration());\n>> \t\tif (ret)\n>> \t\t\treturn ret;\n>> \t}\n>> @@ -69,60 +74,11 @@ int CameraStream::configure()\n>> int CameraStream::process(const libcamera::FrameBuffer &source,\n>> \t\t\t MappedCamera3Buffer *dest, CameraMetadata *metadata)\n>> {\n>> -\tif (!encoder_)\n>> +\tif (!postProcessor_)\n>> \t\treturn 0;\n>> \n>> -\t/* Set EXIF metadata for various tags. */\n>> -\tExif exif;\n>> -\t/* \\todo Set Make and Model from external vendor tags. */\n>> -\texif.setMake(\"libcamera\");\n>> -\texif.setModel(\"cameraModel\");\n>> -\texif.setOrientation(cameraDevice_->orientation());\n>> -\texif.setSize(configuration().size);\n>> -\t/*\n>> -\t * We set the frame's EXIF timestamp as the time of encode.\n>> -\t * Since the precision we need for EXIF timestamp is only one\n>> -\t * second, it is good enough.\n>> -\t */\n>> -\texif.setTimestamp(std::time(nullptr));\n>> -\tif (exif.generate() != 0)\n>> -\t\tLOG(HAL, Error) << \"Failed to generate valid EXIF data\";\n>> -\n>> -\tint jpeg_size = encoder_->encode(&source, dest->maps()[0], exif.data());\n>> -\tif (jpeg_size < 0) {\n>> -\t\tLOG(HAL, Error) << \"Failed to encode stream image\";\n>> -\t\treturn jpeg_size;\n>> -\t}\n>> -\n>> -\t/*\n>> -\t * Fill in the JPEG blob header.\n>> -\t *\n>> -\t * The mapped size of the buffer is being returned as\n>> -\t * substantially larger than the requested JPEG_MAX_SIZE\n>> -\t * (which is referenced from maxJpegBufferSize_). Utilise\n>> -\t * this static size to ensure the correct offset of the blob is\n>> -\t * determined.\n>> -\t *\n>> -\t * \\todo Investigate if the buffer size mismatch is an issue or\n>> -\t * expected behaviour.\n>> -\t */\n>> -\tuint8_t *resultPtr = dest->maps()[0].data() +\n>> -\t\t\t cameraDevice_->maxJpegBufferSize() -\n>> -\t\t\t sizeof(struct camera3_jpeg_blob);\n>> -\tauto *blob = reinterpret_cast(resultPtr);\n>> -\tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n>> -\tblob->jpeg_size = jpeg_size;\n>> -\n>> -\t/* Update the JPEG result Metadata. */\n>> -\tmetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);\n>> -\n>> -\tconst uint32_t jpeg_quality = 95;\n>> -\tmetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n>> -\n>> -\tconst uint32_t jpeg_orientation = 0;\n>> -\tmetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n>> -\n>> -\treturn 0;\n>> +\treturn postProcessor_->process(&source, dest->maps()[0],\n>> +\t\t\t\t metadata, cameraDevice_);\n> There are at least two options to keep the interface generic, avoiding\n> variadic arguments. One of them is to explicitly pass the the camera\n> device to the process function. Another option is to pass it to the\n> PostProcessorJpeg constructor, and store it internally. I'd go for the\n> latter.\nI see. I thought vardiac arguments can be helpful for \n(future)PostProcessors to have the flexibility of parameters they might \nneed when they process(). But I do get your point that they might defeat \nthe entire purpose of keeping the interface generic. Asking for \ncuriousity, is passing the specifics parameters via respective \nPostProcessors' constructors (instead of vardiac args) is the way you \nwould suggest in general to achieve this goal?\n>\n>> }\n>> \n>> FrameBuffer *CameraStream::getBuffer()\n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index 8df0101..8d0f2e3 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -19,7 +19,12 @@\n>> #include \n>> #include \n>> \n>> -class Encoder;\n>> +/*\n>> + * \\todo Ideally we want to replace this include with forward-declaration.\n>> + * If we do that. currently we get a compile error.\n>> + */\n> Let's fix them :-) What compilation error do you get ?\n>\n>> +#include \"post_processor.h\"\n>> +\n>> class CameraDevice;\n>> class CameraMetadata;\n>> class MappedCamera3Buffer;\n>> @@ -135,7 +140,7 @@ private:\n>> \t */\n>> \tunsigned int index_;\n>> \n>> -\tstd::unique_ptr encoder_;\n>> +\tstd::unique_ptr postProcessor_;\n>> \tstd::unique_ptr allocator_;\n>> \tstd::vector buffers_;\n>> \t/*\n>> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n>> deleted file mode 100644\n>> index cf26d67..0000000\n>> --- a/src/android/jpeg/encoder.h\n>> +++ /dev/null\n>> @@ -1,25 +0,0 @@\n>> -/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> -/*\n>> - * Copyright (C) 2020, Google Inc.\n>> - *\n>> - * encoder.h - Image encoding interface\n>> - */\n>> -#ifndef __ANDROID_JPEG_ENCODER_H__\n>> -#define __ANDROID_JPEG_ENCODER_H__\n>> -\n>> -#include \n>> -#include \n>> -#include \n>> -\n>> -class Encoder\n>> -{\n>> -public:\n>> -\tvirtual ~Encoder() {};\n>> -\n>> -\tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n>> -\tvirtual int encode(const libcamera::FrameBuffer *source,\n>> -\t\t\t const libcamera::Span &destination,\n>> -\t\t\t const libcamera::Span &exifData) = 0;\n>> -};\n>> -\n>> -#endif /* __ANDROID_JPEG_ENCODER_H__ */\n> As mentioned above, this should be kept, and used by the JPEG\n> post-processor.\n>\n>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n>> similarity index 67%\n>> rename from src/android/jpeg/encoder_libjpeg.cpp\n>> rename to src/android/jpeg/post_processor_jpeg.cpp\n>> index 510613c..eeb4e95 100644\n>> --- a/src/android/jpeg/encoder_libjpeg.cpp\n>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>> @@ -2,10 +2,14 @@\n>> /*\n>> * Copyright (C) 2020, Google Inc.\n>> *\n>> - * encoder_libjpeg.cpp - JPEG encoding using libjpeg native API\n>> + * post_processor_jpeg.cpp - JPEG Post Processor\n>> */\n>> \n>> -#include \"encoder_libjpeg.h\"\n>> +#include \"post_processor_jpeg.h\"\n>> +\n>> +#include \"exif.h\"\n>> +\n>> +#include \"../camera_device.h\"\n>> \n>> #include \n>> #include \n>> @@ -25,6 +29,14 @@\n>> \n>> using namespace libcamera;\n>> \n>> +#define extract_va_arg(type, arg, last) \\\n>> +{ \\\n>> + va_list ap; \\\n>> + va_start(ap, last); \\\n>> + arg = va_arg(ap, type); \\\n>> + va_end(ap); \\\n>> +}\n>> +\n>> LOG_DEFINE_CATEGORY(JPEG)\n>> \n>> namespace {\n>> @@ -67,7 +79,7 @@ const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)\n>> \n>> } /* namespace */\n>> \n>> -EncoderLibJpeg::EncoderLibJpeg()\n>> +PostProcessorJpeg::PostProcessorJpeg()\n>> \t: quality_(95)\n>> {\n>> \t/* \\todo Expand error handling coverage with a custom handler. */\n>> @@ -76,12 +88,12 @@ EncoderLibJpeg::EncoderLibJpeg()\n>> \tjpeg_create_compress(&compress_);\n>> }\n>> \n>> -EncoderLibJpeg::~EncoderLibJpeg()\n>> +PostProcessorJpeg::~PostProcessorJpeg()\n>> {\n>> \tjpeg_destroy_compress(&compress_);\n>> }\n>> \n>> -int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n>> +int PostProcessorJpeg::configure(const StreamConfiguration &cfg)\n>> {\n>> \tconst struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);\n>> \tif (info.colorSpace == JCS_UNKNOWN)\n>> @@ -104,7 +116,67 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n>> \treturn 0;\n>> }\n>> \n>> -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)\n>> +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n>> +\t\t\t const libcamera::Span &destination,\n>> +\t\t\t CameraMetadata *metadata, ...)\n>> +{\n>> +\tCameraDevice *device = nullptr;\n>> +\textract_va_arg(CameraDevice *, device, metadata);\n>> +\n>> +\t/* Set EXIF metadata for various tags. */\n>> +\tExif exif;\n>> +\t/* \\todo Set Make and Model from external vendor tags. */\n>> +\texif.setMake(\"libcamera\");\n>> +\texif.setModel(\"cameraModel\");\n>> +\texif.setOrientation(device->orientation());\n>> +\texif.setSize(Size {compress_.image_width, compress_.image_height});\n>> +\t/*\n>> +\t * We set the frame's EXIF timestamp as the time of encode.\n>> +\t * Since the precision we need for EXIF timestamp is only one\n>> +\t * second, it is good enough.\n>> +\t */\n>> +\texif.setTimestamp(std::time(nullptr));\n>> +\tif (exif.generate() != 0)\n>> +\t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n>> +\n>> +\tint jpeg_size = encode(source, destination, exif.data());\n>> +\tif (jpeg_size < 0) {\n>> +\t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n>> +\t\treturn jpeg_size;\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Fill in the JPEG blob header.\n>> +\t *\n>> +\t * The mapped size of the buffer is being returned as\n>> +\t * substantially larger than the requested JPEG_MAX_SIZE\n>> +\t * (which is referenced from maxJpegBufferSize_). Utilise\n>> +\t * this static size to ensure the correct offset of the blob is\n>> +\t * determined.\n>> +\t *\n>> +\t * \\todo Investigate if the buffer size mismatch is an issue or\n>> +\t * expected behaviour.\n>> +\t */\n>> +\tuint8_t *resultPtr = destination.data() +\n>> +\t\t\t device->maxJpegBufferSize() -\n>> +\t\t\t sizeof(struct camera3_jpeg_blob);\n>> +\tauto *blob = reinterpret_cast(resultPtr);\n>> +\tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n>> +\tblob->jpeg_size = jpeg_size;\n>> +\n>> +\t/* Update the JPEG result Metadata. */\n>> +\tmetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);\n>> +\n>> +\tconst uint32_t jpeg_quality = 95;\n>> +\tmetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n>> +\n>> +\tconst uint32_t jpeg_orientation = 0;\n>> +\tmetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +void PostProcessorJpeg::compressRGB(const libcamera::MappedBuffer *frame)\n>> {\n>> \tunsigned char *src = static_cast(frame->maps()[0].data());\n>> \t/* \\todo Stride information should come from buffer configuration. */\n>> @@ -122,7 +194,7 @@ void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)\n>> * Compress the incoming buffer from a supported NV format.\n>> * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.\n>> */\n>> -void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n>> +void PostProcessorJpeg::compressNV(const libcamera::MappedBuffer *frame)\n>> {\n>> \tuint8_t tmprowbuf[compress_.image_width * 3];\n>> \n>> @@ -179,9 +251,9 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n>> \t}\n>> }\n>> \n>> -int EncoderLibJpeg::encode(const FrameBuffer *source,\n>> -\t\t\t const libcamera::Span &dest,\n>> -\t\t\t const libcamera::Span &exifData)\n>> +int PostProcessorJpeg::encode(const FrameBuffer *source,\n>> +\t\t\t const libcamera::Span &dest,\n>> +\t\t\t const libcamera::Span &exifData)\n>> {\n>> \tMappedFrameBuffer frame(source, PROT_READ);\n>> \tif (!frame.isValid()) {\n>> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/post_processor_jpeg.h\n>> similarity index 55%\n>> rename from src/android/jpeg/encoder_libjpeg.h\n>> rename to src/android/jpeg/post_processor_jpeg.h\n>> index 1e8df05..7f9ce70 100644\n>> --- a/src/android/jpeg/encoder_libjpeg.h\n>> +++ b/src/android/jpeg/post_processor_jpeg.h\n>> @@ -2,30 +2,37 @@\n>> /*\n>> * Copyright (C) 2020, Google Inc.\n>> *\n>> - * encoder_libjpeg.h - JPEG encoding using libjpeg\n>> + * post_processor_jpeg.h - JPEG Post Processor\n>> */\n>> -#ifndef __ANDROID_JPEG_ENCODER_LIBJPEG_H__\n>> -#define __ANDROID_JPEG_ENCODER_LIBJPEG_H__\n>> +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__\n>> +#define __ANDROID_POST_PROCESSOR_JPEG_H__\n>> \n>> -#include \"encoder.h\"\n>> +#include \"../post_processor.h\"\n>> +#include \"../camera_metadata.h\"\n>> \n>> #include \"libcamera/internal/buffer.h\"\n>> #include \"libcamera/internal/formats.h\"\n>> \n>> #include \n>> \n>> -class EncoderLibJpeg : public Encoder\n>> +class PostProcessorJpeg : public PostProcessor\n>> {\n>> public:\n>> -\tEncoderLibJpeg();\n>> -\t~EncoderLibJpeg();\n>> +\tPostProcessorJpeg();\n>> +\t~PostProcessorJpeg();\n>> \n>> \tint configure(const libcamera::StreamConfiguration &cfg) override;\n>> +\tint process(const libcamera::FrameBuffer *source,\n>> +\t\t const libcamera::Span &destination,\n>> +\t\t CameraMetadata *metadata,\n>> +\t\t ...) override;\n>> +\n>> +\n>> +private:\n>> \tint encode(const libcamera::FrameBuffer *source,\n>> \t\t const libcamera::Span &destination,\n>> -\t\t const libcamera::Span &exifData) override;\n>> +\t\t const libcamera::Span &exifData);\n>> \n>> -private:\n>> \tvoid compressRGB(const libcamera::MappedBuffer *frame);\n>> \tvoid compressNV(const libcamera::MappedBuffer *frame);\n>> \n>> @@ -40,4 +47,4 @@ private:\n>> \tbool nvSwap_;\n>> };\n>> \n>> -#endif /* __ANDROID_JPEG_ENCODER_LIBJPEG_H__ */\n>> +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n>> diff --git a/src/android/meson.build b/src/android/meson.build\n>> index 802bb89..02b3b47 100644\n>> --- a/src/android/meson.build\n>> +++ b/src/android/meson.build\n>> @@ -21,8 +21,8 @@ android_hal_sources = files([\n>> 'camera_metadata.cpp',\n>> 'camera_ops.cpp',\n>> 'camera_stream.cpp',\n>> - 'jpeg/encoder_libjpeg.cpp',\n>> 'jpeg/exif.cpp',\n>> + 'jpeg/post_processor_jpeg.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 A8CE0BEEE0\n\tfor ;\n\tFri, 9 Oct 2020 04:34:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 37668605D1;\n\tFri, 9 Oct 2020 06:34:26 +0200 (CEST)","from mail.uajain.com (static.126.159.217.95.clients.your-server.de\n\t[95.217.159.126])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A00D600F2\n\tfor ;\n\tFri, 9 Oct 2020 06:34:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=uajain.com header.i=@uajain.com\n\theader.b=\"vc400o92\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1602218062; bh=OYt9Ss/oX7ivkMEzgF4Z9kbYN70DuycgbL3eLsrEVOA=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=vc400o92nBh3FGnV0innFCIStnXdxze+J7xBwyUKwa40GA4H5Yerml8hA9YTkgGs3\n\tj58JKb1v4N92+EAvms3jsIybiU2VlR122dVRIuiqplIrojxLmDeEXTyQz0cjrgSKRo\n\tvjKB6XsaQdxvnydUPrkAMQSwbUV2FlN4nW+/afyiEM4q3AbKG1SNXFqrLidQaOilYo\n\twamL94/rn8OQgGhYXfkOeDW8K53s4O9pJKqHX6LtW/gRa0maTsrL3dhQjC+Hgw/ka8\n\t8A6UkFwHwxiADlqltFQ3vYVlWZG6h51OU+PWH4s9ll5Gb3LImItXZ5HLx+SiJvgCMo\n\tUtUd8Y1+fwM1A==","To":"Laurent Pinchart ","References":"<20201008141038.83425-1-email@uajain.com>\n\t<20201008141038.83425-3-email@uajain.com>\n\t<20201008192240.GE3939@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Fri, 9 Oct 2020 10:04:18 +0530","Mime-Version":"1.0","In-Reply-To":"<20201008192240.GE3939@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH 2/3] android: jpeg: Port to\n\tPostProcessor interface","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":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13146,"web_url":"https://patchwork.libcamera.org/comment/13146/","msgid":"","date":"2020-10-09T14:12:48","subject":"Re: [libcamera-devel] [RFC PATCH 2/3] android: jpeg: Port to\n\tPostProcessor interface","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Laurent,\n\nForgot to follow up at one more point.\n\nOn 10/9/20 12:52 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Thu, Oct 08, 2020 at 07:40:37PM +0530, Umang Jain wrote:\n>> Remove the existing Encoder interface completely and use the\n>> PostProcessor interface instead.\n> I think we need to keep the Encoder interface. We want to support other\n> JPEG encoders than the libjpeg-based implementation. Creating a JPEG\n> post-processor is the right thing to do, but it should still rely on the\n> existing encoder interface for the actual JPEG encoding. From a\n> CameraDevice point of view, only the PostProcessor interface will be\n> visible.\n>\n>> Now the ::encode() function will be called by PostProcessor::process()\n>> internally and will not be directly exposed in CameraStream. Similarly,\n>> other operations can be introduced internally in the PostProcessorJpeg,\n>> and can be called through its process() interface.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.h | 1 -\n>> src/android/camera_stream.cpp | 74 +++------------\n>> src/android/camera_stream.h | 9 +-\n>> src/android/jpeg/encoder.h | 25 -----\n>> ...er_libjpeg.cpp => post_processor_jpeg.cpp} | 92 +++++++++++++++++--\n>> ...ncoder_libjpeg.h => post_processor_jpeg.h} | 27 ++++--\n>> src/android/meson.build | 2 +-\n>> 7 files changed, 122 insertions(+), 108 deletions(-)\n>> delete mode 100644 src/android/jpeg/encoder.h\n>> rename src/android/jpeg/{encoder_libjpeg.cpp => post_processor_jpeg.cpp} (67%)\n>> rename src/android/jpeg/{encoder_libjpeg.h => post_processor_jpeg.h} (55%)\n>>\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index 777d1a3..25de12e 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -25,7 +25,6 @@\n>> #include \"libcamera/internal/message.h\"\n>> \n>> #include \"camera_stream.h\"\n>> -#include \"jpeg/encoder.h\"\n>> \n>> class CameraMetadata;\n>> \n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index eac1480..ed3bb41 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -9,9 +9,7 @@\n>> \n>> #include \"camera_device.h\"\n>> #include \"camera_metadata.h\"\n>> -#include \"jpeg/encoder.h\"\n>> -#include \"jpeg/encoder_libjpeg.h\"\n>> -#include \"jpeg/exif.h\"\n>> +#include \"jpeg/post_processor_jpeg.h\"\n>> \n>> using namespace libcamera;\n>> \n>> @@ -24,8 +22,15 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n>> {\n>> \tconfig_ = cameraDevice_->cameraConfiguration();\n>> \n>> -\tif (type_ == Type::Internal || type_ == Type::Mapped)\n>> -\t\tencoder_ = std::make_unique();\n>> +\tif (type_ == Type::Internal || type_ == Type::Mapped) {\n>> +\t\t/*\n>> +\t\t * \\todo There might be multiple post-processors. The logic\n>> +\t\t * which should be instantiated here, is deferred for future.\n>> +\t\t * For now, we only have PostProcessJpeg and that is what we\n> s/PostProcessJpeg/PostProcessorJpeg/\n>\n>> +\t\t * will instantiate here.\n>> +\t\t */\n>> +\t\tpostProcessor_ = std::make_unique();\n>> +\t}\n>> \n>> \tif (type == Type::Internal) {\n>> \t\tallocator_ = std::make_unique(cameraDevice_->camera());\n>> @@ -45,8 +50,8 @@ Stream *CameraStream::stream() const\n>> \n>> int CameraStream::configure()\n>> {\n>> -\tif (encoder_) {\n>> -\t\tint ret = encoder_->configure(configuration());\n>> +\tif (postProcessor_) {\n>> +\t\tint ret = postProcessor_->configure(configuration());\n>> \t\tif (ret)\n>> \t\t\treturn ret;\n>> \t}\n>> @@ -69,60 +74,11 @@ int CameraStream::configure()\n>> int CameraStream::process(const libcamera::FrameBuffer &source,\n>> \t\t\t MappedCamera3Buffer *dest, CameraMetadata *metadata)\n>> {\n>> -\tif (!encoder_)\n>> +\tif (!postProcessor_)\n>> \t\treturn 0;\n>> \n>> -\t/* Set EXIF metadata for various tags. */\n>> -\tExif exif;\n>> -\t/* \\todo Set Make and Model from external vendor tags. */\n>> -\texif.setMake(\"libcamera\");\n>> -\texif.setModel(\"cameraModel\");\n>> -\texif.setOrientation(cameraDevice_->orientation());\n>> -\texif.setSize(configuration().size);\n>> -\t/*\n>> -\t * We set the frame's EXIF timestamp as the time of encode.\n>> -\t * Since the precision we need for EXIF timestamp is only one\n>> -\t * second, it is good enough.\n>> -\t */\n>> -\texif.setTimestamp(std::time(nullptr));\n>> -\tif (exif.generate() != 0)\n>> -\t\tLOG(HAL, Error) << \"Failed to generate valid EXIF data\";\n>> -\n>> -\tint jpeg_size = encoder_->encode(&source, dest->maps()[0], exif.data());\n>> -\tif (jpeg_size < 0) {\n>> -\t\tLOG(HAL, Error) << \"Failed to encode stream image\";\n>> -\t\treturn jpeg_size;\n>> -\t}\n>> -\n>> -\t/*\n>> -\t * Fill in the JPEG blob header.\n>> -\t *\n>> -\t * The mapped size of the buffer is being returned as\n>> -\t * substantially larger than the requested JPEG_MAX_SIZE\n>> -\t * (which is referenced from maxJpegBufferSize_). Utilise\n>> -\t * this static size to ensure the correct offset of the blob is\n>> -\t * determined.\n>> -\t *\n>> -\t * \\todo Investigate if the buffer size mismatch is an issue or\n>> -\t * expected behaviour.\n>> -\t */\n>> -\tuint8_t *resultPtr = dest->maps()[0].data() +\n>> -\t\t\t cameraDevice_->maxJpegBufferSize() -\n>> -\t\t\t sizeof(struct camera3_jpeg_blob);\n>> -\tauto *blob = reinterpret_cast(resultPtr);\n>> -\tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n>> -\tblob->jpeg_size = jpeg_size;\n>> -\n>> -\t/* Update the JPEG result Metadata. */\n>> -\tmetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);\n>> -\n>> -\tconst uint32_t jpeg_quality = 95;\n>> -\tmetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n>> -\n>> -\tconst uint32_t jpeg_orientation = 0;\n>> -\tmetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n>> -\n>> -\treturn 0;\n>> +\treturn postProcessor_->process(&source, dest->maps()[0],\n>> +\t\t\t\t metadata, cameraDevice_);\n> There are at least two options to keep the interface generic, avoiding\n> variadic arguments. One of them is to explicitly pass the the camera\n> device to the process function. Another option is to pass it to the\n> PostProcessorJpeg constructor, and store it internally. I'd go for the\n> latter.\n>\n>> }\n>> \n>> FrameBuffer *CameraStream::getBuffer()\n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index 8df0101..8d0f2e3 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -19,7 +19,12 @@\n>> #include \n>> #include \n>> \n>> -class Encoder;\n>> +/*\n>> + * \\todo Ideally we want to replace this include with forward-declaration.\n>> + * If we do that. currently we get a compile error.\n>> + */\n> Let's fix them :-) What compilation error do you get ?\nThe error I see:\n\n[89/298] Compiling C++ object \nsrc/libcamera/libcamera.so.p/.._android_camera_device.cpp.o\nFAILED: src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o\nc++ -Isrc/libcamera/libcamera.so.p -Isrc/libcamera -I../src/libcamera \n-Iinclude -I../include -I../include/android/hardware/libhardware/include \n-I../include/android/metadata -I../include/android/system/core/include \n-Iinclude/libcamera -fdiagnostics-color=always -pipe \n-D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra \n-Werror -std=c++17 -g -include config.h -fPIC -pthread -MD -MQ \nsrc/libcamera/libcamera.so.p/.._android_camera_device.cpp.o -MF \nsrc/libcamera/libcamera.so.p/.._android_camera_device.cpp.o.d -o \nsrc/libcamera/libcamera.so.p/.._android_camera_device.cpp.o -c \n../src/android/camera_device.cpp\nIn file included from /usr/include/c++/10/memory:83,\n                  from ../src/android/camera_device.h:11,\n                  from ../src/android/camera_device.cpp:8:\n/usr/include/c++/10/bits/unique_ptr.h: In instantiation of ‘void \nstd::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = \nPostProcessor]’:\n/usr/include/c++/10/bits/unique_ptr.h:360:17:   required from \n‘std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = PostProcessor; _Dp \n= std::default_delete]’\n../src/android/camera_stream.h:32:7:   required from ‘void \n__gnu_cxx::new_allocator<_Tp>::destroy(_Up*) [with _Up = CameraStream; \n_Tp = CameraStream]’\n/usr/include/c++/10/bits/alloc_traits.h:531:15:   required from ‘static \nvoid std::allocator_traits \n >::destroy(std::allocator_traits \n >::allocator_type&, _Up*) [with _Up = CameraStream; _Tp = CameraStream; \nstd::allocator_traits >::allocator_type = \nstd::allocator]’\n/usr/include/c++/10/bits/vector.tcc:488:28:   required from ‘void \nstd::vector<_Tp, _Alloc>::_M_realloc_insert(std::vector<_Tp, \n_Alloc>::iterator, _Args&& ...) [with _Args = {CameraDevice*, \nCameraStream::Type, camera3_stream*&, long unsigned int}; _Tp = \nCameraStream; _Alloc = std::allocator; std::vector<_Tp, \n_Alloc>::iterator = std::vector::iterator]’\n/usr/include/c++/10/bits/vector.tcc:121:21:   required from \n‘std::vector<_Tp, _Alloc>::reference std::vector<_Tp, \n_Alloc>::emplace_back(_Args&& ...) [with _Args = {CameraDevice*, \nCameraStream::Type, camera3_stream*&, long unsigned int}; _Tp = \nCameraStream; _Alloc = std::allocator; std::vector<_Tp, \n_Alloc>::reference = CameraStream&]’\n../src/android/camera_device.cpp:1212:38:   required from here\n/usr/include/c++/10/bits/unique_ptr.h:82:16: error: invalid application \nof ‘sizeof’ to incomplete type ‘PostProcessor’\n    82 |  static_assert(sizeof(_Tp)>0,\n       |                ^~~~~~~~~~~\n[98/298] Compiling C++ object \nsrc/libcamera/libcamera.so.p/.._android_camera_stream.cpp.o\nninja: build stopped: subcommand failed.\n\nNot sure why it cannot work with the incomplete type `PostProcessor` \ninterface\n(well, it used to work well with Encoder's interface, right there!)\n>\n>> +#include \"post_processor.h\"\n>> +\n>> class CameraDevice;\n>> class CameraMetadata;\n>> class MappedCamera3Buffer;\n>> @@ -135,7 +140,7 @@ private:\n>> \t */\n>> \tunsigned int index_;\n>> \n>> -\tstd::unique_ptr encoder_;\n>> +\tstd::unique_ptr postProcessor_;\n>> \tstd::unique_ptr allocator_;\n>> \tstd::vector buffers_;\n>> \t/*\n>> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n>> deleted file mode 100644\n>> index cf26d67..0000000\n>> --- a/src/android/jpeg/encoder.h\n>> +++ /dev/null\n>> @@ -1,25 +0,0 @@\n>> -/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> -/*\n>> - * Copyright (C) 2020, Google Inc.\n>> - *\n>> - * encoder.h - Image encoding interface\n>> - */\n>> -#ifndef __ANDROID_JPEG_ENCODER_H__\n>> -#define __ANDROID_JPEG_ENCODER_H__\n>> -\n>> -#include \n>> -#include \n>> -#include \n>> -\n>> -class Encoder\n>> -{\n>> -public:\n>> -\tvirtual ~Encoder() {};\n>> -\n>> -\tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n>> -\tvirtual int encode(const libcamera::FrameBuffer *source,\n>> -\t\t\t const libcamera::Span &destination,\n>> -\t\t\t const libcamera::Span &exifData) = 0;\n>> -};\n>> -\n>> -#endif /* __ANDROID_JPEG_ENCODER_H__ */\n> As mentioned above, this should be kept, and used by the JPEG\n> post-processor.\n>\n>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n>> similarity index 67%\n>> rename from src/android/jpeg/encoder_libjpeg.cpp\n>> rename to src/android/jpeg/post_processor_jpeg.cpp\n>> index 510613c..eeb4e95 100644\n>> --- a/src/android/jpeg/encoder_libjpeg.cpp\n>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>> @@ -2,10 +2,14 @@\n>> /*\n>> * Copyright (C) 2020, Google Inc.\n>> *\n>> - * encoder_libjpeg.cpp - JPEG encoding using libjpeg native API\n>> + * post_processor_jpeg.cpp - JPEG Post Processor\n>> */\n>> \n>> -#include \"encoder_libjpeg.h\"\n>> +#include \"post_processor_jpeg.h\"\n>> +\n>> +#include \"exif.h\"\n>> +\n>> +#include \"../camera_device.h\"\n>> \n>> #include \n>> #include \n>> @@ -25,6 +29,14 @@\n>> \n>> using namespace libcamera;\n>> \n>> +#define extract_va_arg(type, arg, last) \\\n>> +{ \\\n>> + va_list ap; \\\n>> + va_start(ap, last); \\\n>> + arg = va_arg(ap, type); \\\n>> + va_end(ap); \\\n>> +}\n>> +\n>> LOG_DEFINE_CATEGORY(JPEG)\n>> \n>> namespace {\n>> @@ -67,7 +79,7 @@ const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)\n>> \n>> } /* namespace */\n>> \n>> -EncoderLibJpeg::EncoderLibJpeg()\n>> +PostProcessorJpeg::PostProcessorJpeg()\n>> \t: quality_(95)\n>> {\n>> \t/* \\todo Expand error handling coverage with a custom handler. */\n>> @@ -76,12 +88,12 @@ EncoderLibJpeg::EncoderLibJpeg()\n>> \tjpeg_create_compress(&compress_);\n>> }\n>> \n>> -EncoderLibJpeg::~EncoderLibJpeg()\n>> +PostProcessorJpeg::~PostProcessorJpeg()\n>> {\n>> \tjpeg_destroy_compress(&compress_);\n>> }\n>> \n>> -int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n>> +int PostProcessorJpeg::configure(const StreamConfiguration &cfg)\n>> {\n>> \tconst struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);\n>> \tif (info.colorSpace == JCS_UNKNOWN)\n>> @@ -104,7 +116,67 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n>> \treturn 0;\n>> }\n>> \n>> -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)\n>> +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n>> +\t\t\t const libcamera::Span &destination,\n>> +\t\t\t CameraMetadata *metadata, ...)\n>> +{\n>> +\tCameraDevice *device = nullptr;\n>> +\textract_va_arg(CameraDevice *, device, metadata);\n>> +\n>> +\t/* Set EXIF metadata for various tags. */\n>> +\tExif exif;\n>> +\t/* \\todo Set Make and Model from external vendor tags. */\n>> +\texif.setMake(\"libcamera\");\n>> +\texif.setModel(\"cameraModel\");\n>> +\texif.setOrientation(device->orientation());\n>> +\texif.setSize(Size {compress_.image_width, compress_.image_height});\n>> +\t/*\n>> +\t * We set the frame's EXIF timestamp as the time of encode.\n>> +\t * Since the precision we need for EXIF timestamp is only one\n>> +\t * second, it is good enough.\n>> +\t */\n>> +\texif.setTimestamp(std::time(nullptr));\n>> +\tif (exif.generate() != 0)\n>> +\t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n>> +\n>> +\tint jpeg_size = encode(source, destination, exif.data());\n>> +\tif (jpeg_size < 0) {\n>> +\t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n>> +\t\treturn jpeg_size;\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Fill in the JPEG blob header.\n>> +\t *\n>> +\t * The mapped size of the buffer is being returned as\n>> +\t * substantially larger than the requested JPEG_MAX_SIZE\n>> +\t * (which is referenced from maxJpegBufferSize_). Utilise\n>> +\t * this static size to ensure the correct offset of the blob is\n>> +\t * determined.\n>> +\t *\n>> +\t * \\todo Investigate if the buffer size mismatch is an issue or\n>> +\t * expected behaviour.\n>> +\t */\n>> +\tuint8_t *resultPtr = destination.data() +\n>> +\t\t\t device->maxJpegBufferSize() -\n>> +\t\t\t sizeof(struct camera3_jpeg_blob);\n>> +\tauto *blob = reinterpret_cast(resultPtr);\n>> +\tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n>> +\tblob->jpeg_size = jpeg_size;\n>> +\n>> +\t/* Update the JPEG result Metadata. */\n>> +\tmetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);\n>> +\n>> +\tconst uint32_t jpeg_quality = 95;\n>> +\tmetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n>> +\n>> +\tconst uint32_t jpeg_orientation = 0;\n>> +\tmetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +void PostProcessorJpeg::compressRGB(const libcamera::MappedBuffer *frame)\n>> {\n>> \tunsigned char *src = static_cast(frame->maps()[0].data());\n>> \t/* \\todo Stride information should come from buffer configuration. */\n>> @@ -122,7 +194,7 @@ void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)\n>> * Compress the incoming buffer from a supported NV format.\n>> * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.\n>> */\n>> -void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n>> +void PostProcessorJpeg::compressNV(const libcamera::MappedBuffer *frame)\n>> {\n>> \tuint8_t tmprowbuf[compress_.image_width * 3];\n>> \n>> @@ -179,9 +251,9 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n>> \t}\n>> }\n>> \n>> -int EncoderLibJpeg::encode(const FrameBuffer *source,\n>> -\t\t\t const libcamera::Span &dest,\n>> -\t\t\t const libcamera::Span &exifData)\n>> +int PostProcessorJpeg::encode(const FrameBuffer *source,\n>> +\t\t\t const libcamera::Span &dest,\n>> +\t\t\t const libcamera::Span &exifData)\n>> {\n>> \tMappedFrameBuffer frame(source, PROT_READ);\n>> \tif (!frame.isValid()) {\n>> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/post_processor_jpeg.h\n>> similarity index 55%\n>> rename from src/android/jpeg/encoder_libjpeg.h\n>> rename to src/android/jpeg/post_processor_jpeg.h\n>> index 1e8df05..7f9ce70 100644\n>> --- a/src/android/jpeg/encoder_libjpeg.h\n>> +++ b/src/android/jpeg/post_processor_jpeg.h\n>> @@ -2,30 +2,37 @@\n>> /*\n>> * Copyright (C) 2020, Google Inc.\n>> *\n>> - * encoder_libjpeg.h - JPEG encoding using libjpeg\n>> + * post_processor_jpeg.h - JPEG Post Processor\n>> */\n>> -#ifndef __ANDROID_JPEG_ENCODER_LIBJPEG_H__\n>> -#define __ANDROID_JPEG_ENCODER_LIBJPEG_H__\n>> +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__\n>> +#define __ANDROID_POST_PROCESSOR_JPEG_H__\n>> \n>> -#include \"encoder.h\"\n>> +#include \"../post_processor.h\"\n>> +#include \"../camera_metadata.h\"\n>> \n>> #include \"libcamera/internal/buffer.h\"\n>> #include \"libcamera/internal/formats.h\"\n>> \n>> #include \n>> \n>> -class EncoderLibJpeg : public Encoder\n>> +class PostProcessorJpeg : public PostProcessor\n>> {\n>> public:\n>> -\tEncoderLibJpeg();\n>> -\t~EncoderLibJpeg();\n>> +\tPostProcessorJpeg();\n>> +\t~PostProcessorJpeg();\n>> \n>> \tint configure(const libcamera::StreamConfiguration &cfg) override;\n>> +\tint process(const libcamera::FrameBuffer *source,\n>> +\t\t const libcamera::Span &destination,\n>> +\t\t CameraMetadata *metadata,\n>> +\t\t ...) override;\n>> +\n>> +\n>> +private:\n>> \tint encode(const libcamera::FrameBuffer *source,\n>> \t\t const libcamera::Span &destination,\n>> -\t\t const libcamera::Span &exifData) override;\n>> +\t\t const libcamera::Span &exifData);\n>> \n>> -private:\n>> \tvoid compressRGB(const libcamera::MappedBuffer *frame);\n>> \tvoid compressNV(const libcamera::MappedBuffer *frame);\n>> \n>> @@ -40,4 +47,4 @@ private:\n>> \tbool nvSwap_;\n>> };\n>> \n>> -#endif /* __ANDROID_JPEG_ENCODER_LIBJPEG_H__ */\n>> +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n>> diff --git a/src/android/meson.build b/src/android/meson.build\n>> index 802bb89..02b3b47 100644\n>> --- a/src/android/meson.build\n>> +++ b/src/android/meson.build\n>> @@ -21,8 +21,8 @@ android_hal_sources = files([\n>> 'camera_metadata.cpp',\n>> 'camera_ops.cpp',\n>> 'camera_stream.cpp',\n>> - 'jpeg/encoder_libjpeg.cpp',\n>> 'jpeg/exif.cpp',\n>> + 'jpeg/post_processor_jpeg.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 15C36BEEDF\n\tfor ;\n\tFri, 9 Oct 2020 14:12:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 994BB6073B;\n\tFri, 9 Oct 2020 16:12:55 +0200 (CEST)","from mail.uajain.com (static.126.159.217.95.clients.your-server.de\n\t[95.217.159.126])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7523460358\n\tfor ;\n\tFri, 9 Oct 2020 16:12:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=uajain.com header.i=@uajain.com\n\theader.b=\"w7KFCEDd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1602252772; bh=NOiF9MHYuN2isqcAwHQRwOQdxWA/ysBS93fOdeIuW8Y=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=w7KFCEDdl7UJ6BcO1dTK+ZiBhYhmkLmXJjg5HZQSBhbzU44KEGzVEGtfbT3YsTg6P\n\t6Z1uPXdQ1gCzCPLK4QSLjinjUkBaPeTFBPZKtaeTv/RhGMWdIj5xtxLWQ1Om+SO5Gq\n\tmJA2ooToBCJlNXAzBY6uvDBhWuCWNATfvPkYfCK+OrLoV6KrFU6xx86DDH6eEIAATn\n\tPZMUcIP59ZSTycqknvIv1aW4bqkbuDmOr5+69VpKXx+PVJfW/RHt4LVI0XDzY9WTuM\n\trfgEqo9RDYYii9z5pcrcr8McDB19gldSljiJATEpmCYsKNL5fDCekF8FhlNjY0b2aH\n\tCeiw8UdKB2apw==","To":"Laurent Pinchart ","References":"<20201008141038.83425-1-email@uajain.com>\n\t<20201008141038.83425-3-email@uajain.com>\n\t<20201008192240.GE3939@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Fri, 9 Oct 2020 19:42:48 +0530","Mime-Version":"1.0","In-Reply-To":"<20201008192240.GE3939@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH 2/3] android: jpeg: Port to\n\tPostProcessor interface","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":13147,"web_url":"https://patchwork.libcamera.org/comment/13147/","msgid":"<31dd3700-0c4a-c274-040e-b3f77cbca236@ideasonboard.com>","date":"2020-10-09T14:36:02","subject":"Re: [libcamera-devel] [RFC PATCH 2/3] android: jpeg: Port to\n\tPostProcessor interface","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 09/10/2020 15:12, Umang Jain wrote:\n> Hi Laurent,\n> \n> Forgot to follow up at one more point.\n> \n> On 10/9/20 12:52 AM, Laurent Pinchart wrote:\n>> Hi Umang,\n>>\n>> Thank you for the patch.\n>>\n>> On Thu, Oct 08, 2020 at 07:40:37PM +0530, Umang Jain wrote:\n>>> Remove the existing Encoder interface completely and use the\n>>> PostProcessor interface instead.\n>> I think we need to keep the Encoder interface. We want to support other\n>> JPEG encoders than the libjpeg-based implementation. Creating a JPEG\n>> post-processor is the right thing to do, but it should still rely on the\n>> existing encoder interface for the actual JPEG encoding. From a\n>> CameraDevice point of view, only the PostProcessor interface will be\n>> visible.\n>>\n>>> Now the ::encode() function will be called by PostProcessor::process()\n>>> internally and will not be directly exposed in CameraStream. Similarly,\n>>> other operations can be introduced internally in the PostProcessorJpeg,\n>>> and can be called through its process() interface.\n>>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>>   src/android/camera_device.h                   |  1 -\n>>>   src/android/camera_stream.cpp                 | 74 +++------------\n>>>   src/android/camera_stream.h                   |  9 +-\n>>>   src/android/jpeg/encoder.h                    | 25 -----\n>>>   ...er_libjpeg.cpp => post_processor_jpeg.cpp} | 92 +++++++++++++++++--\n>>>   ...ncoder_libjpeg.h => post_processor_jpeg.h} | 27 ++++--\n>>>   src/android/meson.build                       |  2 +-\n>>>   7 files changed, 122 insertions(+), 108 deletions(-)\n>>>   delete mode 100644 src/android/jpeg/encoder.h\n>>>   rename src/android/jpeg/{encoder_libjpeg.cpp =>\n>>> post_processor_jpeg.cpp} (67%)\n>>>   rename src/android/jpeg/{encoder_libjpeg.h =>\n>>> post_processor_jpeg.h} (55%)\n>>>\n>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>>> index 777d1a3..25de12e 100644\n>>> --- a/src/android/camera_device.h\n>>> +++ b/src/android/camera_device.h\n>>> @@ -25,7 +25,6 @@\n>>>   #include \"libcamera/internal/message.h\"\n>>>     #include \"camera_stream.h\"\n>>> -#include \"jpeg/encoder.h\"\n>>>     class CameraMetadata;\n>>>   diff --git a/src/android/camera_stream.cpp\n>>> b/src/android/camera_stream.cpp\n>>> index eac1480..ed3bb41 100644\n>>> --- a/src/android/camera_stream.cpp\n>>> +++ b/src/android/camera_stream.cpp\n>>> @@ -9,9 +9,7 @@\n>>>     #include \"camera_device.h\"\n>>>   #include \"camera_metadata.h\"\n>>> -#include \"jpeg/encoder.h\"\n>>> -#include \"jpeg/encoder_libjpeg.h\"\n>>> -#include \"jpeg/exif.h\"\n>>> +#include \"jpeg/post_processor_jpeg.h\"\n>>>     using namespace libcamera;\n>>>   @@ -24,8 +22,15 @@ CameraStream::CameraStream(CameraDevice\n>>> *cameraDevice, Type type,\n>>>   {\n>>>       config_ = cameraDevice_->cameraConfiguration();\n>>>   -    if (type_ == Type::Internal || type_ == Type::Mapped)\n>>> -        encoder_ = std::make_unique();\n>>> +    if (type_ == Type::Internal || type_ == Type::Mapped) {\n>>> +        /*\n>>> +         * \\todo There might be multiple post-processors. The logic\n>>> +         * which should be instantiated here, is deferred for future.\n>>> +         * For now, we only have PostProcessJpeg and that is what we\n>> s/PostProcessJpeg/PostProcessorJpeg/\n>>\n>>> +         * will instantiate here.\n>>> +         */\n>>> +        postProcessor_ = std::make_unique();\n>>> +    }\n>>>         if (type == Type::Internal) {\n>>>           allocator_ =\n>>> std::make_unique(cameraDevice_->camera());\n>>> @@ -45,8 +50,8 @@ Stream *CameraStream::stream() const\n>>>     int CameraStream::configure()\n>>>   {\n>>> -    if (encoder_) {\n>>> -        int ret = encoder_->configure(configuration());\n>>> +    if (postProcessor_) {\n>>> +        int ret = postProcessor_->configure(configuration());\n>>>           if (ret)\n>>>               return ret;\n>>>       }\n>>> @@ -69,60 +74,11 @@ int CameraStream::configure()\n>>>   int CameraStream::process(const libcamera::FrameBuffer &source,\n>>>                 MappedCamera3Buffer *dest, CameraMetadata *metadata)\n>>>   {\n>>> -    if (!encoder_)\n>>> +    if (!postProcessor_)\n>>>           return 0;\n>>>   -    /* Set EXIF metadata for various tags. */\n>>> -    Exif exif;\n>>> -    /* \\todo Set Make and Model from external vendor tags. */\n>>> -    exif.setMake(\"libcamera\");\n>>> -    exif.setModel(\"cameraModel\");\n>>> -    exif.setOrientation(cameraDevice_->orientation());\n>>> -    exif.setSize(configuration().size);\n>>> -    /*\n>>> -     * We set the frame's EXIF timestamp as the time of encode.\n>>> -     * Since the precision we need for EXIF timestamp is only one\n>>> -     * second, it is good enough.\n>>> -     */\n>>> -    exif.setTimestamp(std::time(nullptr));\n>>> -    if (exif.generate() != 0)\n>>> -        LOG(HAL, Error) << \"Failed to generate valid EXIF data\";\n>>> -\n>>> -    int jpeg_size = encoder_->encode(&source, dest->maps()[0],\n>>> exif.data());\n>>> -    if (jpeg_size < 0) {\n>>> -        LOG(HAL, Error) << \"Failed to encode stream image\";\n>>> -        return jpeg_size;\n>>> -    }\n>>> -\n>>> -    /*\n>>> -     * Fill in the JPEG blob header.\n>>> -     *\n>>> -     * The mapped size of the buffer is being returned as\n>>> -     * substantially larger than the requested JPEG_MAX_SIZE\n>>> -     * (which is referenced from maxJpegBufferSize_). Utilise\n>>> -     * this static size to ensure the correct offset of the blob is\n>>> -     * determined.\n>>> -     *\n>>> -     * \\todo Investigate if the buffer size mismatch is an issue or\n>>> -     * expected behaviour.\n>>> -     */\n>>> -    uint8_t *resultPtr = dest->maps()[0].data() +\n>>> -                 cameraDevice_->maxJpegBufferSize() -\n>>> -                 sizeof(struct camera3_jpeg_blob);\n>>> -    auto *blob = reinterpret_cast>> *>(resultPtr);\n>>> -    blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n>>> -    blob->jpeg_size = jpeg_size;\n>>> -\n>>> -    /* Update the JPEG result Metadata. */\n>>> -    metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);\n>>> -\n>>> -    const uint32_t jpeg_quality = 95;\n>>> -    metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n>>> -\n>>> -    const uint32_t jpeg_orientation = 0;\n>>> -    metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n>>> -\n>>> -    return 0;\n>>> +    return postProcessor_->process(&source, dest->maps()[0],\n>>> +                       metadata, cameraDevice_);\n>> There are at least two options to keep the interface generic, avoiding\n>> variadic arguments. One of them is to explicitly pass the the camera\n>> device to the process function. Another option is to pass it to the\n>> PostProcessorJpeg constructor, and store it internally. I'd go for the\n>> latter.\n>>\n>>>   }\n>>>     FrameBuffer *CameraStream::getBuffer()\n>>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>>> index 8df0101..8d0f2e3 100644\n>>> --- a/src/android/camera_stream.h\n>>> +++ b/src/android/camera_stream.h\n>>> @@ -19,7 +19,12 @@\n>>>   #include \n>>>   #include \n>>>   -class Encoder;\n>>> +/*\n>>> + * \\todo Ideally we want to replace this include with\n>>> forward-declaration.\n>>> + * If we do that. currently we get a compile error.\n>>> + */\n>> Let's fix them :-) What compilation error do you get ?\n> The error I see:\n> \n> [89/298] Compiling C++ object\n> src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o\n> FAILED: src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o\n> c++ -Isrc/libcamera/libcamera.so.p -Isrc/libcamera -I../src/libcamera\n> -Iinclude -I../include -I../include/android/hardware/libhardware/include\n> -I../include/android/metadata -I../include/android/system/core/include\n> -Iinclude/libcamera -fdiagnostics-color=always -pipe\n> -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra\n> -Werror -std=c++17 -g -include config.h -fPIC -pthread -MD -MQ\n> src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o -MF\n> src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o.d -o\n> src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o -c\n> ../src/android/camera_device.cpp\n> In file included from /usr/include/c++/10/memory:83,\n>                  from ../src/android/camera_device.h:11,\n>                  from ../src/android/camera_device.cpp:8:\n> /usr/include/c++/10/bits/unique_ptr.h: In instantiation of ‘void\n> std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp =\n> PostProcessor]’:\n> /usr/include/c++/10/bits/unique_ptr.h:360:17:   required from\n> ‘std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = PostProcessor; _Dp\n> = std::default_delete]’\n> ../src/android/camera_stream.h:32:7:   required from ‘void\n> __gnu_cxx::new_allocator<_Tp>::destroy(_Up*) [with _Up = CameraStream;\n> _Tp = CameraStream]’\n> /usr/include/c++/10/bits/alloc_traits.h:531:15:   required from ‘static\n> void std::allocator_traits\n>>::destroy(std::allocator_traits\n>>::allocator_type&, _Up*) [with _Up = CameraStream; _Tp = CameraStream;\n> std::allocator_traits >::allocator_type =\n> std::allocator]’\n> /usr/include/c++/10/bits/vector.tcc:488:28:   required from ‘void\n> std::vector<_Tp, _Alloc>::_M_realloc_insert(std::vector<_Tp,\n> _Alloc>::iterator, _Args&& ...) [with _Args = {CameraDevice*,\n> CameraStream::Type, camera3_stream*&, long unsigned int}; _Tp =\n> CameraStream; _Alloc = std::allocator; std::vector<_Tp,\n> _Alloc>::iterator = std::vector::iterator]’\n> /usr/include/c++/10/bits/vector.tcc:121:21:   required from\n> ‘std::vector<_Tp, _Alloc>::reference std::vector<_Tp,\n> _Alloc>::emplace_back(_Args&& ...) [with _Args = {CameraDevice*,\n> CameraStream::Type, camera3_stream*&, long unsigned int}; _Tp =\n> CameraStream; _Alloc = std::allocator; std::vector<_Tp,\n> _Alloc>::reference = CameraStream&]’\n> ../src/android/camera_device.cpp:1212:38:   required from here\n> /usr/include/c++/10/bits/unique_ptr.h:82:16: error: invalid application\n> of ‘sizeof’ to incomplete type ‘PostProcessor’\n>    82 |  static_assert(sizeof(_Tp)>0,\n>       |                ^~~~~~~~~~~\n> [98/298] Compiling C++ object\n> src/libcamera/libcamera.so.p/.._android_camera_stream.cpp.o\n> ninja: build stopped: subcommand failed.\n> \n> Not sure why it cannot work with the incomplete type `PostProcessor`\n> interface\n> (well, it used to work well with Encoder's interface, right there!)\n>>\n>>> +#include \"post_processor.h\"\n\nDoes this file get included in ./src/android/camera_device.cpp ?\n--\nKieran\n\n\n>>> +\n>>>   class CameraDevice;\n>>>   class CameraMetadata;\n>>>   class MappedCamera3Buffer;\n>>> @@ -135,7 +140,7 @@ private:\n>>>        */\n>>>       unsigned int index_;\n>>>   -    std::unique_ptr encoder_;\n>>> +    std::unique_ptr postProcessor_;\n>>>       std::unique_ptr allocator_;\n>>>       std::vector buffers_;\n>>>       /*\n>>> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n>>> deleted file mode 100644\n>>> index cf26d67..0000000\n>>> --- a/src/android/jpeg/encoder.h\n>>> +++ /dev/null\n>>> @@ -1,25 +0,0 @@\n>>> -/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>> -/*\n>>> - * Copyright (C) 2020, Google Inc.\n>>> - *\n>>> - * encoder.h - Image encoding interface\n>>> - */\n>>> -#ifndef __ANDROID_JPEG_ENCODER_H__\n>>> -#define __ANDROID_JPEG_ENCODER_H__\n>>> -\n>>> -#include \n>>> -#include \n>>> -#include \n>>> -\n>>> -class Encoder\n>>> -{\n>>> -public:\n>>> -    virtual ~Encoder() {};\n>>> -\n>>> -    virtual int configure(const libcamera::StreamConfiguration &cfg)\n>>> = 0;\n>>> -    virtual int encode(const libcamera::FrameBuffer *source,\n>>> -               const libcamera::Span &destination,\n>>> -               const libcamera::Span &exifData) = 0;\n>>> -};\n>>> -\n>>> -#endif /* __ANDROID_JPEG_ENCODER_H__ */\n>> As mentioned above, this should be kept, and used by the JPEG\n>> post-processor.\n>>\n>>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp\n>>> b/src/android/jpeg/post_processor_jpeg.cpp\n>>> similarity index 67%\n>>> rename from src/android/jpeg/encoder_libjpeg.cpp\n>>> rename to src/android/jpeg/post_processor_jpeg.cpp\n>>> index 510613c..eeb4e95 100644\n>>> --- a/src/android/jpeg/encoder_libjpeg.cpp\n>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>>> @@ -2,10 +2,14 @@\n>>>   /*\n>>>    * Copyright (C) 2020, Google Inc.\n>>>    *\n>>> - * encoder_libjpeg.cpp - JPEG encoding using libjpeg native API\n>>> + * post_processor_jpeg.cpp - JPEG Post Processor\n>>>    */\n>>>   -#include \"encoder_libjpeg.h\"\n>>> +#include \"post_processor_jpeg.h\"\n>>> +\n>>> +#include \"exif.h\"\n>>> +\n>>> +#include \"../camera_device.h\"\n>>>     #include \n>>>   #include \n>>> @@ -25,6 +29,14 @@\n>>>     using namespace libcamera;\n>>>   +#define extract_va_arg(type, arg, last) \\\n>>> +{                                       \\\n>>> +        va_list ap;                     \\\n>>> +        va_start(ap, last);             \\\n>>> +        arg = va_arg(ap, type);         \\\n>>> +        va_end(ap);                     \\\n>>> +}\n>>> +\n>>>   LOG_DEFINE_CATEGORY(JPEG)\n>>>     namespace {\n>>> @@ -67,7 +79,7 @@ const struct JPEGPixelFormatInfo\n>>> &findPixelInfo(const PixelFormat &format)\n>>>     } /* namespace */\n>>>   -EncoderLibJpeg::EncoderLibJpeg()\n>>> +PostProcessorJpeg::PostProcessorJpeg()\n>>>       : quality_(95)\n>>>   {\n>>>       /* \\todo Expand error handling coverage with a custom handler. */\n>>> @@ -76,12 +88,12 @@ EncoderLibJpeg::EncoderLibJpeg()\n>>>       jpeg_create_compress(&compress_);\n>>>   }\n>>>   -EncoderLibJpeg::~EncoderLibJpeg()\n>>> +PostProcessorJpeg::~PostProcessorJpeg()\n>>>   {\n>>>       jpeg_destroy_compress(&compress_);\n>>>   }\n>>>   -int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n>>> +int PostProcessorJpeg::configure(const StreamConfiguration &cfg)\n>>>   {\n>>>       const struct JPEGPixelFormatInfo info =\n>>> findPixelInfo(cfg.pixelFormat);\n>>>       if (info.colorSpace == JCS_UNKNOWN)\n>>> @@ -104,7 +116,67 @@ int EncoderLibJpeg::configure(const\n>>> StreamConfiguration &cfg)\n>>>       return 0;\n>>>   }\n>>>   -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer\n>>> *frame)\n>>> +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n>>> +                   const libcamera::Span &destination,\n>>> +                   CameraMetadata *metadata, ...)\n>>> +{\n>>> +    CameraDevice *device = nullptr;\n>>> +    extract_va_arg(CameraDevice *, device, metadata);\n>>> +\n>>> +    /* Set EXIF metadata for various tags. */\n>>> +    Exif exif;\n>>> +    /* \\todo Set Make and Model from external vendor tags. */\n>>> +    exif.setMake(\"libcamera\");\n>>> +    exif.setModel(\"cameraModel\");\n>>> +    exif.setOrientation(device->orientation());\n>>> +    exif.setSize(Size {compress_.image_width, compress_.image_height});\n>>> +    /*\n>>> +     * We set the frame's EXIF timestamp as the time of encode.\n>>> +     * Since the precision we need for EXIF timestamp is only one\n>>> +     * second, it is good enough.\n>>> +     */\n>>> +    exif.setTimestamp(std::time(nullptr));\n>>> +    if (exif.generate() != 0)\n>>> +        LOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n>>> +\n>>> +    int jpeg_size = encode(source, destination, exif.data());\n>>> +    if (jpeg_size < 0) {\n>>> +        LOG(JPEG, Error) << \"Failed to encode stream image\";\n>>> +        return jpeg_size;\n>>> +    }\n>>> +\n>>> +    /*\n>>> +     * Fill in the JPEG blob header.\n>>> +     *\n>>> +     * The mapped size of the buffer is being returned as\n>>> +     * substantially larger than the requested JPEG_MAX_SIZE\n>>> +     * (which is referenced from maxJpegBufferSize_). Utilise\n>>> +     * this static size to ensure the correct offset of the blob is\n>>> +     * determined.\n>>> +     *\n>>> +     * \\todo Investigate if the buffer size mismatch is an issue or\n>>> +     * expected behaviour.\n>>> +     */\n>>> +    uint8_t *resultPtr = destination.data() +\n>>> +                 device->maxJpegBufferSize() -\n>>> +                 sizeof(struct camera3_jpeg_blob);\n>>> +    auto *blob = reinterpret_cast>> *>(resultPtr);\n>>> +    blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n>>> +    blob->jpeg_size = jpeg_size;\n>>> +\n>>> +    /* Update the JPEG result Metadata. */\n>>> +    metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);\n>>> +\n>>> +    const uint32_t jpeg_quality = 95;\n>>> +    metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n>>> +\n>>> +    const uint32_t jpeg_orientation = 0;\n>>> +    metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n>>> +\n>>> +    return 0;\n>>> +}\n>>> +\n>>> +void PostProcessorJpeg::compressRGB(const libcamera::MappedBuffer\n>>> *frame)\n>>>   {\n>>>       unsigned char *src = static_cast>> *>(frame->maps()[0].data());\n>>>       /* \\todo Stride information should come from buffer\n>>> configuration. */\n>>> @@ -122,7 +194,7 @@ void EncoderLibJpeg::compressRGB(const\n>>> libcamera::MappedBuffer *frame)\n>>>    * Compress the incoming buffer from a supported NV format.\n>>>    * This naively unpacks the semi-planar NV12 to a YUV888 format for\n>>> libjpeg.\n>>>    */\n>>> -void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n>>> +void PostProcessorJpeg::compressNV(const libcamera::MappedBuffer\n>>> *frame)\n>>>   {\n>>>       uint8_t tmprowbuf[compress_.image_width * 3];\n>>>   @@ -179,9 +251,9 @@ void EncoderLibJpeg::compressNV(const\n>>> libcamera::MappedBuffer *frame)\n>>>       }\n>>>   }\n>>>   -int EncoderLibJpeg::encode(const FrameBuffer *source,\n>>> -               const libcamera::Span &dest,\n>>> -               const libcamera::Span &exifData)\n>>> +int PostProcessorJpeg::encode(const FrameBuffer *source,\n>>> +                  const libcamera::Span &dest,\n>>> +                  const libcamera::Span &exifData)\n>>>   {\n>>>       MappedFrameBuffer frame(source, PROT_READ);\n>>>       if (!frame.isValid()) {\n>>> diff --git a/src/android/jpeg/encoder_libjpeg.h\n>>> b/src/android/jpeg/post_processor_jpeg.h\n>>> similarity index 55%\n>>> rename from src/android/jpeg/encoder_libjpeg.h\n>>> rename to src/android/jpeg/post_processor_jpeg.h\n>>> index 1e8df05..7f9ce70 100644\n>>> --- a/src/android/jpeg/encoder_libjpeg.h\n>>> +++ b/src/android/jpeg/post_processor_jpeg.h\n>>> @@ -2,30 +2,37 @@\n>>>   /*\n>>>    * Copyright (C) 2020, Google Inc.\n>>>    *\n>>> - * encoder_libjpeg.h - JPEG encoding using libjpeg\n>>> + * post_processor_jpeg.h - JPEG Post Processor\n>>>    */\n>>> -#ifndef __ANDROID_JPEG_ENCODER_LIBJPEG_H__\n>>> -#define __ANDROID_JPEG_ENCODER_LIBJPEG_H__\n>>> +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__\n>>> +#define __ANDROID_POST_PROCESSOR_JPEG_H__\n>>>   -#include \"encoder.h\"\n>>> +#include \"../post_processor.h\"\n>>> +#include \"../camera_metadata.h\"\n>>>     #include \"libcamera/internal/buffer.h\"\n>>>   #include \"libcamera/internal/formats.h\"\n>>>     #include \n>>>   -class EncoderLibJpeg : public Encoder\n>>> +class PostProcessorJpeg : public PostProcessor\n>>>   {\n>>>   public:\n>>> -    EncoderLibJpeg();\n>>> -    ~EncoderLibJpeg();\n>>> +    PostProcessorJpeg();\n>>> +    ~PostProcessorJpeg();\n>>>         int configure(const libcamera::StreamConfiguration &cfg)\n>>> override;\n>>> +    int process(const libcamera::FrameBuffer *source,\n>>> +            const libcamera::Span &destination,\n>>> +            CameraMetadata *metadata,\n>>> +            ...) override;\n>>> +\n>>> +\n>>> +private:\n>>>       int encode(const libcamera::FrameBuffer *source,\n>>>              const libcamera::Span &destination,\n>>> -           const libcamera::Span &exifData) override;\n>>> +           const libcamera::Span &exifData);\n>>>   -private:\n>>>       void compressRGB(const libcamera::MappedBuffer *frame);\n>>>       void compressNV(const libcamera::MappedBuffer *frame);\n>>>   @@ -40,4 +47,4 @@ private:\n>>>       bool nvSwap_;\n>>>   };\n>>>   -#endif /* __ANDROID_JPEG_ENCODER_LIBJPEG_H__ */\n>>> +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n>>> diff --git a/src/android/meson.build b/src/android/meson.build\n>>> index 802bb89..02b3b47 100644\n>>> --- a/src/android/meson.build\n>>> +++ b/src/android/meson.build\n>>> @@ -21,8 +21,8 @@ android_hal_sources = files([\n>>>       'camera_metadata.cpp',\n>>>       'camera_ops.cpp',\n>>>       'camera_stream.cpp',\n>>> -    'jpeg/encoder_libjpeg.cpp',\n>>>       'jpeg/exif.cpp',\n>>> +    'jpeg/post_processor_jpeg.cpp',\n>>>   ])\n>>>     android_camera_metadata_sources = files([\n> \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 5E2FDBEEE0\n\tfor ;\n\tFri, 9 Oct 2020 14:36:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC8A960359;\n\tFri, 9 Oct 2020 16:36:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC0FC60358\n\tfor ;\n\tFri, 9 Oct 2020 16:36:05 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 15E7A539;\n\tFri, 9 Oct 2020 16:36:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XK7t4aPS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602254165;\n\tbh=hXlSlupETRq1AgHdaAyY+OyO3UXnyXNZc3FlfSDDLHE=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=XK7t4aPSV9dGmsfwzloAWBnEP+BFh641BSYr++hK3hUWoNgE7RtY4KkQdtnhG8E1M\n\texbST2yTHJ4uGVWE02s4pjpQqM92f32UkraO09mZjLVbKU+q3zXCuDRfiojqPcCwUB\n\tdyQQJdBEc2+inuBBTYI0+SY9IZ9BBinGJilznnfs=","To":"Umang Jain ,\n\tLaurent Pinchart ","References":"<20201008141038.83425-1-email@uajain.com>\n\t<20201008141038.83425-3-email@uajain.com>\n\t<20201008192240.GE3939@pendragon.ideasonboard.com>\n\t","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":"<31dd3700-0c4a-c274-040e-b3f77cbca236@ideasonboard.com>","Date":"Fri, 9 Oct 2020 15:36:02 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH 2/3] android: jpeg: Port to\n\tPostProcessor interface","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13151,"web_url":"https://patchwork.libcamera.org/comment/13151/","msgid":"<20201009215811.GG25040@pendragon.ideasonboard.com>","date":"2020-10-09T21:58:11","subject":"Re: [libcamera-devel] [RFC PATCH 2/3] android: jpeg: Port to\n\tPostProcessor interface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Fri, Oct 09, 2020 at 10:04:18AM +0530, Umang Jain wrote:\n> On 10/9/20 12:52 AM, Laurent Pinchart wrote:\n> > On Thu, Oct 08, 2020 at 07:40:37PM +0530, Umang Jain wrote:\n> >> Remove the existing Encoder interface completely and use the\n> >> PostProcessor interface instead.\n> >\n> > I think we need to keep the Encoder interface. We want to support other\n> > JPEG encoders than the libjpeg-based implementation. Creating a JPEG\n> > post-processor is the right thing to do, but it should still rely on the\n> > existing encoder interface for the actual JPEG encoding. From a\n> > CameraDevice point of view, only the PostProcessor interface will be\n> > visible.\n>\n> Oh. I didn't see that. I assumed we will have only one? encoder and even \n> if we had more, it would simply encapsulated within a private function \n> as PostProcessorJpeg::encodeViaX() or whatever. I will bring back the \n> Encoder interface if you say so.\n> \n> Indeed, CameraDevice point-of-view, we only want to expose the \n> PostProcessor interface.\n>\n> >> Now the ::encode() function will be called by PostProcessor::process()\n> >> internally and will not be directly exposed in CameraStream. Similarly,\n> >> other operations can be introduced internally in the PostProcessorJpeg,\n> >> and can be called through its process() interface.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_device.h | 1 -\n> >> src/android/camera_stream.cpp | 74 +++------------\n> >> src/android/camera_stream.h | 9 +-\n> >> src/android/jpeg/encoder.h | 25 -----\n> >> ...er_libjpeg.cpp => post_processor_jpeg.cpp} | 92 +++++++++++++++++--\n> >> ...ncoder_libjpeg.h => post_processor_jpeg.h} | 27 ++++--\n> >> src/android/meson.build | 2 +-\n> >> 7 files changed, 122 insertions(+), 108 deletions(-)\n> >> delete mode 100644 src/android/jpeg/encoder.h\n> >> rename src/android/jpeg/{encoder_libjpeg.cpp => post_processor_jpeg.cpp} (67%)\n> >> rename src/android/jpeg/{encoder_libjpeg.h => post_processor_jpeg.h} (55%)\n> >>\n> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >> index 777d1a3..25de12e 100644\n> >> --- a/src/android/camera_device.h\n> >> +++ b/src/android/camera_device.h\n> >> @@ -25,7 +25,6 @@\n> >> #include \"libcamera/internal/message.h\"\n> >> \n> >> #include \"camera_stream.h\"\n> >> -#include \"jpeg/encoder.h\"\n> >> \n> >> class CameraMetadata;\n> >> \n> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >> index eac1480..ed3bb41 100644\n> >> --- a/src/android/camera_stream.cpp\n> >> +++ b/src/android/camera_stream.cpp\n> >> @@ -9,9 +9,7 @@\n> >> \n> >> #include \"camera_device.h\"\n> >> #include \"camera_metadata.h\"\n> >> -#include \"jpeg/encoder.h\"\n> >> -#include \"jpeg/encoder_libjpeg.h\"\n> >> -#include \"jpeg/exif.h\"\n> >> +#include \"jpeg/post_processor_jpeg.h\"\n> >> \n> >> using namespace libcamera;\n> >> \n> >> @@ -24,8 +22,15 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> >> {\n> >> \tconfig_ = cameraDevice_->cameraConfiguration();\n> >> \n> >> -\tif (type_ == Type::Internal || type_ == Type::Mapped)\n> >> -\t\tencoder_ = std::make_unique();\n> >> +\tif (type_ == Type::Internal || type_ == Type::Mapped) {\n> >> +\t\t/*\n> >> +\t\t * \\todo There might be multiple post-processors. The logic\n> >> +\t\t * which should be instantiated here, is deferred for future.\n> >> +\t\t * For now, we only have PostProcessJpeg and that is what we\n> >\n> > s/PostProcessJpeg/PostProcessorJpeg/\n> >\n> >> +\t\t * will instantiate here.\n> >> +\t\t */\n> >> +\t\tpostProcessor_ = std::make_unique();\n> >> +\t}\n> >> \n> >> \tif (type == Type::Internal) {\n> >> \t\tallocator_ = std::make_unique(cameraDevice_->camera());\n> >> @@ -45,8 +50,8 @@ Stream *CameraStream::stream() const\n> >> \n> >> int CameraStream::configure()\n> >> {\n> >> -\tif (encoder_) {\n> >> -\t\tint ret = encoder_->configure(configuration());\n> >> +\tif (postProcessor_) {\n> >> +\t\tint ret = postProcessor_->configure(configuration());\n> >> \t\tif (ret)\n> >> \t\t\treturn ret;\n> >> \t}\n> >> @@ -69,60 +74,11 @@ int CameraStream::configure()\n> >> int CameraStream::process(const libcamera::FrameBuffer &source,\n> >> \t\t\t MappedCamera3Buffer *dest, CameraMetadata *metadata)\n> >> {\n> >> -\tif (!encoder_)\n> >> +\tif (!postProcessor_)\n> >> \t\treturn 0;\n> >> \n> >> -\t/* Set EXIF metadata for various tags. */\n> >> -\tExif exif;\n> >> -\t/* \\todo Set Make and Model from external vendor tags. */\n> >> -\texif.setMake(\"libcamera\");\n> >> -\texif.setModel(\"cameraModel\");\n> >> -\texif.setOrientation(cameraDevice_->orientation());\n> >> -\texif.setSize(configuration().size);\n> >> -\t/*\n> >> -\t * We set the frame's EXIF timestamp as the time of encode.\n> >> -\t * Since the precision we need for EXIF timestamp is only one\n> >> -\t * second, it is good enough.\n> >> -\t */\n> >> -\texif.setTimestamp(std::time(nullptr));\n> >> -\tif (exif.generate() != 0)\n> >> -\t\tLOG(HAL, Error) << \"Failed to generate valid EXIF data\";\n> >> -\n> >> -\tint jpeg_size = encoder_->encode(&source, dest->maps()[0], exif.data());\n> >> -\tif (jpeg_size < 0) {\n> >> -\t\tLOG(HAL, Error) << \"Failed to encode stream image\";\n> >> -\t\treturn jpeg_size;\n> >> -\t}\n> >> -\n> >> -\t/*\n> >> -\t * Fill in the JPEG blob header.\n> >> -\t *\n> >> -\t * The mapped size of the buffer is being returned as\n> >> -\t * substantially larger than the requested JPEG_MAX_SIZE\n> >> -\t * (which is referenced from maxJpegBufferSize_). Utilise\n> >> -\t * this static size to ensure the correct offset of the blob is\n> >> -\t * determined.\n> >> -\t *\n> >> -\t * \\todo Investigate if the buffer size mismatch is an issue or\n> >> -\t * expected behaviour.\n> >> -\t */\n> >> -\tuint8_t *resultPtr = dest->maps()[0].data() +\n> >> -\t\t\t cameraDevice_->maxJpegBufferSize() -\n> >> -\t\t\t sizeof(struct camera3_jpeg_blob);\n> >> -\tauto *blob = reinterpret_cast(resultPtr);\n> >> -\tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n> >> -\tblob->jpeg_size = jpeg_size;\n> >> -\n> >> -\t/* Update the JPEG result Metadata. */\n> >> -\tmetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);\n> >> -\n> >> -\tconst uint32_t jpeg_quality = 95;\n> >> -\tmetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n> >> -\n> >> -\tconst uint32_t jpeg_orientation = 0;\n> >> -\tmetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n> >> -\n> >> -\treturn 0;\n> >> +\treturn postProcessor_->process(&source, dest->maps()[0],\n> >> +\t\t\t\t metadata, cameraDevice_);\n> >\n> > There are at least two options to keep the interface generic, avoiding\n> > variadic arguments. One of them is to explicitly pass the the camera\n> > device to the process function. Another option is to pass it to the\n> > PostProcessorJpeg constructor, and store it internally. I'd go for the\n> > latter.\n>\n> I see. I thought vardiac arguments can be helpful for \n> (future)PostProcessors to have the flexibility of parameters they might \n> need when they process(). But I do get your point that they might defeat \n> the entire purpose of keeping the interface generic. Asking for \n> curiousity, is passing the specifics parameters via respective \n> PostProcessors' constructors (instead of vardiac args) is the way you \n> would suggest in general to achieve this goal?\n\nYes, that would be the best option I think. The code that creates the\npost-processors will need to be post-processor-aware, so that's where we\nshould handle any specifics. The rest of the code should be generic.\n\nWe can also extend the process() and configure() functions with extra\nparameters that are always passed to the functions, and only used by\nsome post-processors. This should however be limited to fairly generic\nparameters (CameraDevice would qualify), not data that would be very\npost-processor-specific.\n\n> >> }\n> >> \n> >> FrameBuffer *CameraStream::getBuffer()\n> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> >> index 8df0101..8d0f2e3 100644\n> >> --- a/src/android/camera_stream.h\n> >> +++ b/src/android/camera_stream.h\n> >> @@ -19,7 +19,12 @@\n> >> #include \n> >> #include \n> >> \n> >> -class Encoder;\n> >> +/*\n> >> + * \\todo Ideally we want to replace this include with forward-declaration.\n> >> + * If we do that. currently we get a compile error.\n> >> + */\n> >\n> > Let's fix them :-) What compilation error do you get ?\n> >\n> >> +#include \"post_processor.h\"\n> >> +\n> >> class CameraDevice;\n> >> class CameraMetadata;\n> >> class MappedCamera3Buffer;\n> >> @@ -135,7 +140,7 @@ private:\n> >> \t */\n> >> \tunsigned int index_;\n> >> \n> >> -\tstd::unique_ptr encoder_;\n> >> +\tstd::unique_ptr postProcessor_;\n> >> \tstd::unique_ptr allocator_;\n> >> \tstd::vector buffers_;\n> >> \t/*\n> >> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> >> deleted file mode 100644\n> >> index cf26d67..0000000\n> >> --- a/src/android/jpeg/encoder.h\n> >> +++ /dev/null\n> >> @@ -1,25 +0,0 @@\n> >> -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> -/*\n> >> - * Copyright (C) 2020, Google Inc.\n> >> - *\n> >> - * encoder.h - Image encoding interface\n> >> - */\n> >> -#ifndef __ANDROID_JPEG_ENCODER_H__\n> >> -#define __ANDROID_JPEG_ENCODER_H__\n> >> -\n> >> -#include \n> >> -#include \n> >> -#include \n> >> -\n> >> -class Encoder\n> >> -{\n> >> -public:\n> >> -\tvirtual ~Encoder() {};\n> >> -\n> >> -\tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> >> -\tvirtual int encode(const libcamera::FrameBuffer *source,\n> >> -\t\t\t const libcamera::Span &destination,\n> >> -\t\t\t const libcamera::Span &exifData) = 0;\n> >> -};\n> >> -\n> >> -#endif /* __ANDROID_JPEG_ENCODER_H__ */\n> >\n> > As mentioned above, this should be kept, and used by the JPEG\n> > post-processor.\n> >\n> >> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> >> similarity index 67%\n> >> rename from src/android/jpeg/encoder_libjpeg.cpp\n> >> rename to src/android/jpeg/post_processor_jpeg.cpp\n> >> index 510613c..eeb4e95 100644\n> >> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> >> @@ -2,10 +2,14 @@\n> >> /*\n> >> * Copyright (C) 2020, Google Inc.\n> >> *\n> >> - * encoder_libjpeg.cpp - JPEG encoding using libjpeg native API\n> >> + * post_processor_jpeg.cpp - JPEG Post Processor\n> >> */\n> >> \n> >> -#include \"encoder_libjpeg.h\"\n> >> +#include \"post_processor_jpeg.h\"\n> >> +\n> >> +#include \"exif.h\"\n> >> +\n> >> +#include \"../camera_device.h\"\n> >> \n> >> #include \n> >> #include \n> >> @@ -25,6 +29,14 @@\n> >> \n> >> using namespace libcamera;\n> >> \n> >> +#define extract_va_arg(type, arg, last) \\\n> >> +{ \\\n> >> + va_list ap; \\\n> >> + va_start(ap, last); \\\n> >> + arg = va_arg(ap, type); \\\n> >> + va_end(ap); \\\n> >> +}\n> >> +\n> >> LOG_DEFINE_CATEGORY(JPEG)\n> >> \n> >> namespace {\n> >> @@ -67,7 +79,7 @@ const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)\n> >> \n> >> } /* namespace */\n> >> \n> >> -EncoderLibJpeg::EncoderLibJpeg()\n> >> +PostProcessorJpeg::PostProcessorJpeg()\n> >> \t: quality_(95)\n> >> {\n> >> \t/* \\todo Expand error handling coverage with a custom handler. */\n> >> @@ -76,12 +88,12 @@ EncoderLibJpeg::EncoderLibJpeg()\n> >> \tjpeg_create_compress(&compress_);\n> >> }\n> >> \n> >> -EncoderLibJpeg::~EncoderLibJpeg()\n> >> +PostProcessorJpeg::~PostProcessorJpeg()\n> >> {\n> >> \tjpeg_destroy_compress(&compress_);\n> >> }\n> >> \n> >> -int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> >> +int PostProcessorJpeg::configure(const StreamConfiguration &cfg)\n> >> {\n> >> \tconst struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);\n> >> \tif (info.colorSpace == JCS_UNKNOWN)\n> >> @@ -104,7 +116,67 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> >> \treturn 0;\n> >> }\n> >> \n> >> -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)\n> >> +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> >> +\t\t\t const libcamera::Span &destination,\n> >> +\t\t\t CameraMetadata *metadata, ...)\n> >> +{\n> >> +\tCameraDevice *device = nullptr;\n> >> +\textract_va_arg(CameraDevice *, device, metadata);\n> >> +\n> >> +\t/* Set EXIF metadata for various tags. */\n> >> +\tExif exif;\n> >> +\t/* \\todo Set Make and Model from external vendor tags. */\n> >> +\texif.setMake(\"libcamera\");\n> >> +\texif.setModel(\"cameraModel\");\n> >> +\texif.setOrientation(device->orientation());\n> >> +\texif.setSize(Size {compress_.image_width, compress_.image_height});\n> >> +\t/*\n> >> +\t * We set the frame's EXIF timestamp as the time of encode.\n> >> +\t * Since the precision we need for EXIF timestamp is only one\n> >> +\t * second, it is good enough.\n> >> +\t */\n> >> +\texif.setTimestamp(std::time(nullptr));\n> >> +\tif (exif.generate() != 0)\n> >> +\t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> >> +\n> >> +\tint jpeg_size = encode(source, destination, exif.data());\n> >> +\tif (jpeg_size < 0) {\n> >> +\t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n> >> +\t\treturn jpeg_size;\n> >> +\t}\n> >> +\n> >> +\t/*\n> >> +\t * Fill in the JPEG blob header.\n> >> +\t *\n> >> +\t * The mapped size of the buffer is being returned as\n> >> +\t * substantially larger than the requested JPEG_MAX_SIZE\n> >> +\t * (which is referenced from maxJpegBufferSize_). Utilise\n> >> +\t * this static size to ensure the correct offset of the blob is\n> >> +\t * determined.\n> >> +\t *\n> >> +\t * \\todo Investigate if the buffer size mismatch is an issue or\n> >> +\t * expected behaviour.\n> >> +\t */\n> >> +\tuint8_t *resultPtr = destination.data() +\n> >> +\t\t\t device->maxJpegBufferSize() -\n> >> +\t\t\t sizeof(struct camera3_jpeg_blob);\n> >> +\tauto *blob = reinterpret_cast(resultPtr);\n> >> +\tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n> >> +\tblob->jpeg_size = jpeg_size;\n> >> +\n> >> +\t/* Update the JPEG result Metadata. */\n> >> +\tmetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);\n> >> +\n> >> +\tconst uint32_t jpeg_quality = 95;\n> >> +\tmetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n> >> +\n> >> +\tconst uint32_t jpeg_orientation = 0;\n> >> +\tmetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +void PostProcessorJpeg::compressRGB(const libcamera::MappedBuffer *frame)\n> >> {\n> >> \tunsigned char *src = static_cast(frame->maps()[0].data());\n> >> \t/* \\todo Stride information should come from buffer configuration. */\n> >> @@ -122,7 +194,7 @@ void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)\n> >> * Compress the incoming buffer from a supported NV format.\n> >> * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.\n> >> */\n> >> -void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> >> +void PostProcessorJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> >> {\n> >> \tuint8_t tmprowbuf[compress_.image_width * 3];\n> >> \n> >> @@ -179,9 +251,9 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> >> \t}\n> >> }\n> >> \n> >> -int EncoderLibJpeg::encode(const FrameBuffer *source,\n> >> -\t\t\t const libcamera::Span &dest,\n> >> -\t\t\t const libcamera::Span &exifData)\n> >> +int PostProcessorJpeg::encode(const FrameBuffer *source,\n> >> +\t\t\t const libcamera::Span &dest,\n> >> +\t\t\t const libcamera::Span &exifData)\n> >> {\n> >> \tMappedFrameBuffer frame(source, PROT_READ);\n> >> \tif (!frame.isValid()) {\n> >> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> >> similarity index 55%\n> >> rename from src/android/jpeg/encoder_libjpeg.h\n> >> rename to src/android/jpeg/post_processor_jpeg.h\n> >> index 1e8df05..7f9ce70 100644\n> >> --- a/src/android/jpeg/encoder_libjpeg.h\n> >> +++ b/src/android/jpeg/post_processor_jpeg.h\n> >> @@ -2,30 +2,37 @@\n> >> /*\n> >> * Copyright (C) 2020, Google Inc.\n> >> *\n> >> - * encoder_libjpeg.h - JPEG encoding using libjpeg\n> >> + * post_processor_jpeg.h - JPEG Post Processor\n> >> */\n> >> -#ifndef __ANDROID_JPEG_ENCODER_LIBJPEG_H__\n> >> -#define __ANDROID_JPEG_ENCODER_LIBJPEG_H__\n> >> +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__\n> >> +#define __ANDROID_POST_PROCESSOR_JPEG_H__\n> >> \n> >> -#include \"encoder.h\"\n> >> +#include \"../post_processor.h\"\n> >> +#include \"../camera_metadata.h\"\n> >> \n> >> #include \"libcamera/internal/buffer.h\"\n> >> #include \"libcamera/internal/formats.h\"\n> >> \n> >> #include \n> >> \n> >> -class EncoderLibJpeg : public Encoder\n> >> +class PostProcessorJpeg : public PostProcessor\n> >> {\n> >> public:\n> >> -\tEncoderLibJpeg();\n> >> -\t~EncoderLibJpeg();\n> >> +\tPostProcessorJpeg();\n> >> +\t~PostProcessorJpeg();\n> >> \n> >> \tint configure(const libcamera::StreamConfiguration &cfg) override;\n> >> +\tint process(const libcamera::FrameBuffer *source,\n> >> +\t\t const libcamera::Span &destination,\n> >> +\t\t CameraMetadata *metadata,\n> >> +\t\t ...) override;\n> >> +\n> >> +\n> >> +private:\n> >> \tint encode(const libcamera::FrameBuffer *source,\n> >> \t\t const libcamera::Span &destination,\n> >> -\t\t const libcamera::Span &exifData) override;\n> >> +\t\t const libcamera::Span &exifData);\n> >> \n> >> -private:\n> >> \tvoid compressRGB(const libcamera::MappedBuffer *frame);\n> >> \tvoid compressNV(const libcamera::MappedBuffer *frame);\n> >> \n> >> @@ -40,4 +47,4 @@ private:\n> >> \tbool nvSwap_;\n> >> };\n> >> \n> >> -#endif /* __ANDROID_JPEG_ENCODER_LIBJPEG_H__ */\n> >> +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */\n> >> diff --git a/src/android/meson.build b/src/android/meson.build\n> >> index 802bb89..02b3b47 100644\n> >> --- a/src/android/meson.build\n> >> +++ b/src/android/meson.build\n> >> @@ -21,8 +21,8 @@ android_hal_sources = files([\n> >> 'camera_metadata.cpp',\n> >> 'camera_ops.cpp',\n> >> 'camera_stream.cpp',\n> >> - 'jpeg/encoder_libjpeg.cpp',\n> >> 'jpeg/exif.cpp',\n> >> + 'jpeg/post_processor_jpeg.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 94697BEEE0\n\tfor ;\n\tFri, 9 Oct 2020 21:58:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C5A06073B;\n\tFri, 9 Oct 2020 23:58:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E4F860358\n\tfor ;\n\tFri, 9 Oct 2020 23:58:55 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A72D1329;\n\tFri, 9 Oct 2020 23:58:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"m7SzQ5qb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602280734;\n\tbh=OGYf2RCFW9Fdsl8x004/29O42YabTThS3X3bE2H74eM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m7SzQ5qb378lRTI/pj7VPpohQCh0OinP3VfxGwhcSk0qV5PP3Y3E1uJAzxfNakEvc\n\tRQabn+ZRCcqd7W0RUOdSiYYr3ZVoDI+q7KFVGzkpN+K/2KtJvlUhqM2IUUL/wQQJAx\n\tcKw02YAZ6CDK+ccBE4AOp6vJh/we2bKAFBC0KwJw=","Date":"Sat, 10 Oct 2020 00:58:11 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20201009215811.GG25040@pendragon.ideasonboard.com>","References":"<20201008141038.83425-1-email@uajain.com>\n\t<20201008141038.83425-3-email@uajain.com>\n\t<20201008192240.GE3939@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 2/3] android: jpeg: Port to\n\tPostProcessor interface","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\" "}}]