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