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

Message ID 20200922094738.5327-2-jacopo@jmondi.org
State Superseded
Headers show
Series
  • android: camera_device: Add support for internal buffers
Related show

Commit Message

Jacopo Mondi Sept. 22, 2020, 9:47 a.m. UTC
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.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 14 ++++--
 src/android/camera_device.h   | 87 ++++++++++++++++++++++++++++++++++-
 2 files changed, 96 insertions(+), 5 deletions(-)

Comments

Laurent Pinchart Sept. 29, 2020, 1:28 a.m. UTC | #1
On Tue, Sep 22, 2020 at 11:47:31AM +0200, 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.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo@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 70d77a17ef43..e4ffbc02c2da 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -169,9 +169,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)
>  {
>  }
>  
> @@ -1222,12 +1224,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 the 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. */
> @@ -1245,6 +1249,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			LOG(HAL, Info)
>  				<< "Android JPEG stream mapped to libcamera stream " << i;
>  
> +			type = CameraStream::Type::Mapped;
>  			index = i;
>  			break;
>  		}
> @@ -1269,6 +1274,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;
>  		}
> @@ -1287,7 +1293,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 1837748d2efc..9a9406cc257c 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 is produced by processing a libcamera
> +	 * 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  |--->|  FB |
> +	 * +-----+                +-----+    +-----+
> +	 *    |                      |         |
> +	 *    V                      V         |
> +	 * +-----+                +------+     |
> +	 * |  B  |                |   B  |<----+
> +	 * +-----+                +------+
> +	 *   ^                       |
> +	 *   |-------Processing------|

Maybe there's something I don't get here, but isn't the processing from
FB to B, not from B to B ? Shouldn't it look like this ?

	 * +-----+                +-----+
	 * |  A  |                |  L  |
	 * +-----+                +-----+
	 *    |                      |
	 *    V                      V
	 * +-----+                +------+
	 * |  B  |                |  FB  |
	 * +-----+                +------+
	 *    ^                      |
	 *    |------Processing------|

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	 *
> +	 *
> +	 * Mapped:
> +	 *
> +	 * Data for the Android stream is produced by processing a libcamera
> +	 * 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 class 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_; }
> +	Type type() const { return type_; }
>  	unsigned int index() const { return index_; }
>  	Encoder *encoder() const { return encoder_.get(); }
>  
>  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
Jacopo Mondi Sept. 30, 2020, 9:14 a.m. UTC | #2
Hi Laurent,

On Tue, Sep 29, 2020 at 04:28:24AM +0300, Laurent Pinchart wrote:
> On Tue, Sep 22, 2020 at 11:47:31AM +0200, 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.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Signed-off-by: Jacopo Mondi <jacopo@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 70d77a17ef43..e4ffbc02c2da 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -169,9 +169,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)
> >  {
> >  }
> >
> > @@ -1222,12 +1224,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 the 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. */
> > @@ -1245,6 +1249,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  			LOG(HAL, Info)
> >  				<< "Android JPEG stream mapped to libcamera stream " << i;
> >
> > +			type = CameraStream::Type::Mapped;
> >  			index = i;
> >  			break;
> >  		}
> > @@ -1269,6 +1274,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;
> >  		}
> > @@ -1287,7 +1293,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 1837748d2efc..9a9406cc257c 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 is produced by processing a libcamera
> > +	 * 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  |--->|  FB |
> > +	 * +-----+                +-----+    +-----+
> > +	 *    |                      |         |
> > +	 *    V                      V         |
> > +	 * +-----+                +------+     |
> > +	 * |  B  |                |   B  |<----+
> > +	 * +-----+                +------+
> > +	 *   ^                       |
> > +	 *   |-------Processing------|
>
> Maybe there's something I don't get here, but isn't the processing from
> FB to B, not from B to B ? Shouldn't it look like this ?
>
> 	 * +-----+                +-----+
> 	 * |  A  |                |  L  |
> 	 * +-----+                +-----+
> 	 *    |                      |
> 	 *    V                      V
> 	 * +-----+                +------+
> 	 * |  B  |                |  FB  |
> 	 * +-----+                +------+
> 	 *    ^                      |
> 	 *    |------Processing------|

Well, as we haven't really formalized what the arrow means at all, and
this diagram is just for reference, I guess we can play it as we like
the most. The original meant that that libcamera creates a framebuffer
that wraps a buffer allocated by libcamera. I can remove the right
part of the drawing if it's confusing.

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks
  j

>
> > +	 *
> > +	 *
> > +	 * Mapped:
> > +	 *
> > +	 * Data for the Android stream is produced by processing a libcamera
> > +	 * 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 class 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_; }
> > +	Type type() const { return type_; }
> >  	unsigned int index() const { return index_; }
> >  	Encoder *encoder() const { return encoder_.get(); }
> >
> >  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,
>
> Laurent Pinchart
Laurent Pinchart Oct. 2, 2020, 1:58 a.m. UTC | #3
Hi Jacopo,

On Wed, Sep 30, 2020 at 11:14:43AM +0200, Jacopo Mondi wrote:
> On Tue, Sep 29, 2020 at 04:28:24AM +0300, Laurent Pinchart wrote:
> > On Tue, Sep 22, 2020 at 11:47:31AM +0200, 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.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Signed-off-by: Jacopo Mondi <jacopo@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 70d77a17ef43..e4ffbc02c2da 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -169,9 +169,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)
> > >  {
> > >  }
> > >
> > > @@ -1222,12 +1224,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 the 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. */
> > > @@ -1245,6 +1249,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > >  			LOG(HAL, Info)
> > >  				<< "Android JPEG stream mapped to libcamera stream " << i;
> > >
> > > +			type = CameraStream::Type::Mapped;
> > >  			index = i;
> > >  			break;
> > >  		}
> > > @@ -1269,6 +1274,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;
> > >  		}
> > > @@ -1287,7 +1293,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 1837748d2efc..9a9406cc257c 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 is produced by processing a libcamera
> > > +	 * 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  |--->|  FB |
> > > +	 * +-----+                +-----+    +-----+
> > > +	 *    |                      |         |
> > > +	 *    V                      V         |
> > > +	 * +-----+                +------+     |
> > > +	 * |  B  |                |   B  |<----+
> > > +	 * +-----+                +------+
> > > +	 *   ^                       |
> > > +	 *   |-------Processing------|
> >
> > Maybe there's something I don't get here, but isn't the processing from
> > FB to B, not from B to B ? Shouldn't it look like this ?
> >
> > 	 * +-----+                +-----+
> > 	 * |  A  |                |  L  |
> > 	 * +-----+                +-----+
> > 	 *    |                      |
> > 	 *    V                      V
> > 	 * +-----+                +------+
> > 	 * |  B  |                |  FB  |
> > 	 * +-----+                +------+
> > 	 *    ^                      |
> > 	 *    |------Processing------|
> 
> Well, as we haven't really formalized what the arrow means at all, and
> this diagram is just for reference, I guess we can play it as we like
> the most. The original meant that that libcamera creates a framebuffer
> that wraps a buffer allocated by libcamera. I can remove the right
> part of the drawing if it's confusing.

Ah I get it now. I don't mind keeping it, but I think the arrows should
be explained then. I was confused :-)

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks
> 
> > > +	 *
> > > +	 *
> > > +	 * Mapped:
> > > +	 *
> > > +	 * Data for the Android stream is produced by processing a libcamera
> > > +	 * 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 class 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_; }
> > > +	Type type() const { return type_; }
> > >  	unsigned int index() const { return index_; }
> > >  	Encoder *encoder() const { return encoder_.get(); }
> > >
> > >  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
Jacopo Mondi Oct. 2, 2020, 2:06 p.m. UTC | #4
Hi Laurent,

On Fri, Oct 02, 2020 at 04:58:21AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Sep 30, 2020 at 11:14:43AM +0200, Jacopo Mondi wrote:
> > On Tue, Sep 29, 2020 at 04:28:24AM +0300, Laurent Pinchart wrote:
> > > On Tue, Sep 22, 2020 at 11:47:31AM +0200, 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.
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > Signed-off-by: Jacopo Mondi <jacopo@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 70d77a17ef43..e4ffbc02c2da 100644
> > > > --- a/src/android/camera_device.cpp
> > > > +++ b/src/android/camera_device.cpp
> > > > @@ -169,9 +169,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)
> > > >  {
> > > >  }
> > > >
> > > > @@ -1222,12 +1224,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 the 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. */
> > > > @@ -1245,6 +1249,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > >  			LOG(HAL, Info)
> > > >  				<< "Android JPEG stream mapped to libcamera stream " << i;
> > > >
> > > > +			type = CameraStream::Type::Mapped;
> > > >  			index = i;
> > > >  			break;
> > > >  		}
> > > > @@ -1269,6 +1274,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;
> > > >  		}
> > > > @@ -1287,7 +1293,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 1837748d2efc..9a9406cc257c 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 is produced by processing a libcamera
> > > > +	 * 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  |--->|  FB |
> > > > +	 * +-----+                +-----+    +-----+
> > > > +	 *    |                      |         |
> > > > +	 *    V                      V         |
> > > > +	 * +-----+                +------+     |
> > > > +	 * |  B  |                |   B  |<----+
> > > > +	 * +-----+                +------+
> > > > +	 *   ^                       |
> > > > +	 *   |-------Processing------|
> > >
> > > Maybe there's something I don't get here, but isn't the processing from
> > > FB to B, not from B to B ? Shouldn't it look like this ?
> > >
> > > 	 * +-----+                +-----+
> > > 	 * |  A  |                |  L  |
> > > 	 * +-----+                +-----+
> > > 	 *    |                      |
> > > 	 *    V                      V
> > > 	 * +-----+                +------+
> > > 	 * |  B  |                |  FB  |
> > > 	 * +-----+                +------+
> > > 	 *    ^                      |
> > > 	 *    |------Processing------|
> >
> > Well, as we haven't really formalized what the arrow means at all, and
> > this diagram is just for reference, I guess we can play it as we like
> > the most. The original meant that that libcamera creates a framebuffer
> > that wraps a buffer allocated by libcamera. I can remove the right
> > part of the drawing if it's confusing.
>
> Ah I get it now. I don't mind keeping it, but I think the arrows should
> be explained then. I was confused :-)

No worries, I've adopted what you have proposed in v4 which I sent to
the list!

Thanks
  j

>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > Thanks
> >
> > > > +	 *
> > > > +	 *
> > > > +	 * Mapped:
> > > > +	 *
> > > > +	 * Data for the Android stream is produced by processing a libcamera
> > > > +	 * 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 class 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_; }
> > > > +	Type type() const { return type_; }
> > > >  	unsigned int index() const { return index_; }
> > > >  	Encoder *encoder() const { return encoder_.get(); }
> > > >
> > > >  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,
>
> Laurent Pinchart

Patch

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 70d77a17ef43..e4ffbc02c2da 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -169,9 +169,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)
 {
 }
 
@@ -1222,12 +1224,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 the 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. */
@@ -1245,6 +1249,7 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 			LOG(HAL, Info)
 				<< "Android JPEG stream mapped to libcamera stream " << i;
 
+			type = CameraStream::Type::Mapped;
 			index = i;
 			break;
 		}
@@ -1269,6 +1274,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;
 		}
@@ -1287,7 +1293,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 1837748d2efc..9a9406cc257c 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 is produced by processing a libcamera
+	 * 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  |--->|  FB |
+	 * +-----+                +-----+    +-----+
+	 *    |                      |         |
+	 *    V                      V         |
+	 * +-----+                +------+     |
+	 * |  B  |                |   B  |<----+
+	 * +-----+                +------+
+	 *   ^                       |
+	 *   |-------Processing------|
+	 *
+	 *
+	 * Mapped:
+	 *
+	 * Data for the Android stream is produced by processing a libcamera
+	 * 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 class 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_; }
+	Type type() const { return type_; }
 	unsigned int index() const { return index_; }
 	Encoder *encoder() const { return encoder_.get(); }
 
 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