<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>