[libcamera-devel] [PATCH v7 4/6] Move generateThumbnail from PostProcessorJpeg to Encoder
Cheng-Hao Yang
chenghaoyang at chromium.org
Wed Dec 14 10:35:35 CET 2022
Thank you Laurent for the review!
On Wed, Dec 7, 2022 at 12:24 PM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Harvey,
>
> Thank you for the patch.
>
> On Thu, Dec 01, 2022 at 09:27:31AM +0000, Harvey Yang via libcamera-devel
> wrote:
> > From: Harvey Yang <chenghaoyang at chromium.org>
> >
> > In the following patch, generateThumbnail will have a different
> > implementation in the jea encoder. Therefore, this patch moves the
> > generateThumbnail function from PostProcessorJpeg to Encoder.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > ---
> > src/android/jpeg/encoder.h | 5 +
> > src/android/jpeg/encoder_libjpeg.cpp | 122 ++++++++++++++++++-----
> > src/android/jpeg/encoder_libjpeg.h | 28 +++++-
> > src/android/jpeg/post_processor_jpeg.cpp | 54 +---------
> > src/android/jpeg/post_processor_jpeg.h | 11 +-
> > 5 files changed, 130 insertions(+), 90 deletions(-)
> >
> > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
> > index b974d367..7dc1ee27 100644
> > --- a/src/android/jpeg/encoder.h
> > +++ b/src/android/jpeg/encoder.h
> > @@ -22,4 +22,9 @@ public:
> > libcamera::Span<uint8_t> destination,
> > 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;
>
> virtual int generateThumbnail(const libcamera::FrameBuffer &source,
> const libcamera::Size &targetSize,
> unsigned int quality,
> std::vector<unsigned char>
> *thumbnail) = 0;
>
> to be consistent with the coding style. Same below where applicable.
>
>
Done.
> > };
> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp
> b/src/android/jpeg/encoder_libjpeg.cpp
> > index fd62bd9c..cc37fde3 100644
> > --- a/src/android/jpeg/encoder_libjpeg.cpp
> > +++ b/src/android/jpeg/encoder_libjpeg.cpp
> > @@ -71,29 +71,43 @@ const struct JPEGPixelFormatInfo
> &findPixelInfo(const PixelFormat &format)
> > EncoderLibJpeg::EncoderLibJpeg()
> > {
> > /* \todo Expand error handling coverage with a custom handler. */
> > - compress_.err = jpeg_std_error(&jerr_);
> > + image_data_.compress.err = jpeg_std_error(&image_data_.jerr);
> > + thumbnail_data_.compress.err =
> jpeg_std_error(&thumbnail_data_.jerr);
> >
> > - jpeg_create_compress(&compress_);
> > + jpeg_create_compress(&image_data_.compress);
> > + jpeg_create_compress(&thumbnail_data_.compress);
> > }
> >
> > EncoderLibJpeg::~EncoderLibJpeg()
> > {
> > - jpeg_destroy_compress(&compress_);
> > + jpeg_destroy_compress(&image_data_.compress);
> > + jpeg_destroy_compress(&thumbnail_data_.compress);
> > }
> >
> > int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
> > {
> > - const struct JPEGPixelFormatInfo info =
> findPixelInfo(cfg.pixelFormat);
> > + thumbnailer_.configure(cfg.size, cfg.pixelFormat);
> > +
> > + return configureEncoder(&image_data_.compress, cfg.pixelFormat,
> > + cfg.size);
> > +}
> > +
> > +int EncoderLibJpeg::configureEncoder(struct jpeg_compress_struct
> *compress,
> > + libcamera::PixelFormat pixelFormat,
> > + libcamera::Size size)
> > +{
> > + const struct JPEGPixelFormatInfo info = findPixelInfo(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->image_width = size.width;
> > + compress->image_height = size.height;
> > + compress->in_color_space = info.colorSpace;
> >
> > - compress_.input_components = info.colorSpace == JCS_GRAYSCALE ? 1
> : 3;
> > + compress->input_components = info.colorSpace == JCS_GRAYSCALE ? 1
> : 3;
> >
> > - jpeg_set_defaults(&compress_);
> > + jpeg_set_defaults(compress);
> >
> > pixelFormatInfo_ = &info.pixelFormatInfo;
> >
> > @@ -107,13 +121,13 @@ void EncoderLibJpeg::compressRGB(const
> std::vector<Span<uint8_t>> &planes)
> > {
> > unsigned char *src = const_cast<unsigned char *>(planes[0].data());
> > /* \todo Stride information should come from buffer configuration.
> */
> > - unsigned int stride =
> pixelFormatInfo_->stride(compress_.image_width, 0);
> > + 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);
> > + while (compress_->next_scanline < compress_->image_height) {
> > + row_pointer[0] = &src[compress_->next_scanline * stride];
> > + jpeg_write_scanlines(compress_, row_pointer, 1);
> > }
> > }
> >
> > @@ -123,7 +137,7 @@ void EncoderLibJpeg::compressRGB(const
> std::vector<Span<uint8_t>> &planes)
> > */
> > void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>>
> &planes)
> > {
> > - uint8_t tmprowbuf[compress_.image_width * 3];
> > + uint8_t tmprowbuf[compress_->image_width * 3];
> >
> > /*
> > * \todo Use the raw api, and only unpack the cb/cr samples to new
> line
> > @@ -133,10 +147,10 @@ void EncoderLibJpeg::compressNV(const
> std::vector<Span<uint8_t>> &planes)
> > * 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 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 horzSubSample = 2 * compress_->image_width / c_stride;
> > unsigned int vertSubSample =
> pixelFormatInfo_->planes[1].verticalSubSampling;
> >
> > unsigned int c_inc = horzSubSample == 1 ? 2 : 0;
> > @@ -149,14 +163,14 @@ void EncoderLibJpeg::compressNV(const
> std::vector<Span<uint8_t>> &planes)
> > JSAMPROW row_pointer[1];
> > row_pointer[0] = &tmprowbuf[0];
> >
> > - for (unsigned int y = 0; y < compress_.image_height; y++) {
> > + for (unsigned int y = 0; y < compress_->image_height; y++) {
> > unsigned char *dst = &tmprowbuf[0];
> >
> > const unsigned char *src_y = src + y * y_stride;
> > 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) {
> > + for (unsigned int x = 0; x < compress_->image_width; x +=
> 2) {
> > dst[0] = *src_y;
> > dst[1] = *src_cb;
> > dst[2] = *src_cr;
> > @@ -174,13 +188,67 @@ void EncoderLibJpeg::compressNV(const
> std::vector<Span<uint8_t>> &planes)
> > dst += 3;
> > }
> >
> > - jpeg_write_scanlines(&compress_, row_pointer, 1);
> > + jpeg_write_scanlines(compress_, row_pointer, 1);
> > }
> > }
> >
> > +int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer
> &source,
> > + const libcamera::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);
> > +
> > + int ret = configureEncoder(&thumbnail_data_.compress,
> > + thumbnailer_.pixelFormat(), targetSize);
> > + compress_ = &thumbnail_data_.compress;
>
> This is all becoming a bit of spaghetti code with the compress_ pointer.
> It feels like you're bundling two classes into one. It would be cleaner,
> I think, to create an internal JpegEncoder class (similar to your
> JpegData, I'd name it EncoderLibJpeg::Encoder) that handles one encoder
> (moving all the private data members of EncoderLibJpeg to that class).
> You could then instantiate it twice as private data members of
> EncoderLibJpeg. What do you think ? The refactoring of EncoderLibJpeg
> should go in first, before moving the thumbnailer to it.
>
>
Yes, you're absolutely right. This version actually has conflicts in
configuring
the capture and thumbnail with shared member variables.
Updated with your suggestion that EncoderLibJpeg is a wrapper that consists
of two real encoders for captures and thumbnails respectively.
I also separate this patch into two: the first adds the internal encoder,
and the
second moves the thumbnailer and add the |thumbnailEncoder_| in
EncoderLibJpeg.
Please check if I missed anything.
> > +
> > + 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);
>
> camelCase (I know it was like this already, but let's fix it).
>
>
Done.
> > + thumbnail->resize(jpeg_size);
> > +
> > + LOG(JPEG, Debug)
> > + << "Thumbnail compress returned "
> > + << jpeg_size << " bytes";
> > +
> > + return jpeg_size;
> > + }
> > +
> > + return -1;
>
> Also while at it, retur
>
>
`return -EINVAL;`?
> > +}
> > +
> > int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t>
> dest,
> > Span<const uint8_t> exifData, unsigned int
> quality)
> > {
> > + compress_ = &image_data_.compress;
> > +
> > MappedFrameBuffer frame(&source, MappedFrameBuffer::MapFlag::Read);
> > if (!frame.isValid()) {
> > LOG(JPEG, Error) << "Failed to map FrameBuffer : "
> > @@ -198,7 +266,7 @@ int EncoderLibJpeg::encode(const
> std::vector<Span<uint8_t>> &src,
> > unsigned char *destination = dest.data();
> > unsigned long size = dest.size();
> >
> > - jpeg_set_quality(&compress_, quality, TRUE);
> > + jpeg_set_quality(compress_, quality, TRUE);
> >
> > /*
> > * The jpeg_mem_dest will reallocate if the required size is not
> > @@ -208,18 +276,18 @@ int EncoderLibJpeg::encode(const
> std::vector<Span<uint8_t>> &src,
> > * \todo Implement our own custom memory destination to prevent
> > * reallocation and prefer failure with correct reporting.
> > */
> > - jpeg_mem_dest(&compress_, &destination, &size);
> > + jpeg_mem_dest(compress_, &destination, &size);
> >
> > - jpeg_start_compress(&compress_, TRUE);
> > + jpeg_start_compress(compress_, TRUE);
> >
> > if (exifData.size())
> > /* Store Exif data in the JPEG_APP1 data block. */
> > - jpeg_write_marker(&compress_, JPEG_APP0 + 1,
> > + jpeg_write_marker(compress_, JPEG_APP0 + 1,
> > static_cast<const JOCTET
> *>(exifData.data()),
> > exifData.size());
> >
> > - LOG(JPEG, Debug) << "JPEG Encode Starting:" <<
> compress_.image_width
> > - << "x" << compress_.image_height;
> > + LOG(JPEG, Debug) << "JPEG Encode Starting:" <<
> compress_->image_width
> > + << "x" << compress_->image_height;
> >
> > ASSERT(src.size() == pixelFormatInfo_->numPlanes());
> >
> > @@ -228,7 +296,7 @@ int EncoderLibJpeg::encode(const
> std::vector<Span<uint8_t>> &src,
> > else
> > compressRGB(src);
> >
> > - jpeg_finish_compress(&compress_);
> > + jpeg_finish_compress(compress_);
> >
> > return size;
> > }
> > diff --git a/src/android/jpeg/encoder_libjpeg.h
> b/src/android/jpeg/encoder_libjpeg.h
> > index 1b3ac067..1ec2ba13 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:
> > @@ -26,20 +28,40 @@ public:
> > libcamera::Span<uint8_t> destination,
> > 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;
>
> This patch would be easier to review if it didn't bundle a change of
> return type for this function. Furthermore, it should be explained in
> the commit message. Is the return type change needed here, or later in
> the series ? If later in the series, I'd split it to a separate patch.
> In general, please try to have only one change per patch to simplify
> review.
>
>
Right, the return value is actually not needed. Removed.
> > +
> > +private:
> > + struct JpegData {
> > + struct jpeg_compress_struct compress;
> > + struct jpeg_error_mgr jerr;
> > + };
> > +
> > + int configureEncoder(struct jpeg_compress_struct *compress,
> > + libcamera::PixelFormat pixelFormat,
> > + libcamera::Size size);
> > +
> > 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);
> >
> > - struct jpeg_compress_struct compress_;
> > - struct jpeg_error_mgr jerr_;
> > + JpegData image_data_;
> > + JpegData thumbnail_data_;
>
> camelCase for both.
>
>
Removed.
> > +
> > + // |&image_data_.compress| or |&thumbnail_data_.compress|.
>
> C-style comments (/* ... */).
>
> > + struct jpeg_compress_struct *compress_;
> >
> > const libcamera::PixelFormatInfo *pixelFormatInfo_;
> >
> > bool nv_;
> > bool nvSwap_;
> > +
> > + Thumbnailer thumbnailer_;
> > };
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp
> b/src/android/jpeg/post_processor_jpeg.cpp
> > index 0cf56716..60eebb11 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -44,60 +44,11 @@ int PostProcessorJpeg::configure(const
> StreamConfiguration &inCfg,
> >
> > streamSize_ = outCfg.size;
> >
> > - thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> > -
> > encoder_ = std::make_unique<EncoderLibJpeg>();
> >
> > 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,9 @@ 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())
>
> Do you need to check both conditions ? Also, the generateThumbnail()
> function returns -1 in case of error or the JPEG data size otherwise,
> while you only check ret > 0 here. I'd return a bool from the function
> and check that only, moving the thumbnail.empty() check (if needed)
> inside generateThumbnail() to simplify the API.
>
>
Removed the return value.
> > exif.setThumbnail(std::move(thumbnail),
> Exif::Compression::JPEG);
> > }
> >
> > diff --git a/src/android/jpeg/post_processor_jpeg.h
> b/src/android/jpeg/post_processor_jpeg.h
> > index 98309b01..55b23d7d 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"
> > +
>
> You can add a
>
> class Encoder;
>
> declaration instead (just below CameraDevice).
>
> > class CameraDevice;
> >
> > class PostProcessorJpeg : public PostProcessor
> > @@ -25,14 +25,7 @@ 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);
> > -
> > CameraDevice *const cameraDevice_;
> > std::unique_ptr<Encoder> encoder_;
> > libcamera::Size streamSize_;
> > - EncoderLibJpeg thumbnailEncoder_;
> > - Thumbnailer thumbnailer_;
> > };
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221214/93325ca8/attachment.htm>
More information about the libcamera-devel
mailing list