[libcamera-devel] [RFC PATCH 2/3] android: jpeg: Port to PostProcessor interface
Umang Jain
email at uajain.com
Fri Oct 9 16:12:48 CEST 2020
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"
>> +
>> 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([
More information about the libcamera-devel
mailing list