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

8910 diff mbox series

Provide a compressor interface and implement a JPEG compressor using libjpeg.

Signed-off-by: Kieran Bingham <kieran.bingham@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

Hi Kieran,
   just skimming through.. some nits and questions below...

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@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 ?

> +#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 ?

> +
> +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 },
        ..
};

> +
> +}
> +
> +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 ?

> +	 */
> +	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

> +	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

> +
> +	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 ?

> +};
> +
> +#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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On 22/07/2020 09:50, Jacopo Mondi wrote:
> Hi Kieran,
>    just skimming through.. some nits and questions below...
> 
> 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@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 ?
> 
>> +#ifndef __LIBCAMERA_COMPRESSOR_H__
>> +#define __LIBCAMERA_COMPRESSOR_H__
> 
> Maybe __LIBCAMERA_ANDROID_COMPRESSOR__ ?

This will be moved to a more generic area.

I don't know what to name that yet, so it's here for now.

I expect we should have

src/libcamera-helper-library/include/compressor.h


But I don't have src/libcamera-helper-library yet ';-)

Maybe now is the time to create it.

Who wants to propose a name?


>> +
>> +#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 ?

I do indeed expect more compressors.

I've already had two software implementions of the JPEG compressor. One
using the libjpeg API, and one using the libjpeg-turbo API.

I've now ditched the -turbo, with an aim to being able to make use of
the raw libjpeg api in the native implementation.

We will also expect a hardware accelerated JPEG compressor to appear....


And perhaps there will be an h264 compressor, or an h265 compressor
needed sometime ...


>> +
>> +#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 ?


There are two libjpeg APIs. A 'native' and a 'turbo'. They both end up
executing the same code, and are supposedly the same performance, but
the -turbo API is simpler and doesn't have as much control over
arbitrary data formats.

(the -turbo comes from the name of the accelerated libjpeg library - not
the api, but it extends the native-api somewhat).


>> + */
>> +
>> +#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 certainly hope so, but they weren't around when I created this file. I
hadn't swapped over yet, because I have been trying to work on getting
the Camera app to function.


>> +
>> +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 },
>         ..
> };

I hope so ;-)



>> +
>> +}
>> +
>> +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 ?


Early on in development I was /always/ getting a zero or somehow
incorrect stride. I can't remember now, but this is here to make sure I
don't hit it again while developing ;-)

I believe it did happen on IPU3 I thought, but I can't recall to be sure.


>> +	 */
>> +	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

Yes, that will be better, but wasn't availble when this was written.
I'll update.


> 
>> +	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

That's a note to me currently - I didn't expect this to be posted ;-)

> 
>> +	 * 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

I like that idea!



>> +
>> +	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

I think we will very quickly move these to their own component structure
under src, so I might infact rename this to LIBCAMERA_COMPRESSOR_JPEG

> 
>> +
>> +#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 ?

This is just a caching of the configuration that was selected. When it
can come from the PixelFormatInfo, I expect it will store just a
reference to that instead.


>> +};
>> +
>> +#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 ?
> 

it is ... it's under the if get_option('android') section.


> 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@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

On Wed, Jul 22, 2020 at 11:17:50AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>

snip

> >> +	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 ?
>
> This is just a caching of the configuration that was selected. When it
> can come from the PixelFormatInfo, I expect it will store just a
> reference to that instead.
>

My question was more like: do you create one compressor instance per
conversion ? Otherwise I don't see how format information, regardless
where they come from or are stored, can be class members.

As I thought it, the compressor class should be instantiated once, and
provides methods to schedule possibly concurrent conversions, in a
thread as you noted down in the cover letter, with a signal/slot
mechanism to notify the caller when the conversion is done. In this
case format information are -per conversion run- not per instance.

>
> >> +};
> >> +
> >> +#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 ?
> >
>
> it is ... it's under the if get_option('android') section.
>

Oh sorry, missed that.


>
> > 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@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran

Hi Jacopo,

On 22/07/2020 11:31, Jacopo Mondi wrote:
> On Wed, Jul 22, 2020 at 11:17:50AM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
> 
> snip
> 
>>>> +	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 ?
>>
>> This is just a caching of the configuration that was selected. When it
>> can come from the PixelFormatInfo, I expect it will store just a
>> reference to that instead.
>>
> 
> My question was more like: do you create one compressor instance per
> conversion ? Otherwise I don't see how format information, regardless
> where they come from or are stored, can be class members.

The lifetime of a compression object will support multiple consecutive
frames.

All frames must use the same configuration, and (for jpeg), the class
will maintain the libjpeg compression object and allocations (and
configuration), and each call to compress will then simply take one
input frame, and produce one output buffer based on all the
preconfigured parameters.

There's nothing preventing someone from constructing a compression
object each time of course, except then there need to be allocations,
and configuration for each frame which would be redundant.

(by that I mean libjpeg makes internal allocations, so keeping the
lifetime of the Compressor object allows reuse of those internal
allocations for the duration of a whole stream).


> As I thought it, the compressor class should be instantiated once, and
> provides methods to schedule possibly concurrent conversions, in a
> thread as you noted down in the cover letter, with a signal/slot

libjpeg can only perform a single compression at a time, but once the
object has a thread, it can indeed handle these as a queue and become
event driven.

But by 'concurrent' a single libjpeg object could only compress a single
frame at a time. If it runs in it's own thread, then that would then
prevent blocking of the calling thread if that's what you mean.



> mechanism to notify the caller when the conversion is done. In this
> case format information are -per conversion run- not per instance.

No, if you want a different set of configuration options, just create
another instance of the compressor...

I haven't yet looked at threading, but indeed that could be handled by
each Compressor creating a thread, which then signals completion events.

That would then form part of the design of the Compressor object base class.



>>>> +};
>>>> +
>>>> +#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 ?
>>>
>>
>> it is ... it's under the if get_option('android') section.
>>
> 
> Oh sorry, missed that.

:-)

>>> 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@lists.libcamera.org
>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>
>> --
>> Regards
>> --
>> Kieran

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@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@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel