[libcamera-devel] [PATCH v2 4/4] android: camera_stream: Make some member variables constant

[Umang Jain]

Hi Hiro,

Thanks for your work.

On 10/20/20 1:12 PM, Hirokazu Honda wrote:
> CameraStream initializes several member variables in the
> initializer list. Some of them are unchanged after. This makes
> them constant. Especially, doing to |cameraDevice_| represents
> CameraStream doesn't have the ownership of it.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>   src/android/camera_stream.cpp |  7 +++----
>   src/android/camera_stream.h   | 10 +++++-----
>   2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 28a6e09..9644b74 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -38,13 +38,12 @@ LOG_DECLARE_CATEGORY(HAL);
>    * and buffer allocation.
>    */
>   
> -CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
> +CameraStream::CameraStream(CameraDevice const *cameraDevice, Type type,
>   			   camera3_stream_t *camera3Stream, unsigned int index)
> -	: cameraDevice_(cameraDevice), type_(type),
> +	: cameraDevice_(cameraDevice),
> +	  config_(cameraDevice->cameraConfiguration()), type_(type),
>   	  camera3Stream_(camera3Stream), index_(index)
>   {
> -	config_ = cameraDevice_->cameraConfiguration();
> -
>   	if (type_ == Type::Internal || type_ == Type::Mapped) {
>   		/*
>   		 * \todo There might be multiple post-processors. The logic
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index c367a5f..23c139d 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -109,7 +109,7 @@ public:
>   		Internal,
>   		Mapped,
>   	};
> -	CameraStream(CameraDevice *cameraDevice, Type type,
> +	CameraStream(CameraDevice const *cameraDevice, Type type,
>   		     camera3_stream_t *camera3Stream, unsigned int index);
>   
>   	Type type() const { return type_; }
> @@ -124,11 +124,11 @@ public:
>   	void putBuffer(libcamera::FrameBuffer *buffer);
>   
>   private:
> -	CameraDevice *cameraDevice_;
> -	libcamera::CameraConfiguration *config_;
> -	Type type_;
> +	CameraDevice const *cameraDevice_;
> +	const libcamera::CameraConfiguration *config_;
> +	const Type type_;
I am not really sure if type_ should be of const type. It is owned by 
the CameraStream but yes, I don't think type_ would be mysteriously 
assigned to some other Type somewhere underneath. Jacopo, can you please 
pitch in here if this seems good to you?

Reviewed-by: Umang Jain <email at uajain.com>
>   	camera3_stream_t *camera3Stream_;
> -	unsigned int index_;
> +	const unsigned int index_;
>   
>   	std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
>   	std::vector<libcamera::FrameBuffer *> buffers_;