[libcamera-devel] [RFC PATCH 1/6] android: Introduce JPEG compression

[Kieran Bingham]

Hi Jacopo,

On 22/07/2020 09:50, Jacopo Mondi wrote:
> Hi Kieran,
>    just skimming through.. some nits and questions below...

Thank you, this was useful.

One of the main topics of this series was more about patch 4/6. Could I
ask if you could skim that one too please?

It's still a rough sketch there, but your initial thoughts there in
particular are useful I think.


> On Tue, Jul 21, 2020 at 11:01:21PM +0100, Kieran Bingham wrote:
>> Provide a compressor interface and implement a JPEG compressor using libjpeg.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/android/jpeg/compressor.h        |  28 +++
>>  src/android/jpeg/compressor_jpeg.cpp | 279 +++++++++++++++++++++++++++
>>  src/android/jpeg/compressor_jpeg.h   |  44 +++++
>>  src/android/meson.build              |   1 +
>>  src/libcamera/meson.build            |   2 +
>>  5 files changed, 354 insertions(+)
>>  create mode 100644 src/android/jpeg/compressor.h
>>  create mode 100644 src/android/jpeg/compressor_jpeg.cpp
>>  create mode 100644 src/android/jpeg/compressor_jpeg.h
>>
>> diff --git a/src/android/jpeg/compressor.h b/src/android/jpeg/compressor.h
>> new file mode 100644
>> index 000000000000..f95e4a4539cb
>> --- /dev/null
>> +++ b/src/android/jpeg/compressor.h
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * compressor.h - Image compression interface
>> + */
> 
> Don't we usually have an empty line here ?


Not in headers it seems.

> 
>> +#ifndef __LIBCAMERA_COMPRESSOR_H__
>> +#define __LIBCAMERA_COMPRESSOR_H__
> 
> Maybe __LIBCAMERA_ANDROID_COMPRESSOR__ ?
> 
>> +
>> +#include <libcamera/buffer.h>
>> +#include <libcamera/stream.h>
>> +
>> +struct CompressedImage {
>> +	unsigned char *data;
>> +	unsigned long length;
>> +};
>> +
>> +class Compressor
>> +{
>> +public:
>> +	virtual ~Compressor() { };
>> +
>> +	virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
>> +	virtual int compress(const libcamera::FrameBuffer *source, CompressedImage *image) = 0;
>> +	virtual void free(CompressedImage *image) = 0;
>> +};
> 
> Do we expect more compressors ? Why a pure virtual class to define an
> interface ?
> 
>> +
>> +#endif /* __LIBCAMERA_COMPRESSOR_H__ */
>> diff --git a/src/android/jpeg/compressor_jpeg.cpp b/src/android/jpeg/compressor_jpeg.cpp
>> new file mode 100644
>> index 000000000000..78fa5e399d99
>> --- /dev/null
>> +++ b/src/android/jpeg/compressor_jpeg.cpp
>> @@ -0,0 +1,279 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * compressor_jpeg.cpp - JPEG compression using libjpeg native API
> 
> s/native API//
> Or is there a value in saying that ?
> 
>> + */
>> +
>> +#include "compressor_jpeg.h"
>> +
>> +#include <fcntl.h>
>> +#include <iomanip>
>> +#include <iostream>
>> +#include <sstream>
>> +#include <string.h>
>> +#include <sys/mman.h>
>> +#include <unistd.h>
>> +#include <vector>
>> +
>> +#include <libcamera/camera.h>
>> +#include <libcamera/formats.h>
>> +#include <libcamera/pixel_format.h>
>> +
>> +#include "libcamera/internal/log.h"
>> +
>> +using namespace libcamera;
>> +
>> +LOG_DEFINE_CATEGORY(JPEG)
>> +
>> +struct PixelFormatPlaneInfo {
>> +	unsigned int bitsPerPixel;
>> +	unsigned int hSubSampling;
>> +	unsigned int vSubSampling;
>> +};
>> +
>> +struct PixelFormatInfo {
>> +	J_COLOR_SPACE colorSpace;
>> +	unsigned int numPlanes;
>> +	bool nvSwap;
>> +	PixelFormatPlaneInfo planes[3];
>> +};
> 
> Is there a reason why we can't use the libcamera PixelFormatInfo and
> maybe keep an association with the J_COLOR_SPACE field ? The android
> layer can use internal headers, right ?

I've now moved this to use the internal PixelFormatInfo.

It was missing the ability to get the numPlanes, nvSwap, and
horizontalSubSampling, but I've calculated both hSubSamp, and numPlanes.

The nvSwap I've added to the JPEGPixelFormatInfo local map.


>> +
>> +namespace {
>> +
>> +static const std::map<PixelFormat, struct PixelFormatInfo> pixelInfo{
>> +	{ formats::R8, { JCS_GRAYSCALE, 1, false, { { 8, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },
>> +
>> +	/* RGB formats. */
>> +	{ formats::RGB888, { JCS_EXT_BGR, 1, false, { { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },
>> +	{ formats::BGR888, { JCS_EXT_RGB, 1, false, { { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },
>> +
>> +	/* YUV packed formats. */
>> +	{ formats::UYVY, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },
>> +	{ formats::VYUY, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },
>> +	{ formats::YUYV, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },
>> +	{ formats::YVYU, { JCS_YCbCr, 1, false, { { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } } } },
>> +
>> +	/* YUY planar formats. */
>> +	{ formats::NV12, { JCS_YCbCr, 2, false, { { 8, 1, 1 }, { 16, 2, 2 }, { 0, 0, 0 } } } },
>> +	{ formats::NV21, { JCS_YCbCr, 2, true, { { 8, 1, 1 }, { 16, 2, 2 }, { 0, 0, 0 } } } },
>> +	{ formats::NV16, { JCS_YCbCr, 2, false, { { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } } } },
>> +	{ formats::NV61, { JCS_YCbCr, 2, true, { { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } } } },
>> +	{ formats::NV24, { JCS_YCbCr, 2, false, { { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } } } },
>> +	{ formats::NV42, { JCS_YCbCr, 2, true, { { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } } } },
>> +};
> 
> Could this become
> 
> struct JPEGPixelFormatInfo
> {
>         J_COLOR_SPACE colorSpace;
>         PixelFormatInfo pixelFormatInfo;
> };
> 
> std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{
> 	{ formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888 },
>         ..
> };

Yes ;-) (done).


>> +
>> +}
>> +
>> +const struct PixelFormatInfo &findPixelInfo(const PixelFormat &format)
>> +{
>> +	static const struct PixelFormatInfo invalidPixelFormat {
>> +			JCS_UNKNOWN, 0, false, {}
>> +	};
>> +
>> +	const auto iter = pixelInfo.find(format);
>> +	if (iter == pixelInfo.end()) {
>> +		LOG(JPEG, Error) << "Unsupported pixel format for JPEG compressor: "
>> +			  	 << format.toString();
>> +		return invalidPixelFormat;
>> +	}
>> +
>> +	return iter->second;
>> +}
>> +
>> +CompressorJPEG::CompressorJPEG()
>> +	: quality_(95)
>> +{
>> +	/* \todo: Expand error handling coverage. */
>> +	compress_.err = jpeg_std_error(&jerr_);
>> +
>> +	jpeg_create_compress(&compress_);
>> +}
>> +
>> +CompressorJPEG::~CompressorJPEG()
>> +{
>> +	jpeg_destroy_compress(&compress_);
>> +}
>> +
>> +int CompressorJPEG::configure(const StreamConfiguration &cfg)
>> +{
>> +	{
>> +		LOG(JPEG, Warning) << "Configuring pixelformat as : "
>> +					<< cfg.pixelFormat.toString();
>> +		LOG(JPEG, Warning) << "  : " << cfg.toString();
>> +
>> +		std::vector<PixelFormat> formats = cfg.formats().pixelformats();
>> +		LOG(JPEG, Warning) << "StreamConfiguration supports " << formats.size() << " formats:";
>> +		for (const PixelFormat &format : formats)
>> +			LOG(JPEG, Warning) << " - " << format.toString();
>> +	}
>> +
>> +	const struct PixelFormatInfo info = findPixelInfo(cfg.pixelFormat);
>> +	if (info.colorSpace == JCS_UNKNOWN)
>> +		return -ENOTSUP;
>> +
>> +	/*
>> +	 * Todo: The stride given by the stream configuration has caused issues.
>> +	 * Validate it and also handle per-plane strides.
> 
> On which platform ? IPU3 ? Is there an error on how the stride is
> reported ?


So, I can only assume now that it's all resolved. Paul's PixelFormatInfo
reworks are giving me the right value everywhere I've looked so far, and
now I've swapped to using that entirely anyway, so I no longer take the
stride from the cfg.stride.

The cfg.stride doesn't help me for multiplanar (NV12) anyway.


>> +	 */
>> +	stride_ = cfg.stride;
>> +	stride_ = cfg.size.width * info.planes[0].bitsPerPixel / 8;
>> +	/* Saw some errors with strides, so this is a debug/develop check */
>> +	if (cfg.stride != stride_)
>> +		LOG(JPEG, Error) << "*** StreamConfigure provided stride of "
>> +				 << cfg.stride << " rather than " << stride_;
>> +
>> +	compress_.image_width = cfg.size.width;
>> +	compress_.image_height = cfg.size.height;
>> +	compress_.in_color_space =  info.colorSpace;
>> +
>> +	compress_.input_components = info.colorSpace == JCS_GRAYSCALE ? 1 : 3;
>> +
>> +	jpeg_set_defaults(&compress_);
>> +	jpeg_set_quality(&compress_, quality_, TRUE);
>> +
>> +	nv_ = info.numPlanes == 2;
>> +	nvSwap_ = info.nvSwap;
>> +
>> +	/* Use the 'last' plane for subsampling of component info. */
>> +	unsigned int p = info.numPlanes - 1;
>> +	horzSubSample_ = info.planes[p].hSubSampling;
>> +	vertSubSample_ = info.planes[p].vSubSampling;
>> +
> 
> You could infer the sub-sampling from libcamera PixelFormatInfo I
> guess


vSub is provided, and I have had to calculate the hSub.

unsigned int horzSubSample = 2 * compress_.image_width / c_stride;
unsigned int vertSubSample = info->planes[1].verticalSubSampling;


We 'could' make a horizontalSubSampling function but then we'd have:
unsigned int horzSubSample = info->planes[1].horizontalSubSampling();
unsigned int vertSubSample = info->planes[1].verticalSubSampling;


(note the brackets) which would be a bit annoying. Maybe vSub would also
have to become a 'getter' function too.




>> +	return 0;
>> +}
>> +
>> +void CompressorJPEG::compressRGB(const libcamera::MappedFrameBuffer *frame)
>> +{
>> +	unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].address);
>> +
>> +	JSAMPROW row_pointer[1];
>> +
>> +	while (compress_.next_scanline < compress_.image_height) {
>> +		row_pointer[0] = &src[compress_.next_scanline * stride_];
>> +		jpeg_write_scanlines(&compress_, row_pointer, 1);
>> +	}
>> +}
>> +
>> +/*
>> + * A very dull implementation to compress YUYV.
>> + * To be converted to a generic algorithm akin to NV12.
>> + * If it can be shared with NV12 great, but we might be able to further
>> + * optimisze the NV layouts by only depacking the CrCb pixels.
>> + */
>> +void CompressorJPEG::compressYUV(const libcamera::MappedFrameBuffer *frame)
>> +{
>> +	std::vector<uint8_t> tmprowbuf(compress_.image_width * 3);
>> +	unsigned char *input = static_cast<unsigned char *>(frame->maps()[0].address);
>> +
>> +	JSAMPROW row_pointer[1];
>> +	row_pointer[0] = &tmprowbuf[0];
>> +	while (compress_.next_scanline < compress_.image_height) {
>> +		unsigned i, j;
>> +		unsigned offset = compress_.next_scanline * compress_.image_width * 2; //offset to the correct row
>> +		for (i = 0, j = 0; i < compress_.image_width * 2; i += 4, j += 6) { //input strides by 4 bytes, output strides by 6 (2 pixels)
>> +			tmprowbuf[j + 0] = input[offset + i + 0]; // Y (unique to this pixel)
>> +			tmprowbuf[j + 1] = input[offset + i + 1]; // U (shared between pixels)
>> +			tmprowbuf[j + 2] = input[offset + i + 3]; // V (shared between pixels)
>> +			tmprowbuf[j + 3] = input[offset + i + 2]; // Y (unique to this pixel)
>> +			tmprowbuf[j + 4] = input[offset + i + 1]; // U (shared between pixels)
>> +			tmprowbuf[j + 5] = input[offset + i + 3]; // V (shared between pixels)
>> +		}
>> +		jpeg_write_scanlines(&compress_, row_pointer, 1);
>> +	}
>> +}
>> +
>> +/*
>> + * Really inefficient NV unpacking to YUV888 for JPEG compress.
>> + * This /could/ be improved (drastically I hope) ;-)
>> + */
>> +void CompressorJPEG::compressNV(const libcamera::MappedFrameBuffer *frame)
>> +{
>> +	std::vector<uint8_t> tmprowbuf(compress_.image_width * 3);
>> +
>> +	/*
>> +	 * Todo: Use the raw api, and only unpack the cb/cr samples to new line buffers.
> 
> If you want this and other todo items recorded: \todo
> 
>> +	 * If possible, see if we can set appropriate pixel strides too to save even that copy.
>> +	 *
>> +	 * Possible hints at:
>> +	 * https://sourceforge.net/p/libjpeg/mailman/message/30815123/
>> +	 */
>> +	unsigned int c_stride = compress_.image_width * (2 / horzSubSample_);
>> +	unsigned int c_inc = horzSubSample_ == 1 ? 2 : 0;
>> +	unsigned int cb_pos = nvSwap_ ? 1 : 0;
>> +	unsigned int cr_pos = nvSwap_ ? 0 : 1;
>> +
>> +	const unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].address);
>> +	const unsigned char *src_c = src + compress_.image_width * compress_.image_height; // * stride[0] surely
>> +
>> +	JSAMPROW row_pointer[1];
>> +	row_pointer[0] = &tmprowbuf[0];
>> +
>> +	for (unsigned int y = 0; y < compress_.image_height; y++) {
>> +		unsigned char *dst = &tmprowbuf[0];
>> +
>> +		const unsigned char *src_y = src + y * compress_.image_width;
>> +		const unsigned char *src_cb = src_c + (y / vertSubSample_) *
>> +					      c_stride + cb_pos;
>> +		const unsigned char *src_cr = src_c + (y / vertSubSample_) *
>> +					      c_stride + cr_pos;
>> +
>> +		for (unsigned int x = 0; x < compress_.image_width; x += 2) {
>> +			dst[0] = *src_y;
>> +			dst[1] = *src_cb;
>> +			dst[2] = *src_cr;
>> +			src_y++;
>> +			src_cb += c_inc;
>> +			src_cr += c_inc;
>> +			dst += 3;
>> +
>> +			dst[0] = *src_y;
>> +			dst[1] = *src_cb;
>> +			dst[2] = *src_cr;
>> +			src_y++;
>> +			src_cb += 2;
>> +			src_cr += 2;
>> +			dst += 3;
>> +		}
>> +
>> +		jpeg_write_scanlines(&compress_, row_pointer, 1);
>> +	}
>> +}
>> +
>> +int CompressorJPEG::compress(const FrameBuffer *source, CompressedImage *jpeg)
>> +{
>> +	MappedFrameBuffer frame(source, PROT_READ);
>> +	if (!frame.isValid()) {
>> +		LOG(JPEG, Error) << "Failed to map FrameBuffer : "
>> +				 << strerror(frame.error());
>> +		return -frame.error();
>> +	}
>> +
>> +	jpeg_mem_dest(&compress_, &jpeg->data, &jpeg->length);
>> +
>> +	jpeg_start_compress(&compress_, TRUE);
>> +
>> +	LOG(JPEG, Debug) << "JPEG Compress Starting";
>> +	LOG(JPEG, Debug) << "Width: " << compress_.image_width
>> +			 << " height: " << compress_.image_height
>> +			 << " stride: " << stride_;
>> +
>> +	if (nv_)
>> +		compressNV(&frame);
>> +	else if (compress_.in_color_space == JCS_YCbCr)
>> +		compressYUV(&frame);
>> +	else
>> +		compressRGB(&frame);
> 
> If you have your map of PixelFormat to a custom class you can there
> store a pointer to the conversion function associated to each format and
> avoid the switch here

This turned out to be harder than I expected so I'm skipping it for now ;-(


>> +
>> +	LOG(JPEG, Debug) << "JPEG Compress Completed";
>> +
>> +	jpeg_finish_compress(&compress_);
>> +
>> +	return 0;
>> +}
>> +
>> +void CompressorJPEG::free(CompressedImage *jpeg)
>> +{
>> +	::free(jpeg->data);
>> +	jpeg->data = nullptr;
>> +}
>> diff --git a/src/android/jpeg/compressor_jpeg.h b/src/android/jpeg/compressor_jpeg.h
>> new file mode 100644
>> index 000000000000..11009481a2fe
>> --- /dev/null
>> +++ b/src/android/jpeg/compressor_jpeg.h
>> @@ -0,0 +1,44 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * compressor_jpeg.h - JPEG compression using libjpeg
>> + */
>> +#ifndef __COMPRESSOR_JPEG_H__
>> +#define __COMPRESSOR_JPEG_H__
> 
> Same as above, I think ANDROID_ should be part of the inclusion guard
> 
>> +
>> +#include "compressor.h"
>> +
>> +#include <libcamera/buffer.h>
>> +#include <libcamera/stream.h>
>> +
>> +#include <jpeglib.h>
>> +
>> +class CompressorJPEG : public Compressor
>> +{
>> +public:
>> +	CompressorJPEG();
>> +	~CompressorJPEG();
>> +
>> +	int configure(const libcamera::StreamConfiguration &cfg);
>> +	int compress(const libcamera::FrameBuffer *source, CompressedImage *jpeg);
>> +	void free(CompressedImage *jpeg);
>> +
>> +private:
>> +	void compressRGB(const libcamera::MappedFrameBuffer *frame);
>> +	void compressYUV(const libcamera::MappedFrameBuffer *frame);
>> +	void compressNV(const libcamera::MappedFrameBuffer *frame);
>> +
>> +	struct jpeg_compress_struct compress_;
>> +	struct jpeg_error_mgr jerr_;
>> +
>> +	unsigned int quality_;
>> +	unsigned int stride_;
>> +
>> +	bool nv_;
>> +	bool nvSwap_;
>> +	unsigned int horzSubSample_;
>> +	unsigned int vertSubSample_;
> 
> These information should be decoupled from the compressor class, as
> they belong to the format the is being compressed, not to the
> compressor instance. What do you think ? Are compressors throw away
> objects created to run once on a format and thrown away ?

It's now obtained from the PixelFormatInfo.


> 
>> +};
>> +
>> +#endif /* __COMPRESSOR_JPEG_H__ */
>> diff --git a/src/android/meson.build b/src/android/meson.build
>> index 822cad621f01..51dcd99ee62f 100644
>> --- a/src/android/meson.build
>> +++ b/src/android/meson.build
>> @@ -6,6 +6,7 @@ android_hal_sources = files([
>>      'camera_device.cpp',
>>      'camera_metadata.cpp',
>>      'camera_ops.cpp',
>> +    'jpeg/compressor_jpeg.cpp',
>>  ])
>>
>>  android_camera_metadata_sources = files([
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index 3aad4386ffc2..d78e2c1f6eb8 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -124,6 +124,8 @@ if get_option('android')
>>      libcamera_sources += android_hal_sources
>>      includes += android_includes
>>      libcamera_link_with += android_camera_metadata
>> +
>> +    libcamera_deps += dependency('libjpeg')
> 
> Shouldn't this be a dependency introduced by compiling the android HAL
> in ?
> 
> Thanks
>   j
> 
>>  endif
>>
>>  # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
>> --
>> 2.25.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran