<div dir="ltr">Thank you Laurent, Kieran, and all for reviewing and helping land the series of patches!</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Feb 10, 2023 at 6:47 AM Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@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">Quoting Laurent Pinchart via libcamera-devel (2023-02-09 21:55:21)<br>
> Hi Harvey,<br>
> <br>
> Thank you for the patch.<br>
> <br>
> On Wed, Feb 08, 2023 at 03:33:19AM +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>
> > 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 | 56 ++++++++++++++++++++++++<br>
> > src/android/jpeg/encoder_jea.h | 31 +++++++++++++<br>
> > src/android/jpeg/meson.build | 10 ++++-<br>
> > src/android/jpeg/post_processor_jpeg.cpp | 8 ++++<br>
> > 6 files changed, 118 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..7880a6bd<br>
> > --- /dev/null<br>
> > +++ b/src/android/jpeg/encoder_jea.cpp<br>
> > @@ -0,0 +1,56 @@<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 *buffer,<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 =<br>
> > + dynamic_cast<const HALFrameBuffer *>(buffer->srcBuffer);<br>
> > +<br>
> > + if (!jpegCompressor_->CompressImageFromHandle(fb->handle(),<br>
> > + *buffer->camera3Buffer,<br>
> > + size_.width, size_.height,<br>
> > + quality, exifData.data(),<br>
> > + exifData.size(),<br>
> > + &outDataSize))<br>
> > + return -EBUSY;<br>
> > +<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..ffe9df27<br>
> > --- /dev/null<br>
> > +++ b/src/android/jpeg/encoder_jea.h<br>
> > @@ -0,0 +1,31 @@<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 *buffer,<br>
> > + libcamera::Span<const uint8_t> exifData,<br>
> > + unsigned int quality) 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..9162576b 100644<br>
> > --- a/src/android/jpeg/meson.build<br>
> > +++ b/src/android/jpeg/meson.build<br>
> > @@ -1,8 +1,14 @@<br>
> > # SPDX-License-Identifier: CC0-1.0<br>
> > <br>
> > android_hal_sources += files([<br>
> > - 'encoder_libjpeg.cpp',<br>
> > 'exif.cpp',<br>
> > + 'encoder_libjpeg.cpp',<br>
> > + 'thumbnailer.cpp',<br>
> > 'post_processor_jpeg.cpp',<br>
> > - 'thumbnailer.cpp'<br>
> > ])<br>
> <br>
> This doesn't seem right, or needed. I'll fix this locally and push the<br>
> series after running it through CTS.<br>
<br>
Feel free to run again, but I ran this through CTS earlier:<br>
<br>
- <a href="https://results.uk.libcamera.org/0.0.4+10-53502641/2023.02.09_15.09.10/test_result.xml" rel="noreferrer" target="_blank">https://results.uk.libcamera.org/0.0.4+10-53502641/2023.02.09_15.09.10/test_result.xml</a><br>
<br>
8 failures, but not a regression - so this is OK with me.<br>
<br>
--<br>
Kieran<br>
<br>
<br>
> <br>
> > +<br>
> > +platform = get_option('android_platform')<br>
> > +if 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 3e676eb4..40261652 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>
> > @@ -46,7 +50,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,<br>
> > <br>
> > thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);<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>