[libcamera-devel] [PATCH v2 13/20] libcamera: ipu3: Store CameraData as mutable in CameraConfiguration

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jul 09, 2020 at 10:41:21AM +0200, Jacopo Mondi wrote:
> A reference to the CameraData sub-class is stored in the
> IPU3CameraConfiguration as a const pointer, as the now removed comment
> reports "to allow the compiler catch un-wanted modifications during
> validate()".
> 
> As the CameraData contains pointers to the available streams, which are
> actually assigned during validate, the comment and the rationale behind
> that choice seems now moot.

The idea is that IPU3CameraData models the active state of the camera,
while IPU3CameraConfiguration models a configuration. The latter may
need to access the former to query information, but should not modify
it. That's why it's stored as a const pointer. I'd rather explore the
option of storing a const Stream * in CameraConfiguration.

> Store CameraData as a mutable pointer and remove the comment and the
> const_cast<> required to assign streams.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index feabffe641e1..9fed6c1e5aa7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -75,7 +75,7 @@ private:
>  	 * reference to the camera data, store a new reference to the camera.
>  	 */
>  	std::shared_ptr<Camera> camera_;
> -	const IPU3CameraData *data_;
> +	IPU3CameraData *data_;
>  
>  	StreamConfiguration cio2Configuration_;
>  	std::vector<const Stream *> streams_;
> @@ -207,7 +207,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  			cfg.size = cio2Configuration_.size;
>  			cfg.pixelFormat = cio2Configuration_.pixelFormat;
>  			cfg.bufferCount = cio2Configuration_.bufferCount;
> -			cfg.setStream(const_cast<Stream *>(&data_->rawStream_));
> +			cfg.setStream(&data_->rawStream_);
>  
>  			LOG(IPU3, Debug) << "Assigned " << cfg.toString()
>  					 << " to the raw stream";
> @@ -256,27 +256,19 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  		cfg.bufferCount = IPU3_BUFFER_COUNT;
>  		cfg.pixelFormat = formats::NV12;
>  
> -		/*
> -		 * Use a const_cast<> here instead of storing a mutable stream
> -		 * pointer in the configuration to let the compiler catch
> -		 * unwanted modifications of camera data in the configuration
> -		 * validate() implementation.
> -		 */
> -		Stream *stream;
>  		if (mainOutputAvailable &&
>  		    (oldCfg.size == yuvSize || outCount == 1)) {
> -			stream = const_cast<Stream *>(&data_->outStream_);
> +			cfg.setStream(&data_->outStream_);
>  			mainOutputAvailable = false;
>  
>  			LOG(IPU3, Debug) << "Assigned " << cfg.toString()
>  					 << " to the main output";
>  		} else {
> -			stream = const_cast<Stream *>(&data_->vfStream_);
> +			cfg.setStream(&data_->vfStream_);
>  
>  			LOG(IPU3, Debug) << "Assigned " << cfg.toString()
>  					 << " to the viewfinder output";
>  		}
> -		cfg.setStream(stream);
>  
>  		if (cfg.pixelFormat != oldCfg.pixelFormat ||
>  		    cfg.size != oldCfg.size) {

-- 
Regards,

Laurent Pinchart