[libcamera-devel] [RFC PATCH 1/6] android: Introduce JPEG compression
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jul 22 12:17:50 CEST 2020
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 at ideasonboard.com>
>> ---
>> src/android/jpeg/compressor.h | 28 +++
>> src/android/jpeg/compressor_jpeg.cpp | 279 +++++++++++++++++++++++++++
>> src/android/jpeg/compressor_jpeg.h | 44 +++++
>> src/android/meson.build | 1 +
>> src/libcamera/meson.build | 2 +
>> 5 files changed, 354 insertions(+)
>> create mode 100644 src/android/jpeg/compressor.h
>> create mode 100644 src/android/jpeg/compressor_jpeg.cpp
>> create mode 100644 src/android/jpeg/compressor_jpeg.h
>>
>> diff --git a/src/android/jpeg/compressor.h b/src/android/jpeg/compressor.h
>> new file mode 100644
>> index 000000000000..f95e4a4539cb
>> --- /dev/null
>> +++ b/src/android/jpeg/compressor.h
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * compressor.h - Image compression interface
>> + */
>
> Don't we usually have an empty line here ?
>
>> +#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 at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list