| Message ID | 20190402005332.25018-6-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Tue, Apr 02, 2019 at 02:53:32AM +0200, Niklas Söderlund wrote: > Instead of requesting the default configuration for a set of streams > where the application have to figure out which streams provided by the s/have/has/ > camera is best suited for its intended usage have the library figure s/usage/usage,/ > this out by using stream usage hints. > > The application asks the library for a list of streams and a suggested > default configuration for them by supplying a list of stream usage > hints. Once the list is retrieved the application can try and fine-tune > the returned configuration and then try to apply it to the camera. > > Currently no pipeline handler is prepared to handle usage hints but nor > did it make use of the list of Stream IDs which was the previous > interface. The main reason for this is that all cameras currently only > provide one stream each. This will still be the case but the API will be > prepared to expand both pipeline handlers and applications to support > multiple streams. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/camera.h | 3 ++- > src/cam/main.cpp | 2 +- qcam ? :-) > src/libcamera/camera.cpp | 30 ++++++++---------------- > src/libcamera/include/pipeline_handler.h | 2 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++-- > src/libcamera/pipeline/uvcvideo.cpp | 6 ++--- > src/libcamera/pipeline/vimc.cpp | 6 ++--- > src/libcamera/pipeline_handler.cpp | 4 ++-- > src/qcam/main_window.cpp | 5 ++-- > test/camera/capture.cpp | 5 ++-- > test/camera/configuration_default.cpp | 17 +++++--------- > test/camera/configuration_set.cpp | 3 +-- > test/camera/statemachine.cpp | 4 +--- > 13 files changed, 34 insertions(+), 57 deletions(-) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 77e4cd1ee14c2c61..ca5cf9faf866a6a6 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -22,6 +22,7 @@ class PipelineHandler; > class Request; > class Stream; > class StreamConfiguration; > +class StreamHint; > > class Camera final > { > @@ -44,7 +45,7 @@ public: > > const std::set<Stream *> &streams() const; > std::map<Stream *, StreamConfiguration> > - streamConfiguration(std::set<Stream *> &streams); > + streamConfiguration(const std::vector<StreamHint> &hints); > int configureStreams(std::map<Stream *, StreamConfiguration> &config); > > int allocateBuffers(); > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index cc5302ca4e72ea6f..0a248d042cfe772b 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -81,7 +81,7 @@ static int parseOptions(int argc, char *argv[]) > static int prepare_camera_config(std::map<Stream *, StreamConfiguration> *config) > { > std::set<Stream *> streams = camera->streams(); > - *config = camera->streamConfiguration(streams); > + *config = camera->streamConfiguration({ Video() }); > Stream *stream = config->begin()->first; > > if (options.isSet(OptFormat)) { > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 8ee9cc0866167ae1..5a3c8ef6aad691bd 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -345,33 +345,23 @@ const std::set<Stream *> &Camera::streams() const > } > > /** > - * \brief Retrieve a group of stream configurations > - * \param[in] streams A map of stream IDs and configurations to setup > + * \brief Retrieve a group of stream configurations according to usage hints > + * \param[in] hints A list of stream usage hints > * > - * Retrieve the camera's configuration for a specified group of streams. The > - * caller can specifies which of the camera's streams to retrieve configuration > - * from by populating \a streams. > + * Retrieve configuration for a set of desired usages. The caller specifies a > + * list of usage hints and the camera returns a map of suitable streams and > + * a suggested default configuration. s/a suggested default configuration/their suggested default configurations/ We'll have to expand this eventually. There's already one problem I can identify : there's no way for the caller to associate streams with the usage hints. One option would be to return a std::vector<std::pair<Stream *, StreamConfiguration>> with the requirement that the returned vector will have the same order as the hints vector. Other options are possible too. > * > - * The easiest way to populate the array of streams to fetch configuration from > - * is to first retrieve the camera's full array of stream 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. > + * \return A map of stream IDs to configuration if requested usage can be Maybe s/stream IDs/streams/ ? And s/configuration if requested/configurations if the requested/ > + * satisfied, if the request is impossible or on error an empty map is returned. ", or an empty map if the request can't be satisfied or another error occurs", or just ", or an empty map otherwise" ? > */ > std::map<Stream *, StreamConfiguration> > -Camera::streamConfiguration(std::set<Stream *> &streams) > +Camera::streamConfiguration(const std::vector<StreamHint> &hints) > { > - if (disconnected_ || !streams.size()) > + if (disconnected_ || !hints.size() || hints.size() > streams_.size()) > return std::map<Stream *, StreamConfiguration>{}; > > - for (Stream *stream : streams) { > - if (streams_.find(stream) == streams_.end()) > - return std::map<Stream *, StreamConfiguration>{}; > - } > - > - return pipe_->streamConfiguration(this, streams); > + return pipe_->streamConfiguration(this, hints); > } > > /** > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > index acb376e07030ae03..fbc439cec61832a6 100644 > --- a/src/libcamera/include/pipeline_handler.h > +++ b/src/libcamera/include/pipeline_handler.h > @@ -53,7 +53,7 @@ public: > virtual bool match(DeviceEnumerator *enumerator) = 0; > > virtual std::map<Stream *, StreamConfiguration> > - streamConfiguration(Camera *camera, std::set<Stream *> &streams) = 0; > + streamConfiguration(Camera *camera, const std::vector<StreamHint> &hints) = 0; > virtual int configureStreams(Camera *camera, > std::map<Stream *, StreamConfiguration> &config) = 0; > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 55489c31df2d0382..a59cfd9aabff69ae 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -32,7 +32,7 @@ public: > > std::map<Stream *, StreamConfiguration> > streamConfiguration(Camera *camera, > - std::set<Stream *> &streams) override; > + const std::vector<StreamHint> &hints) override; > int configureStreams(Camera *camera, > std::map<Stream *, StreamConfiguration> &config) override; > > @@ -100,7 +100,7 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3() > > std::map<Stream *, StreamConfiguration> > PipelineHandlerIPU3::streamConfiguration(Camera *camera, > - std::set<Stream *> &streams) > + const std::vector<StreamHint> &hints) > { > IPU3CameraData *data = cameraData(camera); > std::map<Stream *, StreamConfiguration> configs; > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index cc3e0cd9afab38b9..700e9f2730caae9c 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -28,7 +28,7 @@ public: > > std::map<Stream *, StreamConfiguration> > streamConfiguration(Camera *camera, > - std::set<Stream *> &streams) override; > + const std::vector<StreamHint> &hints) override; > int configureStreams(Camera *camera, > std::map<Stream *, StreamConfiguration> &config) override; > > @@ -84,14 +84,12 @@ PipelineHandlerUVC::~PipelineHandlerUVC() > > std::map<Stream *, StreamConfiguration> > PipelineHandlerUVC::streamConfiguration(Camera *camera, > - std::set<Stream *> &streams) > + const std::vector<StreamHint> &hints) > { > UVCCameraData *data = cameraData(camera); > - > std::map<Stream *, StreamConfiguration> configs; > StreamConfiguration config{}; > > - LOG(UVC, Debug) << "Retrieving default format"; > config.width = 640; > config.height = 480; > config.pixelFormat = V4L2_PIX_FMT_YUYV; > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index 2e8c26fb7c0b2708..f6517c5217766176 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -28,7 +28,7 @@ public: > > std::map<Stream *, StreamConfiguration> > streamConfiguration(Camera *camera, > - std::set<Stream *> &streams) override; > + const std::vector<StreamHint> &hints) override; > int configureStreams(Camera *camera, > std::map<Stream *, StreamConfiguration> &config) override; > > @@ -84,14 +84,12 @@ PipelineHandlerVimc::~PipelineHandlerVimc() > > std::map<Stream *, StreamConfiguration> > PipelineHandlerVimc::streamConfiguration(Camera *camera, > - std::set<Stream *> &streams) > + const std::vector<StreamHint> &hints) > { > VimcCameraData *data = cameraData(camera); > std::map<Stream *, StreamConfiguration> configs; > - > StreamConfiguration config{}; > > - LOG(VIMC, Debug) << "Retrieving default format"; > config.width = 640; > config.height = 480; > config.pixelFormat = V4L2_PIX_FMT_RGB24; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 1a858f2638ceac5f..fd217eedfec9aee9 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -152,10 +152,10 @@ PipelineHandler::~PipelineHandler() > * \fn PipelineHandler::streamConfiguration() > * \brief Retrieve a group of stream configurations for a specified camera > * \param[in] camera The camera to fetch default configuration from > - * \param[in] streams An array of streams to fetch information about > + * \param[in] hints A list of stream usage hints > * > * Retrieve the species camera's default configuration for a specified group of > - * streams. The caller shall populate the \a streams array with the streams it > + * use-cases. The caller shall populate the \a hints array with the use-cases it > * wish to fetch the configuration from. The map of streams and configuration While at it, s/wish/wishes/ > * returned can then be examined by the caller to learn about the defualt > * parameters for the specified streams. > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index a148aa4d117fff59..fefcc753b5db45c9 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -97,9 +97,8 @@ int MainWindow::startCapture() > { > int ret; > > - Stream *stream = *camera_->streams().begin(); > - std::set<Stream *> streams{ stream }; > - config_ = camera_->streamConfiguration(streams); > + config_ = camera_->streamConfiguration({ Video() }); > + Stream *stream = config_.begin()->first; > ret = camera_->configureStreams(config_); > if (ret < 0) { > std::cout << "Failed to configure camera" << std::endl; > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp > index f6932b7505571712..61489902915291f9 100644 > --- a/test/camera/capture.cpp > +++ b/test/camera/capture.cpp > @@ -42,10 +42,9 @@ protected: > > int run() > { > - Stream *stream = *camera_->streams().begin(); > - std::set<Stream *> streams = { stream }; > std::map<Stream *, StreamConfiguration> conf = > - camera_->streamConfiguration(streams); > + camera_->streamConfiguration({ Video() }); > + Stream *stream = conf.begin()->first; > StreamConfiguration *sconf = &conf.begin()->second; > > if (!configurationValid(conf)) { > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp > index 856cd415f7a6aec1..6feb1e435192e6f6 100644 > --- a/test/camera/configuration_default.cpp > +++ b/test/camera/configuration_default.cpp > @@ -20,14 +20,10 @@ protected: > { > std::map<Stream *, StreamConfiguration> conf; > > - /* > - * Test that asking for default configuration for a valid > - * array of streams returns something valid. > - */ > - std::set<Stream *> streams = { *camera_->streams().begin() }; > - conf = camera_->streamConfiguration(streams); > + /* Test asking for configuration for a video stream. */ > + conf = camera_->streamConfiguration({ Video() }); > if (conf.empty()) { > - cout << "Failed to retrieve configuration for valid streams" > + cout << "Failed to retrieve configuration for video streams" > << endl; > return TestFail; > } > @@ -39,12 +35,11 @@ protected: > > /* > * Test that asking for configuration for an empty array of > - * streams returns an empty list of configurations. > + * stream usage hints returns an empty list of configurations. > */ > - std::set<Stream *> streams_empty = {}; > - conf = camera_->streamConfiguration(streams_empty); > + conf = camera_->streamConfiguration({}); > if (!conf.empty()) { > - cout << "Failed to retrieve configuration for empty streams" > + cout << "Failed to retrieve configuration for empty usage list" > << endl; > return TestFail; > } > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp > index cac1da959f241bc5..383113f2133e6fd3 100644 > --- a/test/camera/configuration_set.cpp > +++ b/test/camera/configuration_set.cpp > @@ -18,9 +18,8 @@ class ConfigurationSet : public CameraTest > protected: > int run() > { > - std::set<Stream *> streams = { *camera_->streams().begin() }; > std::map<Stream *, StreamConfiguration> conf = > - camera_->streamConfiguration(streams); > + camera_->streamConfiguration({ Video() }); > StreamConfiguration *sconf = &conf.begin()->second; > > if (!configurationValid(conf)) { > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp > index f4395f2b1bfed698..e92d9b7992545b5b 100644 > --- a/test/camera/statemachine.cpp > +++ b/test/camera/statemachine.cpp > @@ -235,9 +235,7 @@ protected: > > int run() > { > - Stream *stream = *camera_->streams().begin(); > - std::set<Stream *> streams = { stream }; > - defconf_ = camera_->streamConfiguration(streams); > + defconf_ = camera_->streamConfiguration({ Video() }); > > if (testAvailable() != TestPass) { > cout << "State machine in Available state failed" << endl;
Hi Laurent, Thanks for your feedback. On 2019-04-02 18:06:54 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Tue, Apr 02, 2019 at 02:53:32AM +0200, Niklas Söderlund wrote: > > Instead of requesting the default configuration for a set of streams > > where the application have to figure out which streams provided by the > > s/have/has/ > > > camera is best suited for its intended usage have the library figure > > s/usage/usage,/ > > > this out by using stream usage hints. > > > > The application asks the library for a list of streams and a suggested > > default configuration for them by supplying a list of stream usage > > hints. Once the list is retrieved the application can try and fine-tune > > the returned configuration and then try to apply it to the camera. > > > > Currently no pipeline handler is prepared to handle usage hints but nor > > did it make use of the list of Stream IDs which was the previous > > interface. The main reason for this is that all cameras currently only > > provide one stream each. This will still be the case but the API will be > > prepared to expand both pipeline handlers and applications to support > > multiple streams. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > include/libcamera/camera.h | 3 ++- > > src/cam/main.cpp | 2 +- > > qcam ? :-) > > > src/libcamera/camera.cpp | 30 ++++++++---------------- > > src/libcamera/include/pipeline_handler.h | 2 +- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++-- > > src/libcamera/pipeline/uvcvideo.cpp | 6 ++--- > > src/libcamera/pipeline/vimc.cpp | 6 ++--- > > src/libcamera/pipeline_handler.cpp | 4 ++-- > > src/qcam/main_window.cpp | 5 ++-- I see qcam being address in this patch ;-) > > test/camera/capture.cpp | 5 ++-- > > test/camera/configuration_default.cpp | 17 +++++--------- > > test/camera/configuration_set.cpp | 3 +-- > > test/camera/statemachine.cpp | 4 +--- > > 13 files changed, 34 insertions(+), 57 deletions(-) [snip] > > /** > > - * \brief Retrieve a group of stream configurations > > - * \param[in] streams A map of stream IDs and configurations to setup > > + * \brief Retrieve a group of stream configurations according to usage hints > > + * \param[in] hints A list of stream usage hints > > * > > - * Retrieve the camera's configuration for a specified group of streams. The > > - * caller can specifies which of the camera's streams to retrieve configuration > > - * from by populating \a streams. > > + * Retrieve configuration for a set of desired usages. The caller specifies a > > + * list of usage hints and the camera returns a map of suitable streams and > > + * a suggested default configuration. > > s/a suggested default configuration/their suggested default configurations/ > > We'll have to expand this eventually. I agree this will probably needs to be expanded as we learn what pieces are missing. > > There's already one problem I can identify : there's no way for the > caller to associate streams with the usage hints. One option would be to > return a std::vector<std::pair<Stream *, StreamConfiguration>> with the > requirement that the returned vector will have the same order as the > hints vector. Other options are possible too. Good catch, for some reason I assumed this was already the case... I agree your suggestion of using std::vector<std::pair<>> would solve the issue but it's not a very nice interface for applications to use. After toying around a bit I think I found an other possible solution. How about adding a Stream * to StreamConfiguration and returning a std::vector<StreamConfiguration> where the order is the same as the hints vector? It would also simplify the API towards applications which I really like. The reason this is a std::map currently is due to our organic growth and that we used Stream * as the way to retrieve default configurations in a non so smart way as we always only had one stream that worked but now it seems less smart. If you think this is a semi good idea I propose I add that on top of this change to ease review. Let me know what you think.
Hi Niklas, On Wed, Apr 03, 2019 at 01:52:18AM +0200, Niklas Söderlund wrote: > On 2019-04-02 18:06:54 +0300, Laurent Pinchart wrote: > > On Tue, Apr 02, 2019 at 02:53:32AM +0200, Niklas Söderlund wrote: > >> Instead of requesting the default configuration for a set of streams > >> where the application have to figure out which streams provided by the > > > > s/have/has/ > > > >> camera is best suited for its intended usage have the library figure > > > > s/usage/usage,/ > > > >> this out by using stream usage hints. > >> > >> The application asks the library for a list of streams and a suggested > >> default configuration for them by supplying a list of stream usage > >> hints. Once the list is retrieved the application can try and fine-tune > >> the returned configuration and then try to apply it to the camera. > >> > >> Currently no pipeline handler is prepared to handle usage hints but nor > >> did it make use of the list of Stream IDs which was the previous > >> interface. The main reason for this is that all cameras currently only > >> provide one stream each. This will still be the case but the API will be > >> prepared to expand both pipeline handlers and applications to support > >> multiple streams. > >> > >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> --- > >> include/libcamera/camera.h | 3 ++- > >> src/cam/main.cpp | 2 +- > > > > qcam ? :-) > > > >> src/libcamera/camera.cpp | 30 ++++++++---------------- > >> src/libcamera/include/pipeline_handler.h | 2 +- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++-- > >> src/libcamera/pipeline/uvcvideo.cpp | 6 ++--- > >> src/libcamera/pipeline/vimc.cpp | 6 ++--- > >> src/libcamera/pipeline_handler.cpp | 4 ++-- > >> src/qcam/main_window.cpp | 5 ++-- > > I see qcam being address in this patch ;-) > > >> test/camera/capture.cpp | 5 ++-- > >> test/camera/configuration_default.cpp | 17 +++++--------- > >> test/camera/configuration_set.cpp | 3 +-- > >> test/camera/statemachine.cpp | 4 +--- > >> 13 files changed, 34 insertions(+), 57 deletions(-) > > [snip] > > >> /** > >> - * \brief Retrieve a group of stream configurations > >> - * \param[in] streams A map of stream IDs and configurations to setup > >> + * \brief Retrieve a group of stream configurations according to usage hints > >> + * \param[in] hints A list of stream usage hints > >> * > >> - * Retrieve the camera's configuration for a specified group of streams. The > >> - * caller can specifies which of the camera's streams to retrieve configuration > >> - * from by populating \a streams. > >> + * Retrieve configuration for a set of desired usages. The caller specifies a > >> + * list of usage hints and the camera returns a map of suitable streams and > >> + * a suggested default configuration. > > > > s/a suggested default configuration/their suggested default configurations/ > > > > We'll have to expand this eventually. > > I agree this will probably needs to be expanded as we learn what pieces > are missing. > > > > > There's already one problem I can identify : there's no way for the > > caller to associate streams with the usage hints. One option would be to > > return a std::vector<std::pair<Stream *, StreamConfiguration>> with the > > requirement that the returned vector will have the same order as the > > hints vector. Other options are possible too. > > Good catch, for some reason I assumed this was already the case... > > I agree your suggestion of using std::vector<std::pair<>> would solve > the issue but it's not a very nice interface for applications to use. > After toying around a bit I think I found an other possible solution. > > How about adding a Stream * to StreamConfiguration and returning a > std::vector<StreamConfiguration> where the order is the same as the > hints vector? It would also simplify the API towards applications which > I really like. > > The reason this is a std::map currently is due to our organic growth and > that we used Stream * as the way to retrieve default configurations in a > non so smart way as we always only had one stream that worked but now it > seems less smart. > > If you think this is a semi good idea I propose I add that on top of > this change to ease review. Let me know what you think. I think it's worth a try, let's see how it will look like in applications.
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 77e4cd1ee14c2c61..ca5cf9faf866a6a6 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -22,6 +22,7 @@ class PipelineHandler; class Request; class Stream; class StreamConfiguration; +class StreamHint; class Camera final { @@ -44,7 +45,7 @@ public: const std::set<Stream *> &streams() const; std::map<Stream *, StreamConfiguration> - streamConfiguration(std::set<Stream *> &streams); + streamConfiguration(const std::vector<StreamHint> &hints); int configureStreams(std::map<Stream *, StreamConfiguration> &config); int allocateBuffers(); diff --git a/src/cam/main.cpp b/src/cam/main.cpp index cc5302ca4e72ea6f..0a248d042cfe772b 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -81,7 +81,7 @@ static int parseOptions(int argc, char *argv[]) static int prepare_camera_config(std::map<Stream *, StreamConfiguration> *config) { std::set<Stream *> streams = camera->streams(); - *config = camera->streamConfiguration(streams); + *config = camera->streamConfiguration({ Video() }); Stream *stream = config->begin()->first; if (options.isSet(OptFormat)) { diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 8ee9cc0866167ae1..5a3c8ef6aad691bd 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -345,33 +345,23 @@ const std::set<Stream *> &Camera::streams() const } /** - * \brief Retrieve a group of stream configurations - * \param[in] streams A map of stream IDs and configurations to setup + * \brief Retrieve a group of stream configurations according to usage hints + * \param[in] hints A list of stream usage hints * - * Retrieve the camera's configuration for a specified group of streams. The - * caller can specifies which of the camera's streams to retrieve configuration - * from by populating \a streams. + * Retrieve configuration for a set of desired usages. The caller specifies a + * list of usage hints and the camera returns a map of suitable streams and + * a suggested default configuration. * - * The easiest way to populate the array of streams to fetch configuration from - * is to first retrieve the camera's full array of stream 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. + * \return A map of stream IDs to configuration if requested usage can be + * satisfied, if the request is impossible or on error an empty map is returned. */ std::map<Stream *, StreamConfiguration> -Camera::streamConfiguration(std::set<Stream *> &streams) +Camera::streamConfiguration(const std::vector<StreamHint> &hints) { - if (disconnected_ || !streams.size()) + if (disconnected_ || !hints.size() || hints.size() > streams_.size()) return std::map<Stream *, StreamConfiguration>{}; - for (Stream *stream : streams) { - if (streams_.find(stream) == streams_.end()) - return std::map<Stream *, StreamConfiguration>{}; - } - - return pipe_->streamConfiguration(this, streams); + return pipe_->streamConfiguration(this, hints); } /** diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h index acb376e07030ae03..fbc439cec61832a6 100644 --- a/src/libcamera/include/pipeline_handler.h +++ b/src/libcamera/include/pipeline_handler.h @@ -53,7 +53,7 @@ public: virtual bool match(DeviceEnumerator *enumerator) = 0; virtual std::map<Stream *, StreamConfiguration> - streamConfiguration(Camera *camera, std::set<Stream *> &streams) = 0; + streamConfiguration(Camera *camera, const std::vector<StreamHint> &hints) = 0; virtual int configureStreams(Camera *camera, std::map<Stream *, StreamConfiguration> &config) = 0; diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 55489c31df2d0382..a59cfd9aabff69ae 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -32,7 +32,7 @@ public: std::map<Stream *, StreamConfiguration> streamConfiguration(Camera *camera, - std::set<Stream *> &streams) override; + const std::vector<StreamHint> &hints) override; int configureStreams(Camera *camera, std::map<Stream *, StreamConfiguration> &config) override; @@ -100,7 +100,7 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3() std::map<Stream *, StreamConfiguration> PipelineHandlerIPU3::streamConfiguration(Camera *camera, - std::set<Stream *> &streams) + const std::vector<StreamHint> &hints) { IPU3CameraData *data = cameraData(camera); std::map<Stream *, StreamConfiguration> configs; diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index cc3e0cd9afab38b9..700e9f2730caae9c 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -28,7 +28,7 @@ public: std::map<Stream *, StreamConfiguration> streamConfiguration(Camera *camera, - std::set<Stream *> &streams) override; + const std::vector<StreamHint> &hints) override; int configureStreams(Camera *camera, std::map<Stream *, StreamConfiguration> &config) override; @@ -84,14 +84,12 @@ PipelineHandlerUVC::~PipelineHandlerUVC() std::map<Stream *, StreamConfiguration> PipelineHandlerUVC::streamConfiguration(Camera *camera, - std::set<Stream *> &streams) + const std::vector<StreamHint> &hints) { UVCCameraData *data = cameraData(camera); - std::map<Stream *, StreamConfiguration> configs; StreamConfiguration config{}; - LOG(UVC, Debug) << "Retrieving default format"; config.width = 640; config.height = 480; config.pixelFormat = V4L2_PIX_FMT_YUYV; diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index 2e8c26fb7c0b2708..f6517c5217766176 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -28,7 +28,7 @@ public: std::map<Stream *, StreamConfiguration> streamConfiguration(Camera *camera, - std::set<Stream *> &streams) override; + const std::vector<StreamHint> &hints) override; int configureStreams(Camera *camera, std::map<Stream *, StreamConfiguration> &config) override; @@ -84,14 +84,12 @@ PipelineHandlerVimc::~PipelineHandlerVimc() std::map<Stream *, StreamConfiguration> PipelineHandlerVimc::streamConfiguration(Camera *camera, - std::set<Stream *> &streams) + const std::vector<StreamHint> &hints) { VimcCameraData *data = cameraData(camera); std::map<Stream *, StreamConfiguration> configs; - StreamConfiguration config{}; - LOG(VIMC, Debug) << "Retrieving default format"; config.width = 640; config.height = 480; config.pixelFormat = V4L2_PIX_FMT_RGB24; diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 1a858f2638ceac5f..fd217eedfec9aee9 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -152,10 +152,10 @@ PipelineHandler::~PipelineHandler() * \fn PipelineHandler::streamConfiguration() * \brief Retrieve a group of stream configurations for a specified camera * \param[in] camera The camera to fetch default configuration from - * \param[in] streams An array of streams to fetch information about + * \param[in] hints A list of stream usage hints * * Retrieve the species camera's default configuration for a specified group of - * streams. The caller shall populate the \a streams array with the streams it + * use-cases. The caller shall populate the \a hints array with the use-cases it * wish to fetch the configuration from. The map of streams and configuration * returned can then be examined by the caller to learn about the defualt * parameters for the specified streams. diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index a148aa4d117fff59..fefcc753b5db45c9 100644 --- a/src/qcam/main_window.cpp +++ b/src/qcam/main_window.cpp @@ -97,9 +97,8 @@ int MainWindow::startCapture() { int ret; - Stream *stream = *camera_->streams().begin(); - std::set<Stream *> streams{ stream }; - config_ = camera_->streamConfiguration(streams); + config_ = camera_->streamConfiguration({ Video() }); + Stream *stream = config_.begin()->first; ret = camera_->configureStreams(config_); if (ret < 0) { std::cout << "Failed to configure camera" << std::endl; diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp index f6932b7505571712..61489902915291f9 100644 --- a/test/camera/capture.cpp +++ b/test/camera/capture.cpp @@ -42,10 +42,9 @@ protected: int run() { - Stream *stream = *camera_->streams().begin(); - std::set<Stream *> streams = { stream }; std::map<Stream *, StreamConfiguration> conf = - camera_->streamConfiguration(streams); + camera_->streamConfiguration({ Video() }); + Stream *stream = conf.begin()->first; StreamConfiguration *sconf = &conf.begin()->second; if (!configurationValid(conf)) { diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp index 856cd415f7a6aec1..6feb1e435192e6f6 100644 --- a/test/camera/configuration_default.cpp +++ b/test/camera/configuration_default.cpp @@ -20,14 +20,10 @@ protected: { std::map<Stream *, StreamConfiguration> conf; - /* - * Test that asking for default configuration for a valid - * array of streams returns something valid. - */ - std::set<Stream *> streams = { *camera_->streams().begin() }; - conf = camera_->streamConfiguration(streams); + /* Test asking for configuration for a video stream. */ + conf = camera_->streamConfiguration({ Video() }); if (conf.empty()) { - cout << "Failed to retrieve configuration for valid streams" + cout << "Failed to retrieve configuration for video streams" << endl; return TestFail; } @@ -39,12 +35,11 @@ protected: /* * Test that asking for configuration for an empty array of - * streams returns an empty list of configurations. + * stream usage hints returns an empty list of configurations. */ - std::set<Stream *> streams_empty = {}; - conf = camera_->streamConfiguration(streams_empty); + conf = camera_->streamConfiguration({}); if (!conf.empty()) { - cout << "Failed to retrieve configuration for empty streams" + cout << "Failed to retrieve configuration for empty usage list" << endl; return TestFail; } diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp index cac1da959f241bc5..383113f2133e6fd3 100644 --- a/test/camera/configuration_set.cpp +++ b/test/camera/configuration_set.cpp @@ -18,9 +18,8 @@ class ConfigurationSet : public CameraTest protected: int run() { - std::set<Stream *> streams = { *camera_->streams().begin() }; std::map<Stream *, StreamConfiguration> conf = - camera_->streamConfiguration(streams); + camera_->streamConfiguration({ Video() }); StreamConfiguration *sconf = &conf.begin()->second; if (!configurationValid(conf)) { diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp index f4395f2b1bfed698..e92d9b7992545b5b 100644 --- a/test/camera/statemachine.cpp +++ b/test/camera/statemachine.cpp @@ -235,9 +235,7 @@ protected: int run() { - Stream *stream = *camera_->streams().begin(); - std::set<Stream *> streams = { stream }; - defconf_ = camera_->streamConfiguration(streams); + defconf_ = camera_->streamConfiguration({ Video() }); if (testAvailable() != TestPass) { cout << "State machine in Available state failed" << endl;
Instead of requesting the default configuration for a set of streams where the application have to figure out which streams provided by the camera is best suited for its intended usage have the library figure this out by using stream usage hints. The application asks the library for a list of streams and a suggested default configuration for them by supplying a list of stream usage hints. Once the list is retrieved the application can try and fine-tune the returned configuration and then try to apply it to the camera. Currently no pipeline handler is prepared to handle usage hints but nor did it make use of the list of Stream IDs which was the previous interface. The main reason for this is that all cameras currently only provide one stream each. This will still be the case but the API will be prepared to expand both pipeline handlers and applications to support multiple streams. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/camera.h | 3 ++- src/cam/main.cpp | 2 +- src/libcamera/camera.cpp | 30 ++++++++---------------- src/libcamera/include/pipeline_handler.h | 2 +- src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++-- src/libcamera/pipeline/uvcvideo.cpp | 6 ++--- src/libcamera/pipeline/vimc.cpp | 6 ++--- src/libcamera/pipeline_handler.cpp | 4 ++-- src/qcam/main_window.cpp | 5 ++-- test/camera/capture.cpp | 5 ++-- test/camera/configuration_default.cpp | 17 +++++--------- test/camera/configuration_set.cpp | 3 +-- test/camera/statemachine.cpp | 4 +--- 13 files changed, 34 insertions(+), 57 deletions(-)