[libcamera-devel,RFC,02/11] libcamera: media_device: Open and close media device inside populate()

971 diff mbox series

Remove the need for the caller to open and close the media device when
populating the MediaDeivce. This is done as an effort to make the usage
of the MediaDevice less error prone and the interface stricter.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/device_enumerator.cpp           |  8 +-------
 src/libcamera/media_device.cpp                | 10 ++++++++--
 test/media_device/media_device_print_test.cpp |  6 ------
 test/pipeline/ipu3/ipu3_pipeline_test.cpp     |  5 -----
 4 files changed, 9 insertions(+), 20 deletions(-)

Hi Niklas,

Thank you for the patch.

On Sun, Apr 14, 2019 at 03:34:57AM +0200, Niklas Söderlund wrote:
> Remove the need for the caller to open and close the media device when
> populating the MediaDeivce. This is done as an effort to make the usage

s/MediaDeivce/MediaDevice/

> of the MediaDevice less error prone and the interface stricter.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/device_enumerator.cpp           |  8 +-------
>  src/libcamera/media_device.cpp                | 10 ++++++++--
>  test/media_device/media_device_print_test.cpp |  6 ------
>  test/pipeline/ipu3/ipu3_pipeline_test.cpp     |  5 -----
>  4 files changed, 9 insertions(+), 20 deletions(-)
> 
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index 58b81c354a706f03..6c08806d17dbfeec 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -212,11 +212,7 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)
>  {
>  	std::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode);
>  
> -	int ret = media->open();
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = media->populate();
> +	int ret = media->populate();
>  	if (ret < 0) {
>  		LOG(DeviceEnumerator, Info)
>  			<< "Unable to populate media device " << deviceNode
> @@ -243,8 +239,6 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)
>  			return ret;
>  	}
>  
> -	media->close();
> -
>  	LOG(DeviceEnumerator, Debug)
>  		<< "Added device " << deviceNode << ": " << media->driver();
>  
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 1e9024bf97217bcf..ecb00a1d5abffe80 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -220,6 +220,10 @@ int MediaDevice::populate()
>  
>  	clear();
>  
> +	ret = open();
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * Keep calling G_TOPOLOGY until the version number stays stable.
>  	 */
> @@ -236,7 +240,7 @@ int MediaDevice::populate()
>  			LOG(MediaDevice, Error)
>  				<< "Failed to enumerate topology: "
>  				<< strerror(-ret);
> -			return ret;
> +			goto err_out;

This seems to fix a memory leak when the second call to
MEDIA_IOC_G_TOPOLOGY fails, that's nice. Could you mention it in the
commit message ?

>  		}
>  
>  		if (version == topology.topology_version)
> @@ -260,6 +264,8 @@ int MediaDevice::populate()
>  	    populatePads(topology) &&
>  	    populateLinks(topology))
>  		valid_ = true;

Please add a blank line.

> +err_out:

I'd name this label done as this isn't path restricted to errors.

> +	close();
>  
>  	delete[] ents;
>  	delete[] interfaces;
> @@ -268,7 +274,7 @@ int MediaDevice::populate()
>  
>  	if (!valid_) {
>  		clear();
> -		return -EINVAL;
> +		return ret ? ret : -EINVAL;

That's not very nice. How about adding instead the following before this
check ?

	if (ret)
		return ret;

>  	}
>  
>  	return 0;
> diff --git a/test/media_device/media_device_print_test.cpp b/test/media_device/media_device_print_test.cpp
> index 3eef973939201b27..ceffd538e13fca73 100644
> --- a/test/media_device/media_device_print_test.cpp
> +++ b/test/media_device/media_device_print_test.cpp
> @@ -124,10 +124,6 @@ int MediaDevicePrintTest::testMediaDevice(const string deviceNode)
>  
>  	dev.close();
>  
> -	ret = dev.open();
> -	if (ret)
> -		return ret;
> -
>  	ret = dev.populate();
>  	if (ret)
>  		return ret;
> @@ -135,8 +131,6 @@ int MediaDevicePrintTest::testMediaDevice(const string deviceNode)
>  	/* Print out the media graph. */
>  	printMediaGraph(dev, cerr);
>  
> -	dev.close();
> -
>  	return 0;
>  }
>  
> diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> index 953f0233cde485cb..1d4cc4d4950b8391 100644
> --- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> @@ -71,11 +71,6 @@ int IPU3PipelineTest::init()
>  		return TestSkip;
>  	}
>  
> -	if (cio2->open()) {
> -		cerr << "Failed to open media device " << cio2->deviceNode() << endl;
> -		return TestFail;
> -	}
> -
>  	/*
>  	 * Camera sensor are connected to the CIO2 unit.
>  	 * Count how many sensors are connected in the system