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

9205 diff mbox series

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

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
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...

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

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@jmondi.org>
> 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...
> 
> 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.

Hi Laurent,

On 05/08/2020 02:59, Laurent Pinchart wrote:
> 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@jmondi.org>
>> 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...
>>
>> 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/ ?


The actual compressor interface is not specific to JPEG. It is only used
by JPEG for us, but it could also be used to compress other formats.

So I chose the generic word Image intentionally.


>> + */
>> +#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
> :-)).

Yes, I had been working towards that too. But I was hit by a design
flaw/issue.

CompressedImage allows you to both give a region of memory and return
it, with the size set. I think I see later you already suggest changing
the return type to return the number of bytes used instead so that
solves that though.

The other use case was to pass in a null pointer, to let the compressor
allocate the memory, but that's mostly been a usecase for me to use the
compressor with Cam to test locally (as is the free() method) - so it
could be removed from here.

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

Hrm, that's odd, I thought I recalled reformatting that line already due
to checkstyle.py.

Indeed:

--- src/android/jpeg/compressor.h
+++ src/android/jpeg/compressor.h
@@ -18,8 +18,7 @@
 class Compressor
 {
 public:
-	virtual ~Compressor()
-	{};
+	virtual ~Compressor(){};

And:
--- src/android/jpeg/compressor.h
+++ src/android/jpeg/compressor.h
@@ -18,7 +18,7 @@
 class Compressor
 {
 public:
-	virtual ~Compressor() {};
+	virtual ~Compressor(){};


So checkstyle really wants to hug those braces, yet the precedence
really is set:


git grep "virtual.*{}"
include/libcamera/bound_method.h:       virtual ~BoundMethodPackBase() {}
include/libcamera/bound_method.h:       virtual ~BoundMethodBase() {}
include/libcamera/internal/control_validator.h: virtual
~ControlValidator() {}
include/libcamera/internal/ipa_proxy.h: virtual ~IPAProxyFactory() {}
include/libcamera/internal/media_object.h:      virtual ~MediaObject() {}
include/libcamera/internal/pipeline_handler.h:  virtual ~CameraData() {}
include/libcamera/internal/pipeline_handler.h:  virtual
~PipelineHandlerFactory() {}
include/libcamera/ipa/ipa_interface.h:  virtual ~IPAInterface() {}
src/android/jpeg/compressor.h:  virtual ~Compressor() {};
src/cam/options.h:      virtual ~KeyValueParser() {}
src/ipa/raspberrypi/controller/algorithm.hpp:   virtual ~Algorithm() {}
src/ipa/raspberrypi/md_parser.hpp:      virtual ~MdParser() {}
src/libcamera/pipeline/rkisp1/timeline.h:       virtual ~FrameAction() {}
src/libcamera/pipeline/rkisp1/timeline.h:       virtual ~Timeline() {}
test/libtest/test.h:    virtual void cleanup() {}



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

libjpeg allows you to provide a nullptr and it would do the allocation,
which you must then free.

Which is why this 'free' interface is here.

I use that in my test code to compress a FrameBuffer and have libjpeg
deal with the allocation ... but I guess it can be cut out for now.

Also I see further suggestion to use a custom memory destination to
prevent reallocation anyway, which makes sense, and also prevents this
use case - so I think it's clear it will be better to remove this.


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

\me guess I just dislike the doxygen syntax.
\me: Guess I just dislike the doxygen syntax.



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

I wonder if this is related to the issue Niklas' is seeing from
OpenCamera ...(though not 'here', elsewhere).

I think it looked like either a stride issue or pixelformat issue.

Anyway, that deserves a

 \todo Stride information should come from buffer configuration.

Could we expect stride to be different for each buffer? or would the
values be correct for the stream. Otherwise we're going to have to pass
more information about each buffer around.


>> +
>> +	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 ?
Because Variable Length Arrays are not part of C++?

(But yes, they are an extension provided by both GCC and clang)

Hrm, I've just added -Werror=vla, and I see we're already using them ...
so I guess in this instance we can again, and revisit later if we change
our minds on VLA usage.


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

Oh, I thought it would fail in that case.

/*
 * Prepare for output to a memory buffer.
 * The caller may supply an own initial buffer with appropriate size.
 * Otherwise, or when the actual data output exceeds the given size,
 * the library adapts the buffer size as necessary.
 * The standard library functions malloc/free are used for allocating
 * larger memory, so the buffer is available to the application after
 * finishing compression, and then the application is responsible for
 * freeing the requested memory.
 * Note:  An initial buffer supplied by the caller is expected to be
 * managed by the application.  The library does not free such buffer
 * when allocating a larger buffer.
 */


Urgh, that's a bit horrible. So not only does the caller have to track
which buffer actually came back, they have to free the original if it
changed too.


Indeed, lets have our own custom destination manager to remove all of that.

\todo Implement our own custom memory destination to prevent
reallocation and prefer failure with correct reporting.


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

I like the shorter 640x480 ;-)


>> +
>> +	if (nv_)
>> +		compressNV(&frame);
>> +	else
>> +		compressRGB(&frame);
>> +
>> +	LOG(JPEG, Debug) << "JPEG Compress Completed";

I think I can drop this one now too ;-)


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

Yes, that is probably more accurate indeed.

Equally the compressor_jpeg.cpp of course.



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

Dropped.


>> +
>> +#include "libcamera/internal/buffer.h"
>> +#include "libcamera/internal/formats.h"
>> +
>> +#include <jpeglib.h>
>> +
>> +class CompressorJPEG : public Compressor

Well, I guess this needs to be class CompressorLibJPEG now too ?


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

I think I prefer that indeed.
Especially as other dependencies are coming in too.

This is already masked by the if get_option('android') of course, but it
would be better to describe the dependencies in that subdir.



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