[libcamera-devel] [PATCH 1/8] android: camera_device: Add CameraStream::Type

[Kieran Bingham]

Hi Jacopo,

On 09/09/2020 16:54, Jacopo Mondi wrote:
> Define the CameraStream::Type enumeration and assign it to
> each CameraStream instance at construction time.
> 
> The CameraStream type will be used to decide if memory needs to be
> allocated on its behalf or if the stream is backed by memory externally
> allocated by the Android framework.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 14 ++++--
>  src/android/camera_device.h   | 87 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 96 insertions(+), 5 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 50923df22a8f..3c58523e528e 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -168,9 +168,11 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>  	}
>  }
>  
> -CameraStream::CameraStream(PixelFormat format, Size size,
> +CameraStream::CameraStream(PixelFormat format, Size size, Type type,
>  			   unsigned int index, Encoder *encoder)
> -	: format_(format), size_(size), index_(index), encoder_(encoder)
> +
> +	: format_(format), size_(size), type_(type), index_(index),
> +	  encoder_(encoder)
>  {
>  }
>  
> @@ -1215,12 +1217,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  
>  		config_->addConfiguration(streamConfiguration);
>  		unsigned int index = config_->size() - 1;
> -		streams_.emplace_back(format, size, index);
> +		streams_.emplace_back(format, size, CameraStream::Type::Direct,
> +				      index);
>  		stream->priv = static_cast<void *>(&streams_.back());
>  	}
>  
>  	/* Now handle MJPEG streams, adding a new stream if required. */
>  	if (jpegStream) {
> +		CameraStream::Type type;
>  		int index = -1;
>  
>  		/* Search for a compatible stream in the non-JPEG ones. */
> @@ -1238,6 +1242,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			LOG(HAL, Info)
>  				<< "Android JPEG stream mapped on stream " << i;
>  
> +			type = CameraStream::Type::Mapped;
>  			index = i;
>  			break;
>  		}
> @@ -1262,6 +1267,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
>  				       << " for MJPEG support";
>  
> +			type = CameraStream::Type::Internal;
>  			config_->addConfiguration(streamConfiguration);
>  			index = config_->size() - 1;
>  		}
> @@ -1280,7 +1286,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			return ret;
>  		}
>  
> -		streams_.emplace_back(formats::MJPEG, cfg.size, index, encoder);
> +		streams_.emplace_back(formats::MJPEG, cfg.size, type, index, encoder);
>  		jpegStream->priv = static_cast<void *>(&streams_.back());
>  	}
>  
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 912c59aeb5a8..9dea7c42bdb5 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -30,17 +30,102 @@ class CameraMetadata;
>  class CameraStream
>  {
>  public:
> +	/*
> +	 * Enumeration of CameraStream types.
> +	 *
> +	 * A camera stream associates an Android stream to a libcamera stream.
> +	 * This enumeration describes how the two streams are associated and how
> +	 * and where data produced from libcamera are delivered to the
> +	 * Android framework.
> +	 *
> +	 * Direct:
> +	 *
> +	 * The Android stream is directly mapped onto a libcamera stream: frames
> +	 * are delivered by the library directly in the memory location
> +	 * specified by the Android stream (buffer_handle_t->data) and provided
> +	 * to the framework as they are. The Android stream characteristics are
> +	 * directly translated to the libcamera stream configuration.
> +	 *
> +	 * +-----+                +-----+
> +	 * |  A  |                |  L  |
> +	 * +-----+                +-----+
> +	 *    |                      |
> +	 *    V                      V
> +	 * +-----+                +------+
> +	 * |  B  |<---------------|  FB  |
> +	 * +-----+                +------+
> +	 *
> +	 *
> +	 * Internal:
> +	 *
> +	 * Data for the Android stream are produced by processing a libcamera

stream is produced

> +	 * stream created by the HAL for that purpose. The libcamera stream
> +	 * needs to be supplied with intermediate buffers where the library
> +	 * delivers frames to be processed and then provided to the framework.
> +	 * The libcamera stream configuration is not a direct translation of the
> +	 * Android stream characteristics, but it describes the format and size
> +	 * required for the processing procedure to produce frames in the
> +	 * Android required format.
> +	 *
> +	 * +-----+                +-----+    +----+
> +	 * |  A  |                |  L  |--->|  B |
> +	 * +-----+                +-----+    +----+
> +	 *    |                      |         ^
> +	 *    V                      V         |
> +	 * +-----+                +------+     |
> +	 * |  B  |                |  FB  |-----+
> +	 * +-----+                +------+

I think libcamera always writes to a FrameBuffer right? Shouldn't 'B' be
an intermeddiate step between FB and processing?

The arrows seem a bit odd otherwise, L points to both B and FB, but 'B'
doesn't go anywhere?

> +	 *   ^                       |
> +	 *   |-------Processing------|
> +	 *
> +	 *
> +	 * Mapped:
> +	 *
> +	 * Data for the Android stream are produced by processing a libcamera

/stream are produced/stream is produced/

Other than that, this all sounds good.

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


> +	 * stream associated with another CameraStream. Mapped camera streams do
> +	 * not need any memory to be reserved for them as they process data
> +	 * produced by libcamera for a different stream whose format and size
> +	 * is compatible with the processing procedure requirements to produce
> +	 * frames in the Android required format.
> +	 *
> +	 * +-----+      +-----+          +-----+
> +	 * |  A  |      |  A' |          |  L  |
> +	 * +-----+      +-----+          +-----+
> +	 *    |            |                |
> +	 *    V            V                V
> +	 * +-----+      +-----+          +------+
> +	 * |  B  |      |  B' |<---------|  FB  |
> +	 * +-----+      +-----+          +------+
> +	 *   ^              |
> +	 *   |--Processing--|
> +	 *
> +	 *
> +	 * --------------------------------------------------------------------
> +	 * A  = Android stream
> +	 * L  = libcamera stream
> +	 * B  = memory buffer
> +	 * FB = libcamera FrameBuffer
> +	 * "Processing" = Frame processing procedure (Encoding, scaling etc)
> +	 */
> +	enum Type {
> +		Direct,
> +		Internal,
> +		Mapped,
> +	};
> +
>  	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> -		     unsigned int index, Encoder *encoder = nullptr);
> +		     Type type, unsigned int index, Encoder *encoder = nullptr);
>  
>  	const libcamera::PixelFormat &format() const { return format_; }
>  	const libcamera::Size &size() const { return size_; }
>  	unsigned int index() const { return index_; }
>  	Encoder *encoder() const { return encoder_.get(); }
> +	Type type() const { return type_; }
>  
>  private:
>  	libcamera::PixelFormat format_;
>  	libcamera::Size size_;
> +	Type type_;
>  	/*
>  	 * The index of the libcamera StreamConfiguration as added during
>  	 * configureStreams(). A single libcamera Stream may be used to deliver
> 

-- 
Regards
--
Kieran