[libcamera-devel] [PATCH v2 2/2] android: jpeg: Port to PostProcessor interface
Hirokazu Honda
hiroh at chromium.org
Mon Oct 19 07:59:32 CEST 2020
On Mon, Oct 19, 2020 at 2:43 PM Hirokazu Honda <hiroh at chromium.org> wrote:
>
> On Fri, Oct 16, 2020 at 7:25 PM Kieran Bingham
> <kieran.bingham at ideasonboard.com> wrote:
> >
> > Hi Umang, Laurent,
> >
> > On 16/10/2020 08:15, Umang Jain wrote:
> > > Hi,
> > >
> > > On 10/16/20 12:02 PM, Laurent Pinchart wrote:
> > >> Hi Umang,
> > >>
> > >> Thank you for the patch.
> > >>
> > >> On Fri, Oct 16, 2020 at 11:07:54AM +0530, Umang Jain wrote:
> > >>> Port the CameraStream's JPEG-encoding bits to PostProcessorJpeg.
> > >>> This encapsulates the encoder and EXIF generation code into the
> > >>> PostProcessorJpeg layer and removes these specifics related to JPEG,
> > >>> from the CameraStream itself.
> > >>>
> > >>> Signed-off-by: Umang Jain <email at uajain.com>
> > >>> Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >>
> > >> Kieran, would you be able to test this new version, and push it if all
> > >> looks good to you ?
> > > Yes please, one final testing should be done before actual pushing.
> > > Thanks for the reviews. Thanks to Kieran for removing blockers on this
> > > and testing efforts :)
> >
> > Ok - I had a few hit and misses due to apparent hardware failure (the
> > camera didn't power up, now up with a full power cycle) - and then the
> > cros build not actually building the code.
> >
> > all of that is solved, and confirmed that it's definitely Umang's code
> > running on the device ... successfully ;-)
> >
> > I'll push these patches now.
> >
> > Thanks
> >
> > Kieran
> >
> >
> > >>
> > >>> ---
> > >>> src/android/camera_device.cpp | 1 +
> > >>> src/android/camera_stream.cpp | 77 ++++-------------
> > >>> src/android/camera_stream.h | 4 +-
> > >>> src/android/jpeg/encoder_libjpeg.cpp | 2 +-
> > >>> src/android/jpeg/post_processor_jpeg.cpp | 105 +++++++++++++++++++++++
> > >>> src/android/jpeg/post_processor_jpeg.h | 36 ++++++++
> > >>> src/android/meson.build | 1 +
> > >>> 7 files changed, 164 insertions(+), 62 deletions(-)
> > >>> create mode 100644 src/android/jpeg/post_processor_jpeg.cpp
> > >>> create mode 100644 src/android/jpeg/post_processor_jpeg.h
> > >>>
> > >>> diff --git a/src/android/camera_device.cpp
> > >>> b/src/android/camera_device.cpp
> > >>> index 0983232..d706cf4 100644
> > >>> --- a/src/android/camera_device.cpp
> > >>> +++ b/src/android/camera_device.cpp
> > >>> @@ -7,6 +7,7 @@
> > >>> #include "camera_device.h"
> > >>> #include "camera_ops.h"
> > >>> +#include "post_processor.h"
> > >>> #include <sys/mman.h>
> > >>> #include <tuple>
> > >>> diff --git a/src/android/camera_stream.cpp
> > >>> b/src/android/camera_stream.cpp
> > >>> index d8e45c2..1b8afa8 100644
> > >>> --- a/src/android/camera_stream.cpp
> > >>> +++ b/src/android/camera_stream.cpp
> > >>> @@ -9,9 +9,9 @@
> > >>> #include "camera_device.h"
> > >>> #include "camera_metadata.h"
> > >>> -#include "jpeg/encoder.h"
> > >>> -#include "jpeg/encoder_libjpeg.h"
> > >>> -#include "jpeg/exif.h"
> > >>> +#include "jpeg/post_processor_jpeg.h"
> > >>> +
> > >>> +#include <libcamera/formats.h>
> > >>> using namespace libcamera;
> > >>> @@ -45,8 +45,15 @@ CameraStream::CameraStream(CameraDevice
> > >>> *cameraDevice, Type type,
> > >>> {
> > >>> config_ = cameraDevice_->cameraConfiguration();
> > >>> - if (type_ == Type::Internal || type_ == Type::Mapped)
> > >>> - encoder_ = std::make_unique<EncoderLibJpeg>();
> > >>> + if (type_ == Type::Internal || type_ == Type::Mapped) {
> > >>> + /*
> > >>> + * \todo There might be multiple post-processors. The logic
> > >>> + * which should be instantiated here, is deferred for the
> > >>> + * future. For now, we only have PostProcessorJpeg and that
> > >>> + * is what we instantiate here.
> > >>> + */
> > >>> + postProcessor_ =
> > >>> std::make_unique<PostProcessorJpeg>(cameraDevice_);
> > >>> + }
> > >>> if (type == Type::Internal) {
> > >>> allocator_ =
> > >>> std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> > >>> @@ -66,8 +73,10 @@ Stream *CameraStream::stream() const
> > >>> int CameraStream::configure()
> > >>> {
> > >>> - if (encoder_) {
> > >>> - int ret = encoder_->configure(configuration());
> > >>> + if (postProcessor_) {
> > >>> + StreamConfiguration output = configuration();
> > >>> + output.pixelFormat = formats::MJPEG;
> > >>> + int ret = postProcessor_->configure(configuration(), output);
>
> Hmm, this is strange to me.
> |config_| should be used here and |Config::Size| should be set as well
> as Format?
>
Ah, sorry. I missed configuration() function in this class.
This implementation is correct. Sorry about that.
> > >>> if (ret)
> > >>> return ret;
> > >>> }
> > >>> @@ -90,60 +99,10 @@ int CameraStream::configure()
> > >>> int CameraStream::process(const libcamera::FrameBuffer &source,
> > >>> MappedCamera3Buffer *dest, CameraMetadata *metadata)
> > >>> {
> > >>> - if (!encoder_)
> > >>> + if (!postProcessor_)
> > >>> return 0;
> > >>> - /* Set EXIF metadata for various tags. */
> > >>> - Exif exif;
> > >>> - /* \todo Set Make and Model from external vendor tags. */
> > >>> - exif.setMake("libcamera");
> > >>> - exif.setModel("cameraModel");
> > >>> - exif.setOrientation(cameraDevice_->orientation());
> > >>> - exif.setSize(configuration().size);
> > >>> - /*
> > >>> - * We set the frame's EXIF timestamp as the time of encode.
> > >>> - * Since the precision we need for EXIF timestamp is only one
> > >>> - * second, it is good enough.
> > >>> - */
> > >>> - exif.setTimestamp(std::time(nullptr));
> > >>> - if (exif.generate() != 0)
> > >>> - LOG(HAL, Error) << "Failed to generate valid EXIF data";
> > >>> -
> > >>> - int jpeg_size = encoder_->encode(&source, dest->maps()[0],
> > >>> exif.data());
> > >>> - if (jpeg_size < 0) {
> > >>> - LOG(HAL, Error) << "Failed to encode stream image";
> > >>> - return jpeg_size;
> > >>> - }
> > >>> -
> > >>> - /*
> > >>> - * Fill in the JPEG blob header.
> > >>> - *
> > >>> - * The mapped size of the buffer is being returned as
> > >>> - * substantially larger than the requested JPEG_MAX_SIZE
> > >>> - * (which is referenced from maxJpegBufferSize_). Utilise
> > >>> - * this static size to ensure the correct offset of the blob is
> > >>> - * determined.
> > >>> - *
> > >>> - * \todo Investigate if the buffer size mismatch is an issue or
> > >>> - * expected behaviour.
> > >>> - */
> > >>> - uint8_t *resultPtr = dest->maps()[0].data() +
> > >>> - cameraDevice_->maxJpegBufferSize() -
> > >>> - sizeof(struct camera3_jpeg_blob);
> > >>> - auto *blob = reinterpret_cast<struct camera3_jpeg_blob
> > >>> *>(resultPtr);
> > >>> - blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
> > >>> - blob->jpeg_size = jpeg_size;
> > >>> -
> > >>> - /* Update the JPEG result Metadata. */
> > >>> - metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> > >>> -
> > >>> - const uint32_t jpeg_quality = 95;
> > >>> - metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
> > >>> -
> > >>> - const uint32_t jpeg_orientation = 0;
> > >>> - metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> > >>> -
> > >>> - return 0;
> > >>> + return postProcessor_->process(&source, dest->maps()[0], metadata);
> > >>> }
> > >>> FrameBuffer *CameraStream::getBuffer()
> > >>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > >>> index d3925fb..c367a5f 100644
> > >>> --- a/src/android/camera_stream.h
> > >>> +++ b/src/android/camera_stream.h
> > >>> @@ -19,10 +19,10 @@
> > >>> #include <libcamera/geometry.h>
> > >>> #include <libcamera/pixel_format.h>
> > >>> -class Encoder;
> > >>> class CameraDevice;
> > >>> class CameraMetadata;
> > >>> class MappedCamera3Buffer;
> > >>> +class PostProcessor;
> > >>> class CameraStream
> > >>> {
> > >>> @@ -130,7 +130,6 @@ private:
> > >>> camera3_stream_t *camera3Stream_;
> > >>> unsigned int index_;
> > >>> - std::unique_ptr<Encoder> encoder_;
> > >>> std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
> > >>> std::vector<libcamera::FrameBuffer *> buffers_;
> > >>> /*
> > >>> @@ -138,6 +137,7 @@ private:
> > >>> * an std::vector in CameraDevice.
> > >>> */
> > >>> std::unique_ptr<std::mutex> mutex_;
> > >>> + std::unique_ptr<PostProcessor> postProcessor_;
> > >>> };
> > >>> #endif /* __ANDROID_CAMERA_STREAM__ */
> > >>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp
> > >>> b/src/android/jpeg/encoder_libjpeg.cpp
> > >>> index a77f5b2..8995ba5 100644
> > >>> --- a/src/android/jpeg/encoder_libjpeg.cpp
> > >>> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> > >>> @@ -25,7 +25,7 @@
> > >>> using namespace libcamera;
> > >>> -LOG_DEFINE_CATEGORY(JPEG)
> > >>> +LOG_DECLARE_CATEGORY(JPEG);
> > >>> namespace {
> > >>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp
> > >>> b/src/android/jpeg/post_processor_jpeg.cpp
> > >>> new file mode 100644
> > >>> index 0000000..753c28e
> > >>> --- /dev/null
> > >>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > >>> @@ -0,0 +1,105 @@
> > >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > >>> +/*
> > >>> + * Copyright (C) 2020, Google Inc.
> > >>> + *
> > >>> + * post_processor_jpeg.cpp - JPEG Post Processor
> > >>> + */
> > >>> +
> > >>> +#include "post_processor_jpeg.h"
> > >>> +
> > >>> +#include "../camera_device.h"
> > >>> +#include "../camera_metadata.h"
> > >>> +#include "encoder_libjpeg.h"
> > >>> +#include "exif.h"
> > >>> +
> > >>> +#include <libcamera/formats.h>
> > >>> +
> > >>> +#include "libcamera/internal/log.h"
> > >>> +
> > >>> +using namespace libcamera;
> > >>> +
> > >>> +LOG_DEFINE_CATEGORY(JPEG);
> > >>> +
> > >>> +PostProcessorJpeg::PostProcessorJpeg(CameraDevice *device)
> > >>> + : cameraDevice_(device)
> > >>> +{
> > >>> +}
> > >>> +
> > >>> +int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> > >>> + const StreamConfiguration &outCfg)
> > >>> +{
> > >>> + if (inCfg.size != outCfg.size) {
> > >>> + LOG(JPEG, Error) << "Mismatch of input and output stream
> > >>> sizes";
> > >>> + return -EINVAL;
> > >>> + }
> > >>> +
> > >>> + if (outCfg.pixelFormat != formats::MJPEG) {
> > >>> + LOG(JPEG, Error) << "Output stream pixel format is not JPEG";
> > >>> + return -EINVAL;
> > >>> + }
> > >>> +
> > >>> + streamSize_ = outCfg.size;
> > >>> + encoder_ = std::make_unique<EncoderLibJpeg>();
> > >>> +
> > >>> + return encoder_->configure(inCfg);
> > >>> +}
> > >>> +
> > >>> +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
> > >>> + const libcamera::Span<uint8_t> &destination,
> > >>> + CameraMetadata *metadata)
> > >>> +{
> > >>> + if (!encoder_)
> > >>> + return 0;
> > >>> +
> > >>> + /* Set EXIF metadata for various tags. */
> > >>> + Exif exif;
> > >>> + /* \todo Set Make and Model from external vendor tags. */
> > >>> + exif.setMake("libcamera");
> > >>> + exif.setModel("cameraModel");
> > >>> + exif.setOrientation(cameraDevice_->orientation());
> > >>> + exif.setSize(streamSize_);
> > >>> + /*
> > >>> + * We set the frame's EXIF timestamp as the time of encode.
> > >>> + * Since the precision we need for EXIF timestamp is only one
> > >>> + * second, it is good enough.
> > >>> + */
> > >>> + exif.setTimestamp(std::time(nullptr));
> > >>> + if (exif.generate() != 0)
> > >>> + LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>
> Shall we handle this case?
>
> > >>> +
> > >>> + int jpeg_size = encoder_->encode(source, destination, exif.data());
> > >>> + if (jpeg_size < 0) {
> > >>> + LOG(JPEG, Error) << "Failed to encode stream image";
> > >>> + return jpeg_size;
> > >>> + }
> > >>> +
> > >>> + /*
> > >>> + * Fill in the JPEG blob header.
> > >>> + *
> > >>> + * The mapped size of the buffer is being returned as
> > >>> + * substantially larger than the requested JPEG_MAX_SIZE
> > >>> + * (which is referenced from maxJpegBufferSize_). Utilise
> > >>> + * this static size to ensure the correct offset of the blob is
> > >>> + * determined.
> > >>> + *
> > >>> + * \todo Investigate if the buffer size mismatch is an issue or
> > >>> + * expected behaviour.
> > >>> + */
> > >>> + uint8_t *resultPtr = destination.data() +
> > >>> + cameraDevice_->maxJpegBufferSize() -
> > >>> + sizeof(struct camera3_jpeg_blob);
> > >>> + auto *blob = reinterpret_cast<struct camera3_jpeg_blob
> > >>> *>(resultPtr);
> > >>> + blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
> > >>> + blob->jpeg_size = jpeg_size;
> > >>> +
> > >>> + /* Update the JPEG result Metadata. */
> > >>> + metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> > >>> +
> > >>> + const uint32_t jpeg_quality = 95;
> > >>> + metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
> > >>> +
> > >>> + const uint32_t jpeg_orientation = 0;
> > >>> + metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> > >>> +
> > >>> + return 0;
> > >>> +}
> > >>> diff --git a/src/android/jpeg/post_processor_jpeg.h
> > >>> b/src/android/jpeg/post_processor_jpeg.h
> > >>> new file mode 100644
> > >>> index 0000000..62c8650
> > >>> --- /dev/null
> > >>> +++ b/src/android/jpeg/post_processor_jpeg.h
> > >>> @@ -0,0 +1,36 @@
> > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > >>> +/*
> > >>> + * Copyright (C) 2020, Google Inc.
> > >>> + *
> > >>> + * post_processor_jpeg.h - JPEG Post Processor
> > >>> + */
> > >>> +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__
> > >>> +#define __ANDROID_POST_PROCESSOR_JPEG_H__
> > >>> +
> > >>> +#include "../post_processor.h"
> > >>> +
> > >>> +#include <libcamera/geometry.h>
> > >>> +
> > >>> +#include "libcamera/internal/buffer.h"
> > >>> +
> > >>> +class Encoder;
> > >>> +class CameraDevice;
> > >>> +
> > >>> +class PostProcessorJpeg : public PostProcessor
> > >>> +{
> > >>> +public:
> > >>> + PostProcessorJpeg(CameraDevice *device);
> > >>> +
> > >>> + int configure(const libcamera::StreamConfiguration &incfg,
> > >>> + const libcamera::StreamConfiguration &outcfg) override;
> > >>> + int process(const libcamera::FrameBuffer *source,
> > >>> + const libcamera::Span<uint8_t> &destination,
> > >>> + CameraMetadata *metadata) override;
> > >>> +
> > >>> +private:
> > >>> + CameraDevice *cameraDevice_;
>
> nit: CameraDevice const *cameraDevice_ to represent |cameraDevice_| is
> owned by a client.
>
> > >>> + std::unique_ptr<Encoder> encoder_;
> > >>> + libcamera::Size streamSize_;
> > >>> +};
> > >>> +
> > >>> +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
> > >>> diff --git a/src/android/meson.build b/src/android/meson.build
> > >>> index b2b2293..5a01bea 100644
> > >>> --- a/src/android/meson.build
> > >>> +++ b/src/android/meson.build
> > >>> @@ -24,6 +24,7 @@ android_hal_sources = files([
> > >>> 'camera_worker.cpp',
> > >>> 'jpeg/encoder_libjpeg.cpp',
> > >>> 'jpeg/exif.cpp',
> > >>> + 'jpeg/post_processor_jpeg.cpp',
> > >>> ])
> > >>> android_camera_metadata_sources = files([
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards
> > --
> > Kieran
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list