[libcamera-devel] [PATCH 02/10] libcamera: pipeline: uvcvideo: Create UVCCameraData

[Jacopo Mondi]

Hi Laurent,

On Thu, Feb 28, 2019 at 06:29:05PM +0200, Laurent Pinchart wrote:
> Subclassing CameraData will become mandatory for pipeline handlers.
> Create a new UVCCameraData class and instantiate it when creating
> cameras to prepare for that change. The video_ and stream_ fields of the
> UVC pipeline handler belong to the camera data, move them there.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/pipeline/uvcvideo.cpp | 74 +++++++++++++++++++++--------
>  1 file changed, 53 insertions(+), 21 deletions(-)
>
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index dd20bb085a29..4ad311163a95 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -13,6 +13,7 @@
>  #include "log.h"
>  #include "media_device.h"
>  #include "pipeline_handler.h"
> +#include "utils.h"
>  #include "v4l2_device.h"
>
>  namespace libcamera {
> @@ -42,21 +43,38 @@ public:
>  	bool match(DeviceEnumerator *enumerator);
>
>  private:
> +	class UVCCameraData : public CameraData
> +	{
> +	public:
> +		UVCCameraData()
nit:
        : video_(nullptr)

> +		{
> +		}
> +
> +		~UVCCameraData()
> +		{
> +			delete video_;
> +		}
> +
> +		V4L2Device *video_;
> +		Stream stream_;
> +	};
> +
> +	UVCCameraData *cameraData(const Camera *camera)
> +	{
> +		return static_cast<UVCCameraData *>(
> +			PipelineHandler::cameraData(camera));
> +	}
> +
>  	std::shared_ptr<MediaDevice> media_;
> -	V4L2Device *video_;
> -	Stream stream_;
>  };
>
>  PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> -	: PipelineHandler(manager), media_(nullptr), video_(nullptr)
> +	: PipelineHandler(manager), media_(nullptr)
>  {
>  }
>
>  PipelineHandlerUVC::~PipelineHandlerUVC()
>  {
> -	if (video_)
> -		delete video_;
> -
>  	if (media_)
>  		media_->release();
>  }
> @@ -65,8 +83,9 @@ std::map<Stream *, StreamConfiguration>
>  PipelineHandlerUVC::streamConfiguration(Camera *camera,
>  					std::vector<Stream *> &streams)
>  {
> +	UVCCameraData *data = cameraData(camera);
> +
>  	std::map<Stream *, StreamConfiguration> configs;
> -
>  	StreamConfiguration config{};
>
>  	LOG(UVC, Debug) << "Retrieving default format";
> @@ -75,7 +94,7 @@ PipelineHandlerUVC::streamConfiguration(Camera *camera,
>  	config.pixelFormat = V4L2_PIX_FMT_YUYV;
>  	config.bufferCount = 4;
>
> -	configs[&stream_] = config;
> +	configs[&data->stream_] = config;
>
>  	return configs;
>  }
> @@ -83,7 +102,8 @@ PipelineHandlerUVC::streamConfiguration(Camera *camera,
>  int PipelineHandlerUVC::configureStreams(Camera *camera,
>  					 std::map<Stream *, StreamConfiguration> &config)
>  {
> -	StreamConfiguration *cfg = &config[&stream_];
> +	UVCCameraData *data = cameraData(camera);
> +	StreamConfiguration *cfg = &config[&data->stream_];
>  	int ret;
>
>  	LOG(UVC, Debug) << "Configure the camera for resolution "
> @@ -94,7 +114,7 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,
>  	format.height = cfg->height;
>  	format.fourcc = cfg->pixelFormat;
>
> -	ret = video_->setFormat(&format);
> +	ret = data->video_->setFormat(&format);
>  	if (ret)
>  		return ret;
>
> @@ -108,31 +128,36 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,
>
>  int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)
>  {
> +	UVCCameraData *data = cameraData(camera);
>  	const StreamConfiguration &cfg = stream->configuration();
>
>  	LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers";
>
> -	return video_->exportBuffers(&stream->bufferPool());
> +	return data->video_->exportBuffers(&stream->bufferPool());
>  }
>
>  int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream)
>  {
> -	return video_->releaseBuffers();
> +	UVCCameraData *data = cameraData(camera);
> +	return data->video_->releaseBuffers();
>  }
>
>  int PipelineHandlerUVC::start(const Camera *camera)
>  {
> -	return video_->streamOn();
> +	UVCCameraData *data = cameraData(camera);
> +	return data->video_->streamOn();
>  }
>
>  void PipelineHandlerUVC::stop(const Camera *camera)
>  {
> -	video_->streamOff();
> +	UVCCameraData *data = cameraData(camera);
> +	data->video_->streamOff();
>  }
>
>  int PipelineHandlerUVC::queueRequest(const Camera *camera, Request *request)
>  {
> -	Buffer *buffer = request->findBuffer(&stream_);
> +	UVCCameraData *data = cameraData(camera);
> +	Buffer *buffer = request->findBuffer(&data->stream_);
>  	if (!buffer) {
>  		LOG(UVC, Error)
>  			<< "Attempt to queue request with invalid stream";
> @@ -140,7 +165,7 @@ int PipelineHandlerUVC::queueRequest(const Camera *camera, Request *request)
>  		return -ENOENT;
>  	}
>
> -	video_->queueBuffer(buffer);
> +	data->video_->queueBuffer(buffer);
>
>  	return 0;
>  }
> @@ -150,29 +175,36 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  	DeviceMatch dm("uvcvideo");
>
>  	media_ = enumerator->search(dm);
> -
>  	if (!media_)
>  		return false;
>
>  	media_->acquire();
>
> +	std::unique_ptr<UVCCameraData> data = utils::make_unique<UVCCameraData>();
> +
> +	/* Locate and open the default video node. */
>  	for (MediaEntity *entity : media_->entities()) {
>  		if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
> -			video_ = new V4L2Device(entity);
> +			data->video_ = new V4L2Device(entity);
>  			break;
>  		}
>  	}
>
> -	if (!video_ || video_->open()) {
> -		if (!video_)
> +	if (!data->video_ || data->video_->open()) {
> +		if (!data->video_)

This double (!data->video_) check is weird.
Could you check for !data->video_ after the for loop, and handle the
open() return code here?

This check would also possibly benefit from the video_(nullptr)
initialization in UVCCameraData constructor.

>  			LOG(UVC, Error) << "Could not find a default video device";
>
>  		return false;
>  	}
>
> -	std::vector<Stream *> streams{ &stream_ };
> +	/* Create and register the camera. */
> +	std::vector<Stream *> streams{ &data->stream_ };
>  	std::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams);
> +
> +	setCameraData(camera.get(), std::move(data));
>  	registerCamera(std::move(camera));
> +
> +	/* Enable hot-unplug notifications. */
>  	hotplugMediaDevice(media_.get());
>
>  	return true;
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190301/17bd69a6/attachment.sig>