[libcamera-devel] [PATCH v3 4/6] libcamera: camera: extend camera object to support streams
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Jan 30 12:34:32 CET 2019
Hi Laurent,
On 2019-01-30 02:16:35 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> On Tue, Jan 29, 2019 at 02:35:21AM +0100, Niklas Söderlund wrote:
> > On 2019-01-28 01:17:44 +0200, Laurent Pinchart wrote:
> > > 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/ :-)
> >
> > Thanks!
> >
> > > >
> > > > 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> & ?
> >
> > I think we need to return a copy to make life easier for the
> > application. Consider the use-case
> >
> > std::vector<Stream *> streams = camera->streams();
> >
> > for (Stream *stream : streams)
> > if (stream->isViewfinder())
> > streams.erase(stream);
> >
> > std::map<Stream *, StreamConfiguration> config = camera->streamConfiguration(streams);
>
> You can still do this with
>
> const std::vector<Stream *> &Camera::Streams() const
>
> as the line
>
> std::vector<Stream *> streams = camera->streams();
>
> will create a copy of the vector, while
>
> const std::vector<Stream *> &streams = camera->streams();
>
> could be used if the application needs read-only inspection.
You are correct, will make this const in v5.
>
> > I use Stream * instead of Stream in this example as it is what will be
> > used in v4.
> >
> > > > +
> > > > 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.
> >
> > This is removed in v4.
> >
> > > > +
> > > > 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/ ?
> >
> > Thanks.
> >
> > > > *
> > > > * 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.
> >
> > Good point. This is removed in v4 for a design which makes the issue
> > impossible. Thanks for motivating me to have another go at this!
> >
> > > > + 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.
> >
> > In my editor it's exactly 80 columns wide. I do agree it looks odd
> > anyhow and have changed it for style.
> >
> > > > + * 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 ?
> >
> > I have keep this for v4 as in the new design the Stream objects are
> > owned by the pipeline handler. We should always have a pipeline handler
> > as we keep a shard_ptr<> to it. So I'm open to discuss the removal of
> > this check in v4.
> >
> > > > +
> > > > + 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.
> >
> > Lets improve the API over time.
> >
> > > >
> > > > /*
> > > > * 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list