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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Oct 05, 2020 at 01:46:41PM +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo at 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 at 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_;
>  };
>  

-- 
Regards,

Laurent Pinchart