| Message ID | 20220426091409.1352047-4-chenghaoyang@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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',
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@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 >
Hi Laurent, Uploaded PATCH v5 and fixed the following: > > > 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 was using gmail to view your comment. Now I use ` https://patchwork.libcamera.org/patch`, and the tabs are correctly aligned. Done. > > 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? Done with only one jpeg_set_defaults() being called per frame. Please check. --------- Thanks! On Tue, May 3, 2022 at 11:29 AM Cheng-Hao Yang <chenghaoyang@chromium.org> wrote: > 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@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 >> >
Hi Harvey, On Wed, Jun 22, 2022 at 08:37:58PM +0800, Cheng-Hao Yang wrote: > Hi Laurent, > > Uploaded PATCH v5 and fixed the following: > > > > > 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 was using gmail to view your comment. Now I use ` > https://patchwork.libcamera.org/patch`, and the tabs are correctly aligned. > Done. I'm afraid I can't really help with getting gmail to display the e-mail contents properly. Maybe you have more leverage than I do there ;-) Jokes aside, I think most people avoid webmail for patch review. While on the topic of e-mails, could you plese try to reply inline in the patch instead of copying content and answers to the beginning ? It gets difficult to get the whole context otherwise. > > > 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? > > Done with only one jpeg_set_defaults() being called per frame. Please check. Thank you. > On Tue, May 3, 2022 at 11:29 AM Cheng-Hao Yang wrote: > > > 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@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 > >> > >
Hi Laurent, On Fri, Sep 2, 2022 at 4:09 AM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Harvey, > > On Wed, Jun 22, 2022 at 08:37:58PM +0800, Cheng-Hao Yang wrote: > > Hi Laurent, > > > > Uploaded PATCH v5 and fixed the following: > > > > > > > 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 was using gmail to view your comment. Now I use ` > > https://patchwork.libcamera.org/patch` > <https://patchwork.libcamera.org/patch>, and the tabs are correctly > aligned. > > Done. > > I'm afraid I can't really help with getting gmail to display the e-mail > contents properly. Maybe you have more leverage than I do there ;-) > Jokes aside, I think most people avoid webmail for patch review. > > While on the topic of e-mails, could you plese try to reply inline in > the patch instead of copying content and answers to the beginning ? It > gets difficult to get the whole context otherwise. > > Right, I'll start to do that from now on. Thanks for letting me know :) > > > > 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? > > > > Done with only one jpeg_set_defaults() being called per frame. Please > check. > > Thank you. > > > On Tue, May 3, 2022 at 11:29 AM Cheng-Hao Yang wrote: > > > > > 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@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 > > >> > > > > > > -- > Regards, > > Laurent Pinchart >
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; + + return internalConfigure(cfg); +} + +int EncoderLibJpeg::internalConfigure(const StreamConfiguration &cfg) { 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_); + 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) +{ + /* 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); + + 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; + +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() +{ + encoder_ = std::make_unique<EncoderLibJpeg>(); +} 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']) + +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" #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') android_camera_metadata_sources = files([ 'metadata/camera_metadata.c',
To add JEA in the next CL, this CL reworks JPEG encoder API and move thumbnail encoding code into EncoderLibJpeg's implementation, as JEA supports that as well. Signed-off-by: Harvey Yang <chenghaoyang at chromium.org> --- 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 +- 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