[libcamera-devel] [PATCH v2 2/2] android: jpeg: Port to PostProcessor interface
Hirokazu Honda
hiroh at chromium.org
Mon Oct 19 12:29:54 CEST 2020
On Mon, Oct 19, 2020 at 5:29 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On 19/10/2020 06:43, Hirokazu Honda 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?
> >
>
> As you've already mentioned, this is handled through configuration().
>
> config_ is the CameraConfiguration, where configuration() is the Stream
> Configuration.
>
>
> >>>>> 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?
>
> I recall the discussion from this when it went in originally was that
> there's not much we can do.
>
> If we fail to generate the exif, we probably still want to output the
> image, as it's not fatal, but the error highlights it has happened.
>
> Of course that won't be visible to the end consumer, though the lack of
> exif tags will be.
>
> Do you think that if the exif tags can't be created, the image should
> fail entirely?
>
>
I got it. I don't know much about this exif process.
So if the image encoding process can proceed with this fatal, I am ok
to not handle this as the entire fatal.
> >>>>> +
> >>>>> + 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.
> >
>
> Const would be more accurate here indeed I think.
>
> Could you submit that as a patch perhaps please?
>
I submitted the patch. PTAL.
Regards,
-Hiro
>
> >>>>> + 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
>
> --
> Regards
> --
> Kieran
More information about the libcamera-devel
mailing list