[libcamera-devel,05/15] android: camera_device: Move processing to CameraStream
diff mbox series

Message ID 20201005104649.10812-6-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • CameraStream refactoring
Related show

Commit Message

Laurent Pinchart Oct. 5, 2020, 10:46 a.m. UTC 
From: Jacopo Mondi <jacopo@jmondi.org>

Move the JPEG processing procedure to the individual CameraStream
by augmenting the class with a CameraStream::process() method.

This allows removing the CameraStream::encoder() method.

Signed-off-by: Jacopo Mondi <jacopo@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(-)

Comments

Laurent Pinchart Oct. 5, 2020, 12:08 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Mon, Oct 05, 2020 at 01:46:39PM +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Move the JPEG processing procedure to the individual CameraStream
> by augmenting the class with a CameraStream::process() method.
> 
> This allows removing the CameraStream::encoder() method.
> 
> Signed-off-by: Jacopo Mondi <jacopo@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.

> +
> +	/* 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);
> +
> +	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@ideasonboard.com>

> +		    MappedCamera3Buffer *dest,
> +		    CameraMetadata *metadata);
>  
>  private:
>  	CameraDevice *cameraDevice_;
Kieran Bingham Oct. 5, 2020, 9:42 p.m. UTC | #2 
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@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@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@ideasonboard.com>

+1

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
>> +		    MappedCamera3Buffer *dest,
>> +		    CameraMetadata *metadata);
>>  
>>  private:
>>  	CameraDevice *cameraDevice_;
>
Umang Jain Oct. 6, 2020, 6:17 p.m. UTC | #3 
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@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@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@ideasonboard.com>
> +1
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>>> +		    MappedCamera3Buffer *dest,
>>> +		    CameraMetadata *metadata);
>>>   
>>>   private:
>>>   	CameraDevice *cameraDevice_;

Patch
diff mbox series 

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;
+
+	/* 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);
+
+	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,
+		    MappedCamera3Buffer *dest,
+		    CameraMetadata *metadata);
 
 private:
 	CameraDevice *cameraDevice_;