[libcamera-devel,07/15] android: camera_stream: Fetch format and size from configuration

9954 diff mbox series

From: Jacopo Mondi <jacopo@jmondi.org>

Fetch the format and size of the libcamera::StreamConfiguration
associated with a CameraStream by accessing the configuration by
index.

This removes the need to store the libcamera stream format and sizes
as class members and avoid duplicating information that might get out
of sync.

It also allows to remove the StreamConfiguration from the constructor
parameters list, as it can be identified by its index. While at it,
re-order the constructor parameters order.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp |  8 +++-----
 src/android/camera_stream.cpp | 23 ++++++++++++++---------
 src/android/camera_stream.h   | 18 ++++++------------
 3 files changed, 23 insertions(+), 26 deletions(-)

Hi Jacopo,

Thank you for the patch.

On Mon, Oct 05, 2020 at 01:46:41PM +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Fetch the format and size of the libcamera::StreamConfiguration
> associated with a CameraStream by accessing the configuration by
> index.
> 
> This removes the need to store the libcamera stream format and sizes
> as class members and avoid duplicating information that might get out
> of sync.
> 
> It also allows to remove the StreamConfiguration from the constructor
> parameters list, as it can be identified by its index. While at it,
> re-order the constructor parameters order.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp |  8 +++-----
>  src/android/camera_stream.cpp | 23 ++++++++++++++---------
>  src/android/camera_stream.h   | 18 ++++++------------
>  3 files changed, 23 insertions(+), 26 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 4f8f3e5790ca..adaec54dbfdb 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1206,9 +1206,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		streamConfiguration.pixelFormat = format;
>  
>  		config_->addConfiguration(streamConfiguration);
> -		unsigned int index = config_->size() - 1;
> -		streams_.emplace_back(this, stream, streamConfiguration,
> -				      CameraStream::Type::Direct, index);
> +		streams_.emplace_back(this, CameraStream::Type::Direct,
> +				      stream, config_->size() - 1);
>  		stream->priv = static_cast<void *>(&streams_.back());
>  	}
>  
> @@ -1262,8 +1261,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			index = config_->size() - 1;
>  		}
>  
> -		StreamConfiguration &cfg = config_->at(index);
> -		streams_.emplace_back(this, jpegStream, cfg, type, index);
> +		streams_.emplace_back(this, type, jpegStream, index);
>  		jpegStream->priv = static_cast<void *>(&streams_.back());
>  	}
>  
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 250f0ab0a3b4..6e7419ae31b8 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -16,18 +16,13 @@ using namespace libcamera;
>  
>  LOG_DECLARE_CATEGORY(HAL);
>  
> -CameraStream::CameraStream(CameraDevice *cameraDev,
> -			   camera3_stream_t *androidStream,
> -			   const libcamera::StreamConfiguration &cfg,
> -			   Type type, unsigned int index)
> -	: cameraDevice_(cameraDev), androidStream_(androidStream), type_(type),
> +CameraStream::CameraStream(CameraDevice *cameraDev, Type type,
> +			   camera3_stream_t *androidStream, unsigned int index)
> +	: cameraDevice_(cameraDev), type_(type), androidStream_(androidStream),
>  	  index_(index), encoder_(nullptr)
>  {
>  	config_ = cameraDevice_->cameraConfiguration();
>  
> -	format_ = cfg.pixelFormat;
> -	size_ = cfg.size;
> -
>  	if (type_ == Type::Internal || type_ == Type::Mapped)
>  		encoder_.reset(new EncoderLibJpeg);
>  }
> @@ -42,6 +37,16 @@ Stream *CameraStream::stream() const
>  	return streamConfiguration().stream();
>  }
>  
> +const PixelFormat &CameraStream::format() const
> +{
> +	return streamConfiguration().pixelFormat;
> +}
> +
> +const Size &CameraStream::size() const
> +{
> +	return streamConfiguration().size;
> +}
> +
>  int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
>  {
>  	if (encoder_)
> @@ -62,7 +67,7 @@ int CameraStream::process(libcamera::FrameBuffer *source, MappedCamera3Buffer *d
>  	exif.setMake("libcamera");
>  	exif.setModel("cameraModel");
>  	exif.setOrientation(cameraDevice_->orientation());
> -	exif.setSize(size_);
> +	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
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index fa295a69404f..b67b4c0ca0b3 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -107,18 +107,16 @@ public:
>  		Internal,
>  		Mapped,
>  	};
> -	CameraStream(CameraDevice *cameraDev,
> -		     camera3_stream_t *androidStream,
> -		     const libcamera::StreamConfiguration &cfg,
> -		     Type type, unsigned int index);
> +	CameraStream(CameraDevice *cameraDev, Type type,
> +		     camera3_stream_t *androidStream, unsigned int index);
>  
>  	int outputFormat() const { return androidStream_->format; }
> -	const libcamera::PixelFormat &format() const { return format_; }
> -	const libcamera::Size &size() const { return size_; }
>  	Type type() const { return type_; }
>  
>  	const libcamera::StreamConfiguration &streamConfiguration() const;
>  	libcamera::Stream *stream() const;
> +	const libcamera::PixelFormat &format() const;
> +	const libcamera::Size &size() const;

This is a bit ambiguous. These functions return the format and size of
the libcamera stream, which may differ from the format and size of the
Android stream. I wonder if we shouldn't use
->streamConfiguration().pixelFormat (and the same for size) in the
caller instead, to make it more explicit.

>  
>  	int configure(const libcamera::StreamConfiguration &cfg);
>  	int process(libcamera::FrameBuffer *source,
> @@ -127,13 +125,8 @@ public:
>  
>  private:
>  	CameraDevice *cameraDevice_;
> -	libcamera::CameraConfiguration *config_;
> -	camera3_stream_t *androidStream_;
>  	Type type_;
> -
> -	/* Libcamera facing format and sizes. */
> -	libcamera::PixelFormat format_;
> -	libcamera::Size size_;
> +	camera3_stream_t *androidStream_;
>  
>  	/*
>  	 * The index of the libcamera StreamConfiguration as added during
> @@ -141,6 +134,7 @@ private:
>  	 * one or more streams to the Android framework.
>  	 */
>  	unsigned int index_;
> +	libcamera::CameraConfiguration *config_;
>  	std::unique_ptr<Encoder> encoder_;
>  };
>