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