[libcamera-devel] [PATCH v2 11/12] android: Introduce JPEG compression
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Aug 4 15:09:50 CEST 2020
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 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...
>>
>>
>> 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 at 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 at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list