[libcamera-devel] [RFC PATCH 2/3] android: jpeg: Port to PostProcessor interface
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Oct 9 16:36:02 CEST 2020
Hi Umang,
On 09/10/2020 15:12, Umang Jain wrote:
> Hi Laurent,
>
> Forgot to follow up at one more point.
>
> On 10/9/20 12:52 AM, Laurent Pinchart wrote:
>> Hi Umang,
>>
>> Thank you for the patch.
>>
>> On Thu, Oct 08, 2020 at 07:40:37PM +0530, Umang Jain wrote:
>>> Remove the existing Encoder interface completely and use the
>>> PostProcessor interface instead.
>> I think we need to keep the Encoder interface. We want to support other
>> JPEG encoders than the libjpeg-based implementation. Creating a JPEG
>> post-processor is the right thing to do, but it should still rely on the
>> existing encoder interface for the actual JPEG encoding. From a
>> CameraDevice point of view, only the PostProcessor interface will be
>> visible.
>>
>>> Now the ::encode() function will be called by PostProcessor::process()
>>> internally and will not be directly exposed in CameraStream. Similarly,
>>> other operations can be introduced internally in the PostProcessorJpeg,
>>> and can be called through its process() interface.
>>>
>>> Signed-off-by: Umang Jain <email at uajain.com>
>>> ---
>>> src/android/camera_device.h | 1 -
>>> src/android/camera_stream.cpp | 74 +++------------
>>> src/android/camera_stream.h | 9 +-
>>> src/android/jpeg/encoder.h | 25 -----
>>> ...er_libjpeg.cpp => post_processor_jpeg.cpp} | 92 +++++++++++++++++--
>>> ...ncoder_libjpeg.h => post_processor_jpeg.h} | 27 ++++--
>>> src/android/meson.build | 2 +-
>>> 7 files changed, 122 insertions(+), 108 deletions(-)
>>> delete mode 100644 src/android/jpeg/encoder.h
>>> rename src/android/jpeg/{encoder_libjpeg.cpp =>
>>> post_processor_jpeg.cpp} (67%)
>>> rename src/android/jpeg/{encoder_libjpeg.h =>
>>> post_processor_jpeg.h} (55%)
>>>
>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>>> index 777d1a3..25de12e 100644
>>> --- a/src/android/camera_device.h
>>> +++ b/src/android/camera_device.h
>>> @@ -25,7 +25,6 @@
>>> #include "libcamera/internal/message.h"
>>> #include "camera_stream.h"
>>> -#include "jpeg/encoder.h"
>>> class CameraMetadata;
>>> diff --git a/src/android/camera_stream.cpp
>>> b/src/android/camera_stream.cpp
>>> index eac1480..ed3bb41 100644
>>> --- a/src/android/camera_stream.cpp
>>> +++ b/src/android/camera_stream.cpp
>>> @@ -9,9 +9,7 @@
>>> #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"
>>> using namespace libcamera;
>>> @@ -24,8 +22,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 PostProcessJpeg and that is what we
>> s/PostProcessJpeg/PostProcessorJpeg/
>>
>>> + * will instantiate here.
>>> + */
>>> + postProcessor_ = std::make_unique<PostProcessorJpeg>();
>>> + }
>>> if (type == Type::Internal) {
>>> allocator_ =
>>> std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
>>> @@ -45,8 +50,8 @@ Stream *CameraStream::stream() const
>>> int CameraStream::configure()
>>> {
>>> - if (encoder_) {
>>> - int ret = encoder_->configure(configuration());
>>> + if (postProcessor_) {
>>> + int ret = postProcessor_->configure(configuration());
>>> if (ret)
>>> return ret;
>>> }
>>> @@ -69,60 +74,11 @@ 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, cameraDevice_);
>> There are at least two options to keep the interface generic, avoiding
>> variadic arguments. One of them is to explicitly pass the the camera
>> device to the process function. Another option is to pass it to the
>> PostProcessorJpeg constructor, and store it internally. I'd go for the
>> latter.
>>
>>> }
>>> FrameBuffer *CameraStream::getBuffer()
>>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>>> index 8df0101..8d0f2e3 100644
>>> --- a/src/android/camera_stream.h
>>> +++ b/src/android/camera_stream.h
>>> @@ -19,7 +19,12 @@
>>> #include <libcamera/geometry.h>
>>> #include <libcamera/pixel_format.h>
>>> -class Encoder;
>>> +/*
>>> + * \todo Ideally we want to replace this include with
>>> forward-declaration.
>>> + * If we do that. currently we get a compile error.
>>> + */
>> Let's fix them :-) What compilation error do you get ?
> The error I see:
>
> [89/298] Compiling C++ object
> src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o
> FAILED: src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o
> c++ -Isrc/libcamera/libcamera.so.p -Isrc/libcamera -I../src/libcamera
> -Iinclude -I../include -I../include/android/hardware/libhardware/include
> -I../include/android/metadata -I../include/android/system/core/include
> -Iinclude/libcamera -fdiagnostics-color=always -pipe
> -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra
> -Werror -std=c++17 -g -include config.h -fPIC -pthread -MD -MQ
> src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o -MF
> src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o.d -o
> src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o -c
> ../src/android/camera_device.cpp
> In file included from /usr/include/c++/10/memory:83,
> from ../src/android/camera_device.h:11,
> from ../src/android/camera_device.cpp:8:
> /usr/include/c++/10/bits/unique_ptr.h: In instantiation of ‘void
> std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp =
> PostProcessor]’:
> /usr/include/c++/10/bits/unique_ptr.h:360:17: required from
> ‘std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = PostProcessor; _Dp
> = std::default_delete<PostProcessor>]’
> ../src/android/camera_stream.h:32:7: required from ‘void
> __gnu_cxx::new_allocator<_Tp>::destroy(_Up*) [with _Up = CameraStream;
> _Tp = CameraStream]’
> /usr/include/c++/10/bits/alloc_traits.h:531:15: required from ‘static
> void std::allocator_traits<std::allocator<_Tp1>
>>::destroy(std::allocator_traits<std::allocator<_Tp1>
>>::allocator_type&, _Up*) [with _Up = CameraStream; _Tp = CameraStream;
> std::allocator_traits<std::allocator<_Tp1> >::allocator_type =
> std::allocator<CameraStream>]’
> /usr/include/c++/10/bits/vector.tcc:488:28: required from ‘void
> std::vector<_Tp, _Alloc>::_M_realloc_insert(std::vector<_Tp,
> _Alloc>::iterator, _Args&& ...) [with _Args = {CameraDevice*,
> CameraStream::Type, camera3_stream*&, long unsigned int}; _Tp =
> CameraStream; _Alloc = std::allocator<CameraStream>; std::vector<_Tp,
> _Alloc>::iterator = std::vector<CameraStream>::iterator]’
> /usr/include/c++/10/bits/vector.tcc:121:21: required from
> ‘std::vector<_Tp, _Alloc>::reference std::vector<_Tp,
> _Alloc>::emplace_back(_Args&& ...) [with _Args = {CameraDevice*,
> CameraStream::Type, camera3_stream*&, long unsigned int}; _Tp =
> CameraStream; _Alloc = std::allocator<CameraStream>; std::vector<_Tp,
> _Alloc>::reference = CameraStream&]’
> ../src/android/camera_device.cpp:1212:38: required from here
> /usr/include/c++/10/bits/unique_ptr.h:82:16: error: invalid application
> of ‘sizeof’ to incomplete type ‘PostProcessor’
> 82 | static_assert(sizeof(_Tp)>0,
> | ^~~~~~~~~~~
> [98/298] Compiling C++ object
> src/libcamera/libcamera.so.p/.._android_camera_stream.cpp.o
> ninja: build stopped: subcommand failed.
>
> Not sure why it cannot work with the incomplete type `PostProcessor`
> interface
> (well, it used to work well with Encoder's interface, right there!)
>>
>>> +#include "post_processor.h"
Does this file get included in ./src/android/camera_device.cpp ?
--
Kieran
>>> +
>>> class CameraDevice;
>>> class CameraMetadata;
>>> class MappedCamera3Buffer;
>>> @@ -135,7 +140,7 @@ private:
>>> */
>>> unsigned int index_;
>>> - std::unique_ptr<Encoder> encoder_;
>>> + std::unique_ptr<PostProcessor> postProcessor_;
>>> std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
>>> std::vector<libcamera::FrameBuffer *> buffers_;
>>> /*
>>> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
>>> deleted file mode 100644
>>> index cf26d67..0000000
>>> --- a/src/android/jpeg/encoder.h
>>> +++ /dev/null
>>> @@ -1,25 +0,0 @@
>>> -/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> -/*
>>> - * Copyright (C) 2020, Google Inc.
>>> - *
>>> - * encoder.h - Image encoding interface
>>> - */
>>> -#ifndef __ANDROID_JPEG_ENCODER_H__
>>> -#define __ANDROID_JPEG_ENCODER_H__
>>> -
>>> -#include <libcamera/buffer.h>
>>> -#include <libcamera/span.h>
>>> -#include <libcamera/stream.h>
>>> -
>>> -class Encoder
>>> -{
>>> -public:
>>> - virtual ~Encoder() {};
>>> -
>>> - virtual int configure(const libcamera::StreamConfiguration &cfg)
>>> = 0;
>>> - virtual int encode(const libcamera::FrameBuffer *source,
>>> - const libcamera::Span<uint8_t> &destination,
>>> - const libcamera::Span<const uint8_t> &exifData) = 0;
>>> -};
>>> -
>>> -#endif /* __ANDROID_JPEG_ENCODER_H__ */
>> As mentioned above, this should be kept, and used by the JPEG
>> post-processor.
>>
>>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp
>>> b/src/android/jpeg/post_processor_jpeg.cpp
>>> similarity index 67%
>>> rename from src/android/jpeg/encoder_libjpeg.cpp
>>> rename to src/android/jpeg/post_processor_jpeg.cpp
>>> index 510613c..eeb4e95 100644
>>> --- a/src/android/jpeg/encoder_libjpeg.cpp
>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>>> @@ -2,10 +2,14 @@
>>> /*
>>> * Copyright (C) 2020, Google Inc.
>>> *
>>> - * encoder_libjpeg.cpp - JPEG encoding using libjpeg native API
>>> + * post_processor_jpeg.cpp - JPEG Post Processor
>>> */
>>> -#include "encoder_libjpeg.h"
>>> +#include "post_processor_jpeg.h"
>>> +
>>> +#include "exif.h"
>>> +
>>> +#include "../camera_device.h"
>>> #include <fcntl.h>
>>> #include <iomanip>
>>> @@ -25,6 +29,14 @@
>>> using namespace libcamera;
>>> +#define extract_va_arg(type, arg, last) \
>>> +{ \
>>> + va_list ap; \
>>> + va_start(ap, last); \
>>> + arg = va_arg(ap, type); \
>>> + va_end(ap); \
>>> +}
>>> +
>>> LOG_DEFINE_CATEGORY(JPEG)
>>> namespace {
>>> @@ -67,7 +79,7 @@ const struct JPEGPixelFormatInfo
>>> &findPixelInfo(const PixelFormat &format)
>>> } /* namespace */
>>> -EncoderLibJpeg::EncoderLibJpeg()
>>> +PostProcessorJpeg::PostProcessorJpeg()
>>> : quality_(95)
>>> {
>>> /* \todo Expand error handling coverage with a custom handler. */
>>> @@ -76,12 +88,12 @@ EncoderLibJpeg::EncoderLibJpeg()
>>> jpeg_create_compress(&compress_);
>>> }
>>> -EncoderLibJpeg::~EncoderLibJpeg()
>>> +PostProcessorJpeg::~PostProcessorJpeg()
>>> {
>>> jpeg_destroy_compress(&compress_);
>>> }
>>> -int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
>>> +int PostProcessorJpeg::configure(const StreamConfiguration &cfg)
>>> {
>>> const struct JPEGPixelFormatInfo info =
>>> findPixelInfo(cfg.pixelFormat);
>>> if (info.colorSpace == JCS_UNKNOWN)
>>> @@ -104,7 +116,67 @@ int EncoderLibJpeg::configure(const
>>> StreamConfiguration &cfg)
>>> return 0;
>>> }
>>> -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer
>>> *frame)
>>> +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
>>> + const libcamera::Span<uint8_t> &destination,
>>> + CameraMetadata *metadata, ...)
>>> +{
>>> + CameraDevice *device = nullptr;
>>> + extract_va_arg(CameraDevice *, device, metadata);
>>> +
>>> + /* 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(device->orientation());
>>> + exif.setSize(Size {compress_.image_width, compress_.image_height});
>>> + /*
>>> + * 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 = 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() +
>>> + device->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;
>>> +}
>>> +
>>> +void PostProcessorJpeg::compressRGB(const libcamera::MappedBuffer
>>> *frame)
>>> {
>>> unsigned char *src = static_cast<unsigned char
>>> *>(frame->maps()[0].data());
>>> /* \todo Stride information should come from buffer
>>> configuration. */
>>> @@ -122,7 +194,7 @@ void EncoderLibJpeg::compressRGB(const
>>> libcamera::MappedBuffer *frame)
>>> * Compress the incoming buffer from a supported NV format.
>>> * This naively unpacks the semi-planar NV12 to a YUV888 format for
>>> libjpeg.
>>> */
>>> -void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
>>> +void PostProcessorJpeg::compressNV(const libcamera::MappedBuffer
>>> *frame)
>>> {
>>> uint8_t tmprowbuf[compress_.image_width * 3];
>>> @@ -179,9 +251,9 @@ void EncoderLibJpeg::compressNV(const
>>> libcamera::MappedBuffer *frame)
>>> }
>>> }
>>> -int EncoderLibJpeg::encode(const FrameBuffer *source,
>>> - const libcamera::Span<uint8_t> &dest,
>>> - const libcamera::Span<const uint8_t> &exifData)
>>> +int PostProcessorJpeg::encode(const FrameBuffer *source,
>>> + const libcamera::Span<uint8_t> &dest,
>>> + const libcamera::Span<const uint8_t> &exifData)
>>> {
>>> MappedFrameBuffer frame(source, PROT_READ);
>>> if (!frame.isValid()) {
>>> diff --git a/src/android/jpeg/encoder_libjpeg.h
>>> b/src/android/jpeg/post_processor_jpeg.h
>>> similarity index 55%
>>> rename from src/android/jpeg/encoder_libjpeg.h
>>> rename to src/android/jpeg/post_processor_jpeg.h
>>> index 1e8df05..7f9ce70 100644
>>> --- a/src/android/jpeg/encoder_libjpeg.h
>>> +++ b/src/android/jpeg/post_processor_jpeg.h
>>> @@ -2,30 +2,37 @@
>>> /*
>>> * Copyright (C) 2020, Google Inc.
>>> *
>>> - * encoder_libjpeg.h - JPEG encoding using libjpeg
>>> + * post_processor_jpeg.h - JPEG Post Processor
>>> */
>>> -#ifndef __ANDROID_JPEG_ENCODER_LIBJPEG_H__
>>> -#define __ANDROID_JPEG_ENCODER_LIBJPEG_H__
>>> +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__
>>> +#define __ANDROID_POST_PROCESSOR_JPEG_H__
>>> -#include "encoder.h"
>>> +#include "../post_processor.h"
>>> +#include "../camera_metadata.h"
>>> #include "libcamera/internal/buffer.h"
>>> #include "libcamera/internal/formats.h"
>>> #include <jpeglib.h>
>>> -class EncoderLibJpeg : public Encoder
>>> +class PostProcessorJpeg : public PostProcessor
>>> {
>>> public:
>>> - EncoderLibJpeg();
>>> - ~EncoderLibJpeg();
>>> + PostProcessorJpeg();
>>> + ~PostProcessorJpeg();
>>> int configure(const libcamera::StreamConfiguration &cfg)
>>> override;
>>> + int process(const libcamera::FrameBuffer *source,
>>> + const libcamera::Span<uint8_t> &destination,
>>> + CameraMetadata *metadata,
>>> + ...) override;
>>> +
>>> +
>>> +private:
>>> int encode(const libcamera::FrameBuffer *source,
>>> const libcamera::Span<uint8_t> &destination,
>>> - const libcamera::Span<const uint8_t> &exifData) override;
>>> + const libcamera::Span<const uint8_t> &exifData);
>>> -private:
>>> void compressRGB(const libcamera::MappedBuffer *frame);
>>> void compressNV(const libcamera::MappedBuffer *frame);
>>> @@ -40,4 +47,4 @@ private:
>>> bool nvSwap_;
>>> };
>>> -#endif /* __ANDROID_JPEG_ENCODER_LIBJPEG_H__ */
>>> +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
>>> diff --git a/src/android/meson.build b/src/android/meson.build
>>> index 802bb89..02b3b47 100644
>>> --- a/src/android/meson.build
>>> +++ b/src/android/meson.build
>>> @@ -21,8 +21,8 @@ android_hal_sources = files([
>>> 'camera_metadata.cpp',
>>> 'camera_ops.cpp',
>>> 'camera_stream.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
More information about the libcamera-devel
mailing list