[libcamera-devel] [PATCH 2/3] android: jpeg: Port to PostProcessor interface
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Oct 15 21:32:29 CEST 2020
Hi Umang,
On 15/10/2020 18:14, 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.
Fantastic, getting all the JPEG specifics out of CameraStream is really
good.
>
> Signed-off-by: Umang Jain <email at uajain.com>
> ---
> 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 | 110 +++++++++++++++++++++++
> src/android/jpeg/post_processor_jpeg.h | 36 ++++++++
> src/android/meson.build | 1 +
> 7 files changed, 169 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 c29fcb4..d6a8acb 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 eac1480..2a0ba16 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;
>
> @@ -24,8 +24,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 future.
> + * For now, we only have PostProcessorJpeg and that is what we
> + * will instantiate here.
> + */
> + postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
Agreed, we can leave it to the next PostProcessor to decide how we
create these. We've got one - this is it ;-)
I anticipate we might also have to chain them together too, so we'll
need to figure out how to handle multiple PostProcessors feeding into
each other without turning into a full gstreamer-esque pipeline.
(For instance, we'll have a Format Convertor to take YUYV->NV12, and
then an Encoder to take NV12->MJPEG for UVC pipelines)
> + }
>
> if (type == Type::Internal) {
> allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> @@ -45,8 +52,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);
> if (ret)
> return ret;
> }
> @@ -69,60 +78,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);
I wonder if we should move this to the point that calls
CameraStream::process() directly ... but equally, if we have multiple
post-processors we might have to do more operations here ... so that
makes me think we should keep this function for now..
> }
>
> FrameBuffer *CameraStream::getBuffer()
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 8df0101..c55d90b 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
> {
> @@ -135,7 +135,6 @@ private:
> */
> unsigned int index_;
>
> - std::unique_ptr<Encoder> encoder_;
> std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
> std::vector<libcamera::FrameBuffer *> buffers_;
> /*
> @@ -143,6 +142,7 @@ private:
> * an std::vector in CameraDevice.
> */
> std::unique_ptr<std::mutex> mutex_;
Haha, Maybe I would have had the nitpick-fix patch first ;-) It really
stands out here - but either way is fine.
> + 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..d1ec95b
> --- /dev/null
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -0,0 +1,110 @@
> +/* 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);
I'm wondering if we should separate the categories for the JPEG encoder,
and the PostProcessor layers - but I think it's all JPEG related, so
keeping fewer LOG_CATEGORY's is probably better, so I think I'm happy
with this.
> +
> +PostProcessorJpeg::PostProcessorJpeg(CameraDevice *device)
> + : cameraDevice_(device)
> +{
> +}
> +
> +int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> + const StreamConfiguration &outCfg)
> +{
> + int ret = 0;
> +
> + if (inCfg.size != outCfg.size) {
> + LOG(JPEG, Error) << "Mismatch of input and output stream sizes";
> + return -1;
We could use (negative) errno values here.
either:
EXDEV 18 Invalid cross-device link
or
EINVAL 22 Invalid argument
> + }
> +
> + if (outCfg.pixelFormat != formats::MJPEG) {
> + LOG(JPEG, Error) << "Output stream pixel format is not JPEG";
> + return -1;
Same here of course.
> + }
> +
> + streamSize_ = outCfg.size;
> + encoder_ = std::make_unique<EncoderLibJpeg>();
We could also create the encoder_ during construction, but wherever it
happens, later there will have to be a factory/condition to decide
whether to create the libjpeg encoder, or the hardware accellerated one,
so I don't mind it being here at the moment... however...
> +
> + if (encoder_)
> + ret = encoder_->configure(inCfg);
> +
You need to initialise ret to an error value, above as here it will
return success if the encoder was not created.
> + return ret;
> +}
> +
> +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";
> +
> + 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..72aca9b
> --- /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/internal/buffer.h"
> +
> +#include <libcamera/geometry.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;
> +
> +
There's an extra blank line here.
With negative errno values in PostProcessorJpeg::configure, and the ret
in that function handled correctly if the encoder isn't created:
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> +private:
> + CameraDevice *cameraDevice_;
> + 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 802bb89..eacd544 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -23,6 +23,7 @@ android_hal_sources = files([
> 'camera_stream.cpp',
> 'jpeg/encoder_libjpeg.cpp',
> 'jpeg/exif.cpp',
> + 'jpeg/post_processor_jpeg.cpp',
> ])
>
> android_camera_metadata_sources = files([
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list