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