[libcamera-devel] [PATCH v3 5/6] libcamera: pipeline: extend pipelines to support stream configuration
Jacopo Mondi
jacopo at jmondi.org
Mon Jan 28 12:30:19 CET 2019
Hi Niklas, Laurent
On Mon, Jan 28, 2019 at 02:09:10AM +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Sun, Jan 27, 2019 at 01:22:07AM +0100, Niklas Söderlund wrote:
> > All streams which are to be used for capture needs to be configured at
>
> s/needs/need/
>
> > the same time. This allows the pipeline handler to take any dependences
>
> s/dependences/dependencies/
>
> > between the different streams and there configuration into account when
>
> s/there configuration/their configurations/
>
> > setting up the hardware.
> >
> > Extend the pipeline API and all pipeline implementations with two new
> > functions, one to read the configuration and one to update it. Both
> > functions operate on a group of streams which the pipeline handler
> > should consider when performing the operations.
>
> I don't think we should have a function to read the current
> configuration, and if we did, it shouldn't be virtual, but implemented
> in PipelineHandler using a cache.
>
> What we need is a function for a pipeline handler to return a default
> configuration for a set of streams, in order for applications not to
> have to create a configuration from scratch but start from a known to
> work (and recommended) config.
>
I'm sure this has been considered, and I even re-call we discussed
that possibly, but I fail to remember why StreamConfigurations should
be associated with IDs and not be part of the Stream itself.
I would go futher, and ask why Streams are not created with a default
configuration embedded, instead of asking for a default configuration
with streamConfiguration().
I recall the problem was that not all streams could be part of a
configureStreams() request, and that's why we pass configurations+id.
Isn't enough to include in the requests a reference to the Streams the
application actually wants to setup?
Thanks
j
> > In the current implemented pipelines this is rather easy as they only
> > have one stream each per camera. Furthermore as there is yet no way for
> > the pipeline handlers to interact with the hardware all they do is log
> > that the format have been read or updated. Future work based on more
> > components are needed to make the pipelines actually interact with the
> > hardware.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > src/libcamera/include/pipeline_handler.h | 8 ++++++
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 32 ++++++++++++++++++++++
> > src/libcamera/pipeline/uvcvideo.cpp | 35 ++++++++++++++++++++++++
> > src/libcamera/pipeline/vimc.cpp | 35 ++++++++++++++++++++++++
> > src/libcamera/pipeline_handler.cpp | 34 +++++++++++++++++++++++
> > 5 files changed, 144 insertions(+)
> >
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index ca77a40b96e69b66..c19eb9b73ede8720 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -17,6 +17,8 @@ namespace libcamera {
> > class CameraManager;
> > class DeviceEnumerator;
> > class MediaDevice;
> > +class Stream;
> > +class StreamConfiguration;
> >
> > class Camera;
> > class CameraData
> > @@ -38,6 +40,12 @@ public:
> > PipelineHandler(CameraManager *manager);
> > virtual ~PipelineHandler();
> >
> > + virtual std::map<unsigned int, StreamConfiguration>
> > + streamConfiguration(const Camera *camera,
> > + const std::vector<Stream> &streams) const = 0;
> > + virtual int configureStreams(const Camera *camera,
>
> Even if you don't need to modify the state of the Camera object now, you
> may need that later. Beside, this method is called by the Camera class
> from a non-cost instance, so that won't be a problem.
>
> > + std::map<unsigned int, StreamConfiguration> &config) = 0;
> > +
> > virtual bool match(DeviceEnumerator *enumerator) = 0;
> >
> > protected:
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index dbb2a89163c36cbc..ff4d73f947a683ca 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -28,6 +28,12 @@ public:
> > PipelineHandlerIPU3(CameraManager *manager);
> > ~PipelineHandlerIPU3();
> >
> > + std::map<unsigned int, StreamConfiguration>
> > + streamConfiguration(const Camera *camera,
> > + const std::vector<Stream> &streams) const;
> > + int configureStreams(const Camera *camera,
> > + std::map<unsigned int, StreamConfiguration> &config);
> > +
> > bool match(DeviceEnumerator *enumerator);
> >
> > private:
> > @@ -61,6 +67,32 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3()
> > imgu_->release();
> > }
> >
> > +std::map<unsigned int, StreamConfiguration>
> > +PipelineHandlerIPU3::streamConfiguration(const Camera *camera,
> > + const std::vector<Stream> &streams) const
> > +{
> > + std::map<unsigned int, StreamConfiguration> configs;
> > +
> > + StreamConfiguration config;
> > +
> > + LOG(IPU3, Info) << "TODO: Fetch stream configuration";
> > +
> > + configs[0] = config;
> > +
> > + return configs;
> > +}
> > +
> > +int PipelineHandlerIPU3::configureStreams(const Camera *camera,
> > + std::map<unsigned int, StreamConfiguration> &config)
> > +{
> > + StreamConfiguration *cfg = &config[0];
> > +
> > + LOG(IPU3, Info) << "TODO: Configure the camera for resolution " <<
> > + cfg->width << "x" << cfg->height;
> > +
> > + return 0;
> > +}
> > +
> > bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> > {
> > DeviceMatch cio2_dm("ipu3-cio2");
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index bc4a8d7236be589d..bba4655a0a26b622 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -9,18 +9,27 @@
> > #include <libcamera/stream.h>
> >
> > #include "device_enumerator.h"
> > +#include "log.h"
> > #include "media_device.h"
> > #include "pipeline_handler.h"
> > #include "v4l2_device.h"
> >
> > namespace libcamera {
> >
> > +LOG_DEFINE_CATEGORY(UVC)
> > +
> > class PipelineHandlerUVC : public PipelineHandler
> > {
> > public:
> > PipelineHandlerUVC(CameraManager *manager);
> > ~PipelineHandlerUVC();
> >
> > + std::map<unsigned int, StreamConfiguration>
> > + streamConfiguration(const Camera *camera,
> > + const std::vector<Stream> &streams) const;
> > + int configureStreams(const Camera *camera,
> > + std::map<unsigned int, StreamConfiguration> &config);
> > +
> > bool match(DeviceEnumerator *enumerator);
> >
> > private:
> > @@ -42,6 +51,32 @@ PipelineHandlerUVC::~PipelineHandlerUVC()
> > media_->release();
> > }
> >
> > +std::map<unsigned int, StreamConfiguration>
> > +PipelineHandlerUVC::streamConfiguration(const Camera *camera,
> > + const std::vector<Stream> &streams) const
> > +{
> > + std::map<unsigned int, StreamConfiguration> configs;
> > +
> > + StreamConfiguration config;
> > +
> > + LOG(UVC, Info) << "TODO: Fetch stream configuration";
> > +
> > + configs[0] = config;
> > +
> > + return configs;
> > +}
> > +
> > +int PipelineHandlerUVC::configureStreams(const Camera *camera,
> > + std::map<unsigned int, StreamConfiguration> &config)
> > +{
> > + StreamConfiguration *cfg = &config[0];
> > +
> > + LOG(UVC, Info) << "TODO: Configure the camera for resolution " <<
> > + cfg->width << "x" << cfg->height;
> > +
> > + return 0;
> > +}
> > +
> > bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> > {
> > DeviceMatch dm("uvcvideo");
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index c426a953aea1b3dd..c9810a8e192ac68b 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -9,17 +9,26 @@
> > #include <libcamera/stream.h>
> >
> > #include "device_enumerator.h"
> > +#include "log.h"
> > #include "media_device.h"
> > #include "pipeline_handler.h"
> >
> > namespace libcamera {
> >
> > +LOG_DEFINE_CATEGORY(VIMC)
> > +
> > class PipeHandlerVimc : public PipelineHandler
> > {
> > public:
> > PipeHandlerVimc(CameraManager *manager);
> > ~PipeHandlerVimc();
> >
> > + std::map<unsigned int, StreamConfiguration>
> > + streamConfiguration(const Camera *camera,
> > + const std::vector<Stream> &streams) const;
> > + int configureStreams(const Camera *camera,
> > + std::map<unsigned int, StreamConfiguration> &config);
> > +
> > bool match(DeviceEnumerator *enumerator);
> >
> > private:
> > @@ -37,6 +46,32 @@ PipeHandlerVimc::~PipeHandlerVimc()
> > media_->release();
> > }
> >
> > +std::map<unsigned int, StreamConfiguration>
> > +PipeHandlerVimc::streamConfiguration(const Camera *camera,
> > + const std::vector<Stream> &streams) const
> > +{
> > + std::map<unsigned int, StreamConfiguration> configs;
> > +
> > + StreamConfiguration config;
> > +
> > + LOG(VIMC, Info) << "TODO: Fetch stream configuration";
> > +
> > + configs[0] = config;
> > +
> > + return configs;
> > +}
> > +
> > +int PipeHandlerVimc::configureStreams(const Camera *camera,
> > + std::map<unsigned int, StreamConfiguration> &config)
> > +{
> > + StreamConfiguration *cfg = &config[0];
> > +
> > + LOG(VIMC, Info) << "TODO: Configure the camera for resolution " <<
> > + cfg->width << "x" << cfg->height;
> > +
> > + return 0;
> > +}
> > +
> > bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)
> > {
> > DeviceMatch dm("vimc");
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 1c8640799f714686..ee6d80908f67d564 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -75,6 +75,40 @@ PipelineHandler::~PipelineHandler()
> > {
> > };
> >
> > +
> > +/**
> > + * \fn PipelineHandler::streamConfiguration(const Camera *camera, * const std::vector<Stream> &streams)
>
> You don't need to specify the function parameters when the function
> isn't overloaded.
>
> > + * \brief Retrieve a group of stream configurations for a specified camera
> > + * \param[in] camera The camera to fetch the configuration from
> > + * \param[in] streams An array of streams to fetch information about
> > + *
> > + * Retrieve the species camera's configuration for a specified group of streams.
> > + * The caller shall populate the \a streams array with the streams it wish to
> > + * fetch the configuration from. The map of stream IDs and configuration
> > + * returned can then be examined by the caller to learn about the parameters for
> > + * the specified streams.
> > + *
> > + * The intended companion to this is \a configureStreams() which can be used to
> > + * change the group of streams parameters.
> > + *
> > + * \return A map of successfully retrieved stream IDs and configurations or an
> > + * empty map on error.
> > + */
> > +
> > +/**
> > + * \fn PipelineHandler::configureStreams(const Camera *camera, std::map<unsigned int, StreamConfiguration> &config)
> > + * \brief Configure a group of streams for capture
> > + * \param[in] camera The camera to apply the configuration to
>
> You could simply say "The camera to configure".
>
> > + * \param[in] config A map of stream configurations to apply
> > + *
> > + * Configure the specified group of streams for \a camera according to the
> > + * configuration specified in \a configs. The intended caller of this interface
> > + * is the Camera class which will receive configuration to apply from the
> > + * application.
> > + *
> > + * \return 0 on success or a negative error code on error.
> > + */
> > +
> > /**
> > * \fn PipelineHandler::match(DeviceEnumerator *enumerator)
> > * \brief Match media devices and create camera instances
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190128/bc7c44f1/attachment.sig>
More information about the libcamera-devel
mailing list