[libcamera-devel] [PATCH v3 3/5] libcamera: camera: Generate camera name from a CameraSensor
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Jul 29 10:56:20 CEST 2020
Hi Jacopo,
Thanks for your comments.
On 2020-07-29 10:07:43 +0200, Jacopo Mondi wrote:
> Hi Niklas,
>
> On Wed, Jul 29, 2020 at 01:42:23AM +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 generate unique and
>
> s/how generate/how to generate/
>
> > persistent name for cameras.
>
> or "a unique and persistent name" or "unique and persistent names".
> Source: it 'sound' more correct to me, but as a non-native speaker it
> might just be me.
>
> >
> > All pipelines that implements a CameraSensor (all but UVC) are updated
>
> s/implements/implement
>
> And I would say "use" not implement.
>
> > 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>
> > ---
> > include/libcamera/camera.h | 5 +++++
> > src/libcamera/camera.cpp | 19 +++++++++++++++++++
> > 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, 35 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,
>
> Should this be a const & ? Or is this because most pipeline handlers
> have pointers and it's nicer to pass the pointer without having to
> dereference it ?
The reason is two fold.
1. As you point out pipelines work with sensors as pointers so I thin
it's nicer to keep this interface.
2. The pipe argument is a pointer and I think it's nice to keep that
type of arguments sing the same schema.
>
> > + 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..6ad746bc2ed4ee0a 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -11,9 +11,11 @@
> > #include <iomanip>
> >
> > #include <libcamera/framebuffer_allocator.h>
> > +#include <libcamera/property_ids.h>
>
> Why property_ids.h ?
Wops, thanks for spotting this. This is a left over from v2.
>
> The rest looks good
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks!
>
> Thanks
> j
>
>
> > #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 +475,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);
> > +}
> > +
> > /**
> > * \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;
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list