[libcamera-devel] [PATCH v5 3/5] libcamera: camera: Generate camera name from a CameraSensor
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Aug 2 22:39:04 CEST 2020
Hi Niklas,
Thank you for the patch.
On Wed, Jul 29, 2020 at 01:50:53PM +0200, Niklas Söderlund wrote:
> Add a create() that generates a camera name from information in a
> CameraSensor. The intention is to help pipeline handlers that already
> uses CameraSensor to not have to worry about how to generate a unique
> and persistent name.
>
> All pipelines that use a CameraSensor (all but UVC) are updated to make
> use of this new function. All names of cameras created by these updated
> pipelines are modified.
>
> Before this change example of camera names:
>
> * OF based systems
> ov5695 7-0036
> ov2685 7-003c
>
> * ACPI based systems
> ov13858 8-0010
> ov5670 10-0036
>
> After this change the same cameras are:
>
> * OF based systems
> base/i2c at ff160000/camera at 36
> base/i2c at ff160000/camera at 3c
>
> * ACPI based systems
> \_SB_.PCI0.I2C2.CAM0
> \_SB_.PCI0.I2C4.CAM1
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> * Changes since v3
> - Update commit message.
> - Do not include property_ids.h.
> ---
> include/libcamera/camera.h | 5 +++++
> src/libcamera/camera.cpp | 18 ++++++++++++++++++
> src/libcamera/pipeline/ipu3/ipu3.cpp | 12 +++++-------
> .../pipeline/raspberrypi/raspberrypi.cpp | 3 ++-
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> src/libcamera/pipeline/simple/simple.cpp | 2 +-
> src/libcamera/pipeline/vimc/vimc.cpp | 4 ++--
> 7 files changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 4d1a4a9f52ec0fac..784510c9a79d44b9 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -19,6 +19,7 @@
>
> namespace libcamera {
>
> +class CameraSensor;
> class FrameBuffer;
> class FrameBufferAllocator;
> class PipelineHandler;
> @@ -73,6 +74,10 @@ public:
> const std::string &name,
> const std::set<Stream *> &streams);
>
> + static std::shared_ptr<Camera> create(PipelineHandler *pipe,
> + const CameraSensor *sensor,
> + const std::set<Stream *> &streams);
> +
> Camera(const Camera &) = delete;
> Camera &operator=(const Camera &) = delete;
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 69a1b4428e3f4eca..02e2369ef0089465 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -14,6 +14,7 @@
> #include <libcamera/request.h>
> #include <libcamera/stream.h>
>
> +#include "libcamera/internal/camera_sensor.h"
> #include "libcamera/internal/log.h"
> #include "libcamera/internal/pipeline_handler.h"
> #include "libcamera/internal/utils.h"
> @@ -473,6 +474,23 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
> return std::shared_ptr<Camera>(camera, Deleter());
> }
>
> +/**
> + * \brief Create a camera instance
> + * \param[in] pipe The pipeline handler responsible for the camera device
> + * \param[in] sensor The sensor of the camera device
> + * \param[in] streams Array of streams the camera provides
> + *
> + * Create a camera with name generated from \a sensor.
> + *
> + * \return A shared pointer to the newly created camera object
> + */
> +std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
> + const CameraSensor *sensor,
> + const std::set<Stream *> &streams)
> +{
> + return Camera::create(pipe, sensor->id(), streams);
> +}
I think I'd prefer passing the id() directly to the existing create()
function, to have a single API for pipeline handlers. I agree there's
pros and cons though, so I can be convinced otherwise.
In any case, the name argument to the existing function should be
renamed to id, and the documentation updated accordingly to explain what
the id and and how is has to be unique and stable (if we keep a single
create() implementation, likely with a mention that the id shall be
retrieved from a CameraSensor for the pipeline handlers that can do so).
> +
> /**
> * \brief Retrieve the name of the camera
> * \context This function is \threadsafe.
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index eb00eecfd10a89e4..72da6ed62a7f0de5 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -813,18 +813,16 @@ int PipelineHandlerIPU3::registerCameras()
> &IPU3CameraData::imguOutputBufferReady);
>
> /* Create and register the Camera instance. */
> - std::string cameraName = cio2->sensor()->entity()->name();
> - std::shared_ptr<Camera> camera = Camera::create(this,
> - cameraName,
> - streams);
> -
> - registerCamera(std::move(camera), std::move(data));
> + std::shared_ptr<Camera> camera =
> + Camera::create(this, cio2->sensor(), streams);
>
> LOG(IPU3, Info)
> << "Registered Camera[" << numCameras << "] \""
> - << cameraName << "\""
> + << camera->name() << "\""
> << " connected to CSI-2 receiver " << id;
>
> + registerCamera(std::move(camera), std::move(data));
> +
> numCameras++;
> }
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 82a0a4dfd6824fce..a62dd24b1ab76b87 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -972,7 +972,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> streams.insert(&data->isp_[Isp::Stats]);
>
> /* Create and register the camera. */
> - std::shared_ptr<Camera> camera = Camera::create(this, data->sensor_->model(), streams);
> + std::shared_ptr<Camera> camera =
> + Camera::create(this, data->sensor_, streams);
> registerCamera(std::move(camera), std::move(data));
>
> return true;
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 52a0d862417cc4ec..663e45b109aae9eb 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -971,7 +971,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>
> std::set<Stream *> streams{ &data->stream_ };
> std::shared_ptr<Camera> camera =
> - Camera::create(this, sensor->name(), streams);
> + Camera::create(this, data->sensor_, streams);
> registerCamera(std::move(camera), std::move(data));
>
> return 0;
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 0bab5af86f05d63c..1258f81284590060 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -815,7 +815,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> continue;
>
> std::shared_ptr<Camera> camera =
> - Camera::create(this, data->sensor_->entity()->name(),
> + Camera::create(this, data->sensor_.get(),
> data->streams());
> registerCamera(std::move(camera), std::move(data));
> }
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 4f461b928514022d..e1fc087f111b0bc4 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -432,9 +432,9 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> return false;
>
> /* Create and register the camera. */
> - std::string name{ "VIMC " + data->sensor_->model() };
> std::set<Stream *> streams{ &data->stream_ };
> - std::shared_ptr<Camera> camera = Camera::create(this, name, streams);
> + std::shared_ptr<Camera> camera
> + = Camera::create(this, data->sensor_, streams);
> registerCamera(std::move(camera), std::move(data));
>
> return true;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list