[{"id":11475,"web_url":"https://patchwork.libcamera.org/comment/11475/","msgid":"<20200722085034.k7whf3riwj4sg33i@uno.localdomain>","date":"2020-07-22T08:50:34","subject":"Re: [libcamera-devel] [RFC PATCH 1/6] android: Introduce JPEG\n\tcompression","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n just skimming through.. some nits and questions below...\n\nOn Tue, Jul 21, 2020 at 11:01:21PM +0100, Kieran Bingham wrote:\n> Provide a compressor interface and implement a JPEG compressor using libjpeg.\n>\n> Signed-off-by: Kieran Bingham \n> ---\n> src/android/jpeg/compressor.h | 28 +++\n> src/android/jpeg/compressor_jpeg.cpp | 279 +++++++++++++++++++++++++++\n> src/android/jpeg/compressor_jpeg.h | 44 +++++\n> src/android/meson.build | 1 +\n> src/libcamera/meson.build | 2 +\n> 5 files changed, 354 insertions(+)\n> create mode 100644 src/android/jpeg/compressor.h\n> create mode 100644 src/android/jpeg/compressor_jpeg.cpp\n> create mode 100644 src/android/jpeg/compressor_jpeg.h\n>\n> diff --git a/src/android/jpeg/compressor.h b/src/android/jpeg/compressor.h\n> new file mode 100644\n> index 000000000000..f95e4a4539cb\n> --- /dev/null\n> +++ b/src/android/jpeg/compressor.h\n> @@ -0,0 +1,28 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * compressor.h - Image compression interface\n> + */\n\nDon't we usually have an empty line here ?\n\n> +#ifndef __LIBCAMERA_COMPRESSOR_H__\n> +#define __LIBCAMERA_COMPRESSOR_H__\n\nMaybe __LIBCAMERA_ANDROID_COMPRESSOR__ ?\n\n> +\n> +#include \n> +#include \n> +\n> +struct CompressedImage {\n> +\tunsigned char *data;\n> +\tunsigned long length;\n> +};\n> +\n> +class Compressor\n> +{\n> +public:\n> +\tvirtual ~Compressor() { };\n> +\n> +\tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> +\tvirtual int compress(const libcamera::FrameBuffer *source, CompressedImage *image) = 0;\n> +\tvirtual void free(CompressedImage *image) = 0;\n> +};\n\nDo we expect more compressors ? Why a pure virtual class to define an\ninterface ?\n\n> +\n> +#endif /* __LIBCAMERA_COMPRESSOR_H__ */\n> diff --git a/src/android/jpeg/compressor_jpeg.cpp b/src/android/jpeg/compressor_jpeg.cpp\n> new file mode 100644\n> index 000000000000..78fa5e399d99\n> --- /dev/null\n> +++ b/src/android/jpeg/compressor_jpeg.cpp\n> @@ -0,0 +1,279 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * compressor_jpeg.cpp - JPEG compression using libjpeg native API\n\ns/native API//\nOr is there a value in saying that ?\n\n> + */\n> +\n> +#include \"compressor_jpeg.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(JPEG)\n> +\n> +struct PixelFormatPlaneInfo {\n> +\tunsigned int bitsPerPixel;\n> +\tunsigned int hSubSampling;\n> +\tunsigned int vSubSampling;\n> +};\n> +\n> +struct PixelFormatInfo {\n> +\tJ_COLOR_SPACE colorSpace;\n> +\tunsigned int numPlanes;\n> +\tbool nvSwap;\n> +\tPixelFormatPlaneInfo planes[3];\n> +};\n\nIs there a reason why we can't use the libcamera PixelFormatInfo and\nmaybe keep an association with the J_COLOR_SPACE field ? The android\nlayer can use internal headers, right ?\n\n> +\n> +namespace {\n> +\n> +static const std::map pixelInfo{\n> +\t{ formats::R8, { JCS_GRAYSCALE, 1, false, { { 8, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n> +\n> +\t/* RGB formats. */\n> +\t{ formats::RGB888, { JCS_EXT_BGR, 1, false, { { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n> +\t{ formats::BGR888, { JCS_EXT_RGB, 1, false, { { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n> +\n> +\t/* YUV packed formats. */\n> +\t{ formats::UYVY, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n> +\t{ formats::VYUY, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n> +\t{ formats::YUYV, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n> +\t{ formats::YVYU, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n> +\n> +\t/* YUY planar formats. */\n> +\t{ formats::NV12, { JCS_YCbCr, 2, false, { { 8, 1, 1 }, { 16, 2, 2 }, { 0, 0, 0 } } } },\n> +\t{ formats::NV21, { JCS_YCbCr, 2, true, { { 8, 1, 1 }, { 16, 2, 2 }, { 0, 0, 0 } } } },\n> +\t{ formats::NV16, { JCS_YCbCr, 2, false, { { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } } } },\n> +\t{ formats::NV61, { JCS_YCbCr, 2, true, { { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } } } },\n> +\t{ formats::NV24, { JCS_YCbCr, 2, false, { { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } } } },\n> +\t{ formats::NV42, { JCS_YCbCr, 2, true, { { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } } } },\n> +};\n\nCould this become\n\nstruct JPEGPixelFormatInfo\n{\n J_COLOR_SPACE colorSpace;\n PixelFormatInfo pixelFormatInfo;\n};\n\nstd::map pixelInfo{\n\t{ formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888 },\n ..\n};\n\n> +\n> +}\n> +\n> +const struct PixelFormatInfo &findPixelInfo(const PixelFormat &format)\n> +{\n> +\tstatic const struct PixelFormatInfo invalidPixelFormat {\n> +\t\t\tJCS_UNKNOWN, 0, false, {}\n> +\t};\n> +\n> +\tconst auto iter = pixelInfo.find(format);\n> +\tif (iter == pixelInfo.end()) {\n> +\t\tLOG(JPEG, Error) << \"Unsupported pixel format for JPEG compressor: \"\n> +\t\t\t \t << format.toString();\n> +\t\treturn invalidPixelFormat;\n> +\t}\n> +\n> +\treturn iter->second;\n> +}\n> +\n> +CompressorJPEG::CompressorJPEG()\n> +\t: quality_(95)\n> +{\n> +\t/* \\todo: Expand error handling coverage. */\n> +\tcompress_.err = jpeg_std_error(&jerr_);\n> +\n> +\tjpeg_create_compress(&compress_);\n> +}\n> +\n> +CompressorJPEG::~CompressorJPEG()\n> +{\n> +\tjpeg_destroy_compress(&compress_);\n> +}\n> +\n> +int CompressorJPEG::configure(const StreamConfiguration &cfg)\n> +{\n> +\t{\n> +\t\tLOG(JPEG, Warning) << \"Configuring pixelformat as : \"\n> +\t\t\t\t\t<< cfg.pixelFormat.toString();\n> +\t\tLOG(JPEG, Warning) << \" : \" << cfg.toString();\n> +\n> +\t\tstd::vector formats = cfg.formats().pixelformats();\n> +\t\tLOG(JPEG, Warning) << \"StreamConfiguration supports \" << formats.size() << \" formats:\";\n> +\t\tfor (const PixelFormat &format : formats)\n> +\t\t\tLOG(JPEG, Warning) << \" - \" << format.toString();\n> +\t}\n> +\n> +\tconst struct PixelFormatInfo info = findPixelInfo(cfg.pixelFormat);\n> +\tif (info.colorSpace == JCS_UNKNOWN)\n> +\t\treturn -ENOTSUP;\n> +\n> +\t/*\n> +\t * Todo: The stride given by the stream configuration has caused issues.\n> +\t * Validate it and also handle per-plane strides.\n\nOn which platform ? IPU3 ? Is there an error on how the stride is\nreported ?\n\n> +\t */\n> +\tstride_ = cfg.stride;\n> +\tstride_ = cfg.size.width * info.planes[0].bitsPerPixel / 8;\n> +\t/* Saw some errors with strides, so this is a debug/develop check */\n> +\tif (cfg.stride != stride_)\n> +\t\tLOG(JPEG, Error) << \"*** StreamConfigure provided stride of \"\n> +\t\t\t\t << cfg.stride << \" rather than \" << stride_;\n> +\n> +\tcompress_.image_width = cfg.size.width;\n> +\tcompress_.image_height = cfg.size.height;\n> +\tcompress_.in_color_space = info.colorSpace;\n> +\n> +\tcompress_.input_components = info.colorSpace == JCS_GRAYSCALE ? 1 : 3;\n> +\n> +\tjpeg_set_defaults(&compress_);\n> +\tjpeg_set_quality(&compress_, quality_, TRUE);\n> +\n> +\tnv_ = info.numPlanes == 2;\n> +\tnvSwap_ = info.nvSwap;\n> +\n> +\t/* Use the 'last' plane for subsampling of component info. */\n> +\tunsigned int p = info.numPlanes - 1;\n> +\thorzSubSample_ = info.planes[p].hSubSampling;\n> +\tvertSubSample_ = info.planes[p].vSubSampling;\n> +\n\nYou could infer the sub-sampling from libcamera PixelFormatInfo I\nguess\n\n> +\treturn 0;\n> +}\n> +\n> +void CompressorJPEG::compressRGB(const libcamera::MappedFrameBuffer *frame)\n> +{\n> +\tunsigned char *src = static_cast(frame->maps()[0].address);\n> +\n> +\tJSAMPROW row_pointer[1];\n> +\n> +\twhile (compress_.next_scanline < compress_.image_height) {\n> +\t\trow_pointer[0] = &src[compress_.next_scanline * stride_];\n> +\t\tjpeg_write_scanlines(&compress_, row_pointer, 1);\n> +\t}\n> +}\n> +\n> +/*\n> + * A very dull implementation to compress YUYV.\n> + * To be converted to a generic algorithm akin to NV12.\n> + * If it can be shared with NV12 great, but we might be able to further\n> + * optimisze the NV layouts by only depacking the CrCb pixels.\n> + */\n> +void CompressorJPEG::compressYUV(const libcamera::MappedFrameBuffer *frame)\n> +{\n> +\tstd::vector tmprowbuf(compress_.image_width * 3);\n> +\tunsigned char *input = static_cast(frame->maps()[0].address);\n> +\n> +\tJSAMPROW row_pointer[1];\n> +\trow_pointer[0] = &tmprowbuf[0];\n> +\twhile (compress_.next_scanline < compress_.image_height) {\n> +\t\tunsigned i, j;\n> +\t\tunsigned offset = compress_.next_scanline * compress_.image_width * 2; //offset to the correct row\n> +\t\tfor (i = 0, j = 0; i < compress_.image_width * 2; i += 4, j += 6) { //input strides by 4 bytes, output strides by 6 (2 pixels)\n> +\t\t\ttmprowbuf[j + 0] = input[offset + i + 0]; // Y (unique to this pixel)\n> +\t\t\ttmprowbuf[j + 1] = input[offset + i + 1]; // U (shared between pixels)\n> +\t\t\ttmprowbuf[j + 2] = input[offset + i + 3]; // V (shared between pixels)\n> +\t\t\ttmprowbuf[j + 3] = input[offset + i + 2]; // Y (unique to this pixel)\n> +\t\t\ttmprowbuf[j + 4] = input[offset + i + 1]; // U (shared between pixels)\n> +\t\t\ttmprowbuf[j + 5] = input[offset + i + 3]; // V (shared between pixels)\n> +\t\t}\n> +\t\tjpeg_write_scanlines(&compress_, row_pointer, 1);\n> +\t}\n> +}\n> +\n> +/*\n> + * Really inefficient NV unpacking to YUV888 for JPEG compress.\n> + * This /could/ be improved (drastically I hope) ;-)\n> + */\n> +void CompressorJPEG::compressNV(const libcamera::MappedFrameBuffer *frame)\n> +{\n> +\tstd::vector tmprowbuf(compress_.image_width * 3);\n> +\n> +\t/*\n> +\t * Todo: Use the raw api, and only unpack the cb/cr samples to new line buffers.\n\nIf you want this and other todo items recorded: \\todo\n\n> +\t * If possible, see if we can set appropriate pixel strides too to save even that copy.\n> +\t *\n> +\t * Possible hints at:\n> +\t * https://sourceforge.net/p/libjpeg/mailman/message/30815123/\n> +\t */\n> +\tunsigned int c_stride = compress_.image_width * (2 / horzSubSample_);\n> +\tunsigned int c_inc = horzSubSample_ == 1 ? 2 : 0;\n> +\tunsigned int cb_pos = nvSwap_ ? 1 : 0;\n> +\tunsigned int cr_pos = nvSwap_ ? 0 : 1;\n> +\n> +\tconst unsigned char *src = static_cast(frame->maps()[0].address);\n> +\tconst unsigned char *src_c = src + compress_.image_width * compress_.image_height; // * stride[0] surely\n> +\n> +\tJSAMPROW row_pointer[1];\n> +\trow_pointer[0] = &tmprowbuf[0];\n> +\n> +\tfor (unsigned int y = 0; y < compress_.image_height; y++) {\n> +\t\tunsigned char *dst = &tmprowbuf[0];\n> +\n> +\t\tconst unsigned char *src_y = src + y * compress_.image_width;\n> +\t\tconst unsigned char *src_cb = src_c + (y / vertSubSample_) *\n> +\t\t\t\t\t c_stride + cb_pos;\n> +\t\tconst unsigned char *src_cr = src_c + (y / vertSubSample_) *\n> +\t\t\t\t\t c_stride + cr_pos;\n> +\n> +\t\tfor (unsigned int x = 0; x < compress_.image_width; x += 2) {\n> +\t\t\tdst[0] = *src_y;\n> +\t\t\tdst[1] = *src_cb;\n> +\t\t\tdst[2] = *src_cr;\n> +\t\t\tsrc_y++;\n> +\t\t\tsrc_cb += c_inc;\n> +\t\t\tsrc_cr += c_inc;\n> +\t\t\tdst += 3;\n> +\n> +\t\t\tdst[0] = *src_y;\n> +\t\t\tdst[1] = *src_cb;\n> +\t\t\tdst[2] = *src_cr;\n> +\t\t\tsrc_y++;\n> +\t\t\tsrc_cb += 2;\n> +\t\t\tsrc_cr += 2;\n> +\t\t\tdst += 3;\n> +\t\t}\n> +\n> +\t\tjpeg_write_scanlines(&compress_, row_pointer, 1);\n> +\t}\n> +}\n> +\n> +int CompressorJPEG::compress(const FrameBuffer *source, CompressedImage *jpeg)\n> +{\n> +\tMappedFrameBuffer frame(source, PROT_READ);\n> +\tif (!frame.isValid()) {\n> +\t\tLOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n> +\t\t\t\t << strerror(frame.error());\n> +\t\treturn -frame.error();\n> +\t}\n> +\n> +\tjpeg_mem_dest(&compress_, &jpeg->data, &jpeg->length);\n> +\n> +\tjpeg_start_compress(&compress_, TRUE);\n> +\n> +\tLOG(JPEG, Debug) << \"JPEG Compress Starting\";\n> +\tLOG(JPEG, Debug) << \"Width: \" << compress_.image_width\n> +\t\t\t << \" height: \" << compress_.image_height\n> +\t\t\t << \" stride: \" << stride_;\n> +\n> +\tif (nv_)\n> +\t\tcompressNV(&frame);\n> +\telse if (compress_.in_color_space == JCS_YCbCr)\n> +\t\tcompressYUV(&frame);\n> +\telse\n> +\t\tcompressRGB(&frame);\n\nIf you have your map of PixelFormat to a custom class you can there\nstore a pointer to the conversion function associated to each format and\navoid the switch here\n\n> +\n> +\tLOG(JPEG, Debug) << \"JPEG Compress Completed\";\n> +\n> +\tjpeg_finish_compress(&compress_);\n> +\n> +\treturn 0;\n> +}\n> +\n> +void CompressorJPEG::free(CompressedImage *jpeg)\n> +{\n> +\t::free(jpeg->data);\n> +\tjpeg->data = nullptr;\n> +}\n> diff --git a/src/android/jpeg/compressor_jpeg.h b/src/android/jpeg/compressor_jpeg.h\n> new file mode 100644\n> index 000000000000..11009481a2fe\n> --- /dev/null\n> +++ b/src/android/jpeg/compressor_jpeg.h\n> @@ -0,0 +1,44 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * compressor_jpeg.h - JPEG compression using libjpeg\n> + */\n> +#ifndef __COMPRESSOR_JPEG_H__\n> +#define __COMPRESSOR_JPEG_H__\n\nSame as above, I think ANDROID_ should be part of the inclusion guard\n\n> +\n> +#include \"compressor.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +class CompressorJPEG : public Compressor\n> +{\n> +public:\n> +\tCompressorJPEG();\n> +\t~CompressorJPEG();\n> +\n> +\tint configure(const libcamera::StreamConfiguration &cfg);\n> +\tint compress(const libcamera::FrameBuffer *source, CompressedImage *jpeg);\n> +\tvoid free(CompressedImage *jpeg);\n> +\n> +private:\n> +\tvoid compressRGB(const libcamera::MappedFrameBuffer *frame);\n> +\tvoid compressYUV(const libcamera::MappedFrameBuffer *frame);\n> +\tvoid compressNV(const libcamera::MappedFrameBuffer *frame);\n> +\n> +\tstruct jpeg_compress_struct compress_;\n> +\tstruct jpeg_error_mgr jerr_;\n> +\n> +\tunsigned int quality_;\n> +\tunsigned int stride_;\n> +\n> +\tbool nv_;\n> +\tbool nvSwap_;\n> +\tunsigned int horzSubSample_;\n> +\tunsigned int vertSubSample_;\n\nThese information should be decoupled from the compressor class, as\nthey belong to the format the is being compressed, not to the\ncompressor instance. What do you think ? Are compressors throw away\nobjects created to run once on a format and thrown away ?\n\n> +};\n> +\n> +#endif /* __COMPRESSOR_JPEG_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 822cad621f01..51dcd99ee62f 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -6,6 +6,7 @@ android_hal_sources = files([\n> 'camera_device.cpp',\n> 'camera_metadata.cpp',\n> 'camera_ops.cpp',\n> + 'jpeg/compressor_jpeg.cpp',\n> ])\n>\n> android_camera_metadata_sources = files([\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 3aad4386ffc2..d78e2c1f6eb8 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -124,6 +124,8 @@ if get_option('android')\n> libcamera_sources += android_hal_sources\n> includes += android_includes\n> libcamera_link_with += android_camera_metadata\n> +\n> + libcamera_deps += dependency('libjpeg')\n\nShouldn't this be a dependency introduced by compiling the android HAL\nin ?\n\nThanks\n j\n\n> endif\n>\n> # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.\n> --\n> 2.25.1\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 429B7BDB1B\n\tfor ;\n\tWed, 22 Jul 2020 08:46:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D34E860491;\n\tWed, 22 Jul 2020 10:46:58 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CEC306039C\n\tfor ;\n\tWed, 22 Jul 2020 10:46:56 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 1738BFF80B;\n\tWed, 22 Jul 2020 08:46:55 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 22 Jul 2020 10:50:34 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20200722085034.k7whf3riwj4sg33i@uno.localdomain>","References":"<20200721220126.202065-1-kieran.bingham@ideasonboard.com>\n\t<20200721220126.202065-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200721220126.202065-2-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 1/6] android: Introduce JPEG\n\tcompression","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11476,"web_url":"https://patchwork.libcamera.org/comment/11476/","msgid":"","date":"2020-07-22T10:17:50","subject":"Re: [libcamera-devel] [RFC PATCH 1/6] android: Introduce JPEG\n\tcompression","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 22/07/2020 09:50, Jacopo Mondi wrote:\n> Hi Kieran,\n> just skimming through.. some nits and questions below...\n> \n> On Tue, Jul 21, 2020 at 11:01:21PM +0100, Kieran Bingham wrote:\n>> Provide a compressor interface and implement a JPEG compressor using libjpeg.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/android/jpeg/compressor.h | 28 +++\n>> src/android/jpeg/compressor_jpeg.cpp | 279 +++++++++++++++++++++++++++\n>> src/android/jpeg/compressor_jpeg.h | 44 +++++\n>> src/android/meson.build | 1 +\n>> src/libcamera/meson.build | 2 +\n>> 5 files changed, 354 insertions(+)\n>> create mode 100644 src/android/jpeg/compressor.h\n>> create mode 100644 src/android/jpeg/compressor_jpeg.cpp\n>> create mode 100644 src/android/jpeg/compressor_jpeg.h\n>>\n>> diff --git a/src/android/jpeg/compressor.h b/src/android/jpeg/compressor.h\n>> new file mode 100644\n>> index 000000000000..f95e4a4539cb\n>> --- /dev/null\n>> +++ b/src/android/jpeg/compressor.h\n>> @@ -0,0 +1,28 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * compressor.h - Image compression interface\n>> + */\n> \n> Don't we usually have an empty line here ?\n> \n>> +#ifndef __LIBCAMERA_COMPRESSOR_H__\n>> +#define __LIBCAMERA_COMPRESSOR_H__\n> \n> Maybe __LIBCAMERA_ANDROID_COMPRESSOR__ ?\n\nThis will be moved to a more generic area.\n\nI don't know what to name that yet, so it's here for now.\n\nI expect we should have\n\nsrc/libcamera-helper-library/include/compressor.h\n\n\nBut I don't have src/libcamera-helper-library yet ';-)\n\nMaybe now is the time to create it.\n\nWho wants to propose a name?\n\n\n>> +\n>> +#include \n>> +#include \n>> +\n>> +struct CompressedImage {\n>> +\tunsigned char *data;\n>> +\tunsigned long length;\n>> +};\n>> +\n>> +class Compressor\n>> +{\n>> +public:\n>> +\tvirtual ~Compressor() { };\n>> +\n>> +\tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n>> +\tvirtual int compress(const libcamera::FrameBuffer *source, CompressedImage *image) = 0;\n>> +\tvirtual void free(CompressedImage *image) = 0;\n>> +};\n> \n> Do we expect more compressors ? Why a pure virtual class to define an\n> interface ?\n\nI do indeed expect more compressors.\n\nI've already had two software implementions of the JPEG compressor. One\nusing the libjpeg API, and one using the libjpeg-turbo API.\n\nI've now ditched the -turbo, with an aim to being able to make use of\nthe raw libjpeg api in the native implementation.\n\nWe will also expect a hardware accelerated JPEG compressor to appear....\n\n\nAnd perhaps there will be an h264 compressor, or an h265 compressor\nneeded sometime ...\n\n\n>> +\n>> +#endif /* __LIBCAMERA_COMPRESSOR_H__ */\n>> diff --git a/src/android/jpeg/compressor_jpeg.cpp b/src/android/jpeg/compressor_jpeg.cpp\n>> new file mode 100644\n>> index 000000000000..78fa5e399d99\n>> --- /dev/null\n>> +++ b/src/android/jpeg/compressor_jpeg.cpp\n>> @@ -0,0 +1,279 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * compressor_jpeg.cpp - JPEG compression using libjpeg native API\n> \n> s/native API//\n> Or is there a value in saying that ?\n\n\nThere are two libjpeg APIs. A 'native' and a 'turbo'. They both end up\nexecuting the same code, and are supposedly the same performance, but\nthe -turbo API is simpler and doesn't have as much control over\narbitrary data formats.\n\n(the -turbo comes from the name of the accelerated libjpeg library - not\nthe api, but it extends the native-api somewhat).\n\n\n>> + */\n>> +\n>> +#include \"compressor_jpeg.h\"\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \"libcamera/internal/log.h\"\n>> +\n>> +using namespace libcamera;\n>> +\n>> +LOG_DEFINE_CATEGORY(JPEG)\n>> +\n>> +struct PixelFormatPlaneInfo {\n>> +\tunsigned int bitsPerPixel;\n>> +\tunsigned int hSubSampling;\n>> +\tunsigned int vSubSampling;\n>> +};\n>> +\n>> +struct PixelFormatInfo {\n>> +\tJ_COLOR_SPACE colorSpace;\n>> +\tunsigned int numPlanes;\n>> +\tbool nvSwap;\n>> +\tPixelFormatPlaneInfo planes[3];\n>> +};\n> \n> Is there a reason why we can't use the libcamera PixelFormatInfo and\n> maybe keep an association with the J_COLOR_SPACE field ? The android\n> layer can use internal headers, right ?\n\nI certainly hope so, but they weren't around when I created this file. I\nhadn't swapped over yet, because I have been trying to work on getting\nthe Camera app to function.\n\n\n>> +\n>> +namespace {\n>> +\n>> +static const std::map pixelInfo{\n>> +\t{ formats::R8, { JCS_GRAYSCALE, 1, false, { { 8, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\n>> +\t/* RGB formats. */\n>> +\t{ formats::RGB888, { JCS_EXT_BGR, 1, false, { { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\t{ formats::BGR888, { JCS_EXT_RGB, 1, false, { { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\n>> +\t/* YUV packed formats. */\n>> +\t{ formats::UYVY, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\t{ formats::VYUY, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\t{ formats::YUYV, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\t{ formats::YVYU, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\n>> +\t/* YUY planar formats. */\n>> +\t{ formats::NV12, { JCS_YCbCr, 2, false, { { 8, 1, 1 }, { 16, 2, 2 }, { 0, 0, 0 } } } },\n>> +\t{ formats::NV21, { JCS_YCbCr, 2, true, { { 8, 1, 1 }, { 16, 2, 2 }, { 0, 0, 0 } } } },\n>> +\t{ formats::NV16, { JCS_YCbCr, 2, false, { { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } } } },\n>> +\t{ formats::NV61, { JCS_YCbCr, 2, true, { { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } } } },\n>> +\t{ formats::NV24, { JCS_YCbCr, 2, false, { { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } } } },\n>> +\t{ formats::NV42, { JCS_YCbCr, 2, true, { { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } } } },\n>> +};\n> \n> Could this become\n> \n> struct JPEGPixelFormatInfo\n> {\n> J_COLOR_SPACE colorSpace;\n> PixelFormatInfo pixelFormatInfo;\n> };\n> \n> std::map pixelInfo{\n> \t{ formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888 },\n> ..\n> };\n\nI hope so ;-)\n\n\n\n>> +\n>> +}\n>> +\n>> +const struct PixelFormatInfo &findPixelInfo(const PixelFormat &format)\n>> +{\n>> +\tstatic const struct PixelFormatInfo invalidPixelFormat {\n>> +\t\t\tJCS_UNKNOWN, 0, false, {}\n>> +\t};\n>> +\n>> +\tconst auto iter = pixelInfo.find(format);\n>> +\tif (iter == pixelInfo.end()) {\n>> +\t\tLOG(JPEG, Error) << \"Unsupported pixel format for JPEG compressor: \"\n>> +\t\t\t \t << format.toString();\n>> +\t\treturn invalidPixelFormat;\n>> +\t}\n>> +\n>> +\treturn iter->second;\n>> +}\n>> +\n>> +CompressorJPEG::CompressorJPEG()\n>> +\t: quality_(95)\n>> +{\n>> +\t/* \\todo: Expand error handling coverage. */\n>> +\tcompress_.err = jpeg_std_error(&jerr_);\n>> +\n>> +\tjpeg_create_compress(&compress_);\n>> +}\n>> +\n>> +CompressorJPEG::~CompressorJPEG()\n>> +{\n>> +\tjpeg_destroy_compress(&compress_);\n>> +}\n>> +\n>> +int CompressorJPEG::configure(const StreamConfiguration &cfg)\n>> +{\n>> +\t{\n>> +\t\tLOG(JPEG, Warning) << \"Configuring pixelformat as : \"\n>> +\t\t\t\t\t<< cfg.pixelFormat.toString();\n>> +\t\tLOG(JPEG, Warning) << \" : \" << cfg.toString();\n>> +\n>> +\t\tstd::vector formats = cfg.formats().pixelformats();\n>> +\t\tLOG(JPEG, Warning) << \"StreamConfiguration supports \" << formats.size() << \" formats:\";\n>> +\t\tfor (const PixelFormat &format : formats)\n>> +\t\t\tLOG(JPEG, Warning) << \" - \" << format.toString();\n>> +\t}\n>> +\n>> +\tconst struct PixelFormatInfo info = findPixelInfo(cfg.pixelFormat);\n>> +\tif (info.colorSpace == JCS_UNKNOWN)\n>> +\t\treturn -ENOTSUP;\n>> +\n>> +\t/*\n>> +\t * Todo: The stride given by the stream configuration has caused issues.\n>> +\t * Validate it and also handle per-plane strides.\n> \n> On which platform ? IPU3 ? Is there an error on how the stride is\n> reported ?\n\n\nEarly on in development I was /always/ getting a zero or somehow\nincorrect stride. I can't remember now, but this is here to make sure I\ndon't hit it again while developing ;-)\n\nI believe it did happen on IPU3 I thought, but I can't recall to be sure.\n\n\n>> +\t */\n>> +\tstride_ = cfg.stride;\n>> +\tstride_ = cfg.size.width * info.planes[0].bitsPerPixel / 8;\n>> +\t/* Saw some errors with strides, so this is a debug/develop check */\n>> +\tif (cfg.stride != stride_)\n>> +\t\tLOG(JPEG, Error) << \"*** StreamConfigure provided stride of \"\n>> +\t\t\t\t << cfg.stride << \" rather than \" << stride_;\n>> +\n>> +\tcompress_.image_width = cfg.size.width;\n>> +\tcompress_.image_height = cfg.size.height;\n>> +\tcompress_.in_color_space = info.colorSpace;\n>> +\n>> +\tcompress_.input_components = info.colorSpace == JCS_GRAYSCALE ? 1 : 3;\n>> +\n>> +\tjpeg_set_defaults(&compress_);\n>> +\tjpeg_set_quality(&compress_, quality_, TRUE);\n>> +\n>> +\tnv_ = info.numPlanes == 2;\n>> +\tnvSwap_ = info.nvSwap;\n>> +\n>> +\t/* Use the 'last' plane for subsampling of component info. */\n>> +\tunsigned int p = info.numPlanes - 1;\n>> +\thorzSubSample_ = info.planes[p].hSubSampling;\n>> +\tvertSubSample_ = info.planes[p].vSubSampling;\n>> +\n> \n> You could infer the sub-sampling from libcamera PixelFormatInfo I\n> guess\n\nYes, that will be better, but wasn't availble when this was written.\nI'll update.\n\n\n> \n>> +\treturn 0;\n>> +}\n>> +\n>> +void CompressorJPEG::compressRGB(const libcamera::MappedFrameBuffer *frame)\n>> +{\n>> +\tunsigned char *src = static_cast(frame->maps()[0].address);\n>> +\n>> +\tJSAMPROW row_pointer[1];\n>> +\n>> +\twhile (compress_.next_scanline < compress_.image_height) {\n>> +\t\trow_pointer[0] = &src[compress_.next_scanline * stride_];\n>> +\t\tjpeg_write_scanlines(&compress_, row_pointer, 1);\n>> +\t}\n>> +}\n>> +\n>> +/*\n>> + * A very dull implementation to compress YUYV.\n>> + * To be converted to a generic algorithm akin to NV12.\n>> + * If it can be shared with NV12 great, but we might be able to further\n>> + * optimisze the NV layouts by only depacking the CrCb pixels.\n>> + */\n>> +void CompressorJPEG::compressYUV(const libcamera::MappedFrameBuffer *frame)\n>> +{\n>> +\tstd::vector tmprowbuf(compress_.image_width * 3);\n>> +\tunsigned char *input = static_cast(frame->maps()[0].address);\n>> +\n>> +\tJSAMPROW row_pointer[1];\n>> +\trow_pointer[0] = &tmprowbuf[0];\n>> +\twhile (compress_.next_scanline < compress_.image_height) {\n>> +\t\tunsigned i, j;\n>> +\t\tunsigned offset = compress_.next_scanline * compress_.image_width * 2; //offset to the correct row\n>> +\t\tfor (i = 0, j = 0; i < compress_.image_width * 2; i += 4, j += 6) { //input strides by 4 bytes, output strides by 6 (2 pixels)\n>> +\t\t\ttmprowbuf[j + 0] = input[offset + i + 0]; // Y (unique to this pixel)\n>> +\t\t\ttmprowbuf[j + 1] = input[offset + i + 1]; // U (shared between pixels)\n>> +\t\t\ttmprowbuf[j + 2] = input[offset + i + 3]; // V (shared between pixels)\n>> +\t\t\ttmprowbuf[j + 3] = input[offset + i + 2]; // Y (unique to this pixel)\n>> +\t\t\ttmprowbuf[j + 4] = input[offset + i + 1]; // U (shared between pixels)\n>> +\t\t\ttmprowbuf[j + 5] = input[offset + i + 3]; // V (shared between pixels)\n>> +\t\t}\n>> +\t\tjpeg_write_scanlines(&compress_, row_pointer, 1);\n>> +\t}\n>> +}\n>> +\n>> +/*\n>> + * Really inefficient NV unpacking to YUV888 for JPEG compress.\n>> + * This /could/ be improved (drastically I hope) ;-)\n>> + */\n>> +void CompressorJPEG::compressNV(const libcamera::MappedFrameBuffer *frame)\n>> +{\n>> +\tstd::vector tmprowbuf(compress_.image_width * 3);\n>> +\n>> +\t/*\n>> +\t * Todo: Use the raw api, and only unpack the cb/cr samples to new line buffers.\n> \n> If you want this and other todo items recorded: \\todo\n\nThat's a note to me currently - I didn't expect this to be posted ;-)\n\n> \n>> +\t * If possible, see if we can set appropriate pixel strides too to save even that copy.\n>> +\t *\n>> +\t * Possible hints at:\n>> +\t * https://sourceforge.net/p/libjpeg/mailman/message/30815123/\n>> +\t */\n>> +\tunsigned int c_stride = compress_.image_width * (2 / horzSubSample_);\n>> +\tunsigned int c_inc = horzSubSample_ == 1 ? 2 : 0;\n>> +\tunsigned int cb_pos = nvSwap_ ? 1 : 0;\n>> +\tunsigned int cr_pos = nvSwap_ ? 0 : 1;\n>> +\n>> +\tconst unsigned char *src = static_cast(frame->maps()[0].address);\n>> +\tconst unsigned char *src_c = src + compress_.image_width * compress_.image_height; // * stride[0] surely\n>> +\n>> +\tJSAMPROW row_pointer[1];\n>> +\trow_pointer[0] = &tmprowbuf[0];\n>> +\n>> +\tfor (unsigned int y = 0; y < compress_.image_height; y++) {\n>> +\t\tunsigned char *dst = &tmprowbuf[0];\n>> +\n>> +\t\tconst unsigned char *src_y = src + y * compress_.image_width;\n>> +\t\tconst unsigned char *src_cb = src_c + (y / vertSubSample_) *\n>> +\t\t\t\t\t c_stride + cb_pos;\n>> +\t\tconst unsigned char *src_cr = src_c + (y / vertSubSample_) *\n>> +\t\t\t\t\t c_stride + cr_pos;\n>> +\n>> +\t\tfor (unsigned int x = 0; x < compress_.image_width; x += 2) {\n>> +\t\t\tdst[0] = *src_y;\n>> +\t\t\tdst[1] = *src_cb;\n>> +\t\t\tdst[2] = *src_cr;\n>> +\t\t\tsrc_y++;\n>> +\t\t\tsrc_cb += c_inc;\n>> +\t\t\tsrc_cr += c_inc;\n>> +\t\t\tdst += 3;\n>> +\n>> +\t\t\tdst[0] = *src_y;\n>> +\t\t\tdst[1] = *src_cb;\n>> +\t\t\tdst[2] = *src_cr;\n>> +\t\t\tsrc_y++;\n>> +\t\t\tsrc_cb += 2;\n>> +\t\t\tsrc_cr += 2;\n>> +\t\t\tdst += 3;\n>> +\t\t}\n>> +\n>> +\t\tjpeg_write_scanlines(&compress_, row_pointer, 1);\n>> +\t}\n>> +}\n>> +\n>> +int CompressorJPEG::compress(const FrameBuffer *source, CompressedImage *jpeg)\n>> +{\n>> +\tMappedFrameBuffer frame(source, PROT_READ);\n>> +\tif (!frame.isValid()) {\n>> +\t\tLOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n>> +\t\t\t\t << strerror(frame.error());\n>> +\t\treturn -frame.error();\n>> +\t}\n>> +\n>> +\tjpeg_mem_dest(&compress_, &jpeg->data, &jpeg->length);\n>> +\n>> +\tjpeg_start_compress(&compress_, TRUE);\n>> +\n>> +\tLOG(JPEG, Debug) << \"JPEG Compress Starting\";\n>> +\tLOG(JPEG, Debug) << \"Width: \" << compress_.image_width\n>> +\t\t\t << \" height: \" << compress_.image_height\n>> +\t\t\t << \" stride: \" << stride_;\n>> +\n>> +\tif (nv_)\n>> +\t\tcompressNV(&frame);\n>> +\telse if (compress_.in_color_space == JCS_YCbCr)\n>> +\t\tcompressYUV(&frame);\n>> +\telse\n>> +\t\tcompressRGB(&frame);\n> \n> If you have your map of PixelFormat to a custom class you can there\n> store a pointer to the conversion function associated to each format and\n> avoid the switch here\n\nI like that idea!\n\n\n\n>> +\n>> +\tLOG(JPEG, Debug) << \"JPEG Compress Completed\";\n>> +\n>> +\tjpeg_finish_compress(&compress_);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +void CompressorJPEG::free(CompressedImage *jpeg)\n>> +{\n>> +\t::free(jpeg->data);\n>> +\tjpeg->data = nullptr;\n>> +}\n>> diff --git a/src/android/jpeg/compressor_jpeg.h b/src/android/jpeg/compressor_jpeg.h\n>> new file mode 100644\n>> index 000000000000..11009481a2fe\n>> --- /dev/null\n>> +++ b/src/android/jpeg/compressor_jpeg.h\n>> @@ -0,0 +1,44 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * compressor_jpeg.h - JPEG compression using libjpeg\n>> + */\n>> +#ifndef __COMPRESSOR_JPEG_H__\n>> +#define __COMPRESSOR_JPEG_H__\n> \n> Same as above, I think ANDROID_ should be part of the inclusion guard\n\nI think we will very quickly move these to their own component structure\nunder src, so I might infact rename this to LIBCAMERA_COMPRESSOR_JPEG\n\n> \n>> +\n>> +#include \"compressor.h\"\n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +class CompressorJPEG : public Compressor\n>> +{\n>> +public:\n>> +\tCompressorJPEG();\n>> +\t~CompressorJPEG();\n>> +\n>> +\tint configure(const libcamera::StreamConfiguration &cfg);\n>> +\tint compress(const libcamera::FrameBuffer *source, CompressedImage *jpeg);\n>> +\tvoid free(CompressedImage *jpeg);\n>> +\n>> +private:\n>> +\tvoid compressRGB(const libcamera::MappedFrameBuffer *frame);\n>> +\tvoid compressYUV(const libcamera::MappedFrameBuffer *frame);\n>> +\tvoid compressNV(const libcamera::MappedFrameBuffer *frame);\n>> +\n>> +\tstruct jpeg_compress_struct compress_;\n>> +\tstruct jpeg_error_mgr jerr_;\n>> +\n>> +\tunsigned int quality_;\n>> +\tunsigned int stride_;\n>> +\n>> +\tbool nv_;\n>> +\tbool nvSwap_;\n>> +\tunsigned int horzSubSample_;\n>> +\tunsigned int vertSubSample_;\n> \n> These information should be decoupled from the compressor class, as\n> they belong to the format the is being compressed, not to the\n> compressor instance. What do you think ? Are compressors throw away\n> objects created to run once on a format and thrown away ?\n\nThis is just a caching of the configuration that was selected. When it\ncan come from the PixelFormatInfo, I expect it will store just a\nreference to that instead.\n\n\n>> +};\n>> +\n>> +#endif /* __COMPRESSOR_JPEG_H__ */\n>> diff --git a/src/android/meson.build b/src/android/meson.build\n>> index 822cad621f01..51dcd99ee62f 100644\n>> --- a/src/android/meson.build\n>> +++ b/src/android/meson.build\n>> @@ -6,6 +6,7 @@ android_hal_sources = files([\n>> 'camera_device.cpp',\n>> 'camera_metadata.cpp',\n>> 'camera_ops.cpp',\n>> + 'jpeg/compressor_jpeg.cpp',\n>> ])\n>>\n>> android_camera_metadata_sources = files([\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index 3aad4386ffc2..d78e2c1f6eb8 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -124,6 +124,8 @@ if get_option('android')\n>> libcamera_sources += android_hal_sources\n>> includes += android_includes\n>> libcamera_link_with += android_camera_metadata\n>> +\n>> + libcamera_deps += dependency('libjpeg')\n> \n> Shouldn't this be a dependency introduced by compiling the android HAL\n> in ?\n> \n\nit is ... it's under the if get_option('android') section.\n\n\n> Thanks\n> j\n> \n>> endif\n>>\n>> # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.\n>> --\n>> 2.25.1\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 4FC34C2E68\n\tfor ;\n\tWed, 22 Jul 2020 10:17:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D8CFE6093D;\n\tWed, 22 Jul 2020 12:17:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 207066039F\n\tfor ;\n\tWed, 22 Jul 2020 12:17:53 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 781BF329;\n\tWed, 22 Jul 2020 12:17:52 +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=\"QYobroUJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595413072;\n\tbh=Bp+0VCmscwyJ6HvdCLnE53Z6T7sluVGrW8WMfE3qwY8=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=QYobroUJ4qN4YFrSp2aisy12fpUNgF50b7S6swfB7X86fgiElsn+TkmeNu6KevETO\n\tRuA0tNeNrnPt7HI6C3B4oxFeDFB9/cBApXjzgFQGLOdxQYJbeH/jCpcx5IV41bc/d2\n\tuM8ZX8RDlWOeQb7powhhC9YluISbTLU1aKP9uf2w=","To":"Jacopo Mondi ","References":"<20200721220126.202065-1-kieran.bingham@ideasonboard.com>\n\t<20200721220126.202065-2-kieran.bingham@ideasonboard.com>\n\t<20200722085034.k7whf3riwj4sg33i@uno.localdomain>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 22 Jul 2020 11:17:50 +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":"<20200722085034.k7whf3riwj4sg33i@uno.localdomain>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH 1/6] android: Introduce JPEG\n\tcompression","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11477,"web_url":"https://patchwork.libcamera.org/comment/11477/","msgid":"<20200722103159.lf4t6sb3hburnm6s@uno.localdomain>","date":"2020-07-22T10:31:59","subject":"Re: [libcamera-devel] [RFC PATCH 1/6] android: Introduce JPEG\n\tcompression","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Wed, Jul 22, 2020 at 11:17:50AM +0100, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n\nsnip\n\n> >> +\tunsigned int stride_;\n> >> +\n> >> +\tbool nv_;\n> >> +\tbool nvSwap_;\n> >> +\tunsigned int horzSubSample_;\n> >> +\tunsigned int vertSubSample_;\n> >\n> > These information should be decoupled from the compressor class, as\n> > they belong to the format the is being compressed, not to the\n> > compressor instance. What do you think ? Are compressors throw away\n> > objects created to run once on a format and thrown away ?\n>\n> This is just a caching of the configuration that was selected. When it\n> can come from the PixelFormatInfo, I expect it will store just a\n> reference to that instead.\n>\n\nMy question was more like: do you create one compressor instance per\nconversion ? Otherwise I don't see how format information, regardless\nwhere they come from or are stored, can be class members.\n\nAs I thought it, the compressor class should be instantiated once, and\nprovides methods to schedule possibly concurrent conversions, in a\nthread as you noted down in the cover letter, with a signal/slot\nmechanism to notify the caller when the conversion is done. In this\ncase format information are -per conversion run- not per instance.\n\n>\n> >> +};\n> >> +\n> >> +#endif /* __COMPRESSOR_JPEG_H__ */\n> >> diff --git a/src/android/meson.build b/src/android/meson.build\n> >> index 822cad621f01..51dcd99ee62f 100644\n> >> --- a/src/android/meson.build\n> >> +++ b/src/android/meson.build\n> >> @@ -6,6 +6,7 @@ android_hal_sources = files([\n> >> 'camera_device.cpp',\n> >> 'camera_metadata.cpp',\n> >> 'camera_ops.cpp',\n> >> + 'jpeg/compressor_jpeg.cpp',\n> >> ])\n> >>\n> >> android_camera_metadata_sources = files([\n> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >> index 3aad4386ffc2..d78e2c1f6eb8 100644\n> >> --- a/src/libcamera/meson.build\n> >> +++ b/src/libcamera/meson.build\n> >> @@ -124,6 +124,8 @@ if get_option('android')\n> >> libcamera_sources += android_hal_sources\n> >> includes += android_includes\n> >> libcamera_link_with += android_camera_metadata\n> >> +\n> >> + libcamera_deps += dependency('libjpeg')\n> >\n> > Shouldn't this be a dependency introduced by compiling the android HAL\n> > in ?\n> >\n>\n> it is ... it's under the if get_option('android') section.\n>\n\nOh sorry, missed that.\n\n\n>\n> > Thanks\n> > j\n> >\n> >> endif\n> >>\n> >> # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.\n> >> --\n> >> 2.25.1\n> >>\n> >> _______________________________________________\n> >> libcamera-devel mailing list\n> >> libcamera-devel@lists.libcamera.org\n> >> https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards\n> --\n> Kieran","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 639A0BDB1B\n\tfor ;\n\tWed, 22 Jul 2020 10:28:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6ACD6093D;\n\tWed, 22 Jul 2020 12:28:22 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9057E6039F\n\tfor ;\n\tWed, 22 Jul 2020 12:28:21 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id DAFAF40009;\n\tWed, 22 Jul 2020 10:28:20 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 22 Jul 2020 12:31:59 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20200722103159.lf4t6sb3hburnm6s@uno.localdomain>","References":"<20200721220126.202065-1-kieran.bingham@ideasonboard.com>\n\t<20200721220126.202065-2-kieran.bingham@ideasonboard.com>\n\t<20200722085034.k7whf3riwj4sg33i@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 1/6] android: Introduce JPEG\n\tcompression","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11480,"web_url":"https://patchwork.libcamera.org/comment/11480/","msgid":"<7503c1df-855d-3e82-40da-6d13a5a42e3d@ideasonboard.com>","date":"2020-07-22T11:04:42","subject":"Re: [libcamera-devel] [RFC PATCH 1/6] android: Introduce JPEG\n\tcompression","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 22/07/2020 11:31, Jacopo Mondi wrote:\n> On Wed, Jul 22, 2020 at 11:17:50AM +0100, Kieran Bingham wrote:\n>> Hi Jacopo,\n>>\n> \n> snip\n> \n>>>> +\tunsigned int stride_;\n>>>> +\n>>>> +\tbool nv_;\n>>>> +\tbool nvSwap_;\n>>>> +\tunsigned int horzSubSample_;\n>>>> +\tunsigned int vertSubSample_;\n>>>\n>>> These information should be decoupled from the compressor class, as\n>>> they belong to the format the is being compressed, not to the\n>>> compressor instance. What do you think ? Are compressors throw away\n>>> objects created to run once on a format and thrown away ?\n>>\n>> This is just a caching of the configuration that was selected. When it\n>> can come from the PixelFormatInfo, I expect it will store just a\n>> reference to that instead.\n>>\n> \n> My question was more like: do you create one compressor instance per\n> conversion ? Otherwise I don't see how format information, regardless\n> where they come from or are stored, can be class members.\n\nThe lifetime of a compression object will support multiple consecutive\nframes.\n\nAll frames must use the same configuration, and (for jpeg), the class\nwill maintain the libjpeg compression object and allocations (and\nconfiguration), and each call to compress will then simply take one\ninput frame, and produce one output buffer based on all the\npreconfigured parameters.\n\nThere's nothing preventing someone from constructing a compression\nobject each time of course, except then there need to be allocations,\nand configuration for each frame which would be redundant.\n\n(by that I mean libjpeg makes internal allocations, so keeping the\nlifetime of the Compressor object allows reuse of those internal\nallocations for the duration of a whole stream).\n\n\n> As I thought it, the compressor class should be instantiated once, and\n> provides methods to schedule possibly concurrent conversions, in a\n> thread as you noted down in the cover letter, with a signal/slot\n\nlibjpeg can only perform a single compression at a time, but once the\nobject has a thread, it can indeed handle these as a queue and become\nevent driven.\n\nBut by 'concurrent' a single libjpeg object could only compress a single\nframe at a time. If it runs in it's own thread, then that would then\nprevent blocking of the calling thread if that's what you mean.\n\n\n\n> mechanism to notify the caller when the conversion is done. In this\n> case format information are -per conversion run- not per instance.\n\nNo, if you want a different set of configuration options, just create\nanother instance of the compressor...\n\nI haven't yet looked at threading, but indeed that could be handled by\neach Compressor creating a thread, which then signals completion events.\n\nThat would then form part of the design of the Compressor object base class.\n\n\n\n>>>> +};\n>>>> +\n>>>> +#endif /* __COMPRESSOR_JPEG_H__ */\n>>>> diff --git a/src/android/meson.build b/src/android/meson.build\n>>>> index 822cad621f01..51dcd99ee62f 100644\n>>>> --- a/src/android/meson.build\n>>>> +++ b/src/android/meson.build\n>>>> @@ -6,6 +6,7 @@ android_hal_sources = files([\n>>>> 'camera_device.cpp',\n>>>> 'camera_metadata.cpp',\n>>>> 'camera_ops.cpp',\n>>>> + 'jpeg/compressor_jpeg.cpp',\n>>>> ])\n>>>>\n>>>> android_camera_metadata_sources = files([\n>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>>> index 3aad4386ffc2..d78e2c1f6eb8 100644\n>>>> --- a/src/libcamera/meson.build\n>>>> +++ b/src/libcamera/meson.build\n>>>> @@ -124,6 +124,8 @@ if get_option('android')\n>>>> libcamera_sources += android_hal_sources\n>>>> includes += android_includes\n>>>> libcamera_link_with += android_camera_metadata\n>>>> +\n>>>> + libcamera_deps += dependency('libjpeg')\n>>>\n>>> Shouldn't this be a dependency introduced by compiling the android HAL\n>>> in ?\n>>>\n>>\n>> it is ... it's under the if get_option('android') section.\n>>\n> \n> Oh sorry, missed that.\n\n:-)\n\n>>> Thanks\n>>> j\n>>>\n>>>> endif\n>>>>\n>>>> # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.\n>>>> --\n>>>> 2.25.1\n>>>>\n>>>> _______________________________________________\n>>>> libcamera-devel mailing list\n>>>> libcamera-devel@lists.libcamera.org\n>>>> https://lists.libcamera.org/listinfo/libcamera-devel\n>>\n>> --\n>> Regards\n>> --\n>> Kieran","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 2C9C3BDB1B\n\tfor ;\n\tWed, 22 Jul 2020 11:04:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A96F6093D;\n\tWed, 22 Jul 2020 13:04:46 +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 4DAD46039F\n\tfor ;\n\tWed, 22 Jul 2020 13:04:45 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A8B42329;\n\tWed, 22 Jul 2020 13:04:44 +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=\"BqFdJXBB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595415884;\n\tbh=3mf0aKA8SLiPgg8pR9RYW3KRGWrM4x2jVv+R1rglY8o=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=BqFdJXBBWplJgU+Ay2rRydGQ0suMPQsX8mtBkE4tIDYrsXeeCOmtfPRaqDH0wW4qX\n\tNFprI82P0lI0NhyEOYW45mm3iTXPGeGR2BIUaALZgYk9HGNyaaWCdM1Ud62ShFwHQp\n\t588UR8LKv+sTvU2NKgZmGKRzX67JifArsqERLtKY=","To":"Jacopo Mondi ","References":"<20200721220126.202065-1-kieran.bingham@ideasonboard.com>\n\t<20200721220126.202065-2-kieran.bingham@ideasonboard.com>\n\t<20200722085034.k7whf3riwj4sg33i@uno.localdomain>\n\t\n\t<20200722103159.lf4t6sb3hburnm6s@uno.localdomain>","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":"<7503c1df-855d-3e82-40da-6d13a5a42e3d@ideasonboard.com>","Date":"Wed, 22 Jul 2020 12:04:42 +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":"<20200722103159.lf4t6sb3hburnm6s@uno.localdomain>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH 1/6] android: Introduce JPEG\n\tcompression","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11522,"web_url":"https://patchwork.libcamera.org/comment/11522/","msgid":"<70b3b869-964f-d1b5-69da-a7e30ade50f9@ideasonboard.com>","date":"2020-07-23T13:08:45","subject":"Re: [libcamera-devel] [RFC PATCH 1/6] android: Introduce JPEG\n\tcompression","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 22/07/2020 09:50, Jacopo Mondi wrote:\n> Hi Kieran,\n> just skimming through.. some nits and questions below...\n\nThank you, this was useful.\n\nOne of the main topics of this series was more about patch 4/6. Could I\nask if you could skim that one too please?\n\nIt's still a rough sketch there, but your initial thoughts there in\nparticular are useful I think.\n\n\n> On Tue, Jul 21, 2020 at 11:01:21PM +0100, Kieran Bingham wrote:\n>> Provide a compressor interface and implement a JPEG compressor using libjpeg.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/android/jpeg/compressor.h | 28 +++\n>> src/android/jpeg/compressor_jpeg.cpp | 279 +++++++++++++++++++++++++++\n>> src/android/jpeg/compressor_jpeg.h | 44 +++++\n>> src/android/meson.build | 1 +\n>> src/libcamera/meson.build | 2 +\n>> 5 files changed, 354 insertions(+)\n>> create mode 100644 src/android/jpeg/compressor.h\n>> create mode 100644 src/android/jpeg/compressor_jpeg.cpp\n>> create mode 100644 src/android/jpeg/compressor_jpeg.h\n>>\n>> diff --git a/src/android/jpeg/compressor.h b/src/android/jpeg/compressor.h\n>> new file mode 100644\n>> index 000000000000..f95e4a4539cb\n>> --- /dev/null\n>> +++ b/src/android/jpeg/compressor.h\n>> @@ -0,0 +1,28 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * compressor.h - Image compression interface\n>> + */\n> \n> Don't we usually have an empty line here ?\n\n\nNot in headers it seems.\n\n> \n>> +#ifndef __LIBCAMERA_COMPRESSOR_H__\n>> +#define __LIBCAMERA_COMPRESSOR_H__\n> \n> Maybe __LIBCAMERA_ANDROID_COMPRESSOR__ ?\n> \n>> +\n>> +#include \n>> +#include \n>> +\n>> +struct CompressedImage {\n>> +\tunsigned char *data;\n>> +\tunsigned long length;\n>> +};\n>> +\n>> +class Compressor\n>> +{\n>> +public:\n>> +\tvirtual ~Compressor() { };\n>> +\n>> +\tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n>> +\tvirtual int compress(const libcamera::FrameBuffer *source, CompressedImage *image) = 0;\n>> +\tvirtual void free(CompressedImage *image) = 0;\n>> +};\n> \n> Do we expect more compressors ? Why a pure virtual class to define an\n> interface ?\n> \n>> +\n>> +#endif /* __LIBCAMERA_COMPRESSOR_H__ */\n>> diff --git a/src/android/jpeg/compressor_jpeg.cpp b/src/android/jpeg/compressor_jpeg.cpp\n>> new file mode 100644\n>> index 000000000000..78fa5e399d99\n>> --- /dev/null\n>> +++ b/src/android/jpeg/compressor_jpeg.cpp\n>> @@ -0,0 +1,279 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * compressor_jpeg.cpp - JPEG compression using libjpeg native API\n> \n> s/native API//\n> Or is there a value in saying that ?\n> \n>> + */\n>> +\n>> +#include \"compressor_jpeg.h\"\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \"libcamera/internal/log.h\"\n>> +\n>> +using namespace libcamera;\n>> +\n>> +LOG_DEFINE_CATEGORY(JPEG)\n>> +\n>> +struct PixelFormatPlaneInfo {\n>> +\tunsigned int bitsPerPixel;\n>> +\tunsigned int hSubSampling;\n>> +\tunsigned int vSubSampling;\n>> +};\n>> +\n>> +struct PixelFormatInfo {\n>> +\tJ_COLOR_SPACE colorSpace;\n>> +\tunsigned int numPlanes;\n>> +\tbool nvSwap;\n>> +\tPixelFormatPlaneInfo planes[3];\n>> +};\n> \n> Is there a reason why we can't use the libcamera PixelFormatInfo and\n> maybe keep an association with the J_COLOR_SPACE field ? The android\n> layer can use internal headers, right ?\n\nI've now moved this to use the internal PixelFormatInfo.\n\nIt was missing the ability to get the numPlanes, nvSwap, and\nhorizontalSubSampling, but I've calculated both hSubSamp, and numPlanes.\n\nThe nvSwap I've added to the JPEGPixelFormatInfo local map.\n\n\n>> +\n>> +namespace {\n>> +\n>> +static const std::map pixelInfo{\n>> +\t{ formats::R8, { JCS_GRAYSCALE, 1, false, { { 8, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\n>> +\t/* RGB formats. */\n>> +\t{ formats::RGB888, { JCS_EXT_BGR, 1, false, { { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\t{ formats::BGR888, { JCS_EXT_RGB, 1, false, { { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\n>> +\t/* YUV packed formats. */\n>> +\t{ formats::UYVY, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\t{ formats::VYUY, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\t{ formats::YUYV, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\t{ formats::YVYU, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },\n>> +\n>> +\t/* YUY planar formats. */\n>> +\t{ formats::NV12, { JCS_YCbCr, 2, false, { { 8, 1, 1 }, { 16, 2, 2 }, { 0, 0, 0 } } } },\n>> +\t{ formats::NV21, { JCS_YCbCr, 2, true, { { 8, 1, 1 }, { 16, 2, 2 }, { 0, 0, 0 } } } },\n>> +\t{ formats::NV16, { JCS_YCbCr, 2, false, { { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } } } },\n>> +\t{ formats::NV61, { JCS_YCbCr, 2, true, { { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } } } },\n>> +\t{ formats::NV24, { JCS_YCbCr, 2, false, { { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } } } },\n>> +\t{ formats::NV42, { JCS_YCbCr, 2, true, { { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } } } },\n>> +};\n> \n> Could this become\n> \n> struct JPEGPixelFormatInfo\n> {\n> J_COLOR_SPACE colorSpace;\n> PixelFormatInfo pixelFormatInfo;\n> };\n> \n> std::map pixelInfo{\n> \t{ formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888 },\n> ..\n> };\n\nYes ;-) (done).\n\n\n>> +\n>> +}\n>> +\n>> +const struct PixelFormatInfo &findPixelInfo(const PixelFormat &format)\n>> +{\n>> +\tstatic const struct PixelFormatInfo invalidPixelFormat {\n>> +\t\t\tJCS_UNKNOWN, 0, false, {}\n>> +\t};\n>> +\n>> +\tconst auto iter = pixelInfo.find(format);\n>> +\tif (iter == pixelInfo.end()) {\n>> +\t\tLOG(JPEG, Error) << \"Unsupported pixel format for JPEG compressor: \"\n>> +\t\t\t \t << format.toString();\n>> +\t\treturn invalidPixelFormat;\n>> +\t}\n>> +\n>> +\treturn iter->second;\n>> +}\n>> +\n>> +CompressorJPEG::CompressorJPEG()\n>> +\t: quality_(95)\n>> +{\n>> +\t/* \\todo: Expand error handling coverage. */\n>> +\tcompress_.err = jpeg_std_error(&jerr_);\n>> +\n>> +\tjpeg_create_compress(&compress_);\n>> +}\n>> +\n>> +CompressorJPEG::~CompressorJPEG()\n>> +{\n>> +\tjpeg_destroy_compress(&compress_);\n>> +}\n>> +\n>> +int CompressorJPEG::configure(const StreamConfiguration &cfg)\n>> +{\n>> +\t{\n>> +\t\tLOG(JPEG, Warning) << \"Configuring pixelformat as : \"\n>> +\t\t\t\t\t<< cfg.pixelFormat.toString();\n>> +\t\tLOG(JPEG, Warning) << \" : \" << cfg.toString();\n>> +\n>> +\t\tstd::vector formats = cfg.formats().pixelformats();\n>> +\t\tLOG(JPEG, Warning) << \"StreamConfiguration supports \" << formats.size() << \" formats:\";\n>> +\t\tfor (const PixelFormat &format : formats)\n>> +\t\t\tLOG(JPEG, Warning) << \" - \" << format.toString();\n>> +\t}\n>> +\n>> +\tconst struct PixelFormatInfo info = findPixelInfo(cfg.pixelFormat);\n>> +\tif (info.colorSpace == JCS_UNKNOWN)\n>> +\t\treturn -ENOTSUP;\n>> +\n>> +\t/*\n>> +\t * Todo: The stride given by the stream configuration has caused issues.\n>> +\t * Validate it and also handle per-plane strides.\n> \n> On which platform ? IPU3 ? Is there an error on how the stride is\n> reported ?\n\n\nSo, I can only assume now that it's all resolved. Paul's PixelFormatInfo\nreworks are giving me the right value everywhere I've looked so far, and\nnow I've swapped to using that entirely anyway, so I no longer take the\nstride from the cfg.stride.\n\nThe cfg.stride doesn't help me for multiplanar (NV12) anyway.\n\n\n>> +\t */\n>> +\tstride_ = cfg.stride;\n>> +\tstride_ = cfg.size.width * info.planes[0].bitsPerPixel / 8;\n>> +\t/* Saw some errors with strides, so this is a debug/develop check */\n>> +\tif (cfg.stride != stride_)\n>> +\t\tLOG(JPEG, Error) << \"*** StreamConfigure provided stride of \"\n>> +\t\t\t\t << cfg.stride << \" rather than \" << stride_;\n>> +\n>> +\tcompress_.image_width = cfg.size.width;\n>> +\tcompress_.image_height = cfg.size.height;\n>> +\tcompress_.in_color_space = info.colorSpace;\n>> +\n>> +\tcompress_.input_components = info.colorSpace == JCS_GRAYSCALE ? 1 : 3;\n>> +\n>> +\tjpeg_set_defaults(&compress_);\n>> +\tjpeg_set_quality(&compress_, quality_, TRUE);\n>> +\n>> +\tnv_ = info.numPlanes == 2;\n>> +\tnvSwap_ = info.nvSwap;\n>> +\n>> +\t/* Use the 'last' plane for subsampling of component info. */\n>> +\tunsigned int p = info.numPlanes - 1;\n>> +\thorzSubSample_ = info.planes[p].hSubSampling;\n>> +\tvertSubSample_ = info.planes[p].vSubSampling;\n>> +\n> \n> You could infer the sub-sampling from libcamera PixelFormatInfo I\n> guess\n\n\nvSub is provided, and I have had to calculate the hSub.\n\nunsigned int horzSubSample = 2 * compress_.image_width / c_stride;\nunsigned int vertSubSample = info->planes[1].verticalSubSampling;\n\n\nWe 'could' make a horizontalSubSampling function but then we'd have:\nunsigned int horzSubSample = info->planes[1].horizontalSubSampling();\nunsigned int vertSubSample = info->planes[1].verticalSubSampling;\n\n\n(note the brackets) which would be a bit annoying. Maybe vSub would also\nhave to become a 'getter' function too.\n\n\n\n\n>> +\treturn 0;\n>> +}\n>> +\n>> +void CompressorJPEG::compressRGB(const libcamera::MappedFrameBuffer *frame)\n>> +{\n>> +\tunsigned char *src = static_cast(frame->maps()[0].address);\n>> +\n>> +\tJSAMPROW row_pointer[1];\n>> +\n>> +\twhile (compress_.next_scanline < compress_.image_height) {\n>> +\t\trow_pointer[0] = &src[compress_.next_scanline * stride_];\n>> +\t\tjpeg_write_scanlines(&compress_, row_pointer, 1);\n>> +\t}\n>> +}\n>> +\n>> +/*\n>> + * A very dull implementation to compress YUYV.\n>> + * To be converted to a generic algorithm akin to NV12.\n>> + * If it can be shared with NV12 great, but we might be able to further\n>> + * optimisze the NV layouts by only depacking the CrCb pixels.\n>> + */\n>> +void CompressorJPEG::compressYUV(const libcamera::MappedFrameBuffer *frame)\n>> +{\n>> +\tstd::vector tmprowbuf(compress_.image_width * 3);\n>> +\tunsigned char *input = static_cast(frame->maps()[0].address);\n>> +\n>> +\tJSAMPROW row_pointer[1];\n>> +\trow_pointer[0] = &tmprowbuf[0];\n>> +\twhile (compress_.next_scanline < compress_.image_height) {\n>> +\t\tunsigned i, j;\n>> +\t\tunsigned offset = compress_.next_scanline * compress_.image_width * 2; //offset to the correct row\n>> +\t\tfor (i = 0, j = 0; i < compress_.image_width * 2; i += 4, j += 6) { //input strides by 4 bytes, output strides by 6 (2 pixels)\n>> +\t\t\ttmprowbuf[j + 0] = input[offset + i + 0]; // Y (unique to this pixel)\n>> +\t\t\ttmprowbuf[j + 1] = input[offset + i + 1]; // U (shared between pixels)\n>> +\t\t\ttmprowbuf[j + 2] = input[offset + i + 3]; // V (shared between pixels)\n>> +\t\t\ttmprowbuf[j + 3] = input[offset + i + 2]; // Y (unique to this pixel)\n>> +\t\t\ttmprowbuf[j + 4] = input[offset + i + 1]; // U (shared between pixels)\n>> +\t\t\ttmprowbuf[j + 5] = input[offset + i + 3]; // V (shared between pixels)\n>> +\t\t}\n>> +\t\tjpeg_write_scanlines(&compress_, row_pointer, 1);\n>> +\t}\n>> +}\n>> +\n>> +/*\n>> + * Really inefficient NV unpacking to YUV888 for JPEG compress.\n>> + * This /could/ be improved (drastically I hope) ;-)\n>> + */\n>> +void CompressorJPEG::compressNV(const libcamera::MappedFrameBuffer *frame)\n>> +{\n>> +\tstd::vector tmprowbuf(compress_.image_width * 3);\n>> +\n>> +\t/*\n>> +\t * Todo: Use the raw api, and only unpack the cb/cr samples to new line buffers.\n> \n> If you want this and other todo items recorded: \\todo\n> \n>> +\t * If possible, see if we can set appropriate pixel strides too to save even that copy.\n>> +\t *\n>> +\t * Possible hints at:\n>> +\t * https://sourceforge.net/p/libjpeg/mailman/message/30815123/\n>> +\t */\n>> +\tunsigned int c_stride = compress_.image_width * (2 / horzSubSample_);\n>> +\tunsigned int c_inc = horzSubSample_ == 1 ? 2 : 0;\n>> +\tunsigned int cb_pos = nvSwap_ ? 1 : 0;\n>> +\tunsigned int cr_pos = nvSwap_ ? 0 : 1;\n>> +\n>> +\tconst unsigned char *src = static_cast(frame->maps()[0].address);\n>> +\tconst unsigned char *src_c = src + compress_.image_width * compress_.image_height; // * stride[0] surely\n>> +\n>> +\tJSAMPROW row_pointer[1];\n>> +\trow_pointer[0] = &tmprowbuf[0];\n>> +\n>> +\tfor (unsigned int y = 0; y < compress_.image_height; y++) {\n>> +\t\tunsigned char *dst = &tmprowbuf[0];\n>> +\n>> +\t\tconst unsigned char *src_y = src + y * compress_.image_width;\n>> +\t\tconst unsigned char *src_cb = src_c + (y / vertSubSample_) *\n>> +\t\t\t\t\t c_stride + cb_pos;\n>> +\t\tconst unsigned char *src_cr = src_c + (y / vertSubSample_) *\n>> +\t\t\t\t\t c_stride + cr_pos;\n>> +\n>> +\t\tfor (unsigned int x = 0; x < compress_.image_width; x += 2) {\n>> +\t\t\tdst[0] = *src_y;\n>> +\t\t\tdst[1] = *src_cb;\n>> +\t\t\tdst[2] = *src_cr;\n>> +\t\t\tsrc_y++;\n>> +\t\t\tsrc_cb += c_inc;\n>> +\t\t\tsrc_cr += c_inc;\n>> +\t\t\tdst += 3;\n>> +\n>> +\t\t\tdst[0] = *src_y;\n>> +\t\t\tdst[1] = *src_cb;\n>> +\t\t\tdst[2] = *src_cr;\n>> +\t\t\tsrc_y++;\n>> +\t\t\tsrc_cb += 2;\n>> +\t\t\tsrc_cr += 2;\n>> +\t\t\tdst += 3;\n>> +\t\t}\n>> +\n>> +\t\tjpeg_write_scanlines(&compress_, row_pointer, 1);\n>> +\t}\n>> +}\n>> +\n>> +int CompressorJPEG::compress(const FrameBuffer *source, CompressedImage *jpeg)\n>> +{\n>> +\tMappedFrameBuffer frame(source, PROT_READ);\n>> +\tif (!frame.isValid()) {\n>> +\t\tLOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n>> +\t\t\t\t << strerror(frame.error());\n>> +\t\treturn -frame.error();\n>> +\t}\n>> +\n>> +\tjpeg_mem_dest(&compress_, &jpeg->data, &jpeg->length);\n>> +\n>> +\tjpeg_start_compress(&compress_, TRUE);\n>> +\n>> +\tLOG(JPEG, Debug) << \"JPEG Compress Starting\";\n>> +\tLOG(JPEG, Debug) << \"Width: \" << compress_.image_width\n>> +\t\t\t << \" height: \" << compress_.image_height\n>> +\t\t\t << \" stride: \" << stride_;\n>> +\n>> +\tif (nv_)\n>> +\t\tcompressNV(&frame);\n>> +\telse if (compress_.in_color_space == JCS_YCbCr)\n>> +\t\tcompressYUV(&frame);\n>> +\telse\n>> +\t\tcompressRGB(&frame);\n> \n> If you have your map of PixelFormat to a custom class you can there\n> store a pointer to the conversion function associated to each format and\n> avoid the switch here\n\nThis turned out to be harder than I expected so I'm skipping it for now ;-(\n\n\n>> +\n>> +\tLOG(JPEG, Debug) << \"JPEG Compress Completed\";\n>> +\n>> +\tjpeg_finish_compress(&compress_);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +void CompressorJPEG::free(CompressedImage *jpeg)\n>> +{\n>> +\t::free(jpeg->data);\n>> +\tjpeg->data = nullptr;\n>> +}\n>> diff --git a/src/android/jpeg/compressor_jpeg.h b/src/android/jpeg/compressor_jpeg.h\n>> new file mode 100644\n>> index 000000000000..11009481a2fe\n>> --- /dev/null\n>> +++ b/src/android/jpeg/compressor_jpeg.h\n>> @@ -0,0 +1,44 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * compressor_jpeg.h - JPEG compression using libjpeg\n>> + */\n>> +#ifndef __COMPRESSOR_JPEG_H__\n>> +#define __COMPRESSOR_JPEG_H__\n> \n> Same as above, I think ANDROID_ should be part of the inclusion guard\n> \n>> +\n>> +#include \"compressor.h\"\n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +class CompressorJPEG : public Compressor\n>> +{\n>> +public:\n>> +\tCompressorJPEG();\n>> +\t~CompressorJPEG();\n>> +\n>> +\tint configure(const libcamera::StreamConfiguration &cfg);\n>> +\tint compress(const libcamera::FrameBuffer *source, CompressedImage *jpeg);\n>> +\tvoid free(CompressedImage *jpeg);\n>> +\n>> +private:\n>> +\tvoid compressRGB(const libcamera::MappedFrameBuffer *frame);\n>> +\tvoid compressYUV(const libcamera::MappedFrameBuffer *frame);\n>> +\tvoid compressNV(const libcamera::MappedFrameBuffer *frame);\n>> +\n>> +\tstruct jpeg_compress_struct compress_;\n>> +\tstruct jpeg_error_mgr jerr_;\n>> +\n>> +\tunsigned int quality_;\n>> +\tunsigned int stride_;\n>> +\n>> +\tbool nv_;\n>> +\tbool nvSwap_;\n>> +\tunsigned int horzSubSample_;\n>> +\tunsigned int vertSubSample_;\n> \n> These information should be decoupled from the compressor class, as\n> they belong to the format the is being compressed, not to the\n> compressor instance. What do you think ? Are compressors throw away\n> objects created to run once on a format and thrown away ?\n\nIt's now obtained from the PixelFormatInfo.\n\n\n> \n>> +};\n>> +\n>> +#endif /* __COMPRESSOR_JPEG_H__ */\n>> diff --git a/src/android/meson.build b/src/android/meson.build\n>> index 822cad621f01..51dcd99ee62f 100644\n>> --- a/src/android/meson.build\n>> +++ b/src/android/meson.build\n>> @@ -6,6 +6,7 @@ android_hal_sources = files([\n>> 'camera_device.cpp',\n>> 'camera_metadata.cpp',\n>> 'camera_ops.cpp',\n>> + 'jpeg/compressor_jpeg.cpp',\n>> ])\n>>\n>> android_camera_metadata_sources = files([\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index 3aad4386ffc2..d78e2c1f6eb8 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -124,6 +124,8 @@ if get_option('android')\n>> libcamera_sources += android_hal_sources\n>> includes += android_includes\n>> libcamera_link_with += android_camera_metadata\n>> +\n>> + libcamera_deps += dependency('libjpeg')\n> \n> Shouldn't this be a dependency introduced by compiling the android HAL\n> in ?\n> \n> Thanks\n> j\n> \n>> endif\n>>\n>> # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.\n>> --\n>> 2.25.1\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 EB518BD86F\n\tfor ;\n\tThu, 23 Jul 2020 13:08:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 717B061136;\n\tThu, 23 Jul 2020 15:08:51 +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 7973760939\n\tfor ;\n\tThu, 23 Jul 2020 15:08:49 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CC4C2279;\n\tThu, 23 Jul 2020 15:08:48 +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=\"uCS10H80\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595509729;\n\tbh=UQ2w44LvfrvhKvoIymzhJVom8ncdt2s3O/DizwGKEd4=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=uCS10H809hcf96Zk+3/ICdyceUfTIEwlOtxqpJJNpHr224i0IEVtv6d6t4+UTKGwm\n\tQ0nJWjx8lStJNUniZAUJcFRoE6mAblK49MSKt4bPrHrqV32tkxfzadlZeYAZTUHX01\n\t7+9YqT8ivQjLU5GjK1TBZ18uaiaUFzuG5jdYIdgo=","To":"Jacopo Mondi ","References":"<20200721220126.202065-1-kieran.bingham@ideasonboard.com>\n\t<20200721220126.202065-2-kieran.bingham@ideasonboard.com>\n\t<20200722085034.k7whf3riwj4sg33i@uno.localdomain>","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":"<70b3b869-964f-d1b5-69da-a7e30ade50f9@ideasonboard.com>","Date":"Thu, 23 Jul 2020 14:08:45 +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":"<20200722085034.k7whf3riwj4sg33i@uno.localdomain>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH 1/6] android: Introduce JPEG\n\tcompression","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]