<div dir="ltr">Thanks Laurent!<div><br></div><div><div>Sorry I forgot to send this reply before going on vacation...</div><div>And I should also reply to libcamera-devel as well.</div><div><br></div></div><span class="gmail-im" style="color:rgb(80,0,80)"><div>> Could you pick an error code instead of -1 ? Same below.</div></span><div>Not sure which error code to use when |jpegCompressor_| fails to do the compression. Using -EBUSY for now.</div><div><br></div><div>Followed mostly your suggestions. Please check if I miss anything.</div><div><br></div><div>BR,</div><div>Harvey</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 27, 2022 at 7:58 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">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:09AM +0000, Harvey Yang via libcamera-devel wrote:<br>
> This CL adds JEA implementation to replace Lib Jpeg in cros platform,<br>
> where HW accelerator is available.<br>
> <br>
> Signed-off-by: Harvey Yang <chenghaoyang at <a href="http://chromium.org" rel="noreferrer" target="_blank">chromium.org</a>><br>
> ---<br>
> src/android/cros/camera3_hal.cpp | 3 +<br>
> src/android/jpeg/cros_post_processor_jpeg.cpp | 14 ++++<br>
> src/android/jpeg/encoder_jea.cpp | 82 +++++++++++++++++++<br>
> src/android/jpeg/encoder_jea.h | 35 ++++++++<br>
> src/android/jpeg/meson.build | 13 ++-<br>
> 5 files changed, 144 insertions(+), 3 deletions(-)<br>
> create mode 100644 src/android/jpeg/cros_post_processor_jpeg.cpp<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..ea5577f0 100644<br>
> --- a/src/android/cros/camera3_hal.cpp<br>
> +++ b/src/android/cros/camera3_hal.cpp<br>
> @@ -9,8 +9,11 @@<br>
> <br>
> #include "../camera_hal_manager.h"<br>
> <br>
> +cros::CameraMojoChannelManagerToken *g_cros_camera_token = nullptr;<br>
<br>
gCrosCameraToken to match the coding style, but I'd name the variable<br>
gCrosMojoToken to make it clearer it refers to mojo.<br>
<br>
> +<br>
> static void set_up([[maybe_unused]] cros::CameraMojoChannelManagerToken *token)<br>
> {<br>
> + g_cros_camera_token = token;<br>
> }<br>
> <br>
> static void tear_down()<br>
> diff --git a/src/android/jpeg/cros_post_processor_jpeg.cpp b/src/android/jpeg/cros_post_processor_jpeg.cpp<br>
> new file mode 100644<br>
> index 00000000..7020f0d0<br>
> --- /dev/null<br>
> +++ b/src/android/jpeg/cros_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>
> + * cros_post_processor_jpeg.cpp - CrOS JPEG Post Processor<br>
> + */<br>
> +<br>
> +#include "encoder_jea.h"<br>
> +#include "post_processor_jpeg.h"<br>
> +<br>
> +void PostProcessorJpeg::SetEncoder()<br>
> +{<br>
> + encoder_ = std::make_unique<EncoderJea>();<br>
> +}<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..838e8647<br>
> --- /dev/null<br>
> +++ b/src/android/jpeg/encoder_jea.cpp<br>
> @@ -0,0 +1,82 @@<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/base/log.h><br>
> +<br>
> +#include "libcamera/internal/mapped_framebuffer.h"<br>
> +<br>
> +#include <cros-camera/camera_mojo_channel_manager_token.h><br>
> +<br>
> +#include "../android_framebuffer.h"<br>
> +<br>
> +extern cros::CameraMojoChannelManagerToken *g_cros_camera_token;<br>
<br>
Please add this to a header file as appropriate.<br>
<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 (jpeg_compressor_.get())<br>
<br>
You can write<br>
<br>
if (jpeg_compressor_)<br>
<br>
Same below.<br>
<br>
> + return 0;<br>
> +<br>
> + if (g_cros_camera_token == nullptr)<br>
> + return -ENOTSUP;<br>
> +<br>
> + jpeg_compressor_ = cros::JpegCompressor::GetInstance(g_cros_camera_token);<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 (!jpeg_compressor_.get())<br>
> + return -1;<br>
<br>
Could you pick an error code instead of -1 ? Same below.<br>
<br>
> +<br>
> + uint32_t out_data_size = 0;<br>
<br>
outDataSize<br>
<br>
Same below.<br>
<br>
> +<br>
> + if (!jpeg_compressor_->CompressImageFromHandle(<br>
> + dynamic_cast<const AndroidFrameBuffer *>(<br>
> + streamBuffer->srcBuffer)<br>
> + ->getHandle(),<br>
> + *streamBuffer->camera3Buffer, size_.width, size_.height, quality,<br>
> + exifData.data(), exifData.size(), &out_data_size)) {<br>
> + return -1;<br>
> + }<br>
<br>
An intermediate variable would make this easier to read.<br>
<br>
const AndroidFrameBuffer *fb =<br>
dynamic_cast<const AndroidFrameBuffer *>(streamBuffer->srcBuffer);<br>
<br>
if (!jpeg_compressor_->CompressImageFromHandle(fb->getHandle(),<br>
*streamBuffer->camera3Buffer,<br>
size_.width, size_.height, quality,<br>
exifData.data(), exifData.size(),<br>
&out_data_size))<br>
return -1;<br>
<br>
> +<br>
> + return out_data_size;<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 (!jpeg_compressor_.get())<br>
> + return -1;<br>
> +<br>
> + libcamera::MappedFrameBuffer frame(&source, libcamera::MappedFrameBuffer::MapFlag::Read);<br>
> +<br>
> + if (frame.planes().empty())<br>
> + return 0;<br>
<br>
Isn't this an error ?<br>
<br>
> +<br>
> + uint32_t out_data_size = 0;<br>
> +<br>
> + if (!jpeg_compressor_->GenerateThumbnail(frame.planes()[0].data(),<br>
> + size_.width, size_.height, targetSize.width, targetSize.height,<br>
> + quality, thumbnail->size(), thumbnail->data(), &out_data_size)) {<br>
> + return -1;<br>
> + }<br>
<br>
No need for curly braces, and a bit of line wrap would be nice :-)<br>
<br>
> +<br>
> + return out_data_size;<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..d5c9f1f7<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> jpeg_compressor_;<br>
<br>
jpegCompressor_<br>
<br>
or just compressor_ to shorten lines.<br>
<br>
> +};<br>
> diff --git a/src/android/jpeg/meson.build b/src/android/jpeg/meson.build<br>
> index 94522dc0..8606acc4 100644<br>
> --- a/src/android/jpeg/meson.build<br>
> +++ b/src/android/jpeg/meson.build<br>
> @@ -4,6 +4,13 @@ android_hal_sources += files([<br>
> 'exif.cpp',<br>
> 'post_processor_jpeg.cpp'])<br>
> <br>
> -android_hal_sources += files(['encoder_libjpeg.cpp',<br>
> - 'generic_post_processor_jpeg.cpp',<br>
> - 'thumbnailer.cpp'])<br>
> +platform = get_option('android_platform')<br>
> +if platform == 'generic'<br>
> + android_hal_sources += files(['encoder_libjpeg.cpp',<br>
> + 'generic_post_processor_jpeg.cpp',<br>
> + 'thumbnailer.cpp'])<br>
> +elif platform == 'cros'<br>
> + android_hal_sources += files(['cros_post_processor_jpeg.cpp',<br>
> + 'encoder_jea.cpp'])<br>
> + android_deps += [dependency('libcros_camera')]<br>
> +endif<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div>