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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Oct 06, 2020 at 04:44:26PM +0200, Jacopo Mondi wrote:
> 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.

I like the new order (along with the rest of the patch). Really nice
work on this series.

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

> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp |  8 +++-----
>  src/android/camera_stream.cpp | 15 +++++----------
>  src/android/camera_stream.h   | 18 ++++--------------
>  3 files changed, 12 insertions(+), 29 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 47c423195796..678dca609003 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 9c7819efb679..3946a2cdf844 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -17,18 +17,13 @@ using namespace libcamera;
>  
>  LOG_DECLARE_CATEGORY(HAL);
>  
> -CameraStream::CameraStream(CameraDevice *cameraDevice,
> -			   camera3_stream_t *camera3Stream,
> -			   const libcamera::StreamConfiguration &cfg,
> -			   Type type, unsigned int index)
> -	: cameraDevice_(cameraDevice), camera3Stream_(camera3Stream),
> -	  type_(type), index_(index)
> +CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
> +			   camera3_stream_t *camera3Stream, unsigned int index)
> +	: cameraDevice_(cameraDevice), type_(type),
> +	  camera3Stream_(camera3Stream), index_(index)
>  {
>  	config_ = cameraDevice_->cameraConfiguration();
>  
> -	format_ = cfg.pixelFormat;
> -	size_ = cfg.size;
> -
>  	if (type_ == Type::Internal || type_ == Type::Mapped)
>  		encoder_ = std::make_unique<EncoderLibJpeg>();
>  }
> @@ -63,7 +58,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,
>  	exif.setMake("libcamera");
>  	exif.setModel("cameraModel");
>  	exif.setOrientation(cameraDevice_->orientation());
> -	exif.setSize(size_);
> +	exif.setSize(configuration().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 d8d9d8c4b237..f46cfd605d03 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -106,16 +106,11 @@ public:
>  		Internal,
>  		Mapped,
>  	};
> -	CameraStream(CameraDevice *cameraDevice,
> -		     camera3_stream_t *androidStream,
> -		     const libcamera::StreamConfiguration &cfg,
> -		     Type type, unsigned int index);
> +	CameraStream(CameraDevice *cameraDevice, Type type,
> +		     camera3_stream_t *camera3Stream, unsigned int index);
>  
> -	const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
> -	const libcamera::PixelFormat &format() const { return format_; }
> -	const libcamera::Size &size() const { return size_; }
>  	Type type() const { return type_; }
> -
> +	const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
>  	const libcamera::StreamConfiguration &configuration() const;
>  	libcamera::Stream *stream() const;
>  
> @@ -126,13 +121,8 @@ public:
>  private:
>  	CameraDevice *cameraDevice_;
>  	libcamera::CameraConfiguration *config_;
> -	camera3_stream_t *camera3Stream_;
>  	Type type_;
> -
> -	/* Libcamera facing format and sizes. */
> -	libcamera::PixelFormat format_;
> -	libcamera::Size size_;
> -
> +	camera3_stream_t *camera3Stream_;
>  	/*
>  	 * The index of the libcamera StreamConfiguration as added during
>  	 * configureStreams(). A single libcamera Stream may be used to deliver

-- 
Regards,

Laurent Pinchart