[libcamera-devel] [PATCH v3 12/13] android: Introduce JPEG compression

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Tue, Aug 04, 2020 at 10:47:10PM +0100, Kieran Bingham wrote:
> Provide a compressor interface and implement a JPEG compressor using
> libjpeg.
> 
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> v2:
> 
>  - Convert to use the libcamera format information rather than duplicating it ourselves.
>    - Not easy to get the horizontal subsampling
>    - not easy to determine if we have an nvSwap...
> 
> v3:
>  - Fix frame.error return value (negative inversion)
>  - Fix comments
>  - constify data table
>  - set jpeg image lenght to zero after free
>  - set override on class interface functions.
>  - Remove ununsed fucntion prototype
>  - Rename pixelFormatInfo to pixelFormatInfo_
> ---
>  src/android/jpeg/compressor.h        |  28 ++++
>  src/android/jpeg/compressor_jpeg.cpp | 217 +++++++++++++++++++++++++++
>  src/android/jpeg/compressor_jpeg.h   |  45 ++++++
>  src/android/meson.build              |   1 +
>  src/libcamera/meson.build            |   2 +
>  5 files changed, 293 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..18d8f65eba02
> --- /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

s/Image/JPEG/ ?

> + */
> +#ifndef __ANDROID_JPEG_COMPRESSOR_H__
> +#define __ANDROID_JPEG_COMPRESSOR_H__
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/stream.h>
> +
> +struct CompressedImage {
> +	unsigned char *data;
> +	unsigned long length;
> +};

This can be replaced by a Span<uint8_t> (which MappedBuffer provides you
:-)).

> +
> +class Compressor
> +{
> +public:
> +	virtual ~Compressor(){};

s/{}/ {}/

> +
> +	virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
> +	virtual int compress(const libcamera::FrameBuffer *source, CompressedImage *image) = 0;

s/image/dest/

(maybe src and dst ?)

If we pass a span for the destination, the function should return the
used size on success.

> +	virtual void free(CompressedImage *image) = 0;

This is never used, I think you can drop it.

> +};
> +
> +#endif /* __ANDROID_JPEG_COMPRESSOR_H__ */
> diff --git a/src/android/jpeg/compressor_jpeg.cpp b/src/android/jpeg/compressor_jpeg.cpp
> new file mode 100644
> index 000000000000..9921a294128c
> --- /dev/null
> +++ b/src/android/jpeg/compressor_jpeg.cpp
> @@ -0,0 +1,217 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * compressor_jpeg.cpp - JPEG compression using libjpeg native API
> + */
> +
> +#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/formats.h"
> +#include "libcamera/internal/log.h"
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(JPEG)
> +
> +namespace {
> +
> +struct JPEGPixelFormatInfo {
> +	J_COLOR_SPACE colorSpace;
> +	const PixelFormatInfo &pixelFormatInfo;
> +	bool nvSwap;
> +};
> +
> +const std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{
> +	{ formats::R8, { JCS_GRAYSCALE, PixelFormatInfo::info(formats::R8), false } },
> +
> +	{ formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888), false } },
> +	{ formats::BGR888, { JCS_EXT_RGB, PixelFormatInfo::info(formats::BGR888), false } },
> +
> +	{ formats::NV12, { JCS_YCbCr, PixelFormatInfo::info(formats::NV12), false } },
> +	{ formats::NV21, { JCS_YCbCr, PixelFormatInfo::info(formats::NV21), true } },
> +	{ formats::NV16, { JCS_YCbCr, PixelFormatInfo::info(formats::NV16), false } },
> +	{ formats::NV61, { JCS_YCbCr, PixelFormatInfo::info(formats::NV61), true } },
> +	{ formats::NV24, { JCS_YCbCr, PixelFormatInfo::info(formats::NV24), false } },
> +	{ formats::NV42, { JCS_YCbCr, PixelFormatInfo::info(formats::NV42), true } },
> +};
> +
> +const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)
> +{
> +	static const struct JPEGPixelFormatInfo invalidPixelFormat {
> +		JCS_UNKNOWN, PixelFormatInfo(), 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;
> +}
> +
> +} /* namespace */
> +
> +CompressorJPEG::CompressorJPEG()
> +	: quality_(95)
> +{
> +	/* \todo: Expand error handling coverage with a custom handler. */

s/todo:/todo/

> +	compress_.err = jpeg_std_error(&jerr_);
> +
> +	jpeg_create_compress(&compress_);
> +}
> +
> +CompressorJPEG::~CompressorJPEG()
> +{
> +	jpeg_destroy_compress(&compress_);
> +}
> +
> +int CompressorJPEG::configure(const StreamConfiguration &cfg)
> +{
> +	const struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);
> +	if (info.colorSpace == JCS_UNKNOWN)
> +		return -ENOTSUP;
> +
> +	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);
> +
> +	pixelFormatInfo_ = &info.pixelFormatInfo;
> +
> +	nv_ = pixelFormatInfo_->numPlanes() == 2;
> +	nvSwap_ = info.nvSwap;
> +
> +	return 0;
> +}
> +
> +void CompressorJPEG::compressRGB(const libcamera::MappedBuffer *frame)
> +{
> +	unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data());
> +	unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0);

Note for later, we'll have to replace this with the stride coming from
the frame buffer when it will be available. Same for NV formats.

> +
> +	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);
> +	}
> +}
> +
> +/*
> + * Compress the incoming buffer from a supported NV format.
> + * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.
> + * Utilisation of the RAW api will be implemented on top as a performance
> + * improvement.

You can replace the second sentence with

 * \todo Use the libjpeg RAW API to improve performances

> + */
> +void CompressorJPEG::compressNV(const libcamera::MappedBuffer *frame)
> +{
> +	std::vector<uint8_t> tmprowbuf(compress_.image_width * 3);

How about

	uint8_t tmprowbuf[compress_.image_width * 3];

Or do you think it should be allocated on the heap ?

> +
> +	/*
> +	 * \todo Use the raw api, and only unpack the cb/cr samples to new line
> +	 * buffers. If possible, see if we can set appropriate pixel strides
> +	 * too to save even that copy.

Ah it's here :-) So you can drop the second sentence above.

> +	 *
> +	 * Possible hints at:
> +	 * https://sourceforge.net/p/libjpeg/mailman/message/30815123/
> +	 */
> +	unsigned int y_stride = pixelFormatInfo_->stride(compress_.image_width, 0);
> +	unsigned int c_stride = pixelFormatInfo_->stride(compress_.image_width, 1);
> +
> +	unsigned int horzSubSample = 2 * compress_.image_width / c_stride;
> +	unsigned int vertSubSample = pixelFormatInfo_->planes[1].verticalSubSampling;
> +
> +	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].data());
> +	const unsigned char *src_c = src + y_stride * compress_.image_height;
> +
> +	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);

If your buffer isn't big enough, libjpeg will reallocate by default.
We'll need to implement a custom destination to avoid this, and return
an error. Can you add a \todo ?

> +
> +	jpeg_start_compress(&compress_, TRUE);
> +
> +	LOG(JPEG, Debug) << "JPEG Compress Starting:"
> +			 << " Width: " << compress_.image_width

s/Width/width/

> +			 << " height: " << compress_.image_height;

Or maybe
		<< "JPEG Compress Starting: " << compress_.image_width
		<< "x" << compress_.image_height;

> +
> +	if (nv_)
> +		compressNV(&frame);
> +	else
> +		compressRGB(&frame);
> +
> +	LOG(JPEG, Debug) << "JPEG Compress Completed";
> +
> +	jpeg_finish_compress(&compress_);
> +
> +	return 0;
> +}
> +
> +void CompressorJPEG::free(CompressedImage *jpeg)
> +{
> +	::free(jpeg->data);
> +	jpeg->data = nullptr;
> +	jpeg->length = 0;
> +}
> diff --git a/src/android/jpeg/compressor_jpeg.h b/src/android/jpeg/compressor_jpeg.h
> new file mode 100644
> index 000000000000..2284e8a6bff1
> --- /dev/null
> +++ b/src/android/jpeg/compressor_jpeg.h

Maybe compressor_libjpeg.h ?

> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * compressor_jpeg.h - JPEG compression using libjpeg
> + */
> +#ifndef __ANDROID_JPEG_COMPRESSOR_JPEG_H__

and COMPRESSOR_LIBJPEG here too.

> +#define __ANDROID_JPEG_COMPRESSOR_JPEG_H__
> +
> +#include "compressor.h"
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/stream.h>

Those two headers are already included by compressor.h. As you only need
them to declare the overridden functions, you can drop them here.

> +
> +#include "libcamera/internal/buffer.h"
> +#include "libcamera/internal/formats.h"
> +
> +#include <jpeglib.h>
> +
> +class CompressorJPEG : public Compressor
> +{
> +public:
> +	CompressorJPEG();
> +	~CompressorJPEG();
> +
> +	int configure(const libcamera::StreamConfiguration &cfg) override;
> +	int compress(const libcamera::FrameBuffer *source, CompressedImage *jpeg) override;
> +	void free(CompressedImage *jpeg) override;
> +
> +private:
> +	void compressRGB(const libcamera::MappedBuffer *frame);
> +	void compressNV(const libcamera::MappedBuffer *frame);
> +
> +	struct jpeg_compress_struct compress_;
> +	struct jpeg_error_mgr jerr_;
> +
> +	unsigned int quality_;
> +
> +	const libcamera::PixelFormatInfo *pixelFormatInfo_;
> +
> +	bool nv_;
> +	bool nvSwap_;
> +};
> +
> +#endif /* __ANDROID_JPEG_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
> +

No need for a blank line ?

> +    libcamera_deps += dependency('libjpeg')

Should this be provided via an android_deps set in
src/android/meson.build ?

>  endif
>  
>  # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.

-- 
Regards,

Laurent Pinchart