[libcamera-devel] [PATCH 3/5] libcamera: camera_sensor: Store the camera location

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2019-08-17 12:59:35 +0200, Jacopo Mondi wrote:
> Store the camera sensor location by parsing the
> V4L2_CID_CAMERA_SENSOR_LOCATION V4L2 control.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/camera_sensor.cpp       | 25 +++++++++++++++++++++++++
>  src/libcamera/include/camera_sensor.h |  3 +++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index a7670b449b31..2703d10c719e 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -89,6 +89,31 @@ int CameraSensor::init()
>  	if (ret < 0)
>  		return ret;
>  
> +	/* Retrieve and store the camera sensor location. */
> +	V4L2ControlList controls;
> +	controls.add(V4L2_CID_CAMERA_SENSOR_LOCATION);
> +
> +	ret = subdev_->getControls(&controls);
> +	if (ret) {
> +		LOG(CameraSensor, Error)
> +			<< "Failed to get camera sensor controls: " << ret;
> +		return ret;
> +	}
> +
> +	V4L2Control *locationControl = controls[V4L2_CID_CAMERA_SENSOR_LOCATION];
> +	int64_t v4l2Location = locationControl->value();

I would skip the temporary variable here, but it's just bikeshedding ;-)

> +	switch (v4l2Location) {
> +	case V4L2_LOCATION_FRONT:
> +		location_ = CAMERA_LOCATION_FRONT;
> +		break;
> +	case V4L2_LOCATION_BACK:
> +		location_ = CAMERA_LOCATION_BACK;
> +		break;

Should we not handle V4L2_LOCATION_EXTERNAL also?

> +	default:
> +		LOG(CameraSensor, Error) << "Unsupported camera location";
> +		return -EINVAL;

Is it not a bit harsh to fail if location information is not available?  
Is it possible to set it to V4L2_LOCATION_UNKOWN and move on. For the 
Andriod HAL we could map UNKWON to EXTERNAL and print a warning, or if 
we require location information at that point skip camers which do not 
provided it.

> +	}
> +
>  	/* Enumerate and cache media bus codes and sizes. */
>  	const ImageFormats formats = subdev_->formats(0);
>  	if (formats.isEmpty()) {
> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> index fe033fb374c1..a237a1684605 100644
> --- a/src/libcamera/include/camera_sensor.h
> +++ b/src/libcamera/include/camera_sensor.h
> @@ -10,6 +10,7 @@
>  #include <string>
>  #include <vector>
>  
> +#include <libcamera/control_ids.h>
>  #include <libcamera/geometry.h>
>  
>  #include "log.h"
> @@ -55,6 +56,8 @@ private:
>  
>  	std::vector<unsigned int> mbusCodes_;
>  	std::vector<Size> sizes_;
> +
> +	CameraLocation location_;
>  };
>  
>  } /* namespace libcamera */
> -- 
> 2.22.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund