[libcamera-devel] [PATCH v3 4/6] libcamera: camera: extend camera object to support streams
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jan 28 00:17:44 CET 2019
Hi Niklas,
Thank you for the patch.
On Sun, Jan 27, 2019 at 01:22:06AM +0100, Niklas Söderlund wrote:
> A camera consist of one or more video streams origination from the same
s/consist/consists/
> video source. The different streams could for example have access to
> different hardware blocks in the video pipeline and therefor be able to
> process the video source in different ways.
>
> All static information describing each streams needs to be recorded at
Information is plural, so s/needs/need/
> camera creation. After a camera is created an application can retrieve
> the static information about its stream at any time.
>
> Update all pipeline handlers to register one stream per camera, this
> might be extended in the future for some of the pipelines.
Maybe s/might/will/ :-)
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> include/libcamera/camera.h | 10 ++++-
> src/libcamera/camera.cpp | 55 ++++++++++++++++++++++++++--
> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-
> src/libcamera/pipeline/uvcvideo.cpp | 4 +-
> src/libcamera/pipeline/vimc.cpp | 4 +-
> 5 files changed, 69 insertions(+), 8 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 7e358f8c0aa093cf..786d4d7d66bed5b2 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -15,12 +15,14 @@
> namespace libcamera {
>
> class PipelineHandler;
> +class Stream;
>
> class Camera final
> {
> public:
> static std::shared_ptr<Camera> create(PipelineHandler *pipe,
> - const std::string &name);
> + const std::string &name,
> + const std::vector<Stream> &streams);
>
> Camera(const Camera &) = delete;
> void operator=(const Camera &) = delete;
> @@ -32,6 +34,8 @@ public:
> int acquire();
> void release();
>
> + std::vector<Stream> streams() const;
Do we need to return a copy, or could we return a const
std::vector<Stream> & ?
> +
> private:
> Camera(PipelineHandler *pipe, const std::string &name);
> ~Camera();
> @@ -39,10 +43,14 @@ private:
> friend class PipelineHandler;
> void disconnect();
>
> + bool haveStreamID(unsigned int id) const;
I'd name this hasStreamID, or possibly even hasStreamId.
> +
> std::shared_ptr<PipelineHandler> pipe_;
> std::string name_;
> + std::vector<Stream> streams_;
>
> bool acquired_;
> + bool disconnected_;
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 500976b237bcbd2d..3b2c00d0a4bb45d1 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -6,6 +6,7 @@
> */
>
> #include <libcamera/camera.h>
> +#include <libcamera/stream.h>
>
> #include "log.h"
> #include "pipeline_handler.h"
> @@ -56,13 +57,15 @@ LOG_DECLARE_CATEGORY(Camera)
> * \brief Create a camera instance
> * \param[in] name The name of the camera device
> * \param[in] pipe The pipeline handler responsible for the camera device
> + * \param[in] streams Array of streams the camera shall provide
s/shall provide/provides/ ?
> *
> * The caller is responsible for guaranteeing unicity of the camera name.
> *
> * \return A shared pointer to the newly created camera object
> */
> std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
> - const std::string &name)
> + const std::string &name,
> + const std::vector<Stream> &streams)
> {
> struct Allocator : std::allocator<Camera> {
> void construct(void *p, PipelineHandler *pipe,
> @@ -76,7 +79,19 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
> }
> };
>
> - return std::allocate_shared<Camera>(Allocator(), pipe, name);
> + std::shared_ptr<Camera> camera =
> + std::allocate_shared<Camera>(Allocator(), pipe, name);
> +
> + for (const Stream &stream : streams) {
> + if (camera->haveStreamID(stream.id())) {
> + LOG(Camera, Error) << "Duplication of stream ID";
> + camera.reset();
> + break;
> + }
I think we can do better. Instead of returning a null pointer (which by
the way you don't check for in the pipeline handlers, so I expect
third-party pipeline handlers not to handle that error correctly
either), we should make an API that can't be used incorrectly. For
instance you could pass a vector of streams without any ID assign, and
assign IDs incrementally starting from 0 in this function. The
haveStreamID() method won't be needed anymore.
> + camera->streams_.push_back(stream);
> + }
> +
> + return camera;
> }
>
> /**
> @@ -102,7 +117,8 @@ const std::string &Camera::name() const
> */
>
> Camera::Camera(PipelineHandler *pipe, const std::string &name)
> - : pipe_(pipe->shared_from_this()), name_(name), acquired_(false)
> + : pipe_(pipe->shared_from_this()), name_(name), acquired_(false),
> + disconnected_(false)
> {
> }
>
> @@ -125,10 +141,24 @@ void Camera::disconnect()
> {
> LOG(Camera, Debug) << "Disconnecting camera " << name_;
>
> - /** \todo Block API calls when they will be implemented. */
> + disconnected_ = true;
> disconnected.emit(this);
> }
>
> +/**
> + * \brief Check if camera have a stream ID
> + * \param[in] id Stream ID to check for
> + * \return ture if stream ID exists, else false
> + */
> +bool Camera::haveStreamID(unsigned int id) const
> +{
> + for (const Stream &stream : streams_)
> + if (stream.id() == id)
> + return true;
> +
> + return false;
> +}
> +
> /**
> * \brief Acquire the camera device for exclusive access
> *
> @@ -164,4 +194,21 @@ void Camera::release()
> acquired_ = false;
> }
>
> +/**
> + * \brief Retrieve all the camera's stream information
> + *
> + * Retrieve all of the camera's static stream information. The static
> + * information describes among other things how many streams the camera support
Let's wrap documentation at 80 columns by default if not very
impractical.
> + * and each streams capabilities.
> + *
> + * \return An array of all the camera's streams or an empty list on error.
> + */
> +std::vector<Stream> Camera::streams() const
> +{
> + if (disconnected_)
> + return std::vector<Stream>{};
As this is static information I'm not sure we should fail the call.
Could there be cases where the application might want to get information
from the streams to clean up when the camera gets disconnected ?
> +
> + return streams_;
> +}
> +
> } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index d74655d037728feb..dbb2a89163c36cbc 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -9,6 +9,7 @@
> #include <vector>
>
> #include <libcamera/camera.h>
> +#include <libcamera/stream.h>
>
> #include "device_enumerator.h"
> #include "log.h"
> @@ -197,7 +198,8 @@ void PipelineHandlerIPU3::registerCameras()
> continue;
>
> std::string cameraName = sensor->name() + " " + std::to_string(id);
> - std::shared_ptr<Camera> camera = Camera::create(this, cameraName);
> + std::vector<Stream> streams{ Stream(0) };
> + std::shared_ptr<Camera> camera = Camera::create(this, cameraName, streams);
I think this will become a bit cumbersome to use when the Stream class
will gain more fields, but we can improve the API then.
>
> /*
> * If V4L2 device creation fails, the Camera instance won't be
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index c51e8bc1f2c2bf25..bc4a8d7236be589d 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -6,6 +6,7 @@
> */
>
> #include <libcamera/camera.h>
> +#include <libcamera/stream.h>
>
> #include "device_enumerator.h"
> #include "media_device.h"
> @@ -64,7 +65,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> return false;
> }
>
> - std::shared_ptr<Camera> camera = Camera::create(this, media_->model());
> + std::vector<Stream> streams{ Stream(0) };
> + std::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams);
> registerCamera(std::move(camera));
> hotplugMediaDevice(media_.get());
>
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index f58a97d51619515d..c426a953aea1b3dd 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -6,6 +6,7 @@
> */
>
> #include <libcamera/camera.h>
> +#include <libcamera/stream.h>
>
> #include "device_enumerator.h"
> #include "media_device.h"
> @@ -56,7 +57,8 @@ bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)
>
> media_->acquire();
>
> - std::shared_ptr<Camera> camera = Camera::create(this, "Dummy VIMC Camera");
> + std::vector<Stream> streams{ Stream(0) };
> + std::shared_ptr<Camera> camera = Camera::create(this, "Dummy VIMC Camera", streams);
> registerCamera(std::move(camera));
>
> return true;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list