<div dir="ltr"><div dir="ltr">Thanks Laurent!<br><div><br></div><div>I added some code in |EncoderJea::generateThumbnail| to fix</div><div>a memory issue (that JEA needs the memory of the frame to</div><div>be consecutive).</div><div><br></div><div>Please check again and I'll add your reviewed tag. Thanks again!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Dec 7, 2022 at 1:12 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Harvey,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Dec 01, 2022 at 09:27:33AM +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 Lib Jpeg in cros platform,<br>
<br>
s/Lib Jpeg/libjpeg/<br>
s/cros/CrOS/<br>
<br></blockquote><div><br></div><div>Done.</div><div>  </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> where HW accelerator is available.<br>
<br>
s/HW/hardware/<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> 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         |  2 +<br>
>  src/android/cros_mojo_token.h            | 12 +++<br>
>  src/android/jpeg/encoder_jea.cpp         | 93 ++++++++++++++++++++++++<br>
>  src/android/jpeg/encoder_jea.h           | 35 +++++++++<br>
>  src/android/jpeg/meson.build             | 12 ++-<br>
>  src/android/jpeg/post_processor_jpeg.cpp |  8 ++<br>
>  6 files changed, 160 insertions(+), 2 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..d75afccb 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>
<br>
You can drop [[maybe_unused]].<br>
<br></blockquote><div><br></div><div>Done.</div><div>  </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
>  {<br>
> +     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..66a854c4<br>
> --- /dev/null<br>
> +++ b/src/android/jpeg/encoder_jea.cpp<br>
> @@ -0,0 +1,93 @@<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>
It looks like we could avoid subclassing FrameBuffer (basically dropping<br>
patches 1/6 and 2/6) if we stored the handle in the StreamBuffer class<br>
instead of in HALFrameBuffer. We can keep it as-is for now, I'm<br>
interested in experimenting with inheriting from FrameBuffer, but maybe<br>
that will change in the future.<br>
<br></blockquote><div><br></div><div>Seems cool, while I'm not sure how to elegantly pass the handle from </div><div>|PlatformFrameBufferAllocator| :p<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +     if (!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>
> +     return outDataSize;<br>
> +}<br>
> +<br>
> +int EncoderJea::generateThumbnail(const libcamera::FrameBuffer &source,<br>
> +                               const libcamera::Size &targetSize,<br>
> +                               unsigned int quality,<br>
> +                               std::vector<unsigned char> *thumbnail)<br>
> +{<br>
> +     if (!jpegCompressor_)<br>
> +             return -ENOTSUP;<br>
> +<br>
> +     libcamera::MappedFrameBuffer frame(&source,<br>
> +                                        libcamera::MappedFrameBuffer::MapFlag::Read);<br>
> +<br>
> +     if (frame.planes().empty())<br>
> +             return -ENODATA;<br>
> +<br>
> +     uint32_t outDataSize = 0;<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>
C-style comments.<br>
<br></blockquote><div><br></div><div>Do you mean I should wrap it with '/*' & '*/' ? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +     const int kApp1MaxDataSize = 65532;<br>
<br>
constexpr<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +     thumbnail->resize(kApp1MaxDataSize);<br>
> +<br>
> +     if (!jpegCompressor_->GenerateThumbnail(frame.planes()[0].data(),<br>
> +                                             size_.width, size_.height,<br>
> +                                             targetSize.width,<br>
> +                                             targetSize.height, quality,<br>
> +                                             thumbnail->size(),<br>
> +                                             thumbnail->data(),<br>
> +                                             &outDataSize)) {<br>
> +             thumbnail->clear();<br>
> +             return -EBUSY;<br>
> +     }<br>
> +<br>
> +     thumbnail->resize(outDataSize);<br>
> +     return 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..2eba31c2<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>
> +     int 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..d25016b1 100644<br>
> --- a/src/android/jpeg/meson.build<br>
> +++ b/src/android/jpeg/meson.build<br>
> @@ -1,8 +1,16 @@<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(['encoder_libjpeg.cpp',<br>
> +                                  'thumbnailer.cpp'<br>
> +    ])<br>
<br>
    android_hal_sources += files([<br>
        'encoder_libjpeg.cpp',<br>
        'thumbnailer.cpp',<br>
    ])<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +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 10ac4666..15115424 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>
<br>
C-style comments.<br>
<br></blockquote><div><br></div><div>Sorry, I don't get what you mean. Could you elaborate? :p</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">
>  #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>
<br>
Same here.<br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<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>