<div dir="ltr"><div dir="ltr">Hi Laurent,<br></div><div><br></div><div>Thanks for your review and the updated patches!</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jan 16, 2023 at 8:08 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 Wed, Dec 14, 2022 at 09:33:30AM +0000, Harvey Yang via libcamera-devel wrote:<br>
> From: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> <br>
> This patch adds JEA implementation to replace LibJpeg in CrOS platform,<br>
<br>
s/LibJpeg/libjpeg/<br>
<br>
> where hardware accelerator is available.<br>
> <br>
> Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> ---<br>
> src/android/cros/camera3_hal.cpp | 4 +-<br>
> src/android/cros_mojo_token.h | 12 +++<br>
> src/android/jpeg/encoder_jea.cpp | 105 +++++++++++++++++++++++<br>
> src/android/jpeg/encoder_jea.h | 35 ++++++++<br>
> src/android/jpeg/meson.build | 13 ++-<br>
> src/android/jpeg/post_processor_jpeg.cpp | 8 ++<br>
> 6 files changed, 174 insertions(+), 3 deletions(-)<br>
> create mode 100644 src/android/cros_mojo_token.h<br>
> create mode 100644 src/android/jpeg/encoder_jea.cpp<br>
> create mode 100644 src/android/jpeg/encoder_jea.h<br>
> <br>
> diff --git a/src/android/cros/camera3_hal.cpp b/src/android/cros/camera3_hal.cpp<br>
> index fb863b5f..71acb441 100644<br>
> --- a/src/android/cros/camera3_hal.cpp<br>
> +++ b/src/android/cros/camera3_hal.cpp<br>
> @@ -8,9 +8,11 @@<br>
> #include <cros-camera/cros_camera_hal.h><br>
> <br>
> #include "../camera_hal_manager.h"<br>
> +#include "../cros_mojo_token.h"<br>
> <br>
> -static void set_up([[maybe_unused]] cros::CameraMojoChannelManagerToken *token)<br>
> +static void set_up(cros::CameraMojoChannelManagerToken *token)<br>
> {<br>
> + gCrosMojoToken = token;<br>
> }<br>
> <br>
> static void tear_down()<br>
> diff --git a/src/android/cros_mojo_token.h b/src/android/cros_mojo_token.h<br>
> new file mode 100644<br>
> index 00000000..043c752a<br>
> --- /dev/null<br>
> +++ b/src/android/cros_mojo_token.h<br>
> @@ -0,0 +1,12 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Google Inc.<br>
> + *<br>
> + * cros_mojo_token.h - cros-specific mojo token<br>
> + */<br>
> +<br>
> +#pragma once<br>
> +<br>
> +#include <cros-camera/cros_camera_hal.h><br>
> +<br>
> +inline cros::CameraMojoChannelManagerToken *gCrosMojoToken = nullptr;<br>
> diff --git a/src/android/jpeg/encoder_jea.cpp b/src/android/jpeg/encoder_jea.cpp<br>
> new file mode 100644<br>
> index 00000000..139bde93<br>
> --- /dev/null<br>
> +++ b/src/android/jpeg/encoder_jea.cpp<br>
> @@ -0,0 +1,105 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Google Inc.<br>
> + *<br>
> + * encoder_jea.cpp - JPEG encoding using CrOS JEA<br>
> + */<br>
> +<br>
> +#include "encoder_jea.h"<br>
> +<br>
> +#include "libcamera/internal/mapped_framebuffer.h"<br>
> +<br>
> +#include <cros-camera/camera_mojo_channel_manager_token.h><br>
> +<br>
> +#include "../cros_mojo_token.h"<br>
> +#include "../hal_framebuffer.h"<br>
> +<br>
> +EncoderJea::EncoderJea() = default;<br>
> +<br>
> +EncoderJea::~EncoderJea() = default;<br>
> +<br>
> +int EncoderJea::configure(const libcamera::StreamConfiguration &cfg)<br>
> +{<br>
> + size_ = cfg.size;<br>
> +<br>
> + if (jpegCompressor_)<br>
> + return 0;<br>
> +<br>
> + if (gCrosMojoToken == nullptr)<br>
> + return -ENOTSUP;<br>
> +<br>
> + jpegCompressor_ = cros::JpegCompressor::GetInstance(gCrosMojoToken);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +int EncoderJea::encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,<br>
> + libcamera::Span<const uint8_t> exifData,<br>
> + unsigned int quality)<br>
> +{<br>
> + if (!jpegCompressor_)<br>
> + return -ENOTSUP;<br>
> +<br>
> + uint32_t outDataSize = 0;<br>
> + const HALFrameBuffer *fb = dynamic_cast<const HALFrameBuffer *>(<br>
> + streamBuffer->srcBuffer);<br>
<br>
const HALFrameBuffer *fb =<br>
dynamic_cast<const HALFrameBuffer *>(streamBuffer->srcBuffer);<br>
<br>
> +<br>
> + if (!jpegCompressor_->CompressImageFromHandle(fb->handle(),<br>
> + *streamBuffer->camera3Buffer,<br>
> + size_.width, size_.height,<br>
> + quality, exifData.data(),<br>
> + exifData.size(),<br>
> + &outDataSize))<br>
> + return -EBUSY;<br>
<br>
Is this the right error code ? What can cause CompressImageFromHandle to<br>
fail ?<br>
<br></blockquote><div><br></div><div>There might be multiple different possibilities, according to CrOS' implementation</div><div>[1]. Maybe |-EINVAL| is more accurate?</div><div><br></div><div>[1]: <a href="https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/common/jpeg_compressor_impl.cc#141" target="_blank">https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/common/jpeg_compressor_impl.cc#141</a></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + return outDataSize;<br>
> +}<br>
> +<br>
> +void EncoderJea::generateThumbnail(const libcamera::FrameBuffer &source,<br>
> + const libcamera::Size &targetSize,<br>
> + unsigned int quality,<br>
> + std::vector<unsigned char> *thumbnail)<br>
<br>
checkstyle.py indicates an incorrect alignment here.<br>
<br>
> +{<br>
> + if (!jpegCompressor_)<br>
> + return;<br>
> +<br>
> + libcamera::MappedFrameBuffer frame(&source,<br>
> + libcamera::MappedFrameBuffer::MapFlag::Read);<br>
> +<br>
> + if (frame.planes().empty())<br>
> + return;<br>
> +<br>
> + // JEA needs consecutive memory.<br>
<br>
C-style comment:<br>
<br>
/* JEA needs consecutive memory. */<br>
<br>
Same in two locations below.<br>
<br>
> + unsigned long size = 0, index = 0;<br>
> + for (const auto& plane : frame.planes())<br>
<br>
checkstyle.py indicates that this should be<br>
<br>
for (const auto &plane : frame.planes())<br>
<br>
> + size += plane.size();<br>
> +<br>
> + std::vector<uint8_t> data(size);<br>
> + for (const auto& plane : frame.planes()) {<br>
<br>
Same here.<br>
<br>
> + memcpy(&data[index], plane.data(), plane.size());<br>
> + index += plane.size();<br>
> + }<br>
<br>
That's awful :-( A full-frame memcpy is very costly. Could the JEA API<br>
be fixed to handle multi-planar formats better ? That's of course not a<br>
blocker for this series.<br>
<br></blockquote><div><br></div><div>It would actually fail here [2] if we don't make the memory consecutive.<br>I don't know if it could be fixed properly though...</div><div><br></div><div>[2]: <a href="https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/common/jpeg_compressor_impl.cc#310" target="_blank">https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/common/jpeg_compressor_impl.cc#310</a><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I'm curious if memcpy() + JEA is actually faster than using the CPU to<br>
generate the thumbnail.<br>
<br></blockquote><div><br></div><div>I reckon the answer is no... JEA actually generates the thumbnail in SW</div><div>instead of HW [3], as the size of the image would be small.<br>We could also fall back to using EncoderLibJpeg::Encoder for</div><div>generateThumbnail (maybe just implement it in</div><div>Encoder::generateThumbnail). WDYT?</div><div><br></div><div>[3]: <a href="https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/common/jpeg_compressor_impl.cc#320" target="_blank">https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/common/jpeg_compressor_impl.cc#320</a></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + uint32_t outDataSize = 0;<br>
> +<br>
> + /*<br>
> + * Since the structure of the App1 segment is like:<br>
> + * 0xFF [1 byte marker] [2 bytes size] [data]<br>
> + * And it should not be larger than 64K.<br>
> + */<br>
> + constexpr int kApp1MaxDataSize = 65532;<br>
> + thumbnail->resize(kApp1MaxDataSize);<br>
> +<br>
> + if (!jpegCompressor_->GenerateThumbnail(data.data(),<br>
> + size_.width, size_.height,<br>
> + targetSize.width,<br>
> + targetSize.height, quality,<br>
> + thumbnail->size(),<br>
> + thumbnail->data(),<br>
> + &outDataSize)) {<br>
<br>
How does the JEA compressor know what pixel format the source image uses<br>
?<br>
<br></blockquote><div><br></div><div>I honestly don't know... Maybe it assumes the format to be NV12 [4]?<br></div><div><br></div><div>[4]: <a href="https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/common/jpeg_compressor_impl.cc#619">https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/common/jpeg_compressor_impl.cc#619</a></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + thumbnail->clear();<br>
> + return;<br>
> + }<br>
> +<br>
> + thumbnail->resize(outDataSize);<br>
> +}<br>
> diff --git a/src/android/jpeg/encoder_jea.h b/src/android/jpeg/encoder_jea.h<br>
> new file mode 100644<br>
> index 00000000..817b3202<br>
> --- /dev/null<br>
> +++ b/src/android/jpeg/encoder_jea.h<br>
> @@ -0,0 +1,35 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Google Inc.<br>
> + *<br>
> + * encoder_jea.h - JPEG encoding using CrOS JEA<br>
> + */<br>
> +<br>
> +#pragma once<br>
> +<br>
> +#include <libcamera/geometry.h><br>
> +<br>
> +#include <cros-camera/jpeg_compressor.h><br>
> +<br>
> +#include "encoder.h"<br>
> +<br>
> +class EncoderJea : public Encoder<br>
> +{<br>
> +public:<br>
> + EncoderJea();<br>
> + ~EncoderJea();<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>
> + void generateThumbnail(const libcamera::FrameBuffer &source,<br>
> + const libcamera::Size &targetSize,<br>
> + unsigned int quality,<br>
> + std::vector<unsigned char> *thumbnail) override;<br>
> +<br>
> +private:<br>
> + libcamera::Size size_;<br>
> +<br>
> + std::unique_ptr<cros::JpegCompressor> jpegCompressor_;<br>
> +};<br>
> diff --git a/src/android/jpeg/meson.build b/src/android/jpeg/meson.build<br>
> index 08397a87..2b68f54c 100644<br>
> --- a/src/android/jpeg/meson.build<br>
> +++ b/src/android/jpeg/meson.build<br>
> @@ -1,8 +1,17 @@<br>
> # SPDX-License-Identifier: CC0-1.0<br>
> <br>
> android_hal_sources += files([<br>
> - 'encoder_libjpeg.cpp',<br>
> 'exif.cpp',<br>
> 'post_processor_jpeg.cpp',<br>
> - 'thumbnailer.cpp'<br>
> ])<br>
> +<br>
> +platform = get_option('android_platform')<br>
> +if platform == 'generic'<br>
> + android_hal_sources += files([<br>
> + 'encoder_libjpeg.cpp',<br>
> + 'thumbnailer.cpp'<br>
> + ])<br>
> +elif platform == 'cros'<br>
> + android_hal_sources += files(['encoder_jea.cpp'])<br>
> + android_deps += [dependency('libcros_camera')]<br>
> +endif<br>
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp<br>
> index 918ab492..3424f8e7 100644<br>
> --- a/src/android/jpeg/post_processor_jpeg.cpp<br>
> +++ b/src/android/jpeg/post_processor_jpeg.cpp<br>
> @@ -12,7 +12,11 @@<br>
> #include "../camera_device.h"<br>
> #include "../camera_metadata.h"<br>
> #include "../camera_request.h"<br>
> +#if defined(OS_CHROMEOS)<br>
> +#include "encoder_jea.h"<br>
> +#else // !defined(OS_CHROMEOS)<br>
> #include "encoder_libjpeg.h"<br>
> +#endif<br>
> #include "exif.h"<br>
> <br>
> #include <libcamera/base/log.h><br>
> @@ -44,7 +48,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,<br>
> <br>
> streamSize_ = outCfg.size;<br>
> <br>
> +#if defined(OS_CHROMEOS)<br>
> + encoder_ = std::make_unique<EncoderJea>();<br>
> +#else // !defined(OS_CHROMEOS)<br>
> encoder_ = std::make_unique<EncoderLibJpeg>();<br>
> +#endif<br>
> <br>
> return encoder_->configure(inCfg);<br>
> }<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>