| Message ID | 20201015171457.75678-3-email@uajain.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 15/10/2020 18:14, Umang Jain wrote: > Port the CameraStream's JPEG-encoding bits to PostProcessorJpeg. > This encapsulates the encoder and EXIF generation code into the > PostProcessorJpeg layer and removes these specifics related to JPEG, > from the CameraStream itself. Fantastic, getting all the JPEG specifics out of CameraStream is really good. > > Signed-off-by: Umang Jain <email@uajain.com> > --- > src/android/camera_device.cpp | 1 + > src/android/camera_stream.cpp | 77 ++++------------ > src/android/camera_stream.h | 4 +- > src/android/jpeg/encoder_libjpeg.cpp | 2 +- > src/android/jpeg/post_processor_jpeg.cpp | 110 +++++++++++++++++++++++ > src/android/jpeg/post_processor_jpeg.h | 36 ++++++++ > src/android/meson.build | 1 + > 7 files changed, 169 insertions(+), 62 deletions(-) > create mode 100644 src/android/jpeg/post_processor_jpeg.cpp > create mode 100644 src/android/jpeg/post_processor_jpeg.h > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index c29fcb4..d6a8acb 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -7,6 +7,7 @@ > > #include "camera_device.h" > #include "camera_ops.h" > +#include "post_processor.h" > > #include <sys/mman.h> > #include <tuple> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index eac1480..2a0ba16 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -9,9 +9,9 @@ > > #include "camera_device.h" > #include "camera_metadata.h" > -#include "jpeg/encoder.h" > -#include "jpeg/encoder_libjpeg.h" > -#include "jpeg/exif.h" > +#include "jpeg/post_processor_jpeg.h" > + > +#include <libcamera/formats.h> > > using namespace libcamera; > > @@ -24,8 +24,15 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type, > { > config_ = cameraDevice_->cameraConfiguration(); > > - if (type_ == Type::Internal || type_ == Type::Mapped) > - encoder_ = std::make_unique<EncoderLibJpeg>(); > + if (type_ == Type::Internal || type_ == Type::Mapped) { > + /* > + * \todo There might be multiple post-processors. The logic > + * which should be instantiated here, is deferred for future. > + * For now, we only have PostProcessorJpeg and that is what we > + * will instantiate here. > + */ > + postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_); Agreed, we can leave it to the next PostProcessor to decide how we create these. We've got one - this is it ;-) I anticipate we might also have to chain them together too, so we'll need to figure out how to handle multiple PostProcessors feeding into each other without turning into a full gstreamer-esque pipeline. (For instance, we'll have a Format Convertor to take YUYV->NV12, and then an Encoder to take NV12->MJPEG for UVC pipelines) > + } > > if (type == Type::Internal) { > allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera()); > @@ -45,8 +52,10 @@ Stream *CameraStream::stream() const > > int CameraStream::configure() > { > - if (encoder_) { > - int ret = encoder_->configure(configuration()); > + if (postProcessor_) { > + StreamConfiguration output = configuration(); > + output.pixelFormat == formats::MJPEG; > + int ret = postProcessor_->configure(configuration(), output); > if (ret) > return ret; > } > @@ -69,60 +78,10 @@ int CameraStream::configure() > int CameraStream::process(const libcamera::FrameBuffer &source, > MappedCamera3Buffer *dest, CameraMetadata *metadata) > { > - if (!encoder_) > + if (!postProcessor_) > return 0; > > - /* Set EXIF metadata for various tags. */ > - Exif exif; > - /* \todo Set Make and Model from external vendor tags. */ > - exif.setMake("libcamera"); > - exif.setModel("cameraModel"); > - exif.setOrientation(cameraDevice_->orientation()); > - exif.setSize(configuration().size); > - /* > - * We set the frame's EXIF timestamp as the time of encode. > - * Since the precision we need for EXIF timestamp is only one > - * second, it is good enough. > - */ > - exif.setTimestamp(std::time(nullptr)); > - if (exif.generate() != 0) > - LOG(HAL, Error) << "Failed to generate valid EXIF data"; > - > - int jpeg_size = encoder_->encode(&source, dest->maps()[0], exif.data()); > - if (jpeg_size < 0) { > - LOG(HAL, Error) << "Failed to encode stream image"; > - return jpeg_size; > - } > - > - /* > - * Fill in the JPEG blob header. > - * > - * The mapped size of the buffer is being returned as > - * substantially larger than the requested JPEG_MAX_SIZE > - * (which is referenced from maxJpegBufferSize_). Utilise > - * this static size to ensure the correct offset of the blob is > - * determined. > - * > - * \todo Investigate if the buffer size mismatch is an issue or > - * expected behaviour. > - */ > - uint8_t *resultPtr = dest->maps()[0].data() + > - cameraDevice_->maxJpegBufferSize() - > - sizeof(struct camera3_jpeg_blob); > - auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr); > - blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID; > - blob->jpeg_size = jpeg_size; > - > - /* Update the JPEG result Metadata. */ > - metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1); > - > - const uint32_t jpeg_quality = 95; > - metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1); > - > - const uint32_t jpeg_orientation = 0; > - metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1); > -> - return 0; > + return postProcessor_->process(&source, dest->maps()[0], metadata); I wonder if we should move this to the point that calls CameraStream::process() directly ... but equally, if we have multiple post-processors we might have to do more operations here ... so that makes me think we should keep this function for now.. > } > > FrameBuffer *CameraStream::getBuffer() > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > index 8df0101..c55d90b 100644 > --- a/src/android/camera_stream.h > +++ b/src/android/camera_stream.h > @@ -19,10 +19,10 @@ > #include <libcamera/geometry.h> > #include <libcamera/pixel_format.h> > > -class Encoder; > class CameraDevice; > class CameraMetadata; > class MappedCamera3Buffer; > +class PostProcessor; > > class CameraStream > { > @@ -135,7 +135,6 @@ private: > */ > unsigned int index_; > > - std::unique_ptr<Encoder> encoder_; > std::unique_ptr<libcamera::FrameBufferAllocator> allocator_; > std::vector<libcamera::FrameBuffer *> buffers_; > /* > @@ -143,6 +142,7 @@ private: > * an std::vector in CameraDevice. > */ > std::unique_ptr<std::mutex> mutex_; Haha, Maybe I would have had the nitpick-fix patch first ;-) It really stands out here - but either way is fine. > + std::unique_ptr<PostProcessor> postProcessor_; > }; > > #endif /* __ANDROID_CAMERA_STREAM__ */ > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > index a77f5b2..8995ba5 100644 > --- a/src/android/jpeg/encoder_libjpeg.cpp > +++ b/src/android/jpeg/encoder_libjpeg.cpp > @@ -25,7 +25,7 @@ > > using namespace libcamera; > > -LOG_DEFINE_CATEGORY(JPEG) > +LOG_DECLARE_CATEGORY(JPEG); > > namespace { > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > new file mode 100644 > index 0000000..d1ec95b > --- /dev/null > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -0,0 +1,110 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2020, Google Inc. > + * > + * post_processor_jpeg.cpp - JPEG Post Processor > + */ > + > +#include "post_processor_jpeg.h" > + > +#include "../camera_device.h" > +#include "../camera_metadata.h" > +#include "encoder_libjpeg.h" > +#include "exif.h" > + > +#include <libcamera/formats.h> > + > +#include "libcamera/internal/log.h" > + > +using namespace libcamera; > + > +LOG_DEFINE_CATEGORY(JPEG); I'm wondering if we should separate the categories for the JPEG encoder, and the PostProcessor layers - but I think it's all JPEG related, so keeping fewer LOG_CATEGORY's is probably better, so I think I'm happy with this. > + > +PostProcessorJpeg::PostProcessorJpeg(CameraDevice *device) > + : cameraDevice_(device) > +{ > +} > + > +int PostProcessorJpeg::configure(const StreamConfiguration &inCfg, > + const StreamConfiguration &outCfg) > +{ > + int ret = 0; > + > + if (inCfg.size != outCfg.size) { > + LOG(JPEG, Error) << "Mismatch of input and output stream sizes"; > + return -1; We could use (negative) errno values here. either: EXDEV 18 Invalid cross-device link or EINVAL 22 Invalid argument > + } > + > + if (outCfg.pixelFormat != formats::MJPEG) { > + LOG(JPEG, Error) << "Output stream pixel format is not JPEG"; > + return -1; Same here of course. > + } > + > + streamSize_ = outCfg.size; > + encoder_ = std::make_unique<EncoderLibJpeg>(); We could also create the encoder_ during construction, but wherever it happens, later there will have to be a factory/condition to decide whether to create the libjpeg encoder, or the hardware accellerated one, so I don't mind it being here at the moment... however... > + > + if (encoder_) > + ret = encoder_->configure(inCfg); > + You need to initialise ret to an error value, above as here it will return success if the encoder was not created. > + return ret; > +} > + > +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, > + const libcamera::Span<uint8_t> &destination, > + CameraMetadata *metadata) > +{ > + if (!encoder_) > + return 0; > + > + /* Set EXIF metadata for various tags. */ > + Exif exif; > + /* \todo Set Make and Model from external vendor tags. */ > + exif.setMake("libcamera"); > + exif.setModel("cameraModel"); > + exif.setOrientation(cameraDevice_->orientation()); > + exif.setSize(streamSize_); > + /* > + * We set the frame's EXIF timestamp as the time of encode. > + * Since the precision we need for EXIF timestamp is only one > + * second, it is good enough. > + */ > + exif.setTimestamp(std::time(nullptr)); > + if (exif.generate() != 0) > + LOG(JPEG, Error) << "Failed to generate valid EXIF data"; > + > + int jpeg_size = encoder_->encode(source, destination, exif.data()); > + if (jpeg_size < 0) { > + LOG(JPEG, Error) << "Failed to encode stream image"; > + return jpeg_size; > + } > + > + /* > + * Fill in the JPEG blob header. > + * > + * The mapped size of the buffer is being returned as > + * substantially larger than the requested JPEG_MAX_SIZE > + * (which is referenced from maxJpegBufferSize_). Utilise > + * this static size to ensure the correct offset of the blob is > + * determined. > + * > + * \todo Investigate if the buffer size mismatch is an issue or > + * expected behaviour. > + */ > + uint8_t *resultPtr = destination.data() + > + cameraDevice_->maxJpegBufferSize() - > + sizeof(struct camera3_jpeg_blob); > + auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr); > + blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID; > + blob->jpeg_size = jpeg_size; > + > + /* Update the JPEG result Metadata. */ > + metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1); > + > + const uint32_t jpeg_quality = 95; > + metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1); > + > + const uint32_t jpeg_orientation = 0; > + metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1); > + > + return 0; > +} > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h > new file mode 100644 > index 0000000..72aca9b > --- /dev/null > +++ b/src/android/jpeg/post_processor_jpeg.h > @@ -0,0 +1,36 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2020, Google Inc. > + * > + * post_processor_jpeg.h - JPEG Post Processor > + */ > +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__ > +#define __ANDROID_POST_PROCESSOR_JPEG_H__ > + > +#include "../post_processor.h" > +#include "libcamera/internal/buffer.h" > + > +#include <libcamera/geometry.h> > + > +class Encoder; > +class CameraDevice; > + > +class PostProcessorJpeg : public PostProcessor > +{ > +public: > + PostProcessorJpeg(CameraDevice *device); > + > + int configure(const libcamera::StreamConfiguration &incfg, > + const libcamera::StreamConfiguration &outcfg) override; > + int process(const libcamera::FrameBuffer *source, > + const libcamera::Span<uint8_t> &destination, > + CameraMetadata *metadata) override; > + > + There's an extra blank line here. With negative errno values in PostProcessorJpeg::configure, and the ret in that function handled correctly if the encoder isn't created: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > +private: > + CameraDevice *cameraDevice_; > + std::unique_ptr<Encoder> encoder_; > + libcamera::Size streamSize_; > +}; > + > +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */ > diff --git a/src/android/meson.build b/src/android/meson.build > index 802bb89..eacd544 100644 > --- a/src/android/meson.build > +++ b/src/android/meson.build > @@ -23,6 +23,7 @@ android_hal_sources = files([ > 'camera_stream.cpp', > 'jpeg/encoder_libjpeg.cpp', > 'jpeg/exif.cpp', > + 'jpeg/post_processor_jpeg.cpp', > ]) > > android_camera_metadata_sources = files([ >
Hi Umang, Thank you for the patch. On Thu, Oct 15, 2020 at 08:32:29PM +0100, Kieran Bingham wrote: > On 15/10/2020 18:14, Umang Jain wrote: > > Port the CameraStream's JPEG-encoding bits to PostProcessorJpeg. > > This encapsulates the encoder and EXIF generation code into the > > PostProcessorJpeg layer and removes these specifics related to JPEG, > > from the CameraStream itself. > > Fantastic, getting all the JPEG specifics out of CameraStream is really > good. Yes, it looks much better. > > Signed-off-by: Umang Jain <email@uajain.com> > > --- > > src/android/camera_device.cpp | 1 + > > src/android/camera_stream.cpp | 77 ++++------------ > > src/android/camera_stream.h | 4 +- > > src/android/jpeg/encoder_libjpeg.cpp | 2 +- > > src/android/jpeg/post_processor_jpeg.cpp | 110 +++++++++++++++++++++++ > > src/android/jpeg/post_processor_jpeg.h | 36 ++++++++ > > src/android/meson.build | 1 + > > 7 files changed, 169 insertions(+), 62 deletions(-) > > create mode 100644 src/android/jpeg/post_processor_jpeg.cpp > > create mode 100644 src/android/jpeg/post_processor_jpeg.h > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index c29fcb4..d6a8acb 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -7,6 +7,7 @@ > > > > #include "camera_device.h" > > #include "camera_ops.h" > > +#include "post_processor.h" > > > > #include <sys/mman.h> > > #include <tuple> > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > > index eac1480..2a0ba16 100644 > > --- a/src/android/camera_stream.cpp > > +++ b/src/android/camera_stream.cpp > > @@ -9,9 +9,9 @@ > > > > #include "camera_device.h" > > #include "camera_metadata.h" > > -#include "jpeg/encoder.h" > > -#include "jpeg/encoder_libjpeg.h" > > -#include "jpeg/exif.h" > > +#include "jpeg/post_processor_jpeg.h" > > + > > +#include <libcamera/formats.h> > > > > using namespace libcamera; > > > > @@ -24,8 +24,15 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type, > > { > > config_ = cameraDevice_->cameraConfiguration(); > > > > - if (type_ == Type::Internal || type_ == Type::Mapped) > > - encoder_ = std::make_unique<EncoderLibJpeg>(); > > + if (type_ == Type::Internal || type_ == Type::Mapped) { > > + /* > > + * \todo There might be multiple post-processors. The logic > > + * which should be instantiated here, is deferred for future. s/, is deferred for future/ is deferred to the future/ > > + * For now, we only have PostProcessorJpeg and that is what we > > + * will instantiate here. s/will // > > + */ > > + postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_); > > Agreed, we can leave it to the next PostProcessor to decide how we > create these. We've got one - this is it ;-) > > I anticipate we might also have to chain them together too, so we'll > need to figure out how to handle multiple PostProcessors feeding into > each other without turning into a full gstreamer-esque pipeline. gstreamer is exactly what came to my mind :-) It's tempting to make it overengineered, but let's try to keep it simple. > (For instance, we'll have a Format Convertor to take YUYV->NV12, and > then an Encoder to take NV12->MJPEG for UVC pipelines) It would probably be more efficient to support encoding JPEG from YUYV though. And we'll also need an MJPEG -> YUV/NV12 decompressor as webcams often provide higher resolutions and frame rates in MJPEG. Anyway, that's for later. > > + } > > > > if (type == Type::Internal) { > > allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera()); > > @@ -45,8 +52,10 @@ Stream *CameraStream::stream() const > > > > int CameraStream::configure() > > { > > - if (encoder_) { > > - int ret = encoder_->configure(configuration()); > > + if (postProcessor_) { > > + StreamConfiguration output = configuration(); > > + output.pixelFormat == formats::MJPEG; s/==/=/ > > + int ret = postProcessor_->configure(configuration(), output); > > if (ret) > > return ret; > > } > > @@ -69,60 +78,10 @@ int CameraStream::configure() > > int CameraStream::process(const libcamera::FrameBuffer &source, > > MappedCamera3Buffer *dest, CameraMetadata *metadata) > > { > > - if (!encoder_) > > + if (!postProcessor_) > > return 0; > > > > - /* Set EXIF metadata for various tags. */ > > - Exif exif; > > - /* \todo Set Make and Model from external vendor tags. */ > > - exif.setMake("libcamera"); > > - exif.setModel("cameraModel"); > > - exif.setOrientation(cameraDevice_->orientation()); > > - exif.setSize(configuration().size); > > - /* > > - * We set the frame's EXIF timestamp as the time of encode. > > - * Since the precision we need for EXIF timestamp is only one > > - * second, it is good enough. > > - */ > > - exif.setTimestamp(std::time(nullptr)); > > - if (exif.generate() != 0) > > - LOG(HAL, Error) << "Failed to generate valid EXIF data"; > > - > > - int jpeg_size = encoder_->encode(&source, dest->maps()[0], exif.data()); > > - if (jpeg_size < 0) { > > - LOG(HAL, Error) << "Failed to encode stream image"; > > - return jpeg_size; > > - } > > - > > - /* > > - * Fill in the JPEG blob header. > > - * > > - * The mapped size of the buffer is being returned as > > - * substantially larger than the requested JPEG_MAX_SIZE > > - * (which is referenced from maxJpegBufferSize_). Utilise > > - * this static size to ensure the correct offset of the blob is > > - * determined. > > - * > > - * \todo Investigate if the buffer size mismatch is an issue or > > - * expected behaviour. > > - */ > > - uint8_t *resultPtr = dest->maps()[0].data() + > > - cameraDevice_->maxJpegBufferSize() - > > - sizeof(struct camera3_jpeg_blob); > > - auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr); > > - blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID; > > - blob->jpeg_size = jpeg_size; > > - > > - /* Update the JPEG result Metadata. */ > > - metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1); > > - > > - const uint32_t jpeg_quality = 95; > > - metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1); > > - > > - const uint32_t jpeg_orientation = 0; > > - metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1); > > -> - return 0; > > + return postProcessor_->process(&source, dest->maps()[0], metadata); > > I wonder if we should move this to the point that calls > CameraStream::process() directly ... but equally, if we have multiple > post-processors we might have to do more operations here ... so that > makes me think we should keep this function for now.. I'd keep it for now. > > } > > > > FrameBuffer *CameraStream::getBuffer() > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > > index 8df0101..c55d90b 100644 > > --- a/src/android/camera_stream.h > > +++ b/src/android/camera_stream.h > > @@ -19,10 +19,10 @@ > > #include <libcamera/geometry.h> > > #include <libcamera/pixel_format.h> > > > > -class Encoder; > > class CameraDevice; > > class CameraMetadata; > > class MappedCamera3Buffer; > > +class PostProcessor; > > > > class CameraStream > > { > > @@ -135,7 +135,6 @@ private: > > */ > > unsigned int index_; > > > > - std::unique_ptr<Encoder> encoder_; > > std::unique_ptr<libcamera::FrameBufferAllocator> allocator_; > > std::vector<libcamera::FrameBuffer *> buffers_; > > /* > > @@ -143,6 +142,7 @@ private: > > * an std::vector in CameraDevice. > > */ > > std::unique_ptr<std::mutex> mutex_; > > Haha, Maybe I would have had the nitpick-fix patch first ;-) It really > stands out here - but either way is fine. I've just pushed it to the master branch :-) > > + std::unique_ptr<PostProcessor> postProcessor_; > > }; > > > > #endif /* __ANDROID_CAMERA_STREAM__ */ > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > > index a77f5b2..8995ba5 100644 > > --- a/src/android/jpeg/encoder_libjpeg.cpp > > +++ b/src/android/jpeg/encoder_libjpeg.cpp > > @@ -25,7 +25,7 @@ > > > > using namespace libcamera; > > > > -LOG_DEFINE_CATEGORY(JPEG) > > +LOG_DECLARE_CATEGORY(JPEG); > > > > namespace { > > > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > > new file mode 100644 > > index 0000000..d1ec95b > > --- /dev/null > > +++ b/src/android/jpeg/post_processor_jpeg.cpp > > @@ -0,0 +1,110 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2020, Google Inc. > > + * > > + * post_processor_jpeg.cpp - JPEG Post Processor > > + */ > > + > > +#include "post_processor_jpeg.h" > > + > > +#include "../camera_device.h" > > +#include "../camera_metadata.h" > > +#include "encoder_libjpeg.h" > > +#include "exif.h" > > + > > +#include <libcamera/formats.h> > > + > > +#include "libcamera/internal/log.h" > > + > > +using namespace libcamera; > > + > > +LOG_DEFINE_CATEGORY(JPEG); > > I'm wondering if we should separate the categories for the JPEG encoder, > and the PostProcessor layers - but I think it's all JPEG related, so > keeping fewer LOG_CATEGORY's is probably better, so I think I'm happy > with this. > > > + > > +PostProcessorJpeg::PostProcessorJpeg(CameraDevice *device) > > + : cameraDevice_(device) > > +{ > > +} > > + > > +int PostProcessorJpeg::configure(const StreamConfiguration &inCfg, > > + const StreamConfiguration &outCfg) > > +{ > > + int ret = 0; > > + > > + if (inCfg.size != outCfg.size) { > > + LOG(JPEG, Error) << "Mismatch of input and output stream sizes"; > > + return -1; > > We could use (negative) errno values here. > > either: > > EXDEV 18 Invalid cross-device link > or > EINVAL 22 Invalid argument > > > + } > > + > > + if (outCfg.pixelFormat != formats::MJPEG) { > > + LOG(JPEG, Error) << "Output stream pixel format is not JPEG"; > > + return -1; > > Same here of course. > > > + } > > + > > + streamSize_ = outCfg.size; > > + encoder_ = std::make_unique<EncoderLibJpeg>(); > > We could also create the encoder_ during construction, but wherever it > happens, later there will have to be a factory/condition to decide > whether to create the libjpeg encoder, or the hardware accellerated one, > so I don't mind it being here at the moment... however... > > > + > > + if (encoder_) > > + ret = encoder_->configure(inCfg); > > + > > You need to initialise ret to an error value, above as here it will > return success if the encoder was not created. std::make_unique<>() will never return null, will it ? Shouldn't the check just be dropped ? > > + return ret; > > +} > > + > > +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, > > + const libcamera::Span<uint8_t> &destination, > > + CameraMetadata *metadata) > > +{ > > + if (!encoder_) > > + return 0; > > + > > + /* Set EXIF metadata for various tags. */ > > + Exif exif; > > + /* \todo Set Make and Model from external vendor tags. */ > > + exif.setMake("libcamera"); > > + exif.setModel("cameraModel"); > > + exif.setOrientation(cameraDevice_->orientation()); > > + exif.setSize(streamSize_); > > + /* > > + * We set the frame's EXIF timestamp as the time of encode. > > + * Since the precision we need for EXIF timestamp is only one > > + * second, it is good enough. > > + */ > > + exif.setTimestamp(std::time(nullptr)); > > + if (exif.generate() != 0) > > + LOG(JPEG, Error) << "Failed to generate valid EXIF data"; > > + > > + int jpeg_size = encoder_->encode(source, destination, exif.data()); > > + if (jpeg_size < 0) { > > + LOG(JPEG, Error) << "Failed to encode stream image"; > > + return jpeg_size; > > + } > > + > > + /* > > + * Fill in the JPEG blob header. > > + * > > + * The mapped size of the buffer is being returned as > > + * substantially larger than the requested JPEG_MAX_SIZE > > + * (which is referenced from maxJpegBufferSize_). Utilise > > + * this static size to ensure the correct offset of the blob is > > + * determined. > > + * > > + * \todo Investigate if the buffer size mismatch is an issue or > > + * expected behaviour. > > + */ > > + uint8_t *resultPtr = destination.data() + > > + cameraDevice_->maxJpegBufferSize() - > > + sizeof(struct camera3_jpeg_blob); > > + auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr); > > + blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID; > > + blob->jpeg_size = jpeg_size; > > + > > + /* Update the JPEG result Metadata. */ > > + metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1); > > + > > + const uint32_t jpeg_quality = 95; > > + metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1); > > + > > + const uint32_t jpeg_orientation = 0; > > + metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1); > > + > > + return 0; > > +} > > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h > > new file mode 100644 > > index 0000000..72aca9b > > --- /dev/null > > +++ b/src/android/jpeg/post_processor_jpeg.h > > @@ -0,0 +1,36 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2020, Google Inc. > > + * > > + * post_processor_jpeg.h - JPEG Post Processor > > + */ > > +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__ > > +#define __ANDROID_POST_PROCESSOR_JPEG_H__ > > + > > +#include "../post_processor.h" > > +#include "libcamera/internal/buffer.h" This header... > > + > > +#include <libcamera/geometry.h> > > + ... should go here. > > +class Encoder; > > +class CameraDevice; > > + > > +class PostProcessorJpeg : public PostProcessor > > +{ > > +public: > > + PostProcessorJpeg(CameraDevice *device); > > + > > + int configure(const libcamera::StreamConfiguration &incfg, > > + const libcamera::StreamConfiguration &outcfg) override; > > + int process(const libcamera::FrameBuffer *source, > > + const libcamera::Span<uint8_t> &destination, > > + CameraMetadata *metadata) override; > > + > > + > > There's an extra blank line here. > > With negative errno values in PostProcessorJpeg::configure, and the ret > in that function handled correctly if the encoder isn't created: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > +private: > > + CameraDevice *cameraDevice_; > > + std::unique_ptr<Encoder> encoder_; > > + libcamera::Size streamSize_; > > +}; > > + > > +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */ > > diff --git a/src/android/meson.build b/src/android/meson.build > > index 802bb89..eacd544 100644 > > --- a/src/android/meson.build > > +++ b/src/android/meson.build > > @@ -23,6 +23,7 @@ android_hal_sources = files([ > > 'camera_stream.cpp', > > 'jpeg/encoder_libjpeg.cpp', > > 'jpeg/exif.cpp', > > + 'jpeg/post_processor_jpeg.cpp', > > ]) > > > > android_camera_metadata_sources = files([
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index c29fcb4..d6a8acb 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -7,6 +7,7 @@ #include "camera_device.h" #include "camera_ops.h" +#include "post_processor.h" #include <sys/mman.h> #include <tuple> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index eac1480..2a0ba16 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -9,9 +9,9 @@ #include "camera_device.h" #include "camera_metadata.h" -#include "jpeg/encoder.h" -#include "jpeg/encoder_libjpeg.h" -#include "jpeg/exif.h" +#include "jpeg/post_processor_jpeg.h" + +#include <libcamera/formats.h> using namespace libcamera; @@ -24,8 +24,15 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type, { config_ = cameraDevice_->cameraConfiguration(); - if (type_ == Type::Internal || type_ == Type::Mapped) - encoder_ = std::make_unique<EncoderLibJpeg>(); + if (type_ == Type::Internal || type_ == Type::Mapped) { + /* + * \todo There might be multiple post-processors. The logic + * which should be instantiated here, is deferred for future. + * For now, we only have PostProcessorJpeg and that is what we + * will instantiate here. + */ + postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_); + } if (type == Type::Internal) { allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera()); @@ -45,8 +52,10 @@ Stream *CameraStream::stream() const int CameraStream::configure() { - if (encoder_) { - int ret = encoder_->configure(configuration()); + if (postProcessor_) { + StreamConfiguration output = configuration(); + output.pixelFormat == formats::MJPEG; + int ret = postProcessor_->configure(configuration(), output); if (ret) return ret; } @@ -69,60 +78,10 @@ int CameraStream::configure() int CameraStream::process(const libcamera::FrameBuffer &source, MappedCamera3Buffer *dest, CameraMetadata *metadata) { - if (!encoder_) + if (!postProcessor_) return 0; - /* Set EXIF metadata for various tags. */ - Exif exif; - /* \todo Set Make and Model from external vendor tags. */ - exif.setMake("libcamera"); - exif.setModel("cameraModel"); - exif.setOrientation(cameraDevice_->orientation()); - exif.setSize(configuration().size); - /* - * We set the frame's EXIF timestamp as the time of encode. - * Since the precision we need for EXIF timestamp is only one - * second, it is good enough. - */ - exif.setTimestamp(std::time(nullptr)); - if (exif.generate() != 0) - LOG(HAL, Error) << "Failed to generate valid EXIF data"; - - int jpeg_size = encoder_->encode(&source, dest->maps()[0], exif.data()); - if (jpeg_size < 0) { - LOG(HAL, Error) << "Failed to encode stream image"; - return jpeg_size; - } - - /* - * Fill in the JPEG blob header. - * - * The mapped size of the buffer is being returned as - * substantially larger than the requested JPEG_MAX_SIZE - * (which is referenced from maxJpegBufferSize_). Utilise - * this static size to ensure the correct offset of the blob is - * determined. - * - * \todo Investigate if the buffer size mismatch is an issue or - * expected behaviour. - */ - uint8_t *resultPtr = dest->maps()[0].data() + - cameraDevice_->maxJpegBufferSize() - - sizeof(struct camera3_jpeg_blob); - auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr); - blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID; - blob->jpeg_size = jpeg_size; - - /* Update the JPEG result Metadata. */ - metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1); - - const uint32_t jpeg_quality = 95; - metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1); - - const uint32_t jpeg_orientation = 0; - metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1); - - return 0; + return postProcessor_->process(&source, dest->maps()[0], metadata); } FrameBuffer *CameraStream::getBuffer() diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h index 8df0101..c55d90b 100644 --- a/src/android/camera_stream.h +++ b/src/android/camera_stream.h @@ -19,10 +19,10 @@ #include <libcamera/geometry.h> #include <libcamera/pixel_format.h> -class Encoder; class CameraDevice; class CameraMetadata; class MappedCamera3Buffer; +class PostProcessor; class CameraStream { @@ -135,7 +135,6 @@ private: */ unsigned int index_; - std::unique_ptr<Encoder> encoder_; std::unique_ptr<libcamera::FrameBufferAllocator> allocator_; std::vector<libcamera::FrameBuffer *> buffers_; /* @@ -143,6 +142,7 @@ private: * an std::vector in CameraDevice. */ std::unique_ptr<std::mutex> mutex_; + std::unique_ptr<PostProcessor> postProcessor_; }; #endif /* __ANDROID_CAMERA_STREAM__ */ diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp index a77f5b2..8995ba5 100644 --- a/src/android/jpeg/encoder_libjpeg.cpp +++ b/src/android/jpeg/encoder_libjpeg.cpp @@ -25,7 +25,7 @@ using namespace libcamera; -LOG_DEFINE_CATEGORY(JPEG) +LOG_DECLARE_CATEGORY(JPEG); namespace { diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp new file mode 100644 index 0000000..d1ec95b --- /dev/null +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -0,0 +1,110 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2020, Google Inc. + * + * post_processor_jpeg.cpp - JPEG Post Processor + */ + +#include "post_processor_jpeg.h" + +#include "../camera_device.h" +#include "../camera_metadata.h" +#include "encoder_libjpeg.h" +#include "exif.h" + +#include <libcamera/formats.h> + +#include "libcamera/internal/log.h" + +using namespace libcamera; + +LOG_DEFINE_CATEGORY(JPEG); + +PostProcessorJpeg::PostProcessorJpeg(CameraDevice *device) + : cameraDevice_(device) +{ +} + +int PostProcessorJpeg::configure(const StreamConfiguration &inCfg, + const StreamConfiguration &outCfg) +{ + int ret = 0; + + if (inCfg.size != outCfg.size) { + LOG(JPEG, Error) << "Mismatch of input and output stream sizes"; + return -1; + } + + if (outCfg.pixelFormat != formats::MJPEG) { + LOG(JPEG, Error) << "Output stream pixel format is not JPEG"; + return -1; + } + + streamSize_ = outCfg.size; + encoder_ = std::make_unique<EncoderLibJpeg>(); + + if (encoder_) + ret = encoder_->configure(inCfg); + + return ret; +} + +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, + const libcamera::Span<uint8_t> &destination, + CameraMetadata *metadata) +{ + if (!encoder_) + return 0; + + /* Set EXIF metadata for various tags. */ + Exif exif; + /* \todo Set Make and Model from external vendor tags. */ + exif.setMake("libcamera"); + exif.setModel("cameraModel"); + exif.setOrientation(cameraDevice_->orientation()); + exif.setSize(streamSize_); + /* + * We set the frame's EXIF timestamp as the time of encode. + * Since the precision we need for EXIF timestamp is only one + * second, it is good enough. + */ + exif.setTimestamp(std::time(nullptr)); + if (exif.generate() != 0) + LOG(JPEG, Error) << "Failed to generate valid EXIF data"; + + int jpeg_size = encoder_->encode(source, destination, exif.data()); + if (jpeg_size < 0) { + LOG(JPEG, Error) << "Failed to encode stream image"; + return jpeg_size; + } + + /* + * Fill in the JPEG blob header. + * + * The mapped size of the buffer is being returned as + * substantially larger than the requested JPEG_MAX_SIZE + * (which is referenced from maxJpegBufferSize_). Utilise + * this static size to ensure the correct offset of the blob is + * determined. + * + * \todo Investigate if the buffer size mismatch is an issue or + * expected behaviour. + */ + uint8_t *resultPtr = destination.data() + + cameraDevice_->maxJpegBufferSize() - + sizeof(struct camera3_jpeg_blob); + auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr); + blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID; + blob->jpeg_size = jpeg_size; + + /* Update the JPEG result Metadata. */ + metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1); + + const uint32_t jpeg_quality = 95; + metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1); + + const uint32_t jpeg_orientation = 0; + metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1); + + return 0; +} diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h new file mode 100644 index 0000000..72aca9b --- /dev/null +++ b/src/android/jpeg/post_processor_jpeg.h @@ -0,0 +1,36 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2020, Google Inc. + * + * post_processor_jpeg.h - JPEG Post Processor + */ +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__ +#define __ANDROID_POST_PROCESSOR_JPEG_H__ + +#include "../post_processor.h" +#include "libcamera/internal/buffer.h" + +#include <libcamera/geometry.h> + +class Encoder; +class CameraDevice; + +class PostProcessorJpeg : public PostProcessor +{ +public: + PostProcessorJpeg(CameraDevice *device); + + int configure(const libcamera::StreamConfiguration &incfg, + const libcamera::StreamConfiguration &outcfg) override; + int process(const libcamera::FrameBuffer *source, + const libcamera::Span<uint8_t> &destination, + CameraMetadata *metadata) override; + + +private: + CameraDevice *cameraDevice_; + std::unique_ptr<Encoder> encoder_; + libcamera::Size streamSize_; +}; + +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */ diff --git a/src/android/meson.build b/src/android/meson.build index 802bb89..eacd544 100644 --- a/src/android/meson.build +++ b/src/android/meson.build @@ -23,6 +23,7 @@ android_hal_sources = files([ 'camera_stream.cpp', 'jpeg/encoder_libjpeg.cpp', 'jpeg/exif.cpp', + 'jpeg/post_processor_jpeg.cpp', ]) android_camera_metadata_sources = files([
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> --- src/android/camera_device.cpp | 1 + src/android/camera_stream.cpp | 77 ++++------------ src/android/camera_stream.h | 4 +- src/android/jpeg/encoder_libjpeg.cpp | 2 +- src/android/jpeg/post_processor_jpeg.cpp | 110 +++++++++++++++++++++++ src/android/jpeg/post_processor_jpeg.h | 36 ++++++++ src/android/meson.build | 1 + 7 files changed, 169 insertions(+), 62 deletions(-) create mode 100644 src/android/jpeg/post_processor_jpeg.cpp create mode 100644 src/android/jpeg/post_processor_jpeg.h