[libcamera-devel] [PATCH 05/15] android: camera_device: Move processing to CameraStream
Umang Jain
email at uajain.com
Tue Oct 6 20:17:12 CEST 2020
Hi all,
On 10/6/20 3:12 AM, Kieran Bingham wrote:
> 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.
Broadly agree with the direction. This is where I en-vision the
PostProcessor layer comes into play.
> 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_;
More information about the libcamera-devel
mailing list