| Message ID | 20190127002208.18913-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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;
Hi Niklas, On 27/01/2019 00:22, Niklas Söderlund wrote: > A camera consist of one or more video streams origination from the same A camera *consists* of one or more video streams *originating* from ... > video source. The different streams could for example have access to > different hardware blocks in the video pipeline and therefor be able to therefore > process the video source in different ways. > > All static information describing each streams needs to be recorded at describing each stream > 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. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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; > + > 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; have? should this be 'hasStreamID()' ? Looking at the implementation, Yes - I think so. > + > std::shared_ptr<PipelineHandler> pipe_; > std::string name_; > + std::vector<Stream> streams_; > > bool acquired_; > + bool disconnected_; Is this related to streams? > }; > > } /* 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 > * > * 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"; What about the pipeline adding a stream to a camera instead, and the ID being allocated by the camera device? Or does this make things the wrong way around? I guess it would also have to be a function which only the Pipeline was able to call somehow... (we wouldn't want a public Camera API that could arbitrarily add streams!).. > + camera.reset(); > + break; > + } > + 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 Check if the camera has a particular stream ID > + * \param[in] id Stream ID to check for > + * \return ture if stream ID exists, else false s/ture/True/ > + */ > +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 > + * and each streams capabilities. > + * > + * \return An array of all the camera's streams or an empty list on error. Is if(disconnected) an error? I'd say if a camera is disconnected it has no streams ... I don't think that's an error? Perhaps: \return An array of all the camera's streams valid streams. > + */ > +std::vector<Stream> Camera::streams() const > +{ > + if (disconnected_) > + return std::vector<Stream>{}; > + > + 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); > > /* > * 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); Out of interest, UVC exposes two video device nodes. One for the video data, and one for 'meta' data. Would we consider this meta data to be a second stream? > registerCamera(std::move(camera)); > > return true; >
Hi Kieran, Thanks for your feedback. On 2019-01-28 11:34:23 +0000, Kieran Bingham wrote: > Hi Niklas, > > On 27/01/2019 00:22, Niklas Söderlund wrote: > > A camera consist of one or more video streams origination from the same > > A camera *consists* of one or more video streams *originating* from ... > > > video source. The different streams could for example have access to > > different hardware blocks in the video pipeline and therefor be able to > > therefore > > > process the video source in different ways. > > > > All static information describing each streams needs to be recorded at > > > describing each stream > Thanks! > > > 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. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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; > > + > > 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; > > have? should this be 'hasStreamID()' ? > > Looking at the implementation, Yes - I think so. I have renamed this hasStreamId merging your and Laurents comments on this. > > > + > > std::shared_ptr<PipelineHandler> pipe_; > > std::string name_; > > + std::vector<Stream> streams_; > > > > bool acquired_; > > + bool disconnected_; > > Is this related to streams? Yes, if a camera is disconnected operations should not be forwarded to the pipeline handler. Laurent thinks streams information should still be available after a disconnect so I have moved this to the first patch who adds operations which needs a connected camera. > > > }; > > > > } /* 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 > > * > > * 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"; > > What about the pipeline adding a stream to a camera instead, and the ID > being allocated by the camera device? For the ID to have any real meaning the pipeline needs to assign the id as it is the real user of it. When configuring a camera the pipeline handler needs to know which streams are to be used and it currently do this by the use of the id. This might change in the future as the API evolve. > > Or does this make things the wrong way around? > > I guess it would also have to be a function which only the Pipeline was > able to call somehow... > > (we wouldn't want a public Camera API that could arbitrarily add streams!).. :-) > > > > + camera.reset(); > > + break; > > + } > > + 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 > > Check if the camera has a particular stream ID > > > > + * \param[in] id Stream ID to check for > > + * \return ture if stream ID exists, else false > > s/ture/True/ Thanks. > > > > + */ > > +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 > > + * and each streams capabilities. > > + * > > + * \return An array of all the camera's streams or an empty list on error. > > Is if(disconnected) an error? > > I'd say if a camera is disconnected it has no streams ... I don't think > that's an error? > > Perhaps: > > \return An array of all the camera's streams valid streams. Merging your and Laurents comment a disconnected camera will keep exposing the static stream information so I have update this to: \return An array of all the camera's streams. > > > > + */ > > +std::vector<Stream> Camera::streams() const > > +{ > > + if (disconnected_) > > + return std::vector<Stream>{}; > > + > > + 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); > > > > /* > > * 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); > > Out of interest, UVC exposes two video device nodes. One for the video > data, and one for 'meta' data. > > Would we consider this meta data to be a second stream? Possibly, to be honest I don't know what the second video node is used for. If it carries meta data maybe it should be used by a 3A sometime in the future :-) > > > > registerCamera(std::move(camera)); > > > > return true; > > > > -- > Regards > -- > Kieran
Hi Kieran, Scratch all comments regarding the stream id in my previous reply. I have remodeled this to get rid of the stream ids completely. On 2019-01-29 00:00:15 +0100, Niklas Söderlund wrote: > Hi Kieran, > > Thanks for your feedback. > > On 2019-01-28 11:34:23 +0000, Kieran Bingham wrote: > > Hi Niklas, > > > > On 27/01/2019 00:22, Niklas Söderlund wrote: > > > A camera consist of one or more video streams origination from the same > > > > A camera *consists* of one or more video streams *originating* from ... > > > > > video source. The different streams could for example have access to > > > different hardware blocks in the video pipeline and therefor be able to > > > > therefore > > > > > process the video source in different ways. > > > > > > All static information describing each streams needs to be recorded at > > > > > > describing each stream > > > > Thanks! > > > > > > 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. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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; > > > + > > > 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; > > > > have? should this be 'hasStreamID()' ? > > > > Looking at the implementation, Yes - I think so. > > I have renamed this hasStreamId merging your and Laurents comments on > this. > > > > > > + > > > std::shared_ptr<PipelineHandler> pipe_; > > > std::string name_; > > > + std::vector<Stream> streams_; > > > > > > bool acquired_; > > > + bool disconnected_; > > > > Is this related to streams? > > Yes, if a camera is disconnected operations should not be forwarded to > the pipeline handler. Laurent thinks streams information should still be > available after a disconnect so I have moved this to the first patch who > adds operations which needs a connected camera. > > > > > > }; > > > > > > } /* 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 > > > * > > > * 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"; > > > > What about the pipeline adding a stream to a camera instead, and the ID > > being allocated by the camera device? > > For the ID to have any real meaning the pipeline needs to assign the id > as it is the real user of it. When configuring a camera the pipeline > handler needs to know which streams are to be used and it currently do > this by the use of the id. This might change in the future as the API > evolve. > > > > > Or does this make things the wrong way around? > > > > I guess it would also have to be a function which only the Pipeline was > > able to call somehow... > > > > (we wouldn't want a public Camera API that could arbitrarily add streams!).. > > :-) > > > > > > > > + camera.reset(); > > > + break; > > > + } > > > + 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 > > > > Check if the camera has a particular stream ID > > > > > > > + * \param[in] id Stream ID to check for > > > + * \return ture if stream ID exists, else false > > > > s/ture/True/ > > Thanks. > > > > > > > > + */ > > > +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 > > > + * and each streams capabilities. > > > + * > > > + * \return An array of all the camera's streams or an empty list on error. > > > > Is if(disconnected) an error? > > > > I'd say if a camera is disconnected it has no streams ... I don't think > > that's an error? > > > > Perhaps: > > > > \return An array of all the camera's streams valid streams. > > Merging your and Laurents comment a disconnected camera will keep > exposing the static stream information so I have update this to: > > \return An array of all the camera's streams. > > > > > > > > + */ > > > +std::vector<Stream> Camera::streams() const > > > +{ > > > + if (disconnected_) > > > + return std::vector<Stream>{}; > > > + > > > + 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); > > > > > > /* > > > * 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); > > > > Out of interest, UVC exposes two video device nodes. One for the video > > data, and one for 'meta' data. > > > > Would we consider this meta data to be a second stream? > > Possibly, to be honest I don't know what the second video node is used > for. If it carries meta data maybe it should be used by a 3A sometime in > the future :-) > > > > > > > > registerCamera(std::move(camera)); > > > > > > return true; > > > > > > > -- > > Regards > > -- > > Kieran > > -- > Regards, > Niklas Söderlund
Hi Laurent, Thanks for your feedback. On 2019-01-28 01:17:44 +0200, Laurent Pinchart wrote: > 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/ :-) Thanks! > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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); 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
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@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. > 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;
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@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
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; + 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; + 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 * * 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; + } + 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 + * 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>{}; + + 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); /* * 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;
A camera consist of one or more video streams origination from the same 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 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. Signed-off-by: Niklas Söderlund <niklas.soderlund@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(-)