[libcamera-devel] [PATCH 01/10] libcamera: pipeline: Fix double release of media devices

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2019-02-28 18:29:04 +0200, Laurent Pinchart wrote:
> Media devices are acquired in the match() function of pipeline handlers,
> and explicitly released if no match is found. The pipeline handler is
> then deleted, which causes a second release of the media device in the
> destructor. Fix this by removing the explicit release in the match()
> function.

Nice catch.

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

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 28 +++++++++++-----------------
>  src/libcamera/pipeline/uvcvideo.cpp  |  1 -
>  src/libcamera/pipeline/vimc.cpp      |  5 +----
>  3 files changed, 12 insertions(+), 22 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 9694d0ce51ab..cf5c28577393 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -278,19 +278,20 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
>  	imgu_dm.add("ipu3-imgu 1 viewfinder");
>  	imgu_dm.add("ipu3-imgu 1 3a stat");
>  
> +	/*
> +	 * It is safe to acquire both media devices at this point as
> +	 * DeviceEnumerator::search() skips the busy ones for us.
> +	 */
>  	cio2_ = enumerator->search(cio2_dm);
>  	if (!cio2_)
>  		return false;
>  
> +	cio2_->acquire();
> +
>  	imgu_ = enumerator->search(imgu_dm);
>  	if (!imgu_)
>  		return false;
>  
> -	/*
> -	 * It is safe to acquire both media devices at this point as
> -	 * DeviceEnumerator::search() skips the busy ones for us.
> -	 */
> -	cio2_->acquire();
>  	imgu_->acquire();
>  
>  	/*
> @@ -301,25 +302,18 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
>  	 * not need to be changed after.
>  	 */
>  	if (cio2_->open())
> -		goto error_release_mdev;
> +		return false;
>  
> -	if (cio2_->disableLinks())
> -		goto error_close_cio2;
> +	if (cio2_->disableLinks()) {
> +		cio2_->close();
> +		return false;
> +	}
>  
>  	registerCameras();
>  
>  	cio2_->close();
>  
>  	return true;
> -
> -error_close_cio2:
> -	cio2_->close();
> -
> -error_release_mdev:
> -	cio2_->release();
> -	imgu_->release();
> -
> -	return false;
>  }
>  
>  /*
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index b7b8ff109109..dd20bb085a29 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -167,7 +167,6 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  		if (!video_)
>  			LOG(UVC, Error) << "Could not find a default video device";
>  
> -		media_->release();
>  		return false;
>  	}
>  
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 812777cffbb3..640fca5cb0e7 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -165,11 +165,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>  	media_->acquire();
>  
>  	video_ = new V4L2Device(media_->getEntityByName("Raw Capture 1"));
> -
> -	if (video_->open()) {
> -		media_->release();
> +	if (video_->open())
>  		return false;
> -	}
>  
>  	std::vector<Stream *> streams{ &stream_ };
>  	std::shared_ptr<Camera> camera = Camera::create(this, "VIMC Sensor B",
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund