[libcamera-devel] [PATCH v3 3/4] Rework JPEG encoder API and update PostProcessorJpeg and EncoderLibJpeg
Cheng-Hao Yang
chenghaoyang at chromium.org
Tue May 3 05:29:49 CEST 2022
Hi Laurent,
Sorry I forgot to send this reply before going on vacation...
> The e-mail address should use "@" instead of " at ".
Oh I see! I checked the libcamera archives for examples, and thought
that was intentional haha.
Updated.
> cfg_ is only used here, I suppose to reconfigure the libjpeg encoder for
the
Is this a deprecated comment?
> > 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.
I think I need your help on the indentation format. Any doc or rule I can
study? Thanks!
> 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.
Oh I didn't notice there's such a macro to use. Thanks! I think we can get
rid of the PostProcessorJpeg::createEncoder() then.
> 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.
Originally for each frame we need at least one call to jpeg_set_defaults()
to encode the thumbnail, and we need two now with patch v3, so I thought it
was fine in terms of the complexity, while you're right that I'm not sure
about the complexity of jpeg_set_defaults(), even looking into the
implementation [1]. I can also modify the code not to configure twice per
frame. WDYT?
[1]: https://github.com/kornelski/libjpeg/blob/master/jcparam.c#L268
On Wed, Apr 27, 2022 at 7:44 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220503/16865184/attachment-0001.htm>
More information about the libcamera-devel
mailing list