| Message ID | 20201016053754.17251-3-email@uajain.com |
|---|---|
| State | Accepted |
| Commit | b2b8c4dc941b979ddf5e9013530e76c67e6a12fe |
| Headers | show |
| Series |
|
| Related | show |
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@uajain.com> > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Kieran, would you be able to test this new version, and push it if all looks good to you ? > --- > 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); > 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"; > + > + 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_; > + 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([
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@uajain.com> >> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 :) > >> --- >> 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); >> 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"; >> + >> + 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_; >> + 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([
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@uajain.com> >>> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@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); >>> 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"; >>> + >>> + 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_; >>> + 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Fri, Oct 16, 2020 at 7:25 PM Kieran Bingham <kieran.bingham@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@uajain.com> > >>> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@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? > >>> 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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards > -- > Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Mon, Oct 19, 2020 at 2:43 PM Hirokazu Honda <hiroh@chromium.org> wrote: > > On Fri, Oct 16, 2020 at 7:25 PM Kieran Bingham > <kieran.bingham@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@uajain.com> > > >>> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards > > -- > > Kieran > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Hiro, On 19/10/2020 06:43, Hirokazu Honda wrote: > On Fri, Oct 16, 2020 at 7:25 PM Kieran Bingham > <kieran.bingham@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@uajain.com> >>>>> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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? >>>>> + >>>>> + 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? >>>>> + 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@lists.libcamera.org >>> https://lists.libcamera.org/listinfo/libcamera-devel >> >> -- >> Regards >> -- >> Kieran >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
On Mon, Oct 19, 2020 at 5:29 PM Kieran Bingham <kieran.bingham@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@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@uajain.com> > >>>>> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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@lists.libcamera.org > >>> https://lists.libcamera.org/listinfo/libcamera-devel > >> > >> -- > >> Regards > >> -- > >> Kieran > >> _______________________________________________ > >> libcamera-devel mailing list > >> libcamera-devel@lists.libcamera.org > >> https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards > -- > Kieran
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); 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"; + + 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_; + 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([