[libcamera-devel] [PATCH v8 7/7] Add JEA implementation
Cheng-Hao Yang
chenghaoyang at chromium.org
Tue Jan 17 11:31:36 CET 2023
Hi Laurent,
Thanks for your review and the updated patches!
On Mon, Jan 16, 2023 at 8:08 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Harvey,
>
> Thank you for the patch.
>
> On Wed, Dec 14, 2022 at 09:33:30AM +0000, Harvey Yang via libcamera-devel
> wrote:
> > From: Harvey Yang <chenghaoyang at chromium.org>
> >
> > This patch adds JEA implementation to replace LibJpeg in CrOS platform,
>
> s/LibJpeg/libjpeg/
>
> > where hardware accelerator is available.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > ---
> > src/android/cros/camera3_hal.cpp | 4 +-
> > src/android/cros_mojo_token.h | 12 +++
> > src/android/jpeg/encoder_jea.cpp | 105 +++++++++++++++++++++++
> > src/android/jpeg/encoder_jea.h | 35 ++++++++
> > src/android/jpeg/meson.build | 13 ++-
> > src/android/jpeg/post_processor_jpeg.cpp | 8 ++
> > 6 files changed, 174 insertions(+), 3 deletions(-)
> > create mode 100644 src/android/cros_mojo_token.h
> > create mode 100644 src/android/jpeg/encoder_jea.cpp
> > create mode 100644 src/android/jpeg/encoder_jea.h
> >
> > diff --git a/src/android/cros/camera3_hal.cpp
> b/src/android/cros/camera3_hal.cpp
> > index fb863b5f..71acb441 100644
> > --- a/src/android/cros/camera3_hal.cpp
> > +++ b/src/android/cros/camera3_hal.cpp
> > @@ -8,9 +8,11 @@
> > #include <cros-camera/cros_camera_hal.h>
> >
> > #include "../camera_hal_manager.h"
> > +#include "../cros_mojo_token.h"
> >
> > -static void set_up([[maybe_unused]] cros::CameraMojoChannelManagerToken
> *token)
> > +static void set_up(cros::CameraMojoChannelManagerToken *token)
> > {
> > + gCrosMojoToken = token;
> > }
> >
> > static void tear_down()
> > diff --git a/src/android/cros_mojo_token.h
> b/src/android/cros_mojo_token.h
> > new file mode 100644
> > index 00000000..043c752a
> > --- /dev/null
> > +++ b/src/android/cros_mojo_token.h
> > @@ -0,0 +1,12 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * cros_mojo_token.h - cros-specific mojo token
> > + */
> > +
> > +#pragma once
> > +
> > +#include <cros-camera/cros_camera_hal.h>
> > +
> > +inline cros::CameraMojoChannelManagerToken *gCrosMojoToken = nullptr;
> > diff --git a/src/android/jpeg/encoder_jea.cpp
> b/src/android/jpeg/encoder_jea.cpp
> > new file mode 100644
> > index 00000000..139bde93
> > --- /dev/null
> > +++ b/src/android/jpeg/encoder_jea.cpp
> > @@ -0,0 +1,105 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * encoder_jea.cpp - JPEG encoding using CrOS JEA
> > + */
> > +
> > +#include "encoder_jea.h"
> > +
> > +#include "libcamera/internal/mapped_framebuffer.h"
> > +
> > +#include <cros-camera/camera_mojo_channel_manager_token.h>
> > +
> > +#include "../cros_mojo_token.h"
> > +#include "../hal_framebuffer.h"
> > +
> > +EncoderJea::EncoderJea() = default;
> > +
> > +EncoderJea::~EncoderJea() = default;
> > +
> > +int EncoderJea::configure(const libcamera::StreamConfiguration &cfg)
> > +{
> > + size_ = cfg.size;
> > +
> > + if (jpegCompressor_)
> > + return 0;
> > +
> > + if (gCrosMojoToken == nullptr)
> > + return -ENOTSUP;
> > +
> > + jpegCompressor_ =
> cros::JpegCompressor::GetInstance(gCrosMojoToken);
> > +
> > + return 0;
> > +}
> > +
> > +int EncoderJea::encode(Camera3RequestDescriptor::StreamBuffer
> *streamBuffer,
> > + libcamera::Span<const uint8_t> exifData,
> > + unsigned int quality)
> > +{
> > + if (!jpegCompressor_)
> > + return -ENOTSUP;
> > +
> > + uint32_t outDataSize = 0;
> > + const HALFrameBuffer *fb = dynamic_cast<const HALFrameBuffer *>(
> > + streamBuffer->srcBuffer);
>
> const HALFrameBuffer *fb =
> dynamic_cast<const HALFrameBuffer
> *>(streamBuffer->srcBuffer);
>
> > +
> > + if (!jpegCompressor_->CompressImageFromHandle(fb->handle(),
> > +
> *streamBuffer->camera3Buffer,
> > + size_.width,
> size_.height,
> > + quality,
> exifData.data(),
> > + exifData.size(),
> > + &outDataSize))
> > + return -EBUSY;
>
> Is this the right error code ? What can cause CompressImageFromHandle to
> fail ?
>
>
There might be multiple different possibilities, according to CrOS'
implementation
[1]. Maybe |-EINVAL| is more accurate?
[1]:
https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/common/jpeg_compressor_impl.cc#141
> > +
> > + return outDataSize;
> > +}
> > +
> > +void EncoderJea::generateThumbnail(const libcamera::FrameBuffer &source,
> > + const libcamera::Size &targetSize,
> > + unsigned int quality,
> > + std::vector<unsigned char> *thumbnail)
>
> checkstyle.py indicates an incorrect alignment here.
>
> > +{
> > + if (!jpegCompressor_)
> > + return;
> > +
> > + libcamera::MappedFrameBuffer frame(&source,
> > +
> libcamera::MappedFrameBuffer::MapFlag::Read);
> > +
> > + if (frame.planes().empty())
> > + return;
> > +
> > + // JEA needs consecutive memory.
>
> C-style comment:
>
> /* JEA needs consecutive memory. */
>
> Same in two locations below.
>
> > + unsigned long size = 0, index = 0;
> > + for (const auto& plane : frame.planes())
>
> checkstyle.py indicates that this should be
>
> for (const auto &plane : frame.planes())
>
> > + size += plane.size();
> > +
> > + std::vector<uint8_t> data(size);
> > + for (const auto& plane : frame.planes()) {
>
> Same here.
>
> > + memcpy(&data[index], plane.data(), plane.size());
> > + index += plane.size();
> > + }
>
> That's awful :-( A full-frame memcpy is very costly. Could the JEA API
> be fixed to handle multi-planar formats better ? That's of course not a
> blocker for this series.
>
>
It would actually fail here [2] if we don't make the memory consecutive.
I don't know if it could be fixed properly though...
[2]:
https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/common/jpeg_compressor_impl.cc#310
> I'm curious if memcpy() + JEA is actually faster than using the CPU to
> generate the thumbnail.
>
>
I reckon the answer is no... JEA actually generates the thumbnail in SW
instead of HW [3], as the size of the image would be small.
We could also fall back to using EncoderLibJpeg::Encoder for
generateThumbnail (maybe just implement it in
Encoder::generateThumbnail). WDYT?
[3]:
https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/common/jpeg_compressor_impl.cc#320
> +
> > + uint32_t outDataSize = 0;
> > +
> > + /*
> > + * Since the structure of the App1 segment is like:
> > + * 0xFF [1 byte marker] [2 bytes size] [data]
> > + * And it should not be larger than 64K.
> > + */
> > + constexpr int kApp1MaxDataSize = 65532;
> > + thumbnail->resize(kApp1MaxDataSize);
> > +
> > + if (!jpegCompressor_->GenerateThumbnail(data.data(),
> > + size_.width, size_.height,
> > + targetSize.width,
> > + targetSize.height, quality,
> > + thumbnail->size(),
> > + thumbnail->data(),
> > + &outDataSize)) {
>
> How does the JEA compressor know what pixel format the source image uses
> ?
>
>
I honestly don't know... Maybe it assumes the format to be NV12 [4]?
[4]:
https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/common/jpeg_compressor_impl.cc#619
> > + thumbnail->clear();
> > + return;
> > + }
> > +
> > + thumbnail->resize(outDataSize);
> > +}
> > diff --git a/src/android/jpeg/encoder_jea.h
> b/src/android/jpeg/encoder_jea.h
> > new file mode 100644
> > index 00000000..817b3202
> > --- /dev/null
> > +++ b/src/android/jpeg/encoder_jea.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * encoder_jea.h - JPEG encoding using CrOS JEA
> > + */
> > +
> > +#pragma once
> > +
> > +#include <libcamera/geometry.h>
> > +
> > +#include <cros-camera/jpeg_compressor.h>
> > +
> > +#include "encoder.h"
> > +
> > +class EncoderJea : public Encoder
> > +{
> > +public:
> > + EncoderJea();
> > + ~EncoderJea();
> > +
> > + int configure(const libcamera::StreamConfiguration &cfg) override;
> > + int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> > + libcamera::Span<const uint8_t> exifData,
> > + unsigned int quality) override;
> > + void generateThumbnail(const libcamera::FrameBuffer &source,
> > + const libcamera::Size &targetSize,
> > + unsigned int quality,
> > + std::vector<unsigned char> *thumbnail)
> override;
> > +
> > +private:
> > + libcamera::Size size_;
> > +
> > + std::unique_ptr<cros::JpegCompressor> jpegCompressor_;
> > +};
> > diff --git a/src/android/jpeg/meson.build b/src/android/jpeg/meson.build
> > index 08397a87..2b68f54c 100644
> > --- a/src/android/jpeg/meson.build
> > +++ b/src/android/jpeg/meson.build
> > @@ -1,8 +1,17 @@
> > # SPDX-License-Identifier: CC0-1.0
> >
> > android_hal_sources += files([
> > - 'encoder_libjpeg.cpp',
> > 'exif.cpp',
> > 'post_processor_jpeg.cpp',
> > - 'thumbnailer.cpp'
> > ])
> > +
> > +platform = get_option('android_platform')
> > +if platform == 'generic'
> > + android_hal_sources += files([
> > + 'encoder_libjpeg.cpp',
> > + 'thumbnailer.cpp'
> > + ])
> > +elif platform == 'cros'
> > + android_hal_sources += files(['encoder_jea.cpp'])
> > + android_deps += [dependency('libcros_camera')]
> > +endif
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp
> b/src/android/jpeg/post_processor_jpeg.cpp
> > index 918ab492..3424f8e7 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -12,7 +12,11 @@
> > #include "../camera_device.h"
> > #include "../camera_metadata.h"
> > #include "../camera_request.h"
> > +#if defined(OS_CHROMEOS)
> > +#include "encoder_jea.h"
> > +#else // !defined(OS_CHROMEOS)
> > #include "encoder_libjpeg.h"
> > +#endif
> > #include "exif.h"
> >
> > #include <libcamera/base/log.h>
> > @@ -44,7 +48,11 @@ int PostProcessorJpeg::configure(const
> StreamConfiguration &inCfg,
> >
> > streamSize_ = outCfg.size;
> >
> > +#if defined(OS_CHROMEOS)
> > + encoder_ = std::make_unique<EncoderJea>();
> > +#else // !defined(OS_CHROMEOS)
> > encoder_ = std::make_unique<EncoderLibJpeg>();
> > +#endif
> >
> > return encoder_->configure(inCfg);
> > }
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230117/609454ae/attachment.htm>
More information about the libcamera-devel
mailing list