[libcamera-devel,v2,11/12] android: Introduce JPEG compression
diff mbox series

Message ID 20200803161816.107113-12-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • android: jpeg
Related show

Commit Message

Kieran Bingham Aug. 3, 2020, 4:18 p.m. UTC 
Provide a compressor interface and implement a JPEG compressor using libjpeg.

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


Possible Todos:

  - Convert to RAW api if possible.
  - Add custom JPEG error handler

 src/android/jpeg/compressor.h        |  28 ++++
 src/android/jpeg/compressor_jpeg.cpp | 215 +++++++++++++++++++++++++++
 src/android/jpeg/compressor_jpeg.h   |  46 ++++++
 src/android/meson.build              |   1 +
 src/libcamera/meson.build            |   2 +
 5 files changed, 292 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

Comments

Jacopo Mondi Aug. 4, 2020, 12:34 p.m. UTC | #1 
Hi Kieran,
   a few minor comments below

On Mon, Aug 03, 2020 at 05:18:15PM +0100, Kieran Bingham wrote:
> Provide a compressor interface and implement a JPEG compressor using libjpeg.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@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...
>
>
> Possible Todos:
>
>   - Convert to RAW api if possible.
>   - Add custom JPEG error handler

Do we want to record these, even if Doxygen does not parse this
component ?

>
>  src/android/jpeg/compressor.h        |  28 ++++
>  src/android/jpeg/compressor_jpeg.cpp | 215 +++++++++++++++++++++++++++
>  src/android/jpeg/compressor_jpeg.h   |  46 ++++++
>  src/android/meson.build              |   1 +
>  src/libcamera/meson.build            |   2 +
>  5 files changed, 292 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
> + */
> +#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;
> +};
> +
> +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;
> +};
> +
> +#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..83149417878d
> --- /dev/null
> +++ b/src/android/jpeg/compressor_jpeg.cpp
> @@ -0,0 +1,215 @@
> +/* 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;
> +};
> +
> +std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{

const ?

> +	{ 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. */
> +	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);
> +
> +	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.
> + */
> +void CompressorJPEG::compressNV(const libcamera::MappedBuffer *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 possible, see if we can set appropriate pixel strides too to save even that copy.

Comments on 80 cols ?

Ah, this is recorded then! good

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

I haven't really reviewed that part, but if it works, it's all good,
right ? :)

> +
> +		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 : "
                                                              ^ need
                                                              this space?
> +				 << strerror(frame.error());
> +		return -frame.error();

I assume the leading '-' is intentional

> +	}
> +
> +	jpeg_mem_dest(&compress_, &jpeg->data, &jpeg->length);
> +
> +	jpeg_start_compress(&compress_, TRUE);
> +
> +	LOG(JPEG, Debug) << "JPEG Compress Starting:"
> +			 << " Width: " << compress_.image_width
> +			 << " height: " << compress_.image_height;
> +
> +	if (nv_)
> +		compressNV(&frame);
> +	else
> +		compressRGB(&frame);

If one wants to be pedantic, the distinction here seems to be if we
have a single place or more. It's not only NV__ which is multi-planar
(or better, semi planar, as NV12 is, in example, the semi-planar
version of the three-plane YUV420 format) or RAW which is single
planar (ie. packed YUV)

But I see you only support NV variations and some RAW formats, if this
is not going to be updated I think then the naming is fine.

> +
> +	LOG(JPEG, Debug) << "JPEG Compress Completed";
> +
> +	jpeg_finish_compress(&compress_);
> +
> +	return 0;
> +}
> +
> +void CompressorJPEG::free(CompressedImage *jpeg)
> +{
> +	::free(jpeg->data);
> +	jpeg->data = nullptr;

Would setting length to zero hurt ?

> +}
> diff --git a/src/android/jpeg/compressor_jpeg.h b/src/android/jpeg/compressor_jpeg.h
> new file mode 100644
> index 000000000000..3c0c3ff68449
> --- /dev/null
> +++ b/src/android/jpeg/compressor_jpeg.h
> @@ -0,0 +1,46 @@
> +/* 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__
> +#define __ANDROID_JPEG_COMPRESSOR_JPEG_H__
> +
> +#include "compressor.h"
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/stream.h>
> +
> +#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);
> +	int compress(const libcamera::FrameBuffer *source, CompressedImage *jpeg);
> +	void free(CompressedImage *jpeg);

These should be marked with 'override'

> +
> +private:
> +	void compressRGB(const libcamera::MappedBuffer *frame);
> +	void compressYUV(const libcamera::MappedBuffer *frame);

This seems not to be implemented

> +	void compressNV(const libcamera::MappedBuffer *frame);
> +
> +	struct jpeg_compress_struct compress_;
> +	struct jpeg_error_mgr jerr_;
> +
> +	unsigned int quality_;
> +
> +	const libcamera::PixelFormatInfo *pixelFormatInfo;

as a class member this should end with _

Mostly nit, nothing blocking this patch for real

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> +
> +	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
> +
> +    libcamera_deps += dependency('libjpeg')
>  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
Kieran Bingham Aug. 4, 2020, 1:09 p.m. UTC | #2 
Hi Jacopo,

On 04/08/2020 13:34, Jacopo Mondi wrote:
> Hi Kieran,
>    a few minor comments below
> 
> On Mon, Aug 03, 2020 at 05:18:15PM +0100, Kieran Bingham wrote:
>> Provide a compressor interface and implement a JPEG compressor using libjpeg.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@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...
>>
>>
>> Possible Todos:
>>
>>   - Convert to RAW api if possible.
>>   - Add custom JPEG error handler
> 
> Do we want to record these, even if Doxygen does not parse this
> component ?
> 

I'll add the custom jpeg error handler todo. I have a patch/WIP, but
haven't been able to get it quite right yet, hence it's not in this series.


Actually, both of these todo's are already in the code in relevant places.

>>
>>  src/android/jpeg/compressor.h        |  28 ++++
>>  src/android/jpeg/compressor_jpeg.cpp | 215 +++++++++++++++++++++++++++
>>  src/android/jpeg/compressor_jpeg.h   |  46 ++++++
>>  src/android/meson.build              |   1 +
>>  src/libcamera/meson.build            |   2 +
>>  5 files changed, 292 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
>> + */
>> +#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;

I've contemplated making this a Span<uint8_t> like with the MappedBuffer
changes too ... but there are some nuances that this buffer is used to
both provide the available buffer size, and return the consumed bytes.
So maybe that should change, and have an explicit bytesUsed field added.

Or compress() could return the Span of actually generated data... but
then I'd probably have to add an error() function too :S


>> +};
>> +
>> +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;
>> +};
>> +
>> +#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..83149417878d
>> --- /dev/null
>> +++ b/src/android/jpeg/compressor_jpeg.cpp
>> @@ -0,0 +1,215 @@
>> +/* 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;
>> +};
>> +
>> +std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{
> 
> const ?

Sure ;-)

> 
>> +	{ 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 } },
>> +

FWIW, I was considering dropping the R8, RGB888 entries here, as we only
really support NV12/NV21 in the android layer currently, but they're
quite cheap...



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

aha, so this is already recorded, but I'll expand this.


>> +	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);
>> +
>> +	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.
>> + */
>> +void CompressorJPEG::compressNV(const libcamera::MappedBuffer *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 possible, see if we can set appropriate pixel strides too to save even that copy.
> 
> Comments on 80 cols ?

I'll reformat the line lengths now.


> 
> Ah, this is recorded then! good
> 
>> +	 *
>> +	 * 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;
>> +		}
> 
> I haven't really reviewed that part, but if it works, it's all good,
> right ? :)

This is lifted from src/qcam/format_convertor.cpp:
FormatConverter::convertNV(), and adjusted for local requirements.

I hope to drop this function, and replace with the RAW api, but yes it
works, and allows testing of the JPEG compressor.


> 
>> +
>> +		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 : "
>                                                               ^ need
>                                                               this space?
>> +				 << strerror(frame.error());
>> +		return -frame.error();
> 
> I assume the leading '-' is intentional

Err, given the documentation I just added for this:

> * \return 0 on success or a negative error code otherwise

Then no ;-)

Fixed.

> 
>> +	}
>> +
>> +	jpeg_mem_dest(&compress_, &jpeg->data, &jpeg->length);
>> +
>> +	jpeg_start_compress(&compress_, TRUE);
>> +
>> +	LOG(JPEG, Debug) << "JPEG Compress Starting:"
>> +			 << " Width: " << compress_.image_width
>> +			 << " height: " << compress_.image_height;
>> +
>> +	if (nv_)
>> +		compressNV(&frame);
>> +	else
>> +		compressRGB(&frame);
> 
> If one wants to be pedantic, the distinction here seems to be if we
> have a single place or more. It's not only NV__ which is multi-planar
> (or better, semi planar, as NV12 is, in example, the semi-planar
> version of the three-plane YUV420 format) or RAW which is single
> planar (ie. packed YUV)
> 
> But I see you only support NV variations and some RAW formats, if this
> is not going to be updated I think then the naming is fine.

I have patches on top which add other YUV unpacking, but it's so ugly,
and *unused* that I have removed it, and I hope it can be added in a
better form /if and when required/ or as part of using the RAW api to
parse data without unpacking.


>> +
>> +	LOG(JPEG, Debug) << "JPEG Compress Completed";
>> +
>> +	jpeg_finish_compress(&compress_);
>> +
>> +	return 0;
>> +}
>> +
>> +void CompressorJPEG::free(CompressedImage *jpeg)
>> +{
>> +	::free(jpeg->data);
>> +	jpeg->data = nullptr;
> 
> Would setting length to zero hurt ?

Nope ;-) I'll add that too for consistency.


> 
>> +}
>> diff --git a/src/android/jpeg/compressor_jpeg.h b/src/android/jpeg/compressor_jpeg.h
>> new file mode 100644
>> index 000000000000..3c0c3ff68449
>> --- /dev/null
>> +++ b/src/android/jpeg/compressor_jpeg.h
>> @@ -0,0 +1,46 @@
>> +/* 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__
>> +#define __ANDROID_JPEG_COMPRESSOR_JPEG_H__
>> +
>> +#include "compressor.h"
>> +
>> +#include <libcamera/buffer.h>
>> +#include <libcamera/stream.h>
>> +
>> +#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);
>> +	int compress(const libcamera::FrameBuffer *source, CompressedImage *jpeg);
>> +	void free(CompressedImage *jpeg);
> 
> These should be marked with 'override'

Added.

> 
>> +
>> +private:
>> +	void compressRGB(const libcamera::MappedBuffer *frame);
>> +	void compressYUV(const libcamera::MappedBuffer *frame);
> 
> This seems not to be implemented

Oops, that's supposed to be in the patch on top (i.e. removed from here).

> 
>> +	void compressNV(const libcamera::MappedBuffer *frame);
>> +
>> +	struct jpeg_compress_struct compress_;
>> +	struct jpeg_error_mgr jerr_;
>> +
>> +	unsigned int quality_;
>> +
>> +	const libcamera::PixelFormatInfo *pixelFormatInfo;
> 
> as a class member this should end with _

Yes, indeed it should.

> 
> Mostly nit, nothing blocking this patch for real
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks, all nits updated I believe.


> 
> Thanks
>   j
> 
>> +
>> +	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
>> +
>> +    libcamera_deps += dependency('libjpeg')
>>  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

Patch
diff mbox series 

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
+ */
+#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;
+};
+
+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;
+};
+
+#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..83149417878d
--- /dev/null
+++ b/src/android/jpeg/compressor_jpeg.cpp
@@ -0,0 +1,215 @@ 
+/* 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;
+};
+
+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. */
+	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);
+
+	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.
+ */
+void CompressorJPEG::compressNV(const libcamera::MappedBuffer *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 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 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);
+
+	jpeg_start_compress(&compress_, TRUE);
+
+	LOG(JPEG, Debug) << "JPEG Compress Starting:"
+			 << " Width: " << compress_.image_width
+			 << " height: " << 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;
+}
diff --git a/src/android/jpeg/compressor_jpeg.h b/src/android/jpeg/compressor_jpeg.h
new file mode 100644
index 000000000000..3c0c3ff68449
--- /dev/null
+++ b/src/android/jpeg/compressor_jpeg.h
@@ -0,0 +1,46 @@ 
+/* 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__
+#define __ANDROID_JPEG_COMPRESSOR_JPEG_H__
+
+#include "compressor.h"
+
+#include <libcamera/buffer.h>
+#include <libcamera/stream.h>
+
+#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);
+	int compress(const libcamera::FrameBuffer *source, CompressedImage *jpeg);
+	void free(CompressedImage *jpeg);
+
+private:
+	void compressRGB(const libcamera::MappedBuffer *frame);
+	void compressYUV(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
+
+    libcamera_deps += dependency('libjpeg')
 endif
 
 # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.