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