[libcamera-devel] [PATCH 05/15] android: camera_device: Move processing to CameraStream
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Oct 5 23:42:25 CEST 2020
Hi Jacopo,
On 05/10/2020 13:08, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Oct 05, 2020 at 01:46:39PM +0300, Laurent Pinchart wrote:
>> From: Jacopo Mondi <jacopo at jmondi.org>
>>
>> Move the JPEG processing procedure to the individual CameraStream
>> by augmenting the class with a CameraStream::process() method.
It sounds like it just keeps getting better and better ;-)
>> This allows removing the CameraStream::encoder() method.>>
>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>> ---
>> src/android/camera_device.cpp | 68 ++---------------------------------
>> src/android/camera_device.h | 8 +++++
>> src/android/camera_stream.cpp | 63 ++++++++++++++++++++++++++++++++
>> src/android/camera_stream.h | 7 +++-
>> 4 files changed, 80 insertions(+), 66 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 2c4dd4dee28c..c44904300e0a 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -23,9 +23,6 @@
>> #include "camera_metadata.h"
>> #include "system/graphics.h"
>>
>> -#include "jpeg/encoder_libjpeg.h"
>> -#include "jpeg/exif.h"
>> -
>> using namespace libcamera;
>>
>> namespace {
>> @@ -133,12 +130,6 @@ const std::map<int, const Camera3Format> camera3FormatsMap = {
>>
>> LOG_DECLARE_CATEGORY(HAL);
>>
>> -class MappedCamera3Buffer : public MappedBuffer
>> -{
>> -public:
>> - MappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags);
>> -};
>> -
>> MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>> int flags)
>> {
>> @@ -1471,12 +1462,6 @@ void CameraDevice::requestComplete(Request *request)
>> if (cameraStream->outputFormat() != HAL_PIXEL_FORMAT_BLOB)
>> continue;
>>
>> - Encoder *encoder = cameraStream->encoder();
>> - if (!encoder) {
>> - LOG(HAL, Error) << "Failed to identify encoder";
>> - continue;
>> - }
>> -
>> StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
>> Stream *stream = streamConfiguration->stream();
>> FrameBuffer *buffer = request->findBuffer(stream);
>> @@ -1497,59 +1482,12 @@ void CameraDevice::requestComplete(Request *request)
>> continue;
>> }
>>
>> - /* 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(orientation_);
>> - exif.setSize(cameraStream->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(buffer, mapped.maps()[0], exif.data());
>> - if (jpeg_size < 0) {
>> - LOG(HAL, Error) << "Failed to encode stream image";
>> + int ret = cameraStream->process(buffer, &mapped,
>> + resultMetadata.get());
>> + if (ret) {
>> status = CAMERA3_BUFFER_STATUS_ERROR;
>> continue;
>> }
>> -
>> - /*
>> - * 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 = mapped.maps()[0].data() +
>> - 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. */
>> - resultMetadata->addEntry(ANDROID_JPEG_SIZE,
>> - &jpeg_size, 1);
>> -
>> - const uint32_t jpeg_quality = 95;
>> - resultMetadata->addEntry(ANDROID_JPEG_QUALITY,
>> - &jpeg_quality, 1);
>> -
>> - const uint32_t jpeg_orientation = 0;
>> - resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION,
>> - &jpeg_orientation, 1);
>> }
>>
>> /* Prepare to call back the Android camera stack. */
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index 4e326fa3e1fb..826023b453f7 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -20,6 +20,7 @@
>> #include <libcamera/request.h>
>> #include <libcamera/stream.h>
>>
>> +#include "libcamera/internal/buffer.h"
>> #include "libcamera/internal/log.h"
>> #include "libcamera/internal/message.h"
>>
>> @@ -28,6 +29,12 @@
>>
>> class CameraMetadata;
>>
>> +class MappedCamera3Buffer : public libcamera::MappedBuffer
>> +{
>> +public:
>> + MappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags);
>> +};
>> +
>> class CameraDevice : protected libcamera::Loggable
>> {
>> public:
>> @@ -50,6 +57,7 @@ public:
>>
>> int facing() const { return facing_; }
>> int orientation() const { return orientation_; }
>> + unsigned int maxJpegBufferSize() const { return maxJpegBufferSize_; }
>>
>> void setCallbacks(const camera3_callback_ops_t *callbacks);
>> const camera_metadata_t *getStaticMetadata();
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index 5c1f1d7da416..072edc9457bb 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -8,10 +8,14 @@
>> #include "camera_stream.h"
>>
>> #include "camera_device.h"
>> +#include "camera_metadata.h"
>> #include "jpeg/encoder_libjpeg.h"
>> +#include "jpeg/exif.h"
>>
>> using namespace libcamera;
>>
>> +LOG_DECLARE_CATEGORY(HAL);
>> +
>> CameraStream::CameraStream(CameraDevice *cameraDev,
>> camera3_stream_t *androidStream,
>> const libcamera::StreamConfiguration &cfg,
>> @@ -35,3 +39,62 @@ int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
>>
>> return 0;
>> }
>> +
>> +int CameraStream::process(libcamera::FrameBuffer *source, MappedCamera3Buffer *dest,
>> + CameraMetadata *metadata)
>> +{
>> + if (!encoder_)
>> + return -EINVAL;
>
> Maybe that's addressed by subsequent patches in this series, but down
> the line I would envision process() being called for all CameraStream
> instances, so if no processing is required, this should then return 0.
I agree here, The CameraDevice shouldn't know/care 'if/what' processing
is required by a CameraStream. The CameraStream will deal with it all.
Of course, in the current implementation, it might likely still require
the CameraDevice to notify the CameraStream that a request has completed.
At some point, I could see the notification being the Buffer complete
notification, which might be earlier than the Request complete ... (to
reduce delays) - however we will need to make all of this asynchronous
before we go there.
>> +
>> + /* 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(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);
I can see further refactoring of this to move JPEG specific handling to
a JPEG specific CameraStream instance or such - but that's not needed now.
This is all looking like good progress to me.
>> +
>> + return 0;
>> +}
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index 2d71a50c78a4..dbcd1e53219f 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -11,6 +11,7 @@
>>
>> #include <hardware/camera3.h>
>>
>> +#include <libcamera/buffer.h>
>> #include <libcamera/camera.h>
>> #include <libcamera/geometry.h>
>> #include <libcamera/pixel_format.h>
>> @@ -18,6 +19,8 @@
>> #include "jpeg/encoder.h"
>>
>> class CameraDevice;
>> +class CameraMetadata;
>> +class MappedCamera3Buffer;
>>
>> class CameraStream
>> {
>> @@ -114,9 +117,11 @@ public:
>> const libcamera::Size &size() const { return size_; }
>> Type type() const { return type_; }
>> unsigned int index() const { return index_; }
>> - Encoder *encoder() const { return encoder_.get(); }
>>
>> int configure(const libcamera::StreamConfiguration &cfg);
>> + int process(libcamera::FrameBuffer *source,
>
> Can source be const ?
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
+1
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>> + MappedCamera3Buffer *dest,
>> + CameraMetadata *metadata);
>>
>> private:
>> CameraDevice *cameraDevice_;
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list