[libcamera-devel] [PATCH v3 3/4] Rework JPEG encoder API and update PostProcessorJpeg and EncoderLibJpeg
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Apr 27 01:44:42 CEST 2022
Hi Harvey,
Thank you for the patch.
On Tue, Apr 26, 2022 at 09:14:08AM +0000, Harvey Yang via libcamera-devel wrote:
> To add JEA in the next CL, this CL reworks JPEG encoder API and move
Same comment as in 1/4 for "CL" (I won't repeat it for the other patches
in this series, please update them as applicable).
> thumbnail encoding code into EncoderLibJpeg's implementation, as JEA
> supports that as well.
>
> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
The e-mail address should use "@" instead of " at ".
> ---
> src/android/jpeg/encoder.h | 9 ++-
> src/android/jpeg/encoder_libjpeg.cpp | 70 +++++++++++++++++++
> src/android/jpeg/encoder_libjpeg.h | 21 +++++-
> .../jpeg/generic_post_processor_jpeg.cpp | 14 ++++
> src/android/jpeg/meson.build | 9 +++
> src/android/jpeg/post_processor_jpeg.cpp | 60 ++--------------
> src/android/jpeg/post_processor_jpeg.h | 11 +--
> src/android/meson.build | 5 +-
This is more reviewable than in the previous version, but there are
still quite a few changes bundled in the same patch. As a v4 will be
needed anyway, could you split this further as follows ? Feel free to
change the order as appropriate.
- Add meson.build file in jpeg/ directory
- Move thumbnail generate to Encoder class
- Pass streamBuffer to encode() to prepare for JPEG encoders that need
access to the HAL buffer handle
- Add PostProcessorJpeg::SetEncoder()
> 8 files changed, 128 insertions(+), 71 deletions(-)
> create mode 100644 src/android/jpeg/generic_post_processor_jpeg.cpp
> create mode 100644 src/android/jpeg/meson.build
>
> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
> index b974d367..6d527d91 100644
> --- a/src/android/jpeg/encoder.h
> +++ b/src/android/jpeg/encoder.h
> @@ -12,14 +12,19 @@
> #include <libcamera/framebuffer.h>
> #include <libcamera/stream.h>
>
> +#include "../camera_request.h"
> +
> class Encoder
> {
> public:
> virtual ~Encoder() = default;
>
> virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
> - virtual int encode(const libcamera::FrameBuffer &source,
> - libcamera::Span<uint8_t> destination,
> + virtual int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> libcamera::Span<const uint8_t> exifData,
> unsigned int quality) = 0;
> + virtual int generateThumbnail(const libcamera::FrameBuffer &source,
> + const libcamera::Size &targetSize,
> + unsigned int quality,
> + std::vector<unsigned char> *thumbnail) = 0;
> };
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index 21a3b33d..b5591e33 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -24,6 +24,8 @@
> #include "libcamera/internal/formats.h"
> #include "libcamera/internal/mapped_framebuffer.h"
>
> +#include "../camera_buffer.h"
> +
> using namespace libcamera;
>
> LOG_DECLARE_CATEGORY(JPEG)
> @@ -82,8 +84,17 @@ EncoderLibJpeg::~EncoderLibJpeg()
> }
>
> int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
> +{
> + thumbnailer_.configure(cfg.size, cfg.pixelFormat);
> + cfg_ = cfg;
I'd store the size and pixel format only, not the full
StreamConfiguration, as they are all you need. internalConfigure()
should then take a size and pixel format, which will make it easier to
use in generateThumbnail().
> +
> + return internalConfigure(cfg);
> +}
> +
> +int EncoderLibJpeg::internalConfigure(const StreamConfiguration &cfg)
Let's name the function configureEncoder().
> {
> const struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);
> +
> if (info.colorSpace == JCS_UNKNOWN)
> return -ENOTSUP;
>
> @@ -178,6 +189,65 @@ void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes)
> }
> }
>
> +int EncoderLibJpeg::encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> + libcamera::Span<const uint8_t> exifData,
> + unsigned int quality)
> +{
> + internalConfigure(cfg_);
cfg_ is only used here, I suppose to reconfigure the libjpeg encoder for
the
> + return encode(*streamBuffer->srcBuffer, streamBuffer->dstBuffer.get()->plane(0), exifData, quality);
Let's shorten the line a bit (we aim for 80 columns as a soft target)
return encode(*streamBuffer->srcBuffer, streamBuffer->dstBuffer.get()->plane(0),
exifData, quality);
> +}
> +
> +int EncoderLibJpeg::generateThumbnail(
> + const libcamera::FrameBuffer &source,
> + const libcamera::Size &targetSize,
> + unsigned int quality,
> + std::vector<unsigned char> *thumbnail)
int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer &source,
const libcamera::Size &targetSize,
unsigned int quality,
std::vector<unsigned char> *thumbnail)
to match the coding style.
> +{
> + /* Stores the raw scaled-down thumbnail bytes. */
> + std::vector<unsigned char> rawThumbnail;
> +
> + thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);
> +
> + StreamConfiguration thCfg;
> + thCfg.size = targetSize;
> + thCfg.pixelFormat = thumbnailer_.pixelFormat();
> + int ret = internalConfigure(thCfg);
You now use the same libjpeg encoder instance for both the main image
and the thumbnail. This leads to EncoderLibJpeg::internalConfigure()
being called twice for every frame. The function looks fairly cheap so
it should be fine, but could you confirm that jpeg_set_defaults()
doesn't cause a large overhead ? Please mention this change in the
commit message of the appropriate patch, it's an important one.
> +
> + if (!rawThumbnail.empty() && !ret) {
> + /*
> + * \todo Avoid value-initialization of all elements of the
> + * vector.
> + */
> + thumbnail->resize(rawThumbnail.size());
> +
> + /*
> + * Split planes manually as the encoder expects a vector of
> + * planes.
> + *
> + * \todo Pass a vector of planes directly to
> + * Thumbnailer::createThumbnailer above and remove the manual
> + * planes split from here.
> + */
> + std::vector<Span<uint8_t>> thumbnailPlanes;
> + const PixelFormatInfo &formatNV12 = PixelFormatInfo::info(formats::NV12);
> + size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);
> + size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);
> + thumbnailPlanes.push_back({ rawThumbnail.data(), yPlaneSize });
> + thumbnailPlanes.push_back({ rawThumbnail.data() + yPlaneSize, uvPlaneSize });
> +
> + int jpeg_size = encode(thumbnailPlanes, *thumbnail, {}, quality);
> + thumbnail->resize(jpeg_size);
> +
> + LOG(JPEG, Debug)
> + << "Thumbnail compress returned "
> + << jpeg_size << " bytes";
> +
> + return jpeg_size;
> + }
> +
> + return -1;
> +}
> +
> int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
> Span<const uint8_t> exifData, unsigned int quality)
> {
> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> index 1b3ac067..56b27bae 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/encoder_libjpeg.h
> @@ -15,6 +15,8 @@
>
> #include <jpeglib.h>
>
> +#include "thumbnailer.h"
> +
> class EncoderLibJpeg : public Encoder
> {
> public:
> @@ -22,19 +24,32 @@ public:
> ~EncoderLibJpeg();
>
> int configure(const libcamera::StreamConfiguration &cfg) override;
> + int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> + libcamera::Span<const uint8_t> exifData,
> + unsigned int quality) override;
> + int generateThumbnail(
> + const libcamera::FrameBuffer &source,
> + const libcamera::Size &targetSize,
> + unsigned int quality,
> + std::vector<unsigned char> *thumbnail) override;
Same here.
> +
> +private:
> + int internalConfigure(const libcamera::StreamConfiguration &cfg);
> +
> int encode(const libcamera::FrameBuffer &source,
> libcamera::Span<uint8_t> destination,
> libcamera::Span<const uint8_t> exifData,
> - unsigned int quality) override;
> + unsigned int quality);
> int encode(const std::vector<libcamera::Span<uint8_t>> &planes,
> libcamera::Span<uint8_t> destination,
> libcamera::Span<const uint8_t> exifData,
> unsigned int quality);
>
> -private:
> void compressRGB(const std::vector<libcamera::Span<uint8_t>> &planes);
> void compressNV(const std::vector<libcamera::Span<uint8_t>> &planes);
>
> + libcamera::StreamConfiguration cfg_;
> +
> struct jpeg_compress_struct compress_;
> struct jpeg_error_mgr jerr_;
>
> @@ -42,4 +57,6 @@ private:
>
> bool nv_;
> bool nvSwap_;
> +
> + Thumbnailer thumbnailer_;
> };
> diff --git a/src/android/jpeg/generic_post_processor_jpeg.cpp b/src/android/jpeg/generic_post_processor_jpeg.cpp
> new file mode 100644
> index 00000000..890f6972
> --- /dev/null
> +++ b/src/android/jpeg/generic_post_processor_jpeg.cpp
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * generic_post_processor_jpeg.cpp - Generic JPEG Post Processor
> + */
> +
> +#include "encoder_libjpeg.h"
> +#include "post_processor_jpeg.h"
> +
> +void PostProcessorJpeg::SetEncoder()
We use camelCase for function names, not CamelCase. Let's name the
function createEncoder(), as it doesn't just set it but actually creates
it.
> +{
> + encoder_ = std::make_unique<EncoderLibJpeg>();
I'm tempted to simplify this by using conditional compilation (we have a
OS_CHROMEOS macro), the function could then go to
post_processor_jpeg.cpp.
> +}
> diff --git a/src/android/jpeg/meson.build b/src/android/jpeg/meson.build
> new file mode 100644
> index 00000000..94522dc0
> --- /dev/null
> +++ b/src/android/jpeg/meson.build
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +android_hal_sources += files([
> + 'exif.cpp',
> + 'post_processor_jpeg.cpp'])
The '])' should be on the next line.
> +
> +android_hal_sources += files(['encoder_libjpeg.cpp',
> + 'generic_post_processor_jpeg.cpp',
> + 'thumbnailer.cpp'])
Similar comment here,
android_hal_sources += files([
'encoder_libjpeg.cpp',
'generic_post_processor_jpeg.cpp',
'thumbnailer.cpp',
])
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index d72ebc3c..7ceba60e 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -9,10 +9,10 @@
>
> #include <chrono>
>
> +#include "../android_framebuffer.h"
Is this needed ?
> #include "../camera_device.h"
> #include "../camera_metadata.h"
> #include "../camera_request.h"
> -#include "encoder_libjpeg.h"
> #include "exif.h"
>
> #include <libcamera/base/log.h>
> @@ -44,60 +44,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
>
> streamSize_ = outCfg.size;
>
> - thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> -
> - encoder_ = std::make_unique<EncoderLibJpeg>();
> + SetEncoder();
>
> return encoder_->configure(inCfg);
> }
>
> -void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> - const Size &targetSize,
> - unsigned int quality,
> - std::vector<unsigned char> *thumbnail)
> -{
> - /* Stores the raw scaled-down thumbnail bytes. */
> - std::vector<unsigned char> rawThumbnail;
> -
> - thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);
> -
> - StreamConfiguration thCfg;
> - thCfg.size = targetSize;
> - thCfg.pixelFormat = thumbnailer_.pixelFormat();
> - int ret = thumbnailEncoder_.configure(thCfg);
> -
> - if (!rawThumbnail.empty() && !ret) {
> - /*
> - * \todo Avoid value-initialization of all elements of the
> - * vector.
> - */
> - thumbnail->resize(rawThumbnail.size());
> -
> - /*
> - * Split planes manually as the encoder expects a vector of
> - * planes.
> - *
> - * \todo Pass a vector of planes directly to
> - * Thumbnailer::createThumbnailer above and remove the manual
> - * planes split from here.
> - */
> - std::vector<Span<uint8_t>> thumbnailPlanes;
> - const PixelFormatInfo &formatNV12 = PixelFormatInfo::info(formats::NV12);
> - size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);
> - size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);
> - thumbnailPlanes.push_back({ rawThumbnail.data(), yPlaneSize });
> - thumbnailPlanes.push_back({ rawThumbnail.data() + yPlaneSize, uvPlaneSize });
> -
> - int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,
> - *thumbnail, {}, quality);
> - thumbnail->resize(jpeg_size);
> -
> - LOG(JPEG, Debug)
> - << "Thumbnail compress returned "
> - << jpeg_size << " bytes";
> - }
> -}
> -
> void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
> {
> ASSERT(encoder_);
> @@ -164,8 +115,8 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
>
> if (thumbnailSize != Size(0, 0)) {
> std::vector<unsigned char> thumbnail;
> - generateThumbnail(source, thumbnailSize, quality, &thumbnail);
> - if (!thumbnail.empty())
> + ret = encoder_->generateThumbnail(source, thumbnailSize, quality, &thumbnail);
> + if (ret > 0 && !thumbnail.empty())
> exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
> }
>
> @@ -194,8 +145,7 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
> const uint8_t quality = ret ? *entry.data.u8 : 95;
> resultMetadata->addEntry(ANDROID_JPEG_QUALITY, quality);
>
> - int jpeg_size = encoder_->encode(source, destination->plane(0),
> - exif.data(), quality);
> + int jpeg_size = encoder_->encode(streamBuffer, exif.data(), quality);
> if (jpeg_size < 0) {
> LOG(JPEG, Error) << "Failed to encode stream image";
> processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 98309b01..a09f8798 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -8,11 +8,11 @@
> #pragma once
>
> #include "../post_processor.h"
> -#include "encoder_libjpeg.h"
> -#include "thumbnailer.h"
>
> #include <libcamera/geometry.h>
>
> +#include "encoder.h"
> +
> class CameraDevice;
>
> class PostProcessorJpeg : public PostProcessor
> @@ -25,14 +25,9 @@ public:
> void process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
>
> private:
> - void generateThumbnail(const libcamera::FrameBuffer &source,
> - const libcamera::Size &targetSize,
> - unsigned int quality,
> - std::vector<unsigned char> *thumbnail);
> + void SetEncoder();
>
> CameraDevice *const cameraDevice_;
> std::unique_ptr<Encoder> encoder_;
> libcamera::Size streamSize_;
> - EncoderLibJpeg thumbnailEncoder_;
> - Thumbnailer thumbnailer_;
> };
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 27be27bb..026b8b3c 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -48,10 +48,6 @@ android_hal_sources = files([
> 'camera_ops.cpp',
> 'camera_request.cpp',
> 'camera_stream.cpp',
> - 'jpeg/encoder_libjpeg.cpp',
> - 'jpeg/exif.cpp',
> - 'jpeg/post_processor_jpeg.cpp',
> - 'jpeg/thumbnailer.cpp',
> 'yuv/post_processor_yuv.cpp'
> ])
>
> @@ -59,6 +55,7 @@ android_cpp_args = []
>
> subdir('cros')
> subdir('mm')
> +subdir('jpeg')
Please keep these alphabetically sorted.
>
> android_camera_metadata_sources = files([
> 'metadata/camera_metadata.c',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list