| Message ID | 20190127002208.18913-7-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:08AM +0100, Niklas Söderlund wrote: > Extend the camera to support reading and configuring of formats for s/of formats/formats/ > groups of streams. The implementation in the Camera are minimalistic as > the heavy lifting are done by the pipeline handler implementations. > > The most important functionality the camera provides in this context is > validation of data structures passed to it from the application and > access control to the pipeline handler. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/camera.h | 4 +++ > src/libcamera/camera.cpp | 68 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 72 insertions(+) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 786d4d7d66bed5b2..798fee2487940208 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -7,6 +7,7 @@ > #ifndef __LIBCAMERA_CAMERA_H__ > #define __LIBCAMERA_CAMERA_H__ > > +#include <map> > #include <memory> > #include <string> > > @@ -16,6 +17,7 @@ namespace libcamera { > > class PipelineHandler; > class Stream; > +class StreamConfiguration; > > class Camera final > { > @@ -35,6 +37,8 @@ public: > void release(); > > std::vector<Stream> streams() const; > + std::map<unsigned int, StreamConfiguration> streamConfiguration(const std::vector<Stream> &streams) const; > + int configureStreams(std::map<unsigned int, StreamConfiguration> &config); > > private: > Camera(PipelineHandler *pipe, const std::string &name); > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 3b2c00d0a4bb45d1..e41143972b0b537a 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -211,4 +211,72 @@ std::vector<Stream> Camera::streams() const > return streams_; > } > > +/** > + * \brief Retrieve a group of stream configurations > + * \param[in] streams A map of stream IDs and configurations to setup > + * > + * Retrieve the camera's configuration for a specified group of streams. The > + * caller can specify which of the camera's streams to retrieve configuration > + * from by populating \a streams. > + * > + * The easiest way to populate the array of streams to fetch configuration from > + * is to first retrieve the camera's full array of stream by with streams() and > + * then potentially trim it down to only contain the streams the caller > + * are interested in. > + * > + * \return A map of successfully retrieved stream IDs and configurations or an > + * empty list on error. > + */ > +std::map<unsigned int, StreamConfiguration> > +Camera::streamConfiguration(const std::vector<Stream> &streams) const > +{ > + std::map<unsigned int, StreamConfiguration> config; > + > + if (disconnected_ || !streams.size()) > + return config; > + > + for (const Stream &stream : streams) > + if (!haveStreamID(stream.id())) > + return config; > + > + return pipe_->streamConfiguration(this, streams); > +} > + > +/** > + * \brief Configure the camera's streams prior to capture > + * \param[in] config A map of stream IDs and configurations to setup > + * > + * Prior to starting capture, the camera must be configured to select a > + * group of streams to be involved in the capture and their configuration. > + * The caller specify which streams are to be involved and their configuration s/specify/specifies/ > + * by populating \a config. > + * > + * The easiest way to populate the array of config is to fetch an initial > + * configuration from the camera with streamConfiguration() and then change the > + * parameters to fit the callers need and once all the streams parameters are s/callers need/caller's needs/ > + * configured hand that over to configureStreams() to actually setup the camera. > + * > + * Exclusive access to the camera shall be ensured by a call to acquire() prior > + * to calling this function, otherwise an -EACCES error is be returned. You should document that -ENODEV will be returned in case the camera is disconnected, and -EINVAL if the configuration isn't valid. > + * > + * \return 0 on success or a negative error code on error. > + */ > +int Camera::configureStreams(std::map<unsigned int, StreamConfiguration> &config) > +{ > + if (disconnected_) > + return -ENODEV; > + > + if (!acquired_) > + return -EACCES; > + > + if (!config.size()) > + return -EINVAL; > + > + for (const auto &pair : config) > + if (!haveStreamID(pair.first)) > + return -EINVAL; Could we make this error impossible by indexing the map on a Stream pointer ? > + return pipe_->configureStreams(this, config); > +} > + > } /* namespace libcamera */
Hi Laurent, Thanks for your feedback. On 2019-01-28 02:35:23 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Sun, Jan 27, 2019 at 01:22:08AM +0100, Niklas Söderlund wrote: > > Extend the camera to support reading and configuring of formats for > > s/of formats/formats/ Thanks. > > > groups of streams. The implementation in the Camera are minimalistic as > > the heavy lifting are done by the pipeline handler implementations. > > > > The most important functionality the camera provides in this context is > > validation of data structures passed to it from the application and > > access control to the pipeline handler. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > include/libcamera/camera.h | 4 +++ > > src/libcamera/camera.cpp | 68 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 72 insertions(+) > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > index 786d4d7d66bed5b2..798fee2487940208 100644 > > --- a/include/libcamera/camera.h > > +++ b/include/libcamera/camera.h > > @@ -7,6 +7,7 @@ > > #ifndef __LIBCAMERA_CAMERA_H__ > > #define __LIBCAMERA_CAMERA_H__ > > > > +#include <map> > > #include <memory> > > #include <string> > > > > @@ -16,6 +17,7 @@ namespace libcamera { > > > > class PipelineHandler; > > class Stream; > > +class StreamConfiguration; > > > > class Camera final > > { > > @@ -35,6 +37,8 @@ public: > > void release(); > > > > std::vector<Stream> streams() const; > > + std::map<unsigned int, StreamConfiguration> streamConfiguration(const std::vector<Stream> &streams) const; > > + int configureStreams(std::map<unsigned int, StreamConfiguration> &config); > > > > private: > > Camera(PipelineHandler *pipe, const std::string &name); > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 3b2c00d0a4bb45d1..e41143972b0b537a 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -211,4 +211,72 @@ std::vector<Stream> Camera::streams() const > > return streams_; > > } > > > > +/** > > + * \brief Retrieve a group of stream configurations > > + * \param[in] streams A map of stream IDs and configurations to setup > > + * > > + * Retrieve the camera's configuration for a specified group of streams. The > > + * caller can specify which of the camera's streams to retrieve configuration > > + * from by populating \a streams. > > + * > > + * The easiest way to populate the array of streams to fetch configuration from > > + * is to first retrieve the camera's full array of stream by with streams() and > > + * then potentially trim it down to only contain the streams the caller > > + * are interested in. > > + * > > + * \return A map of successfully retrieved stream IDs and configurations or an > > + * empty list on error. > > + */ > > +std::map<unsigned int, StreamConfiguration> > > +Camera::streamConfiguration(const std::vector<Stream> &streams) const > > +{ > > + std::map<unsigned int, StreamConfiguration> config; > > + > > + if (disconnected_ || !streams.size()) > > + return config; > > + > > + for (const Stream &stream : streams) > > + if (!haveStreamID(stream.id())) > > + return config; > > + > > + return pipe_->streamConfiguration(this, streams); > > +} > > + > > +/** > > + * \brief Configure the camera's streams prior to capture > > + * \param[in] config A map of stream IDs and configurations to setup > > + * > > + * Prior to starting capture, the camera must be configured to select a > > + * group of streams to be involved in the capture and their configuration. > > + * The caller specify which streams are to be involved and their configuration > > s/specify/specifies/ > > > + * by populating \a config. > > + * > > + * The easiest way to populate the array of config is to fetch an initial > > + * configuration from the camera with streamConfiguration() and then change the > > + * parameters to fit the callers need and once all the streams parameters are > > s/callers need/caller's needs/ > > > + * configured hand that over to configureStreams() to actually setup the camera. > > + * > > + * Exclusive access to the camera shall be ensured by a call to acquire() prior > > + * to calling this function, otherwise an -EACCES error is be returned. > > You should document that -ENODEV will be returned in case the camera is > disconnected, and -EINVAL if the configuration isn't valid. Good point. > > > + * > > + * \return 0 on success or a negative error code on error. > > + */ > > +int Camera::configureStreams(std::map<unsigned int, StreamConfiguration> &config) > > +{ > > + if (disconnected_) > > + return -ENODEV; > > + > > + if (!acquired_) > > + return -EACCES; > > + > > + if (!config.size()) > > + return -EINVAL; > > + > > + for (const auto &pair : config) > > + if (!haveStreamID(pair.first)) > > + return -EINVAL; > > Could we make this error impossible by indexing the map on a Stream > pointer ? I had a try at this for v4, it looks much better. Thanks for the suggestion. > > > + return pipe_->configureStreams(this, config); > > +} > > + > > } /* namespace libcamera */ > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 786d4d7d66bed5b2..798fee2487940208 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -7,6 +7,7 @@ #ifndef __LIBCAMERA_CAMERA_H__ #define __LIBCAMERA_CAMERA_H__ +#include <map> #include <memory> #include <string> @@ -16,6 +17,7 @@ namespace libcamera { class PipelineHandler; class Stream; +class StreamConfiguration; class Camera final { @@ -35,6 +37,8 @@ public: void release(); std::vector<Stream> streams() const; + std::map<unsigned int, StreamConfiguration> streamConfiguration(const std::vector<Stream> &streams) const; + int configureStreams(std::map<unsigned int, StreamConfiguration> &config); private: Camera(PipelineHandler *pipe, const std::string &name); diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 3b2c00d0a4bb45d1..e41143972b0b537a 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -211,4 +211,72 @@ std::vector<Stream> Camera::streams() const return streams_; } +/** + * \brief Retrieve a group of stream configurations + * \param[in] streams A map of stream IDs and configurations to setup + * + * Retrieve the camera's configuration for a specified group of streams. The + * caller can specify which of the camera's streams to retrieve configuration + * from by populating \a streams. + * + * The easiest way to populate the array of streams to fetch configuration from + * is to first retrieve the camera's full array of stream by with streams() and + * then potentially trim it down to only contain the streams the caller + * are interested in. + * + * \return A map of successfully retrieved stream IDs and configurations or an + * empty list on error. + */ +std::map<unsigned int, StreamConfiguration> +Camera::streamConfiguration(const std::vector<Stream> &streams) const +{ + std::map<unsigned int, StreamConfiguration> config; + + if (disconnected_ || !streams.size()) + return config; + + for (const Stream &stream : streams) + if (!haveStreamID(stream.id())) + return config; + + return pipe_->streamConfiguration(this, streams); +} + +/** + * \brief Configure the camera's streams prior to capture + * \param[in] config A map of stream IDs and configurations to setup + * + * Prior to starting capture, the camera must be configured to select a + * group of streams to be involved in the capture and their configuration. + * The caller specify which streams are to be involved and their configuration + * by populating \a config. + * + * The easiest way to populate the array of config is to fetch an initial + * configuration from the camera with streamConfiguration() and then change the + * parameters to fit the callers need and once all the streams parameters are + * configured hand that over to configureStreams() to actually setup the camera. + * + * Exclusive access to the camera shall be ensured by a call to acquire() prior + * to calling this function, otherwise an -EACCES error is be returned. + * + * \return 0 on success or a negative error code on error. + */ +int Camera::configureStreams(std::map<unsigned int, StreamConfiguration> &config) +{ + if (disconnected_) + return -ENODEV; + + if (!acquired_) + return -EACCES; + + if (!config.size()) + return -EINVAL; + + for (const auto &pair : config) + if (!haveStreamID(pair.first)) + return -EINVAL; + + return pipe_->configureStreams(this, config); +} + } /* namespace libcamera */
Extend the camera to support reading and configuring of formats for groups of streams. The implementation in the Camera are minimalistic as the heavy lifting are done by the pipeline handler implementations. The most important functionality the camera provides in this context is validation of data structures passed to it from the application and access control to the pipeline handler. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/camera.h | 4 +++ src/libcamera/camera.cpp | 68 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+)